On 11.03.2009 13:47 Uhr, Carl-Daniel Hailfinger wrote:
I don't know the code well, so this is not a real review. Just a nitpick.
On 11.03.2009 13:23, Stefan Reinauer wrote:
+static void smbus_set_subsystem(device_t dev, unsigned vendor, unsigned device) +{
- if (!vendor || !device) {
This case should output a diagnostic message at SPEW or DEBUG level.
I don't think this is a good idea. It's a perfectly fine case that we don't want to warn about. Plus, the calling function already does a debug level print in the case that a subsystem function is specified.
pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,pci_read_config32(dev, 0));PCI_VENDOR_ID instead of 0 for consistency.
Good point.
- } else {
pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,((device & 0xffff) << 16) | (vendor & 0xffff));- }
+}
Same for the other *_set_subsystem. Maybe we even want a generic function to handle this since the *_set_subsystem functions in the patch seem to be identical.
Yes, mostly. I was going to look into that as a next step. So far I only found information from intel that they suggest this behavior, so I didn't want to make it a global rule.