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@xxxxxxxxxx> >>>> --- >>>> 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. Regards, Hans
