Re: [flashrom] [PATCH 3/3] [RFC] Unify usbdev_status and pcidev_status into dev_status.

[Carl-Daniel Hailfinger]

Am 17.11.2012 20:09 schrieb Stefan Tauner:
> usbdev_status was created for the ft2232 programmer. Its IDs are semantically
> different because they indicate USB instead of PCI IDs, but apart from that both
> data structures are equal. This change makes life easier for everything involved
> in handling and printing the status of devices that is noted in that structure.
>
> It is still possible to distinguish between PCI and USB devices by using the
> struct programmer's type field. It still seems a bit hacky, but i think we are better
> off with it anyway. If we really would want to distinguish between the different
> types of IDs i'd rater make the IDs a union of different ID types inside struct
> dev_status.
>
> Signed-off-by: Stefan Tauner

Thanks a lot for this cleanup! It kills substantial amounts of
duplicated code.

Acked-by: Carl-Daniel Hailfinger

Apart from the "devs" vs. "devices" naming (which you already resolved
in your other reply), there is only one minor nit:
> diff --git a/programmer.h b/programmer.h
> index 21fa707..90033a9 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -243,12 +250,6 @@ int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data);
>  int rpci_write_long(struct pci_dev *dev, int reg, uint32_t data);
>  #endif
>
> -/* print.c */
> -#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
> -/* Not needed for CONFIG_INTERNAL, but for all other PCI-based programmers. */
> -void print_supported_pcidevs(const struct pcidev_status *devs);
> -#endif
>
> struct usbdev_status {
>    uint16_t vendor_id;
>    uint16_t device_id;
> @@ -264,6 +265,12 @@ struct pcidev_status {
>    const char *device_name;
>  };
>
> +/* print.c */
> +#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
> +/* Not needed for CONFIG_INTERNAL, but for all other PCI-based programmers. */
> +void print_supported_pcidevs(const struct pcidev_status *devs);
> +#endif
>
> #if CONFIG_INTERNAL == 1
>  /* board_enable.c */
>  int board_parse_parameter(const char *boardstring, const char **vendor, const char **model);

AFAICS prints_supported_pcidevs has been killed by this patch, no need
to keep its declaration in a header.

Regards,
Carl-Daniel