[flashrom] [PATCH 1/3] Refactor PCI and USB device status printing.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sun Nov 18 20:45:38 CET 2012
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 <stefan.tauner at student.tuwien.ac.at>
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.
> .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.
> + /* 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.
> +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.
> + 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 "
> + "flashrom at flashrom.org\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.
> + 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
> @@ -90,9 +90,21 @@ enum programmer {
> PROGRAMMER_INVALID /* This must always be the last entry. */
> };
>
> +enum programmer_type {
> + PCI = 1, /* to detect uninitialized values */
> + USB,
> + OTHER,
> +};
> +
> struct programmer_entry {
> const char *vendor;
> const char *name;
> + const enum programmer_type type;
> + union {
> + const struct pcidev_status *const pci;
> + const struct usbdev_status *const usb;
> + const char *const note;
Neat!
> + } devices;
>
> int (*init) (void);
>
> @@ -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.
> +
> #if CONFIG_INTERNAL == 1
> /* board_enable.c */
> int board_parse_parameter(const char *boardstring, const char **vendor, const char **model);
> @@ -422,13 +442,6 @@ extern const struct pcidev_status ata_hpt[];
>
> /* ft2232_spi.c */
> #if CONFIG_FT2232_SPI == 1
> -struct usbdev_status {
> - uint16_t vendor_id;
> - uint16_t device_id;
> - const enum test_state status;
> - const char *vendor_name;
> - const char *device_name;
> -};
> int ft2232_spi_init(void);
> extern const struct usbdev_status devs_ft2232spi[];
> void print_supported_usbdevs(const struct usbdev_status *devs);
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list