Re: [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