| Message ID | 20240812203952.42804-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand |
Hi Wolfram, On Mon, Aug 12, 2024 at 10:39:48PM GMT, 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. > > Reviewed-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v4: > - Use a single snprintf() with a conditional argument for the 2 names > - Add a comment that the adapter-name is used by platform code > > Changes in v3: > - This is a new patch in v3 of this patch-set > --- > drivers/i2c/busses/i2c-i801.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 328c0dab6b14..299fe9d3afab 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1763,8 +1763,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > > i801_add_tco(priv); > > + /* > + * adapter.name is used by platform code to find the main I801 adapter > + * to instantiante i2c_clients, do not change. > + */ > snprintf(priv->adapter.name, sizeof(priv->adapter.name), > - "SMBus I801 adapter at %04lx", priv->smba); > + "SMBus %s adapter at %04lx", > + (priv->features & FEATURE_IDF) ? "I801 IDF" : "I801", > + priv->smba); > + do you have any comment here? Thanks, Andi > err = i2c_add_adapter(&priv->adapter); > if (err) { > platform_device_unregister(priv->tco_pdev); > -- > 2.46.0 >
Hi Andi, Hans, all, > > + /* > > + * adapter.name is used by platform code to find the main I801 adapter > > + * to instantiante i2c_clients, do not change. > > + */ > > snprintf(priv->adapter.name, sizeof(priv->adapter.name), > > - "SMBus I801 adapter at %04lx", priv->smba); > > + "SMBus %s adapter at %04lx", > > + (priv->features & FEATURE_IDF) ? "I801 IDF" : "I801", > > + priv->smba); > > + > > do you have any comment here? I have been scratching my head about this patch for a while. I still think that it is quite fragile to base features on this naming. The comment above helps, but I still have a bad feeling about it. I noticed now that the i801 driver does not use the algo_data field so far. Has it been considered to put a flag there? Happy hacking, Wolfram
On Wed, Aug 14, 2024 at 08:35:09PM +0200, Wolfram Sang wrote: > Hi Andi, Hans, all, > > > > + /* > > > + * adapter.name is used by platform code to find the main I801 adapter > > > + * to instantiante i2c_clients, do not change. > > > + */ > > > snprintf(priv->adapter.name, sizeof(priv->adapter.name), > > > - "SMBus I801 adapter at %04lx", priv->smba); > > > + "SMBus %s adapter at %04lx", > > > + (priv->features & FEATURE_IDF) ? "I801 IDF" : "I801", > > > + priv->smba); > > > + > > > > do you have any comment here? > > I have been scratching my head about this patch for a while. I still > think that it is quite fragile to base features on this naming. The > comment above helps, but I still have a bad feeling about it. > > I noticed now that the i801 driver does not use the algo_data field so > far. Has it been considered to put a flag there? Hmm... algo_data by the naming seems has to be related to the algorithm, but AFAIU here we have simply more than one _identical_ adapters. How is this semantically related?
> Hmm... algo_data by the naming seems has to be related to the algorithm, > but AFAIU here we have simply more than one _identical_ adapters. How > is this semantically related? You like the naming approach better?
On Wed, Aug 14, 2024 at 08:51:52PM +0200, Wolfram Sang wrote: > > > Hmm... algo_data by the naming seems has to be related to the algorithm, > > but AFAIU here we have simply more than one _identical_ adapters. How > > is this semantically related? > > You like the naming approach better? There are zillions of naming matching code in the kernel on different levels. TBH, I have no preferences here, but I definitely see nothing worse in this approach than in the other.
> There are zillions of naming matching code in the kernel on different levels. > TBH, I have no preferences here, but I definitely see nothing worse in this > approach than in the other. Okay then. I am currently not aware of such naming matching code, but I trust you. As I don't seem to find a better way right now, I'll stand aside and do not want to delay any further. We can improve later if we need to, right?
On Mon, Aug 12, 2024 at 10:39:48PM +0200, 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. > > Reviewed-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 328c0dab6b14..299fe9d3afab 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1763,8 +1763,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) i801_add_tco(priv); + /* + * adapter.name is used by platform code to find the main I801 adapter + * to instantiante i2c_clients, do not change. + */ snprintf(priv->adapter.name, sizeof(priv->adapter.name), - "SMBus I801 adapter at %04lx", priv->smba); + "SMBus %s adapter at %04lx", + (priv->features & FEATURE_IDF) ? "I801 IDF" : "I801", + priv->smba); + err = i2c_add_adapter(&priv->adapter); if (err) { platform_device_unregister(priv->tco_pdev);