[PATCH] Change programmer selection in cli and generic code

[Carl-Daniel Hailfinger]

Change programmer selection in cli and generic code

Bugfix: Do not accept multiple conflicting --programmer selections.
Restriction: Do not accept multiple --programmer selections even if
there is no conflict.
Do not rely on exported programmer variable anymore.
programmer_init requires the programmer as first parameter.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-programmer_selection_fix/cli_classic.c
===================================================================
--- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
@@ -111,6 +111,7 @@
 #endif
    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
    int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
+	enum programmer prog = PROGRAMMER_INVALID;
    int ret = 0;

static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
@@ -258,8 +259,16 @@
 #endif
    		break;
    	case 'p':
-			for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
-				name = programmer_table[programmer].name;
+			if (prog != PROGRAMMER_INVALID) {
+				fprintf(stderr, "Error: --programmer specified "
+					"more than once. You can specify "
+					"multiple progammer parameters with "
+					"",". Please see the man page for "
+					"details.\n");
+				cli_classic_abort_usage();
+			}
+			for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
+				name = programmer_table[prog].name;
    			namelen = strlen(name);
    			if (strncmp(optarg, name, namelen) == 0) {
    				switch (optarg[namelen]) {
@@ -283,7 +292,7 @@
    				break;
    			}
    		}
-			if (programmer == PROGRAMMER_INVALID) {
+			if (prog == PROGRAMMER_INVALID) {
    			fprintf(stderr, "Error: Unknown programmer "
    				"%s.\n", optarg);
    			cli_classic_abort_usage();
@@ -332,14 +341,6 @@
    }
 #endif

-#if CONFIG_INTERNAL == 1
-	if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
-		fprintf(stderr, "Error: --mainboard requires the internal "
-				"programmer. Aborting.\n");
-		cli_classic_abort_usage();
-	}
-#endif
-
    if (chip_to_probe) {
    	for (flash = flashchips; flash && flash->name; flash++)
    		if (!strcmp(flash->name, chip_to_probe))
@@ -355,10 +356,21 @@
    	flash = NULL;
    }

+	if (prog == PROGRAMMER_INVALID)
+		prog = default_programmer;
+
+#if CONFIG_INTERNAL == 1
+	if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
+		fprintf(stderr, "Error: --mainboard requires the internal "
+				"programmer. Aborting.\n");
+		cli_classic_abort_usage();
+	}
+#endif
+
    /* FIXME: Delay calibration should happen in programmer code. */
    myusec_calibrate_delay();

-	if (programmer_init(pparam)) {
+	if (programmer_init(prog, pparam)) {
    	fprintf(stderr, "Error: Programmer initialization failed.\n");
    	ret = 1;
    	goto out_shutdown;
Index: flashrom-programmer_selection_fix/flashrom.c
===================================================================
--- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
@@ -42,10 +42,12 @@
 char *chip_to_probe = NULL;
 int verbose = 0;

+enum programmer programmer = PROGRAMMER_INVALID;
+
 #if CONFIG_INTERNAL == 1
-enum programmer programmer = PROGRAMMER_INTERNAL;
+enum programmer default_programmer = PROGRAMMER_INTERNAL;
 #elif CONFIG_DUMMY == 1
-enum programmer programmer = PROGRAMMER_DUMMY;
+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
@@ -55,7 +57,7 @@
 #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
-enum programmer programmer =
+enum programmer default_programmer =
 #if CONFIG_NIC3COM == 1
    PROGRAMMER_NIC3COM
 #endif
@@ -515,9 +517,15 @@
    return 0;
 }

-int programmer_init(char *param)
+int programmer_init(enum programmer prog, char *param)
 {
    int ret;
+
+	if (prog >= PROGRAMMER_INVALID) {
+		msg_perr("Invalid programmer specified!\n");
+		return -1;
+	}
+	programmer = prog;
    /* Initialize all programmer specific data. */
    /* Default to unlimited decode sizes. */
    max_rom_decode = (const struct decode_sizes) {
Index: flashrom-programmer_selection_fix/programmer.h
===================================================================
--- flashrom-programmer_selection_fix/programmer.h	(Revision 1427)
+++ flashrom-programmer_selection_fix/programmer.h	(Arbeitskopie)
@@ -86,6 +86,7 @@
 };

extern enum programmer programmer;
+extern enum programmer default_programmer;

struct programmer_entry {
    const char *vendor;
@@ -110,7 +111,7 @@

extern const struct programmer_entry programmer_table[];

-int programmer_init(char *param);
+int programmer_init(enum programmer prog, char *param);
 int programmer_shutdown(void);

enum bitbang_spi_master_type {

[PATCH] Change programmer selection in cli and generic code

[Stefan Tauner]

On Sun, 04 Sep 2011 02:33:38 +0200
Carl-Daniel Hailfinger  wrote:
> Change programmer selection in cli and generic code
>
> Bugfix: Do not accept multiple conflicting --programmer selections.
> Restriction: Do not accept multiple --programmer selections even if
> there is no conflict.
> Do not rely on exported programmer variable anymore.
> programmer_init requires the programmer as first parameter.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-programmer_selection_fix/cli_classic.c
>
> --- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
> @@ -111,6 +111,7 @@
>  #endif
>    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
>    int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
>
> enum programmer prog = PROGRAMMER_INVALID;
> int ret = 0;
>
> static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
>
> @@ -258,8 +259,16 @@
>  #endif
>    		break;
>    	case 'p':
> 	for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
> 		name = programmer_table[programmer].name;
> 	if (prog != PROGRAMMER_INVALID) {
> 		fprintf(stderr, "Error: --programmer specified "
> 			"more than once. You can specify "
> 			"multiple progammer parameters with "
> 			"\",\". Please see the man page for "

- You can specify multiple progammer parameters with ",".
+ You can specify multiple parameters for a progammer separated with ",".
or similar... the above one sounds like multiple programmers are
supported somehow imo.
> 			"details.\n");
> 		cli_classic_abort_usage();
> 	}
> 	for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
> 		name = programmer_table[prog].name;
> 	namelen = strlen(name);
> 	if (strncmp(optarg, name, namelen) == 0) {
> 		switch (optarg[namelen]) {
>
> @@ -283,7 +292,7 @@
>    				break;
>    			}
>    		}
> 	if (programmer == PROGRAMMER_INVALID) {
> 	if (prog == PROGRAMMER_INVALID) {
> 	fprintf(stderr, "Error: Unknown programmer "
> 		"%s.\n", optarg);
> 	cli_classic_abort_usage();
>
> @@ -332,14 +341,6 @@
>    }
>  #endif
>
> -#if CONFIG_INTERNAL == 1
> if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
> fprintf(stderr, "Error: --mainboard requires the internal "
> 		"programmer. Aborting.\n");
> cli_classic_abort_usage();
> }
>
> -#endif
> if (chip_to_probe) {
>   for (flash = flashchips; flash && flash->name; flash++)
>   	if (!strcmp(flash->name, chip_to_probe))
>
> @@ -355,10 +356,21 @@
>    	flash = NULL;
>    }
> if (prog == PROGRAMMER_INVALID)
> prog = default_programmer;
>
> +#if CONFIG_INTERNAL == 1
> if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
> fprintf(stderr, "Error: --mainboard requires the internal "
> 		"programmer. Aborting.\n");
> cli_classic_abort_usage();
> }
>
> +#endif
> /* FIXME: Delay calibration should happen in programmer code. */
>  myusec_calibrate_delay();
> if (programmer_init(pparam)) {
> if (programmer_init(prog, pparam)) {
> fprintf(stderr, "Error: Programmer initialization failed.\n");
> ret = 1;
> goto out_shutdown;
> Index: flashrom-programmer_selection_fix/flashrom.c
>
> --- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
> @@ -42,10 +42,12 @@
>  char *chip_to_probe = NULL;
>  int verbose = 0;
>
> +enum programmer programmer = PROGRAMMER_INVALID;

everything below (i.e. the declaration and definition of
default_programmer) should be moved to the cli file(s).
selecting the default programmer is in the scope of the library user.
and it slashes away one global.

this may make chromiumos cry a bit though.

if it is moved, these ifdefs could go inline into main()... i do not
suggest this, just mentioning. a static variable is probably better.
> #if CONFIG_INTERNAL == 1
> -enum programmer programmer = PROGRAMMER_INTERNAL;
> +enum programmer default_programmer = PROGRAMMER_INTERNAL;
>  #elif CONFIG_DUMMY == 1
> -enum programmer programmer = PROGRAMMER_DUMMY;
> +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
>
> @@ -55,7 +57,7 @@
>  #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

way too much convenience for a (non-existent?) theoretical package
maintainer at the cost of really existing library maintainers. i would
remove the monster if and check for a compiler flag DEFAULT_PROGRAMMER
or so first. if that's not defined, look for internal and dummy as
fall-back then and just bail out with an error similar to the one below
(but with mentioning DEFAULT_PROGRAMMER).

this would also have benefits for the packager: he could select a
default programmer no matter what other programmers are enabled.
> #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
>  #endif
> -enum programmer programmer =
> +enum programmer default_programmer =
>  #if CONFIG_NIC3COM == 1
>    PROGRAMMER_NIC3COM
>  #endif
> @@ -515,9 +517,15 @@
>    return 0;
>  }
>
> -int programmer_init(char *param)
> +int programmer_init(enum programmer prog, char *param)
>  {
>    int ret;
> if (prog >= PROGRAMMER_INVALID) {
> msg_perr("Invalid programmer specified!\n");
> return -1;
> }
> programmer = prog;
> /* Initialize all programmer specific data. */
> /* Default to unlimited decode sizes. */
> max_rom_decode = (const struct decode_sizes) {
> Index: flashrom-programmer_selection_fix/programmer.h
>
> --- flashrom-programmer_selection_fix/programmer.h	(Revision 1427)
> +++ flashrom-programmer_selection_fix/programmer.h	(Arbeitskopie)
> @@ -86,6 +86,7 @@
>  };
>
> extern enum programmer programmer;
> +extern enum programmer default_programmer;

to be removed, see above
> struct programmer_entry {
>    const char *vendor;
> @@ -110,7 +111,7 @@
>
> extern const struct programmer_entry programmer_table[];
>
> -int programmer_init(char *param);
> +int programmer_init(enum programmer prog, char *param);
>  int programmer_shutdown(void);
>
> enum bitbang_spi_master_type {

[PATCH] Change programmer selection in cli and generic code

[Bernd Blaauw]

Op 4-9-2011 13:03, Stefan Tauner schreef:
> > 			"multiple progammer parameters with "
> > 			"\",\". Please see the man page for "
> You can specify multiple progammer parameters with ",".
> You can specify multiple parameters for a progammer separated with ",".

prog*R*ammer?

sorry, nothing else to add here for now.

[PATCH] Change programmer selection in cli and generic code

[Stefan Tauner]

On Sun, 04 Sep 2011 13:15:49 +0200
Bernd Blaauw  wrote:
> Op 4-9-2011 13:03, Stefan Tauner schreef:
> > > 			"multiple progammer parameters with "
> > > 			"\",\". Please see the man page for "
> > You can specify multiple progammer parameters with ",".
> > You can specify multiple parameters for a progammer separated with ",".
>
> prog*R*ammer?

:D
overlooking such things even when editing around them is creepy. thanks!

[PATCH] Change programmer selection in cli and generic code

[Carl-Daniel Hailfinger]

Am 04.09.2011 13:03 schrieb Stefan Tauner:
> On Sun, 04 Sep 2011 02:33:38 +0200
> Carl-Daniel Hailfinger  wrote
> > Change programmer selection in cli and generic code
> >
> > Bugfix: Do not accept multiple conflicting --programmer selections.
> > Restriction: Do not accept multiple --programmer selections even if
> > there is no conflict.
> > Do not rely on exported programmer variable anymore.
> > programmer_init requires the programmer as first parameter.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-programmer_selection_fix/cli_classic.c
> >
> > --- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
> > +++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
> > @@ -258,8 +259,16 @@
> >  #endif
> >    		break;
> >    	case 'p':
> > 	for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
> > 		name = programmer_table[programmer].name;
> > 	if (prog != PROGRAMMER_INVALID) {
> > 		fprintf(stderr, "Error: --programmer specified "
> > 			"more than once. You can specify "
> > 			"multiple progammer parameters with "
> > 			"\",\". Please see the man page for "
> You can specify multiple progammer parameters with ",".
> You can specify multiple parameters for a progammer separated with ",".
>
> or similar... the above one sounds like multiple programmers are
> supported somehow imo.

Yes, I saw that problem shortly after submission, but I couldn't see a
way to phrase it in a better way. Your suggestion looks good.
> > 			"details.\n");
> > 		cli_classic_abort_usage();
> > 	}
> > 	for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
> > 		name = programmer_table[prog].name;
> > 	namelen = strlen(name);
> > 	if (strncmp(optarg, name, namelen) == 0) {
> > 		switch (optarg[namelen]) {
> > Index: flashrom-programmer_selection_fix/flashrom.c
> >
> > --- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
> > +++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
> > @@ -42,10 +42,12 @@
> >  char *chip_to_probe = NULL;
> >  int verbose = 0;
> >
> > +enum programmer programmer = PROGRAMMER_INVALID;
>
> everything below (i.e. the declaration and definition of
> default_programmer) should be moved to the cli file(s).
> selecting the default programmer is in the scope of the library user.
> and it slashes away one global.

Right.
> this may make chromiumos cry a bit though.

They duplicate the CLI anyway, so I don't see this as a big problem for
them.
> if it is moved, these ifdefs could go inline into main()... i do not
> suggest this, just mentioning. a static variable is probably better.

I prefer static.
> > #if CONFIG_INTERNAL == 1
> > -enum programmer programmer = PROGRAMMER_INTERNAL;
> > +enum programmer default_programmer = PROGRAMMER_INTERNAL;
> >  #elif CONFIG_DUMMY == 1
> > -enum programmer programmer = PROGRAMMER_DUMMY;
> > +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
> >
> > @@ -55,7 +57,7 @@
> >  #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
>
> way too much convenience for a (non-existent?) theoretical package
> maintainer at the cost of really existing library maintainers.

Really? I use it constantly during development for compile/run tests and
I'm not a package maintainer. It also serves as sanity check once a new
programmer is added.
> i would
> remove the monster if and check for a compiler flag DEFAULT_PROGRAMMER
> or so first. if that's not defined, look for internal and dummy as
> fall-back then and just bail out with an error similar to the one below
> (but with mentioning DEFAULT_PROGRAMMER).

That would make test compiles/runs with various programmers
enabled/disabled a real nightmare for me. And yes, I test all
compilation/run of all 2048 possible CONFIG_* combinations from time to
time. After all, I don't want to ship a potentially broken flashrom.
> this would also have benefits for the packager: he could select a
> default programmer no matter what other programmers are enabled.

Hm. Not so sure about those claimed benefits...
> > #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
> >  #endif
> > -enum programmer programmer =
> > +enum programmer default_programmer =
> >  #if CONFIG_NIC3COM == 1
> >    PROGRAMMER_NIC3COM
> >  #endif

New patch.

Change programmer selection in cli and generic code

Bugfix: Do not accept multiple conflicting --programmer selections.
Restriction: Do not accept multiple --programmer selections even if
there is no conflict.
Unexport the programmer variable.
programmer_init requires the programmer as first parameter.
The default programmer selection is now part of cli_classic.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-programmer_selection_fix/it87spi.c
===================================================================
--- flashrom-programmer_selection_fix/it87spi.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/it87spi.c	(Arbeitskopie)
@@ -129,10 +129,8 @@
    enter_conf_mode_ite(port);
    /* NOLDN, reg 0x24, mask out lowest bit (suspend) */
    tmp = sio_read(port, 0x24) & 0xFE;
-	/* If IT87SPI was not explicitly selected, we want to check
-	 * quickly if LPC->SPI translation is active.
-	 */
-	if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) {
+	/* Check if LPC->SPI translation is active. */
+	if (!(tmp & 0x0e)) {
    	msg_pdbg("No IT87* serial flash segment enabled.\n");
    	exit_conf_mode_ite(port);
    	/* Nothing to do. */
Index: flashrom-programmer_selection_fix/cli_classic.c
===================================================================
--- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
@@ -31,6 +31,74 @@
 #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|"
@@ -111,6 +179,7 @@
 #endif
    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
    int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
+	enum programmer prog = PROGRAMMER_INVALID;
    int ret = 0;

static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
@@ -258,8 +327,16 @@
 #endif
    		break;
    	case 'p':
-			for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
-				name = programmer_table[programmer].name;
+			if (prog != PROGRAMMER_INVALID) {
+				fprintf(stderr, "Error: --programmer specified "
+					"more than once. You can separate "
+					"multiple\nparameters for a programmer "
+					"with ",". Please see the man page "
+					"for details.\n");
+				cli_classic_abort_usage();
+			}
+			for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
+				name = programmer_table[prog].name;
    			namelen = strlen(name);
    			if (strncmp(optarg, name, namelen) == 0) {
    				switch (optarg[namelen]) {
@@ -283,7 +360,7 @@
    				break;
    			}
    		}
-			if (programmer == PROGRAMMER_INVALID) {
+			if (prog == PROGRAMMER_INVALID) {
    			fprintf(stderr, "Error: Unknown programmer "
    				"%s.\n", optarg);
    			cli_classic_abort_usage();
@@ -332,14 +409,6 @@
    }
 #endif

-#if CONFIG_INTERNAL == 1
-	if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
-		fprintf(stderr, "Error: --mainboard requires the internal "
-				"programmer. Aborting.\n");
-		cli_classic_abort_usage();
-	}
-#endif
-
    if (chip_to_probe) {
    	for (flash = flashchips; flash && flash->name; flash++)
    		if (!strcmp(flash->name, chip_to_probe))
@@ -355,10 +424,21 @@
    	flash = NULL;
    }

+	if (prog == PROGRAMMER_INVALID)
+		prog = default_programmer;
+
+#if CONFIG_INTERNAL == 1
+	if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
+		fprintf(stderr, "Error: --mainboard requires the internal "
+				"programmer. Aborting.\n");
+		cli_classic_abort_usage();
+	}
+#endif
+
    /* FIXME: Delay calibration should happen in programmer code. */
    myusec_calibrate_delay();

-	if (programmer_init(pparam)) {
+	if (programmer_init(prog, pparam)) {
    	fprintf(stderr, "Error: Programmer initialization failed.\n");
    	ret = 1;
    	goto out_shutdown;
Index: flashrom-programmer_selection_fix/flashrom.c
===================================================================
--- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
@@ -42,73 +42,7 @@
 char *chip_to_probe = NULL;
 int verbose = 0;

-#if CONFIG_INTERNAL == 1
-enum programmer programmer = PROGRAMMER_INTERNAL;
-#elif CONFIG_DUMMY == 1
-enum programmer 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
-enum programmer 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 enum programmer programmer = PROGRAMMER_INVALID;

static char *programmer_param = NULL;

@@ -515,9 +449,15 @@
    return 0;
 }

-int programmer_init(char *param)
+int programmer_init(enum programmer prog, char *param)
 {
    int ret;
+
+	if (prog >= PROGRAMMER_INVALID) {
+		msg_perr("Invalid programmer specified!\n");
+		return -1;
+	}
+	programmer = prog;
    /* Initialize all programmer specific data. */
    /* Default to unlimited decode sizes. */
    max_rom_decode = (const struct decode_sizes) {
Index: flashrom-programmer_selection_fix/programmer.h
===================================================================
--- flashrom-programmer_selection_fix/programmer.h	(Revision 1427)
+++ flashrom-programmer_selection_fix/programmer.h	(Arbeitskopie)
@@ -85,8 +85,6 @@
    PROGRAMMER_INVALID /* This must always be the last entry. */
 };

-extern enum programmer programmer;
-
 struct programmer_entry {
    const char *vendor;
    const char *name;
@@ -110,7 +108,7 @@

extern const struct programmer_entry programmer_table[];

-int programmer_init(char *param);
+int programmer_init(enum programmer prog, char *param);
 int programmer_shutdown(void);

enum bitbang_spi_master_type {

[PATCH] Change programmer selection in cli and generic code

[Stefan Tauner]

On Mon, 05 Sep 2011 01:15:35 +0200
Carl-Daniel Hailfinger  wrote:
> > i would
> > remove the monster if and check for a compiler flag DEFAULT_PROGRAMMER
> > or so first. if that's not defined, look for internal and dummy as
> > fall-back then and just bail out with an error similar to the one below
> > (but with mentioning DEFAULT_PROGRAMMER).
>
> That would make test compiles/runs with various programmers
> enabled/disabled a real nightmare for me. And yes, I test all
> compilation/run of all 2048 possible CONFIG_* combinations from time to
> time. After all, I don't want to ship a potentially broken flashrom.

why? when you set the enabled programmers with a script you could also
set the default programmer to one of them programmatically.
will look at the second revision later.

[PATCH] Change programmer selection in cli and generic code

[Stefan Tauner]

On Mon, 05 Sep 2011 01:15:35 +0200
Carl-Daniel Hailfinger  wrote:
> New patch.
>
> Change programmer selection in cli and generic code
>
> Bugfix: Do not accept multiple conflicting --programmer selections.
> Restriction: Do not accept multiple --programmer selections even if
> there is no conflict.
> Unexport the programmer variable.
> programmer_init requires the programmer as first parameter.
> The default programmer selection is now part of cli_classic.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-programmer_selection_fix/it87spi.c
>
> --- flashrom-programmer_selection_fix/it87spi.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/it87spi.c	(Arbeitskopie)
> @@ -129,10 +129,8 @@
>    enter_conf_mode_ite(port);
>    /* NOLDN, reg 0x24, mask out lowest bit (suspend) */
>    tmp = sio_read(port, 0x24) & 0xFE;
> /* If IT87SPI was not explicitly selected, we want to check
> * quickly if LPC->SPI translation is active.
> */
> if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) {
> /* Check if LPC->SPI translation is active. */
> if (!(tmp & 0x0e)) {

just curious: why was this needed/wanted before?
> […]
> Index: flashrom-programmer_selection_fix/flashrom.c
> ===================================================================
> --- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
> […]
> @@ -515,9 +449,15 @@
>    return 0;
>  }
>
> -int programmer_init(char *param)
> +int programmer_init(enum programmer prog, char *param)
>  {
>    int ret;
> if (prog >= PROGRAMMER_INVALID) {

should we also check against < 0? enums are based on int. the default
starting point is 0 (if the first entry does not have a specific value
assigned with =), but i guess one could cast (enum programmer)-1 or so?
untested and maybe stupid... :)
> msg_perr("Invalid programmer specified!\n");
> return -1;

why so negative? ;)
> […]

apart from that and our default programmer dispute it looks good to me.
so please think of it as
Acked-by: Stefan Tauner

[PATCH] Change programmer selection in cli and generic code

[Carl-Daniel Hailfinger]

Am 05.09.2011 23:57 schrieb Stefan Tauner:
> On Mon, 05 Sep 2011 01:15:35 +0200
> Carl-Daniel Hailfinger  wrote:
> > New patch.
> >
> > Change programmer selection in cli and generic code
> >
> > Bugfix: Do not accept multiple conflicting --programmer selections.
> > Restriction: Do not accept multiple --programmer selections even if
> > there is no conflict.
> > Unexport the programmer variable.
> > programmer_init requires the programmer as first parameter.
> > The default programmer selection is now part of cli_classic.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-programmer_selection_fix/it87spi.c
> >
> > --- flashrom-programmer_selection_fix/it87spi.c	(Revision 1427)
> > +++ flashrom-programmer_selection_fix/it87spi.c	(Arbeitskopie)
> > @@ -129,10 +129,8 @@
> >    enter_conf_mode_ite(port);
> >    /* NOLDN, reg 0x24, mask out lowest bit (suspend) */
> >    tmp = sio_read(port, 0x24) & 0xFE;
> > /* If IT87SPI was not explicitly selected, we want to check
> > * quickly if LPC->SPI translation is active.
> > */
> > if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) {
> > /* Check if LPC->SPI translation is active. */
> > if (!(tmp & 0x0e)) {
>
> just curious: why was this needed/wanted before?

This was a hunk I forgot to remove when I killed the separate it87spi
programmer. That piece of code was the only location which needed a
global programmer variable.
> > […]
> > Index: flashrom-programmer_selection_fix/flashrom.c
> > ===================================================================
> > --- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
> > +++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
> > […]
> > @@ -515,9 +449,15 @@
> >    return 0;
> >  }
> >
> > -int programmer_init(char *param)
> > +int programmer_init(enum programmer prog, char *param)
> >  {
> >    int ret;
> > if (prog >= PROGRAMMER_INVALID) {
>
> should we also check against < 0? enums are based on int.

Not sure about that:
http://software.intel.com/en-us/articles/strict-ansi-switch-in-linux-and-mac...
Are they really guaranteed to be signed int? If not, some compilers will
warn about "comparison always yields false".
> the default
> starting point is 0 (if the first entry does not have a specific value
> assigned with =), but i guess one could cast (enum programmer)-1 or so?
> untested and maybe stupid... :)

I thought about it, and decided to only check for >= PROGRAMMER_INVALID.
> > msg_perr("Invalid programmer specified!\n");
> > return -1;
>
> why so negative? ;)
> > […]
>
> apart from that and our default programmer dispute it looks good to me.
> so please think of it as
> Acked-by: Stefan Tauner

Thanks, I'll commit in the next ~24 hours. If you're unhappy about my
enum answer, please tell me and I'll dig into the standard again.

Regards,
Carl-Daniel

[PATCH] Change programmer selection in cli and generic code

[Stefan Tauner]

On Tue, 06 Sep 2011 02:20:31 +0200
Carl-Daniel Hailfinger  wrote:
> > should we also check against < 0? enums are based on int.
>
> Not sure about that:
> http://software.intel.com/en-us/articles/strict-ansi-switch-in-linux-and-mac...
> Are they really guaranteed to be signed int? If not, some compilers will
> warn about "comparison always yields false".

6.7.2.2 Enumeration specifiers
paragraph 2:
"The expression that defines the value of an enumeration constant shall
be an integer constant expression that has a value representable as an int."
but that does only restrict the "input" type of the "initializers".

the really important part is paragraph 4:
"Each enumerated type shall be compatible with char, a signed integer
type, or an unsigned integer type. The choice of type is
implementation-defined, but shall be capable of representing the values
of all the members of the enumeration. The enumerated type is
incomplete until after the } that terminates the list of enumerator
declarations."

so eventually this is another implementation-defined behavior, hurray.

gcc does this:
http://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Structures-unions-enumerations-a...

define a precondition for programmer_init that parameter prog must be
in the range defined by the enum and else the behavior is undefined,
and think of the > part of the check as a user-friendly bonus :)
a detailed description of what the functions exported by libflashrom do
is required anyway...

i am fine with the current implementation.

[PATCH] Change programmer selection in cli and generic code

[Carl-Daniel Hailfinger]

Am 05.09.2011 23:57 schrieb Stefan Tauner:
> Acked-by: Stefan Tauner

Thanks, committed in r1433.

Regards,
Carl-Daniel

[PATCH] Change programmer selection in cli and generic code

[Uwe Hermann]

On Mon, Sep 05, 2011 at 01:15:35AM +0200, Carl-Daniel Hailfinger wrote:
> Change programmer selection in cli and generic code
>
> Bugfix: Do not accept multiple conflicting --programmer selections.
> Restriction: Do not accept multiple --programmer selections even if
> there is no conflict.
> Unexport the programmer variable.
> programmer_init requires the programmer as first parameter.
> The default programmer selection is now part of cli_classic.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Uwe Hermann

+1 for making 'programmer' static.

Uwe.