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

From: Enrico Weigelt, metux IT consult
Date: Wed Jan 08 2020 - 05:18:16 EST


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