[coreboot] [PATCH] v3: Replace magic numbers with symbolic constants
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Oct 9 11:25:12 CEST 2008
On 09.10.2008 04:37, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
>
>> Replace magic numbers with existing symbolic constants. SB600 is
>> heavily affected. This mostly targets pci_*_config*() calls.
>>
>> This is part of my quest to make existing code more readable
>> without looking up magic numbers.
>>
>
> In general very nice, but a few comments follow..
>
>
>
>> - dword = pci_read_config32(dev, 0x04);
>> + dword = pci_read_config32(dev, PCI_COMMAND);
>> dword |= 0x10;
>> dword &= ~(1<<6); // PERSP Parity Error Response
>> - pci_write_config32(dev, 0x04, dword);
>> + pci_write_config32(dev, PCI_COMMAND, dword);
>>
>
> I think it would be nice to fix whitespace when the line is touched
> anyway.
>
Sure thing. I saw it the second after hitting "send".
>> - pci_write_config32(nb_dev, 0x1C, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
>> - pci_write_config32(nb_dev, 0x20, 0x00000000);
>> + pci_write_config32(nb_dev, PCI_BASE_ADDRESS_3, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
>> + pci_write_config32(nb_dev, PCI_BASE_ADDRESS_4, 0x00000000);
>>
>
> Hm, maybe PCI_BAR3 or PCI_BAR_3?
>
Hm yes. I decided to use existing #defines, so it was the long form. I
can send a separate patch for shorter names.
>> - if (!pci_conf1_find_device(0x1002, 0x4385, &dev)){
>> + if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev)){
>>
>
> Not sure about these at all. I think I prefer the number.
>
The numbers are a bit too magic for my taste.
>> - byte |= (1 << 0);
>> - byte |= (1 << 5);
>> + byte |= (1 << 0) | (1 << 5);
>>
>
> I like this. Others may not.
>
>
>
>> - /* Program the 2C to 0x43801002 */
>>
>
> Gotta love comments like this. :)
>
>
>
>> - dword = 0x43801002;
>> - pci_write_config32(dev, 0x2c, dword);
>> + /* Program the subsystem device/vendor to 0x43801002 */
>> +#warning Subsystem ID setting should be in a separate function
>> + dword = (0x4380 << 16) | PCI_VENDOR_ID_ATI;
>> + pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, dword);
>>
>
> I think
>
> pci_write_config32(dev,PCI_SUBSYSTEM_VENDOR_ID, 0x43801002);
>
> looks pretty nice.
Actually, that's probably the next cleanup step. This
dword = val;
pci_write_config32(dev, ..., dword);
is a less than stellar style.
> I like the warning.
>
Expect more of them as I walk through the code. ;-)
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list