[coreboot] [PATCH] v3: Replace magic numbers with symbolic constants

[Peter Stuge]

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.


> -	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?


> -	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.


> -	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. I like the warning.


//Peter