Re: [flashrom] [7/7, RFC] Unify usbdev_status and pcidev_status into dev_status.

[Kyösti Mälkki]

Mostly looked good to me, I did not rebase or compile.
Find a few comments inlined below.

Kyösti

On Sat, 2012-03-03 at 19:11 +0000, Stefan Tauner wrote:
> diff --git a/programmer.h b/programmer.h
> index 9f90fe5..b52727e 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -93,15 +93,22 @@ enum programmer_type {
>    OTHER,
>  };
>
> +struct dev_status {
> uint16_t vendor_id;
> uint16_t device_id;
> const enum test_state status;
> const char *vendor_name;
> const char *device_name;
>
> +};

I find the name "dev_status" odd, these are the arrays for listing
and probing the VID/DID too, and the "status" is just documentation.
> struct programmer_entry {
>    const char *vendor;
>    const char *name;
>    enum programmer_type type;
>    union {
> const struct pcidev_status *const pci;
> const struct usbdev_status *const usb;
> const struct dev_status *const status;
> const char * const note;
> } devices;
> } devs;

I don't know why the "devices->devs" rename is here. Maybe add comment
that "type" is the selector for the union. It might be confused for
some internal/nic/sata enumeration.
> /* 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
> -void print_supported_pcidevs(const struct pcidev_status *devs);
> +void print_supported_devs(const struct dev_status *devs);
>  #endif

That #if does not look nice.

[7/7, RFC] Unify usbdev_status and pcidev_status into dev_status.

[Stefan Tauner]

On Fri, 16 Nov 2012 22:48:22 +0200
Kyösti Mälkki  wrote:
> > diff --git a/programmer.h b/programmer.h
> > index 9f90fe5..b52727e 100644
> > --- a/programmer.h
> > +++ b/programmer.h
> > @@ -93,15 +93,22 @@ enum programmer_type {
> >    OTHER,
> >  };
> >
> > +struct dev_status {
> > uint16_t vendor_id;
> > uint16_t device_id;
> > const enum test_state status;
> > const char *vendor_name;
> > const char *device_name;
> >
> > +};
>
> I find the name "dev_status" odd, these are the arrays for listing
> and probing the VID/DID too, and the "status" is just documentation.

i missed this before sending version 5.0 of my patches. you do have a
point. any suggestions? "dev" might be ok, but it begs for conflicts.

[7/7, RFC] Unify usbdev_status and pcidev_status into dev_status.

[Kyösti Mälkki]

On Tue, 2012-12-25 at 12:41 +0100, Stefan Tauner wrote:
> On Fri, 16 Nov 2012 22:48:22 +0200
> Kyösti Mälkki  wrote:
> > > diff --git a/programmer.h b/programmer.h
> > > index 9f90fe5..b52727e 100644
> > > --- a/programmer.h
> > > +++ b/programmer.h
> > > @@ -93,15 +93,22 @@ enum programmer_type {
> > >    OTHER,
> > >  };
> > >
> > > +struct dev_status {
> > > uint16_t vendor_id;
> > > uint16_t device_id;
> > > const enum test_state status;
> > > const char *vendor_name;
> > > const char *device_name;
> > >
> > > +};
> >
> > I find the name "dev_status" odd, these are the arrays for listing
> > and probing the VID/DID too, and the "status" is just documentation.
>
> i missed this before sending version 5.0 of my patches. you do have a
> point. any suggestions? "dev" might be ok, but it begs for conflicts.

Maybe dev_probe_info or dev_probe ?