Re: [PATCH 6/6] (v3) drivers: hwmon: i5k_amb: simplify probing / device identification

[Enrico Weigelt, metux IT consult]

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@xxxxxxxxx -- +49-151-27565287