[Qemu-devel] Re: [PATCH 05/20] eepro100: Add all supported devices to pci.c

[Gerd Hoffmann]

On 03/03/10 12:51, Michael S. Tsirkin wrote:
> On Sun, Feb 21, 2010 at 10:08:49PM +0100, Stefan Weil wrote:
> > > >   static const char * const pci_nic_models[] = {
> > > >       "ne2k_pci",
> > > > +    "i82550",
> > > >       "i82551",
> > > > +    "i82557a",
> > > >       "i82557b",
> > > > +    "i82557c",
> > > > +    "i82558a",
> > > > +    "i82558b",
> > > > +    "i82559a",
> > > > +    "i82559b",
> > > > +    "i82559c",
> > > >       "i82559er",
> > > > +    "i82562",
> > > >       "rtl8139",
> > > >       "e1000",
> > > >       "pcnet",

> > > One wonders: would it be cleaner to have a single eepro100 device
> > > with specific model as qdev option?

No.  I think it would be cleaner to use qdev even more.  For that 
specific driver it would make sense to create a private Info struct, 
i.e. something like this:

E100PCIDeviceInfo {
   PCIDeviceInfo pci;
   [ model specific stuff such as has_extended_tcb_support here ]
};

Then go

static E100PCIDeviceInfo eepro100_info[] = {
    {
        .pci.qdev.name = "i82550",
        [ usual qdev stuff here ]
        has_extended_tcb_support = 0 | 1;
        [ more e100 device description bits here ]
    },
    [ ... ]
};

Then you can simply zap all the one-liner wrapper functions around 
nic_init().  nic_init() can get a pointer using something like this:

E100PCIDeviceInfo *einfo = DO_UPCAST(E100PCIDeviceInfo,
         pci.qdev, pci_dev->qdev.info);

and setup the device accordingly.

> > I prefer the "official" device names which are also
> > used in the Intel documentation. eepro100 or e100
> > are marketing names (more of them exist).

qdev has aliases.  You can add .qdev.alias = "eepro100" to one of the 
variants, then the eepro100 name will work as well.

Adding the marketing name to the .desc test is probably a good idea too 
because alot of people know the marketing name only.

> > Please note that this patch was marked as optional.
> > The arrays pci_nic_names and pci_nic_models are
> > not really needed, and as soon as the table of available
> > nics is automatically derived from the device information,
> > all variants of i825xx are supported automatically.

> Gerd, any input on this patch?

Looks fine to me.  When we switch over to walking the list of registered 
devices instead of having a hard-coded list of pci nic drivers all the 
eepro100 variants will show up in the list anyway.

cheers,
  Gerd