Am 04.09.2011 13:03 schrieb Stefan Tauner:
> On Sun, 04 Sep 2011 02:33:38 +0200
> Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]>
>>
>> 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 <[email protected]>
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 {
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
