| Message ID | 20240621122503.10034-3-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
Hi, On 6/22/24 5:07 PM, Pali Rohár wrote: > On Saturday 22 June 2024 16:29:21 Hans de Goede wrote: >> Hi, >> >> On 6/22/24 4:23 PM, Pali Rohár wrote: >>> On Saturday 22 June 2024 16:14:11 Hans de Goede wrote: >>>> Hi Pali, >>>> >>>> On 6/22/24 4:08 PM, Pali Rohár wrote: >>>>> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote: >>>>>> Hi, >>>>>> >>>>>> On 6/22/24 2:46 PM, Pali Rohár wrote: >>>>>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote: >>>>>>>> On chipsets with a second 'Integrated Device Function' SMBus controller use >>>>>>>> a different adapter-name for the second IDF adapter. >>>>>>>> >>>>>>>> This allows platform glue code which is looking for the primary i801 >>>>>>>> adapter to manually instantiate i2c_clients on to differentiate >>>>>>>> between the 2. >>>>>>>> >>>>>>>> This allows such code to find the primary i801 adapter by name, without >>>>>>>> needing to duplicate the PCI-ids to feature-flags mapping from i2c-i801.c. >>>>>>>> >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> --- >>>>>>>> drivers/i2c/busses/i2c-i801.c | 9 +++++++-- >>>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >>>>>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644 >>>>>>>> --- a/drivers/i2c/busses/i2c-i801.c >>>>>>>> +++ b/drivers/i2c/busses/i2c-i801.c >>>>>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >>>>>>>> >>>>>>>> i801_add_tco(priv); >>>>>>>> >>>>>>>> - snprintf(priv->adapter.name, sizeof(priv->adapter.name), >>>>>>>> - "SMBus I801 adapter at %04lx", priv->smba); >>>>>>>> + if (priv->features & FEATURE_IDF) >>>>>>>> + snprintf(priv->adapter.name, sizeof(priv->adapter.name), >>>>>>>> + "SMBus I801 IDF adapter at %04lx", priv->smba); >>>>>>>> + else >>>>>>>> + snprintf(priv->adapter.name, sizeof(priv->adapter.name), >>>>>>>> + "SMBus I801 adapter at %04lx", priv->smba); >>>>>>>> + >>>>>>> >>>>>>> User visible name is identifier for user / human. >>>>>>> >>>>>>> If somebody is going to read this code in next 10 years then can ask >>>>>>> question why to have different name for IDF FEATURE and not also for >>>>>>> other features? And can come to conclusion to unify all names to be >>>>>>> same (why not? it is user identifier). >>>>>> >>>>>> That is a good point, I'll add a comment about this for the next >>>>>> version. >>>>>> >>>>>>> Depending on user names between different kernel subsystem is fragile, >>>>>>> specially for future as rename can happen. >>>>>> >>>>>> Relying no devices names to find devices is standard practice. E.g. >>>>>> this is how 99% of the platform drivers bind to platform devices >>>>>> by the driver and the device having the same name. >>>>> >>>>> But here it is adapter name which is more likely description, not the >>>>> device name which is used for binding. >>>> >>>> It is still matching on a name. >>>> >>>>>>> If you are depending on FEATURE_IDF flag then check for the flag >>>>>>> directly, and not hiding the flag by serializing it into the user >>>>>>> visible name (char[] variable) and then de-serializing it in different >>>>>>> kernel subsystem. If the flag is not exported yet then export it via >>>>>>> some function or other API. >>>>>> >>>>>> Exporting this through some new function is non trivial and adds >>>>>> extra dependencies between modules, causing issues when one is builtin >>>>>> and the other is build as a module. >>>>> >>>>> Access to "struct i801_priv *" is not possible? For example via >>>>> dev_get_drvdata() on "struct device *" which you have in >>>>> smo8800_find_i801()? >>>>> >>>>> Because if it is possible then you can create an inline function in some >>>>> shared header file which access this flag. Not perfect (as accessing >>>>> private data is not the best thing) but can avoid dependences between >>>>> modules. >>>> >>>> Prodding inside another drivers private driver struct is a big nono >>>> and much much more fragile then the name checking. >>> >>> I know, that is why I wrote to access this structure and flags in >>> separate function which can be an inline in e.g. i2c-i801.h header file. >> >> We would still need to be very very sure the device we are calling that >> function on actually has the i2c-i801.c driver bound to it, so that >> e.g. we are not dereferencing a NULL pointer drvdata, or worse >> poking at some other drivers private data because we are calling >> the helper on the wrong device. >> >> To make sure that is the case we would need to e.g. check that: >> a) The device in question is an i2c adapter >> b) the adapter name matches, so we would still be doing name matching ... >> >> Really just matching on the adapter name is by far the cleanest >> option here. >> >> Regards, >> >> Hans > > Ok, I have looked at it now. > > For a) you are already using i2c_verify_adapter(). So this part is done. > > For b) you do not need to check adapter name, but rather adapter driver > name (in case driver data are begin accessed). And I think you can use > dev->driver.name to get it. This would still be checking a name and on top of that requires adding a new helper to i2c-i801.c so we are still checking a name and the code has gotten more complex. Regards, Hans
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index d2d2a6dbe29f..5ac5bbd60d45 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) i801_add_tco(priv); - snprintf(priv->adapter.name, sizeof(priv->adapter.name), - "SMBus I801 adapter at %04lx", priv->smba); + if (priv->features & FEATURE_IDF) + snprintf(priv->adapter.name, sizeof(priv->adapter.name), + "SMBus I801 IDF adapter at %04lx", priv->smba); + else + snprintf(priv->adapter.name, sizeof(priv->adapter.name), + "SMBus I801 adapter at %04lx", priv->smba); + err = i2c_add_adapter(&priv->adapter); if (err) { platform_device_unregister(priv->tco_pdev);
On chipsets with a second 'Integrated Device Function' SMBus controller use a different adapter-name for the second IDF adapter. This allows platform glue code which is looking for the primary i801 adapter to manually instantiate i2c_clients on to differentiate between the 2. This allows such code to find the primary i801 adapter by name, without needing to duplicate the PCI-ids to feature-flags mapping from i2c-i801.c. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-i801.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)