On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote:
Sorry, but:
versatile_pci.c: d->config[0x04] = 0x00;
versatile_pci.c: d->config[0x05] = 0x00;
versatile_pci.c: d->config[0x06] = 0x20;
versatile_pci.c: d->config[0x07] = 0x02;
To:
pci_config_set_command(d, 0);
pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ);
Is a huge improvement.
Yes but
pci_set_word(d->config + PCI_COMMAND, 0);
pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
PCI_STATUS_66MHZ);
is not much worse, and that API is already there.
And advatage is it uses macros from linux which have
higher chance to be correct than what we come up with.
Oh, pci_set_word() is certainly an improvement. Personally, I prefer
passing the PCIDevice as a context and adding individual accessors but
anything is better than open coded config.
I'm staring at a PCI config space diagram right
now and I'm *still* not even sure I'm interpreting the versatile_pci
code correctly :-)
I spent time cleaning up devices, just did not get to bridges.
What I did is write patches and verify that compiled code
did not change at all. This guarantees no breakage.
Care to volunteer to complete that work?
Separately people that are familiar with device can clean it up.
It's on my radar but I've got another PCI series in flight. I've got a
branch pci-cleanup on staging that's a work in progress for adding a
proper region API along with PCI memory accessors.
Once I find a little more time to finish converting VGA devices, I'll post.
Regards,
Anthony Liguori
