Re: [flashrom] [PATCH] No default driver if more than one driver is available

[Carl-Daniel Hailfinger]

Am 17.07.2012 00:42 schrieb Stefan Tauner:
> On Mon, 16 Jul 2012 23:28:22 +0200
> Carl-Daniel Hailfinger  wrote:
> > New version.
> >
> > Always require the --programmer parameter on the command line if any
> > flash chip access (probe/read/write/erase/...) is requested.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-programmer_no_default/cli_classic.c
> >
> > --- flashrom-programmer_no_default/cli_classic.c	(Revision 1547)
> > +++ flashrom-programmer_no_default/cli_classic.c	(Arbeitskopie)
> > @@ -107,11 +39,11 @@
> >  #endif
> >             "-E|-r <file>|-w <file>|-v <file>]\n"
> >           "       [-c <chipname>] [-l <file>] [-o <file>]\n"
> >       "       [-i <image>] [-p <programmername>[:<parameters>]]\n\n");
> >       "       [-i <image>] -p <programmername>[:<parameters>]\n\n");
>
> dont save changed line counts in the wrong places. please move the -p
> part right after "flashrom" here and in the other bits (e.g. manpage).
> i would even argue that all mandatory parameters should be mentioned
> first (i.e. -E | ... should be moved up too).

Hm yes. But how do we tell people that they need -p only for
erase/read/write operations? Removing the square brackets around -p is
not correct because it implies that -p is even required for --help.
> > printf("Please note that the command line interface for flashrom has "
> >             "changed between\n"
> >       "0.9.1 and 0.9.2 and will change again before flashrom 1.0.\n"
> >       "0.9.5 and 0.9.6 and will change again before flashrom 1.0.\n"
>
> either name all (known) CLI changes or just say something like "has
> been changed before…" imho, but it is ok if you want to leave the hunk
> as it is.

Or maybe "[...] may change before flashrom 1.0"
> >       "Do not use flashrom in scripts or other automated tools "
> >         "without checking\n"
> >       "that your flashrom version won't interpret options in a "
> > Index: flashrom-programmer_no_default/flashrom.8
> >
> > --- flashrom-programmer_no_default/flashrom.8	(Revision 1547)
> > +++ flashrom-programmer_no_default/flashrom.8	(Arbeitskopie)
> > @@ -7,7 +7,7 @@
> >  \fB-v\fR <file>]
> >           [\fB-c\fR <chipname>] \
> >  [\fB-l\fR <file>]
> >     [\fB\-i\fR <image>] [\fB\-p\fR <programmername>[:<parameters>]]
> >     [\fB\-i\fR <image>] \fB\-p\fR <programmername>[:<parameters>]
>
> see above

Similar question here.
> > .SH DESCRIPTION
> >  .B flashrom
> >  is a utility for detecting, reading, writing, verifying and erasing flash
> > @@ -117,13 +117,12 @@
> >  All addresses are offsets within the file, not absolute addresses!
> >  If you only want to update the normal image in a ROM you can say:
> >  .sp
> > -.B "  flashrom --layout rom.layout --image normal -w agami_aruma.rom"
> > +.B "  flashrom -p prog --layout rom.layout --image normal -w agami_aruma.rom"
> >  .sp
> >  To update normal and fallback but leave the VGA BIOS alone, say:
> >  .sp
> > -.B "  flashrom -l rom.layout -i normal "
> > -.br
> > -.B "           -i fallback -w agami_aruma.rom"
> > +.B "  flashrom -p prog -l rom.layout -i normal \
> > +-i fallback -w agami_aruma.rom"
>
> maybe rename the image name as proposed by idwer...?

Hm. I don't really get the point of using an abstract name in a concrete
example. Could you please enlighten me?
> > .sp
> >  Currently overlapping sections are not supported.
> >  .TP
> > Index: flashrom-programmer_no_default/flashrom.c
> > ===================================================================
> > --- flashrom-programmer_no_default/flashrom.c	(Revision 1547)
> > +++ flashrom-programmer_no_default/flashrom.c	(Arbeitskopie)
> > @@ -59,6 +59,10 @@
> >  /* Is writing allowed with this programmer? */
> >  int programmer_may_write;
> >
> > +#if CONFIG_INTERNAL+CONFIG_DUMMY+CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_PONY_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV+CONFIG_LINUX_SPI < 1
> > +#error You have to enable at least one programmer!
> > +#endif
>
> why did you choose this location for the check? wouldnt it make more
> sense to have it in the makefile?

Good point. I wanted to implement it in the Makefile, but that would
have taken ~50 lines of code or even more due to the braindead language
of GNUmake files.
> > const struct programmer_entry programmer_table[] = {
> >  #if CONFIG_INTERNAL == 1
> >    {
>
> Acked-by: Stefan Tauner

Thanks! I'll wait for your comments to the questions above before I
resubmit and commit.

Regards,
Carl-Daniel