On Wed, Mar 03, 2010 at 02:31:53PM +0100, Gerd Hoffmann wrote:
> 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.
That's my thinking as well.
>>> 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
--
MST
