[coreboot] r3705 - in trunk/coreboot-v2/src: include/device northbridge/intel/i945 southbridge/intel/i82801gx

Stefan Reinauer stepan at coresystems.de
Wed Oct 29 17:35:54 CET 2008


Peter Stuge wrote:
> svn at coreboot.org wrote:
>   
>>  static const struct pci_driver mc_driver __pci_driver = {
>>  	.ops    = &mc_ops,
>>  	.vendor = PCI_VENDOR_ID_INTEL,
>> -	.device = 0x27a0,
>> +	.device = PCI_DEVICE_ID_INTEL_945_HOST_BRIDGE,
>>     
>
> Sorry, but I don't really agree with these changes.
>
>
>   
>>  static const struct pci_driver i82801gx_ide __pci_driver = {
>>  	.ops	= &ide_ops,
>>  	.vendor	= PCI_VENDOR_ID_INTEL,
>> -	.device	= 0x27df,
>> +	.device	= PCI_DEVICE_ID_INTEL_82801GB_IDE,
>>  };
>>     
>
> My reasoning is that #defines should add information to the code and
> not be an end in itself.
>
>
>   
>> -static const struct pci_driver i82801ex_usb_ehci __pci_driver = {
>>  	.ops	= &usb_ehci_ops,
>>  	.vendor	= PCI_VENDOR_ID_INTEL,
>> -	.device	= 0x27cc,
>> +	.device	= PCI_DEVICE_ID_INTEL_82801GB_EHCI,
>>  };
>>     
>
> In all these cases I quote above it is already perfectly clear from
> the struct name which device is refered to, in which case I think it
> would be more informative to have the literal device PCI id.
>   

I agree with Peter here. Using the #defines makes the code quite harder
to understand / follow in case new PCI IDs are required for the drivers.
In fact, I started using the numeric values because some wrong values
went undetected while the macro names looked fine on the first look.


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.dehttp://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866





More information about the coreboot mailing list