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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Sat Jul 21 00:51:48 CEST 2012


Am 17.07.2012 00:42 schrieb Stefan Tauner:
> On Mon, 16 Jul 2012 23:28:22 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net>
>>
>> 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 <stefan.tauner at student.tuwien.ac.at>

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

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list