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

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Tue Jul 17 00:42:35 CEST 2012 

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)
> @@ -31,74 +31,6 @@
>  #include "flashchips.h"
>  #include "programmer.h"
>  
> -#if CONFIG_INTERNAL == 1
> -static enum programmer default_programmer = PROGRAMMER_INTERNAL;
> -#elif CONFIG_DUMMY == 1
> -static enum programmer default_programmer = PROGRAMMER_DUMMY;
> -#else
> -/* If neither internal nor dummy are selected, we must pick a sensible default.
> - * Since there is no reason to prefer a particular external programmer, we fail
> - * if more than one of them is selected. If only one is selected, it is clear
> - * that the user wants that one to become the default.
> - */
> -#if 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_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1
> -#error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
> -#endif
> -static enum programmer default_programmer =
> -#if CONFIG_NIC3COM == 1
> -	PROGRAMMER_NIC3COM
> -#endif
> -#if CONFIG_NICREALTEK == 1
> -	PROGRAMMER_NICREALTEK
> -#endif
> -#if CONFIG_NICNATSEMI == 1
> -	PROGRAMMER_NICNATSEMI
> -#endif
> -#if CONFIG_GFXNVIDIA == 1
> -	PROGRAMMER_GFXNVIDIA
> -#endif
> -#if CONFIG_DRKAISER == 1
> -	PROGRAMMER_DRKAISER
> -#endif
> -#if CONFIG_SATASII == 1
> -	PROGRAMMER_SATASII
> -#endif
> -#if CONFIG_ATAHPT == 1
> -	PROGRAMMER_ATAHPT
> -#endif
> -#if CONFIG_FT2232_SPI == 1
> -	PROGRAMMER_FT2232_SPI
> -#endif
> -#if CONFIG_SERPROG == 1
> -	PROGRAMMER_SERPROG
> -#endif
> -#if CONFIG_BUSPIRATE_SPI == 1
> -	PROGRAMMER_BUSPIRATE_SPI
> -#endif
> -#if CONFIG_DEDIPROG == 1
> -	PROGRAMMER_DEDIPROG
> -#endif
> -#if CONFIG_RAYER_SPI == 1
> -	PROGRAMMER_RAYER_SPI
> -#endif
> -#if CONFIG_NICINTEL == 1
> -	PROGRAMMER_NICINTEL
> -#endif
> -#if CONFIG_NICINTEL_SPI == 1
> -	PROGRAMMER_NICINTEL_SPI
> -#endif
> -#if CONFIG_OGP_SPI == 1
> -	PROGRAMMER_OGP_SPI
> -#endif
> -#if CONFIG_SATAMV == 1
> -	PROGRAMMER_SATAMV
> -#endif
> -#if CONFIG_LINUX_SPI == 1
> -	PROGRAMMER_LINUX_SPI
> -#endif
> -;
> -#endif
> -
>  static void cli_classic_usage(const char *name)
>  {
>  	printf("Usage: flashrom [-n] [-V] [-f] [-h|-R|-L|"
> @@ -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).

>  
>  	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.

>  	       "Do not use flashrom in scripts or other automated tools "
>  	         "without checking\n"
>  	       "that your flashrom version won't interpret options in a "
> @@ -360,8 +292,9 @@
>  				}
>  			}
>  			if (prog == PROGRAMMER_INVALID) {
> -				fprintf(stderr, "Error: Unknown programmer "
> -					"%s.\n", optarg);
> +				fprintf(stderr, "Error: Unknown programmer \"%s\". Valid choices are:\n",
> +					optarg);
> +				list_programmers_linebreak(0, 80, 0);
>  				cli_classic_abort_usage();
>  			}
>  			break;
> @@ -468,8 +401,13 @@
>  		flash = NULL;
>  	}
>  
> -	if (prog == PROGRAMMER_INVALID)
> -		prog = default_programmer;
> +	if (prog == PROGRAMMER_INVALID) {
> +		msg_perr("Please select a programmer with the --programmer parameter.\n"
> +			 "Valid choices are:\n");
> +		list_programmers_linebreak(0, 80, 0);
> +		ret = 1;
> +		goto out;
> +	}
>  
>  	/* FIXME: Delay calibration should happen in programmer code. */
>  	myusec_calibrate_delay();
> 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

>  .SH DESCRIPTION
>  .B flashrom
>  is a utility for detecting, reading, writing, verifying and erasing flash
> @@ -63,7 +63,7 @@
>  feel that the time for verification takes too long.
>  .sp
>  Typical usage is:
> -.B "flashrom \-n \-w <file>"
> +.B "flashrom \-p prog \-n \-w <file>"
>  .sp
>  This option is only useful in combination with
>  .BR \-\-write .
> @@ -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...?

>  .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?

>  const struct programmer_entry programmer_table[] = {
>  #if CONFIG_INTERNAL == 1
>  	{

Acked-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner

More information about the flashrom mailing list