Messages in this thread | | | Subject | Re: [PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing / device identification | From | "Enrico Weigelt, metux IT consult" <> | Date | Wed, 8 Jan 2020 11:16:47 +0100 |
| |
On 11.12.19 00:25, Bjorn Helgaas wrote:
Hi,
> 2) I despise the pci_get_device() interfaces because they're > inefficient, not hotplug-safe, they circumvent the device model > claim mechanism, and it's hard to do the reference counting > correctly.
Agreed, but for that we'd have to edac and hwmo driver together into one (or maybe a MFD). That would be a major rewrite, and I don't have the necessary HW to test it.
> 3) There are several things going on in this patch and it would be > easier to read if you could split them into separate patches: > > - Removing the redundancy between chipset_ids[] and i5k_amb_ids[]. > This seems like a nice change. > > - The "chipset_ids[i].fbd0 + 1" thing was weird and the new > ".driver_data + 1" is still weird. Those are PCI device IDs, > and addition is not a valid operation on those IDs. IMHO both > PCI_DEVICE_ID_INTEL_5000_FBD0 and PCI_DEVICE_ID_INTEL_5000_FBD1 > should be listed explicitly in the driver instead of trying to > compute PCI_DEVICE_ID_INTEL_5000_FBD1.
Ok, good point.
> - Replacing the hard-coding of PCI_VENDOR_ID_INTEL with the vendor > ID from i5k_amb_ids[] seems worthwhile and should be its own > separate patch (if possible). > > - Changing to use pci_get_device_by_id(). This should be trivial > to verify, like the other patches.
Ok, I'll try to split it up more clearly, once I've got some more spare time.
--mtx
--- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
| |