[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