| Message ID | 20250407145546.270683-9-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote: > The supplier device of an I2C adapter is the device that calls > i2c_add_adapter() or variants and i2c_del_adapter(). > > Most of the time this supplier device is the parent of the adapter dev. > > Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the > parent of the adapter dev points to the adapter dev the mux is connected dev --> device (in both cases) > to instead of the supplier of this adapter. > > Introduce i2c_get_adapter_supplier() and a new supplier field in the > adapter structure in order to ease the adapter supplier retrieval. ... > +/** > + * i2c_get_adapter_supplier() - Get the supplier of an adapter > + * @adapter: the adapter to get the supplier from > + * > + * return: Return: > + * Look up and return the &struct device corresponding to the device supplying > + * this adapter. @adapter > + * The user must call put_device() once done with the supplier returned. > + */ > +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter) > +{ > + return get_device(adapter->supplier ?: adapter->dev.parent); What will be the meaning when both are set? Why dev.parent is not the same as supplier in this case? Looking at the commit message example, it seems like you want to provide a physdev or sysdev (as term supplier seems more devlink:ish), like it's done elsewhere. And in the same way _always_ initialise it. In such a case, the ambiguity will be gone. > +}
Hi Andy, On Mon, 7 Apr 2025 18:27:07 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote: > > The supplier device of an I2C adapter is the device that calls > > i2c_add_adapter() or variants and i2c_del_adapter(). > > > > Most of the time this supplier device is the parent of the adapter dev. > > > > Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the > > parent of the adapter dev points to the adapter dev the mux is connected > > dev --> device (in both cases) Will be updated in the newt iteration. > > > to instead of the supplier of this adapter. > > > > Introduce i2c_get_adapter_supplier() and a new supplier field in the > > adapter structure in order to ease the adapter supplier retrieval. > > ... > > > +/** > > + * i2c_get_adapter_supplier() - Get the supplier of an adapter > > + * @adapter: the adapter to get the supplier from > > + * > > + * return: > > Return: Will be updated. > > > + * Look up and return the &struct device corresponding to the device supplying > > + * this adapter. > > @adapter Will be updated. > > > + * The user must call put_device() once done with the supplier returned. > > + */ > > +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter) > > +{ > > + return get_device(adapter->supplier ?: adapter->dev.parent); > > What will be the meaning when both are set? Why dev.parent is not the same > as supplier in this case? Looking at the commit message example, it seems > like you want to provide a physdev or sysdev (as term supplier seems more > devlink:ish), like it's done elsewhere. And in the same way _always_ initialise > it. In such a case, the ambiguity will be gone. When both are set (this is case for i2c muxes), the adapter->supplier the device that register the I2C adapter using i2c_add_adapter() or variant. In other word, the device that creates the I2C adapter. The adapter->dev.parent is most of the time the device that register the I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent is the adapter the i2c mux is connected to. Between physdev and sysdev, I really prefer physdev and, if renaming from supplier to physdev is still needed (and wanted), I will rename it. Let me know. For initialization, I don't want to modify all the I2C controller drivers. What I can do is to initialize adapter->supplier using adapter->dev.parent during the i2c_register_adapter() call if it was not already initialize by the caller (i.e. the I2C controller driver). Does it make sense ? Best regards, Hervé
On Tue, Apr 08, 2025 at 03:08:36PM +0200, Herve Codina wrote: > On Mon, 7 Apr 2025 18:27:07 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote: ... > > > + return get_device(adapter->supplier ?: adapter->dev.parent); > > > > What will be the meaning when both are set? Why dev.parent is not the same > > as supplier in this case? Looking at the commit message example, it seems > > like you want to provide a physdev or sysdev (as term supplier seems more > > devlink:ish), like it's done elsewhere. And in the same way _always_ initialise > > it. In such a case, the ambiguity will be gone. > > When both are set (this is case for i2c muxes), the adapter->supplier the > device that register the I2C adapter using i2c_add_adapter() or variant. > In other word, the device that creates the I2C adapter. > > The adapter->dev.parent is most of the time the device that register the > I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent > is the adapter the i2c mux is connected to. > > Between physdev and sysdev, I really prefer physdev and, if renaming from > supplier to physdev is still needed (and wanted), I will rename it. Let me > know. The terms supplier/consumer are widely used in terms of power and devlink. I think here should not be used the term supplier. > For initialization, I don't want to modify all the I2C controller drivers. > What I can do is to initialize adapter->supplier using adapter->dev.parent > during the i2c_register_adapter() call if it was not already initialize by > the caller (i.e. the I2C controller driver). This can be done in the I²C core, but I'm not insisting on this part. We can start from your function only and then decide later on how to proceed (depending on how many users of that field appear and what they want to do with it). > Does it make sense ?
On Tue, 8 Apr 2025 16:47:51 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Apr 08, 2025 at 03:08:36PM +0200, Herve Codina wrote: > > On Mon, 7 Apr 2025 18:27:07 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Apr 07, 2025 at 04:55:37PM +0200, Herve Codina wrote: > > ... > > > > > + return get_device(adapter->supplier ?: adapter->dev.parent); > > > > > > What will be the meaning when both are set? Why dev.parent is not the same > > > as supplier in this case? Looking at the commit message example, it seems > > > like you want to provide a physdev or sysdev (as term supplier seems more > > > devlink:ish), like it's done elsewhere. And in the same way _always_ initialise > > > it. In such a case, the ambiguity will be gone. > > > > When both are set (this is case for i2c muxes), the adapter->supplier the > > device that register the I2C adapter using i2c_add_adapter() or variant. > > In other word, the device that creates the I2C adapter. > > > > The adapter->dev.parent is most of the time the device that register the > > I2C adapter except for i2c muxes. For I2C muxes, this adapter->dev.parent > > is the adapter the i2c mux is connected to. > > > > Between physdev and sysdev, I really prefer physdev and, if renaming from > > supplier to physdev is still needed (and wanted), I will rename it. Let me > > know. > > The terms supplier/consumer are widely used in terms of power and devlink. > I think here should not be used the term supplier. physdev seems good. I will use that. > > > For initialization, I don't want to modify all the I2C controller drivers. > > What I can do is to initialize adapter->supplier using adapter->dev.parent > > during the i2c_register_adapter() call if it was not already initialize by > > the caller (i.e. the I2C controller driver). > > This can be done in the I²C core, but I'm not insisting on this part. > We can start from your function only and then decide later on how to > proceed (depending on how many users of that field appear and what > they want to do with it). > Right I think I can keep my function as it. Wolfram any opinion? Best regards, Hervé
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 7ad1ad5c8c3f..e3eeac0b2b49 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1917,6 +1917,22 @@ struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode) } EXPORT_SYMBOL(i2c_get_adapter_by_fwnode); +/** + * i2c_get_adapter_supplier() - Get the supplier of an adapter + * @adapter: the adapter to get the supplier from + * + * return: + * Look up and return the &struct device corresponding to the device supplying + * this adapter. + * + * The user must call put_device() once done with the supplier returned. + */ +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adapter) +{ + return get_device(adapter->supplier ?: adapter->dev.parent); +} +EXPORT_SYMBOL(i2c_get_adapter_supplier); + static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p, u32 def_val, bool use_def) { diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 2e4903b7f7bc..04b85703bcd6 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -746,6 +746,7 @@ struct i2c_adapter { int timeout; /* in jiffies */ int retries; struct device dev; /* the adapter device */ + struct device *supplier; /* the device that supply this adapter */ unsigned long locked_flags; /* owned by the I2C core */ #define I2C_ALF_IS_SUSPENDED 0 #define I2C_ALF_SUSPEND_REPORTED 1 @@ -913,6 +914,8 @@ struct i2c_adapter *i2c_get_adapter(int nr); void i2c_put_adapter(struct i2c_adapter *adap); unsigned int i2c_adapter_depth(struct i2c_adapter *adapter); +struct device *i2c_get_adapter_supplier(struct i2c_adapter *adap); + void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults); /* Return the functionality mask */
The supplier device of an I2C adapter is the device that calls i2c_add_adapter() or variants and i2c_del_adapter(). Most of the time this supplier device is the parent of the adapter dev. Exceptions exist with i2c muxes. Indeed, in case of i2c muxes, the parent of the adapter dev points to the adapter dev the mux is connected to instead of the supplier of this adapter. Introduce i2c_get_adapter_supplier() and a new supplier field in the adapter structure in order to ease the adapter supplier retrieval. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- drivers/i2c/i2c-core-base.c | 16 ++++++++++++++++ include/linux/i2c.h | 3 +++ 2 files changed, 19 insertions(+)