On Sun, 18 Nov 2012 20:45:38 +0100 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 17.11.2012 20:09 schrieb Stefan Tauner: > > To be able to get rid of lots of #ifdefs and centralize programmer-specific > > data more... > > - introduce two new fields to struct programmer_entry, namely > > enum type (OTHER, USB, PCI) and union devices (pcidev_status, > > usbdev_status > > or char *note). > > - use those fields to generate device listings in print.c and print_wiki.c. > > > > Bonus: add printing of USB devices to print_wiki.c and count supported PCI > > and USB devices. > > > > Signed-off-by: Stefan Tauner <[email protected]> > > Good idea. > I have a few comments, otherwise it looks OK. > Note: I have not checked the wiki output, I only reviewed the code. I > trust you have checked the wiki output for changes. > > > --- > > flashrom.c | 48 ++++++++++++++++++- > > ft2232_spi.c | 13 ------ > > pcidev.c | 13 ------ > > print.c | 148 > > +++++++++++++++++++--------------------------------------- > > print_wiki.c | 141 ++++++++++++++++++++++++++++++++++--------------------- > > programmer.h | 41 ++++++++++------ > > 6 files changed, 211 insertions(+), 193 deletions(-) > > > > diff --git a/flashrom.c b/flashrom.c > > index 2299b06..d873276 100644 > > --- a/flashrom.c > > +++ b/flashrom.c > > @@ -64,6 +64,8 @@ const struct programmer_entry programmer_table[] = { > > #if CONFIG_INTERNAL == 1 > > { > > .name = "internal", > > + .type = OTHER, > > + .devices.note = NULL, > > .init = internal_init, > > .map_flash_region = physmap, > > .unmap_flash_region = physunmap, > > @@ -84,6 +89,8 @@ const struct programmer_entry programmer_table[] = { > > #if CONFIG_NIC3COM == 1 > > { > > .name = "nic3com", > > + .type = PCI, > > + .devices.pci = nics_3com, > > If we reference the device list here, can we kill the code in each > programmer which mentions the device list explicitly and use > pgm->devices->pci there? Otherwise there's a good chance someone will > forget one or the other reference and wonder why --list and actual > recognized programmer devices differ. no idea if it is possible, but i have put it on my todo list. i am not sure we really want these programmer-specific bits here though. i would rather want to declare the complete programmer structs in the programmer files and just add pointers to them to this array... i think... now, and need to look at it in detail. in any case this is nothing we want to do soon imho. i (i.e. the todo.txt :) wont forget about it. > > .init = nic3com_init, > > .map_flash_region = fallback_map, > > .unmap_flash_region = fallback_unmap, > > @@ -185,6 +212,9 @@ const struct programmer_entry programmer_table[] = { > > #if CONFIG_DEDIPROG == 1 > > { > > .name = "dediprog", > > + .type = OTHER, > > Regression. The old code lists all devices. Not the one in trunk: "Supported devices for the dediprog programmer: Dediprog SF100" the corresponding hunk contains: -#if CONFIG_DEDIPROG == 1 - msg_ginfo("\nSupported devices for the %s programmer:\n", - programmer_table[PROGRAMMER_DEDIPROG].name); - /* FIXME */ - msg_ginfo("Dediprog SF100\n"); -#endif > > > + /* FIXME */ > > + .devices.note = "Dediprog SF100\n", > > .init = dediprog_init, > > .map_flash_region = fallback_map, > > .unmap_flash_region = fallback_unmap, > > diff --git a/print.c b/print.c > > index d7bdbfe..4b23ca0 100644 > > --- a/print.c > > +++ b/print.c > > @@ -433,8 +433,32 @@ static void print_supported_boards_helper(const struct > > board_info *boards, > > } > > #endif > > > > For consistency, we should introduce NEED_USB here. That would need > Makefile glue, but I think it's worth it, especially now that there are > Dediprog patches which change the libusb-0 dependency to a libusb-1 > dependency. still waiting for that... :/ > > +void print_supported_usbdevs(const struct usbdev_status *devs) > > +{ > > + int i; > > + > > + msg_pinfo("USB devices:\n"); > > + for (i = 0; devs[i].vendor_name != NULL; i++) { > > + msg_pinfo("%s %s [%04x:%04x]%s\n", devs[i].vendor_name, > > devs[i].device_name, devs[i].vendor_id, > > + devs[i].device_id, (devs[i].status == NT) ? " > > (untested)" : ""); > > + } > > +} > > + > > +#if NEED_PCI == 1 > > +void print_supported_pcidevs(const struct pcidev_status *devs) > > +{ > > + int i; > > + > > + for (i = 0; devs[i].vendor_name != NULL; i++) { > > + msg_pinfo("%s %s [%04x:%04x]%s\n", devs[i].vendor_name, > > devs[i].device_name, devs[i].vendor_id, > > + devs[i].device_id, (devs[i].status == NT) ? " > > (untested)" : ""); > > + } > > +} > > +#endif > > + > > int print_supported(void) > > { > > + unsigned int i; > > if (print_supported_chips()) > > return 1; > > > > @@ -449,107 +473,31 @@ int print_supported(void) > > msg_ginfo("\n"); > > print_supported_boards_helper(laptops_known, "laptops"); > > #endif > > -#if CONFIG_DUMMY == 1 > > - msg_ginfo("\nSupported devices for the %s programmer:\n", > > - programmer_table[PROGRAMMER_DUMMY].name); > > - /* FIXME */ > > - msg_ginfo("Dummy device, does nothing and logs all accesses\n"); > > -#endif > > [...] > > -#if CONFIG_LINUX_SPI == 1 > > - msg_ginfo("\nSupported devices for the %s programmer:\n", > > - programmer_table[PROGRAMMER_LINUX_SPI].name); > > - msg_ginfo("Device files /dev/spidev*.*\n"); > > + for (i = 0; i < PROGRAMMER_INVALID; i++) { > > + const struct programmer_entry prog = programmer_table[i]; > > + switch (prog.type) { > > NEED_USB again. > > > > + case USB: > > + msg_ginfo("\nSupported USB devices for the %s > > programmer:\n", prog.name); > > + print_supported_usbdevs(prog.devices.usb); > > + break; > > +#if NEED_PCI == 1 > > + case PCI: > > + msg_ginfo("\nSupported PCI devices for the %s > > programmer:\n", prog.name); > > + print_supported_pcidevs(prog.devices.pci); > > + break; > > #endif > > + case OTHER: > > + if (prog.devices.note == NULL) > > + break; > > Interesting logic. I'm not saying it is wrong, but it's not immediately > obvious that .note=NULL means the programmer won't be listed at all. I > fear this might be forgotten by future programmer driver authors, but > then again I should update my programmer code generation script and push > it to svn so this won't be a problem in the future. hm well, it is more like an error/fail safe case, i just did not bother to add a check for it yet (or at least i thought so ;). i have done so now and there is a programmer loop now in selfcheck() that checks for valid (i.e. non-null for most of the cases) - name - vendor... wait what? our programmers have a vendor? no! they do not! i have removed this cruft from the struct. i wonder why you did not notice that when writing your programmer generator. - type - devices list/note - init function - delay function - map_flash_region function - unmap_flash_region function basically every field is checked. and then i found out why i really did what i did back then. the internal programmer is special and to allow the print code to easily skip it i added the note == null option. damn :) i have now added a special case for the internal programmer in the checking code so that it is the only one where type = OTHER and note = NULL is expected and allowed. > > + msg_ginfo("\nSupported devices for the %s > > programmer:\n", prog.name); > > + msg_ginfo("%s", prog.devices.note); > > + break; > > + default: > > + msg_gerr("\n%s: %s: Uninitialized programmer type! > > Please report a bug at " > > + "[email protected]\n", __func__, > > prog.name); > > + break; > > + } > > + } > > return 0; > > } > > > > diff --git a/print_wiki.c b/print_wiki.c > > index 617053c..17e2e85 100644 > > --- a/print_wiki.c > > +++ b/print_wiki.c > > @@ -302,9 +298,18 @@ static void print_supported_chips_wiki(int cols) > > printf("|}\n\n"); > > } > > > > -/* Not needed for CONFIG_INTERNAL, but for all other PCI-based > > programmers. */ > > -#if > > CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > > >= 1 > > -static void print_supported_pcidevs_wiki(const struct pcidev_status *devs) > > +/* Following functions are not needed when no PCI/USB programmers are > > compiled in, > > + * but since print_wiki code has no size constraints we include it > > unconditionally. */ > > Heh. > > > > +static int count_supported_pcidevs_wiki(const struct pcidev_status *devs) > > +{ > > + unsigned int count = 0; > > + unsigned int i = 0; > > + for (i = 0; devs[i].vendor_name != NULL; i++) > > AFAICS this should segfault in case a driver has prog.type=PCI and > prog.devices.pci==NULL. Not a problem, but maybe we should catch this > case (as well as the USB case) in a selftest function during startup? > That would also help during programmer development. ah you suggested that... my well-behaved unconsciousness made me implement that before i re-read the complete mail :) > > + count++; > > + return count; > > +} > > + > > +static void print_supported_pcidevs_wiki_helper(const struct pcidev_status > > *devs) > > { > > int i = 0; > > static int c = 0; > > @@ -313,14 +318,81 @@ static void print_supported_pcidevs_wiki(const struct > > pcidev_status *devs) > > c = !c; > > > > for (i = 0; devs[i].vendor_name != NULL; i++) { > > - printf("|- bgcolor=\"#%s\"\n| %s || %s || " > > - "%04x:%04x || {{%s}}\n", (c) ? "eeeeee" : "dddddd", > > - devs[i].vendor_name, devs[i].device_name, > > - devs[i].vendor_id, devs[i].device_id, > > + printf("|- bgcolor=\"#%s\"\n| %s || %s || %04x:%04x || > > {{%s}}\n", (c) ? "eeeeee" : "dddddd", > > + devs[i].vendor_name, devs[i].device_name, > > devs[i].vendor_id, devs[i].device_id, > > (devs[i].status == NT) ? "?3" : "OK"); > > } > > } > > -#endif > > + > > +static int count_supported_usbdevs_wiki(const struct usbdev_status *devs) > > +{ > > + unsigned int count = 0; > > + unsigned int i = 0; > > + for (i = 0; devs[i].vendor_name != NULL; i++) > > + count++; > > + return count; > > +} > > + > > +static void print_supported_usbdevs_wiki_helper(const struct usbdev_status > > *devs) > > +{ > > + int i = 0; > > + static int c = 0; > > + > > + /* Alternate colors if the vendor changes. */ > > + c = !c; > > + > > + for (i = 0; devs[i].vendor_name != NULL; i++) { > > + printf("|- bgcolor=\"#%s\"\n| %s || %s || %04x:%04x || > > {{%s}}\n", (c) ? "eeeeee" : "dddddd", > > + devs[i].vendor_name, devs[i].device_name, > > devs[i].vendor_id, devs[i].device_id, > > + (devs[i].status == NT) ? "?3" : "OK"); > > + } > > +} > > + > > +static void print_supported_devs_wiki() > > +{ > > + unsigned int pci_count = 0; > > + unsigned int usb_count = 0; > > + unsigned int i; > > + > > + for (i = 0; i < PROGRAMMER_INVALID; i++) { > > + const struct programmer_entry prog = programmer_table[i]; > > + switch (prog.type) { > > + case USB: > > + usb_count += > > count_supported_usbdevs_wiki(prog.devices.usb); > > + break; > > + case PCI: > > + pci_count += > > count_supported_pcidevs_wiki(prog.devices.pci); > > + break; > > + case OTHER: > > + default: > > + break; > > + } > > + } > > + > > + printf("\n== PCI Devices ==\n\n" > > + "Total amount of supported PCI devices flashrom can use as a > > programmer: '''%d'''\n\n" > > + "{%s%s", pci_count, th_start, programmer_th); > > + > > + for (i = 0; i < PROGRAMMER_INVALID; i++) { > > + const struct programmer_entry prog = programmer_table[i]; > > + if (prog.type == PCI) { > > + print_supported_pcidevs_wiki_helper(prog.devices.pci); > > + } > > + } > > + printf("\n|}\n\n|}\n"); > > + > > + printf("\n== USB Devices ==\n\n" > > + "Total amount of supported USB devices flashrom can use as a > > programmer: '''%d'''\n\n" > > + "{%s%s", usb_count, th_start, programmer_th); > > + > > + for (i = 0; i < PROGRAMMER_INVALID; i++) { > > + const struct programmer_entry prog = programmer_table[i]; > > + if (prog.type == USB) { > > + print_supported_usbdevs_wiki_helper(prog.devices.usb); > > + } > > + } > > + printf("\n|}\n\n|}\n"); > > +} > > > > void print_supported_wiki(void) > > { > > @@ -332,41 +404,6 @@ void print_supported_wiki(void) > > print_supported_chipsets_wiki(3); > > print_supported_boards_wiki(); > > #endif > > - printf("%s%s%s", programmer_intro, th_start, programmer_th); > > - > > -#if CONFIG_NIC3COM == 1 > > - print_supported_pcidevs_wiki(nics_3com); > > -#endif > > -#if CONFIG_NICREALTEK == 1 > > - print_supported_pcidevs_wiki(nics_realtek); > > -#endif > > -#if CONFIG_NICNATSEMI == 1 > > - print_supported_pcidevs_wiki(nics_natsemi); > > -#endif > > -#if CONFIG_GFXNVIDIA == 1 > > - print_supported_pcidevs_wiki(gfx_nvidia); > > -#endif > > -#if CONFIG_DRKAISER == 1 > > - print_supported_pcidevs_wiki(drkaiser_pcidev); > > -#endif > > -#if CONFIG_SATASII == 1 > > - print_supported_pcidevs_wiki(satas_sii); > > -#endif > > -#if CONFIG_ATAHPT == 1 > > - print_supported_pcidevs_wiki(ata_hpt); > > -#endif > > -#if CONFIG_NICINTEL == 1 > > - print_supported_pcidevs_wiki(nics_intel); > > -#endif > > -#if CONFIG_NICINTEL_SPI == 1 > > - print_supported_pcidevs_wiki(nics_intel_spi); > > -#endif > > -#if CONFIG_OGP_SPI == 1 > > - print_supported_pcidevs_wiki(ogp_spi); > > -#endif > > -#if CONFIG_SATAMV == 1 > > - print_supported_pcidevs_wiki(satas_mv); > > -#endif > > - printf("\n|}\n\n|}\n"); > > + print_supported_devs_wiki(); > > } > > > > diff --git a/programmer.h b/programmer.h > > index dedec67..21fa707 100644 > > --- a/programmer.h > > +++ b/programmer.h > > @@ -221,13 +233,6 @@ void internal_delay(int usecs); > > extern uint32_t io_base_addr; > > extern struct pci_access *pacc; > > extern struct pci_dev *pcidev_dev; > > -struct pcidev_status { > > - uint16_t vendor_id; > > - uint16_t device_id; > > - const enum test_state status; > > - const char *vendor_name; > > - const char *device_name; > > -}; > > uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); > > uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); > > /* rpci_write_* are reversible writes. The original PCI config space > > register > > @@ -244,6 +249,21 @@ int rpci_write_long(struct pci_dev *dev, int reg, > > uint32_t data); > > void print_supported_pcidevs(const struct pcidev_status *devs); > > #endif > > > > +struct usbdev_status { > > + uint16_t vendor_id; > > + uint16_t device_id; > > + const enum test_state status; > > + const char *vendor_name; > > + const char *device_name; > > +}; > > +struct pcidev_status { > > + uint16_t vendor_id; > > + uint16_t device_id; > > + const enum test_state status; > > + const char *vendor_name; > > + const char *device_name; > > +}; > > Hm. You moved the struct pcidev_status and usbdev_status outside the > #ifdef protecting it. Given that this has no impact on code generation, > I don't have a big problem with it. > > This is a question we should eventually decide: Do we want unused struct > declarations inside #ifdef or not? Such a decision should not hold up > merging this code, I just wanted to mention it. didnt that come up already once? dunno. my rule of thumb is: if it would require to add a new #ifdef that was not there before, then skip that and add the definition without it, else add it to the existing #ifdef. in this case however it is actually needed because i include the wiki print functions for devices unconditionally, and in theory that would not compile then (likewise for usbdev_status): CONFIG_INTERNAL=no CONFIG_SERPROG=yes CONFIG_RAYER_SPI=no CONFIG_PONY_SPI=no CONFIG_NIC3COM=no CONFIG_GFXNVIDIA=no CONFIG_SATASII=no CONFIG_FT2232_SPI=no CONFIG_DUMMY=no CONFIG_DRKAISER=no CONFIG_NICREALTEK=no CONFIG_NICINTEL=no CONFIG_NICINTEL_SPI=no CONFIG_OGP_SPI=no CONFIG_BUSPIRATE_SPI=no CONFIG_SATAMV=no CONFIG_LINUX_SPI=no CONFIG_PRINT_WIKI=yes make cc -MMD -Os -Wall -Wshadow -Werror -D'CONFIG_DEFAULT_PROGRAMMER=PROGRAMMER_INVALID' -D'CONFIG_SERPROG=1' -D'CONFIG_PRINT_WIKI=1' -D'HAVE_UTSNAME=1' -D'FLASHROM_VERSION="0.9.6.1-r1627"' -o print_wiki.o -c print_wiki.c print_wiki.c: In function ‘count_supported_pcidevs_wiki’: print_wiki.c:307:2: error: invalid use of undefined type ‘struct pcidev_status’ yes, i am mad. :P the code will come later together with the other patches (refactored/partially merged). -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
