| Message ID | 20230522104157.333217-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: Add i2c_get_match_data() | expand |
Hi Biju, On Mon, May 22, 2023 at 12:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Add i2c_get_match_data() similar to of_device_get_match_data(), > so that we can optimize the driver code that uses both I2C and > DT-based matching. > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -99,8 +99,8 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz) > } > EXPORT_SYMBOL_GPL(i2c_freq_mode_string); > > -const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, > - const struct i2c_client *client) > +static const struct i2c_device_id *i2c_match_device_id(const struct i2c_device_id *id, > + const struct i2c_client *client) > { > if (!(id && client)) > return NULL; > @@ -110,10 +110,30 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, > return id; > id++; > } > + > return NULL; > } > + > +const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, > + const struct i2c_client *client) > +{ > + return i2c_match_device_id(id, client); > +} > EXPORT_SYMBOL_GPL(i2c_match_id); Is there any reason why you're adding a new intermediate? > > +const void *i2c_get_match_data(const struct i2c_device_id *id, > + const struct i2c_client *client) I think this should take the id table from the i2c_driver, as pointed to by client->dev.driver, instead of from an explicitly passed parameter. > +{ > + const struct i2c_device_id *match; > + > + match = i2c_match_device_id(id, client); > + if (!match) > + return NULL; > + > + return (const void *)match->driver_data; One can discuss about the returned type: personally, I won't mind "const void *" for consistency with other subsystems (e.g. DT), but as i2c_device_id.driver_data has type "kernel_ulong_t", other people might prefer the latter. > +} > +EXPORT_SYMBOL(i2c_get_match_data); Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Monday, May 22, 2023 12:29 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Wolfram Sang <wsa@kernel.org>; linux-i2c@vger.kernel.org; Alessandro > Zummo <a.zummo@towertech.it>; Alexandre Belloni > <alexandre.belloni@bootlin.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas- > soc@vger.kernel.org > Subject: Re: [PATCH] i2c: Add i2c_get_match_data() > > Hi Biju, > > On Mon, May 22, 2023 at 12:42 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > Add i2c_get_match_data() similar to of_device_get_match_data(), so > > that we can optimize the driver code that uses both I2C and DT-based > > matching. > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -99,8 +99,8 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz) } > > EXPORT_SYMBOL_GPL(i2c_freq_mode_string); > > > > -const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > *id, > > - const struct > i2c_client *client) > > +static const struct i2c_device_id *i2c_match_device_id(const struct > i2c_device_id *id, > > + const struct > > +i2c_client *client) > > { > > if (!(id && client)) > > return NULL; > > @@ -110,10 +110,30 @@ const struct i2c_device_id *i2c_match_id(const > struct i2c_device_id *id, > > return id; > > id++; > > } > > + > > return NULL; > > } > > + > > +const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > *id, > > + const struct i2c_client > > +*client) { > > + return i2c_match_device_id(id, client); } > > EXPORT_SYMBOL_GPL(i2c_match_id); > > Is there any reason why you're adding a new intermediate? The same code is shared between below function. As discussed below, it is not required. > > > > > +const void *i2c_get_match_data(const struct i2c_device_id *id, > > + const struct i2c_client *client) > > I think this should take the id table from the i2c_driver, as pointed to > by client->dev.driver, instead of from an explicitly passed parameter. You mean const void *i2c_get_match_data(const struct i2c_client *client)?? struct i2c_driver *driver = client->dev.driver; And then call match = i2c_match_id(driver->id_table, client)?? > > > +{ > > + const struct i2c_device_id *match; > > + > > + match = i2c_match_device_id(id, client); > > + if (!match) > > + return NULL; > > + > > + return (const void *)match->driver_data; > > One can discuss about the returned type: personally, I won't mind "const > void *" for consistency with other subsystems (e.g. DT), but as > i2c_device_id.driver_data has type "kernel_ulong_t", other people might > prefer the latter. The advantage of void*, is upper layer don't need casting. eg: with kernel_ulong_t as return parameter: isl1208->config = (struct isl1208_config *)i2c_get_match_data; Cheers, Biju > > > +} > > +EXPORT_SYMBOL(i2c_get_match_data); >
Hi Biju, On Mon, May 22, 2023 at 2:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: Monday, May 22, 2023 12:29 PM > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Wolfram Sang <wsa@kernel.org>; linux-i2c@vger.kernel.org; Alessandro > > Zummo <a.zummo@towertech.it>; Alexandre Belloni > > <alexandre.belloni@bootlin.com>; Prabhakar Mahadev Lad > > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas- > > soc@vger.kernel.org > > Subject: Re: [PATCH] i2c: Add i2c_get_match_data() > > > > Hi Biju, > > > > On Mon, May 22, 2023 at 12:42 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > Add i2c_get_match_data() similar to of_device_get_match_data(), so > > > that we can optimize the driver code that uses both I2C and DT-based > > > matching. > > > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/i2c/i2c-core-base.c > > > +++ b/drivers/i2c/i2c-core-base.c > > > @@ -99,8 +99,8 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz) } > > > EXPORT_SYMBOL_GPL(i2c_freq_mode_string); > > > > > > -const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > > *id, > > > - const struct > > i2c_client *client) > > > +static const struct i2c_device_id *i2c_match_device_id(const struct > > i2c_device_id *id, > > > + const struct > > > +i2c_client *client) > > > { > > > if (!(id && client)) > > > return NULL; > > > @@ -110,10 +110,30 @@ const struct i2c_device_id *i2c_match_id(const > > struct i2c_device_id *id, > > > return id; > > > id++; > > > } > > > + > > > return NULL; > > > } > > > + > > > +const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > > *id, > > > + const struct i2c_client > > > +*client) { > > > + return i2c_match_device_id(id, client); } > > > EXPORT_SYMBOL_GPL(i2c_match_id); > > > > Is there any reason why you're adding a new intermediate? > > The same code is shared between below function. > As discussed below, it is not required. > > > > > > > > > +const void *i2c_get_match_data(const struct i2c_device_id *id, > > > + const struct i2c_client *client) > > > > I think this should take the id table from the i2c_driver, as pointed to > > by client->dev.driver, instead of from an explicitly passed parameter. > > You mean const void *i2c_get_match_data(const struct i2c_client *client)?? > > struct i2c_driver *driver = client->dev.driver; also needs to_i2c_driver() > > And then call > > match = i2c_match_id(driver->id_table, client)?? Exactly. > > > +{ > > > + const struct i2c_device_id *match; > > > + > > > + match = i2c_match_device_id(id, client); > > > + if (!match) > > > + return NULL; > > > + > > > + return (const void *)match->driver_data; > > > > One can discuss about the returned type: personally, I won't mind "const > > void *" for consistency with other subsystems (e.g. DT), but as > > i2c_device_id.driver_data has type "kernel_ulong_t", other people might > > prefer the latter. > > The advantage of void*, is upper layer don't need casting. Depending on what the driver wants... Some drivers want integers, other want pointers... > eg: with kernel_ulong_t as return parameter: > > isl1208->config = (struct isl1208_config *)i2c_get_match_data; I know ;-) For the isl1208 case, "void *" simplifies the driver... Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH] i2c: Add i2c_get_match_data() > > Hi Biju, > > On Mon, May 22, 2023 at 2:38 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Sent: Monday, May 22, 2023 12:29 PM > > > To: Biju Das <biju.das.jz@bp.renesas.com> > > > Cc: Wolfram Sang <wsa@kernel.org>; linux-i2c@vger.kernel.org; > > > Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > > > <alexandre.belloni@bootlin.com>; Prabhakar Mahadev Lad > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas- > > > soc@vger.kernel.org > > > Subject: Re: [PATCH] i2c: Add i2c_get_match_data() > > > > > > Hi Biju, > > > > > > On Mon, May 22, 2023 at 12:42 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > Add i2c_get_match_data() similar to of_device_get_match_data(), so > > > > that we can optimize the driver code that uses both I2C and > > > > DT-based matching. > > > > > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > > > --- a/drivers/i2c/i2c-core-base.c > > > > +++ b/drivers/i2c/i2c-core-base.c > > > > @@ -99,8 +99,8 @@ const char *i2c_freq_mode_string(u32 > > > > bus_freq_hz) } EXPORT_SYMBOL_GPL(i2c_freq_mode_string); > > > > > > > > -const struct i2c_device_id *i2c_match_id(const struct > > > > i2c_device_id > > > *id, > > > > - const struct > > > i2c_client *client) > > > > +static const struct i2c_device_id *i2c_match_device_id(const > > > > +struct > > > i2c_device_id *id, > > > > + const > > > > +struct i2c_client *client) > > > > { > > > > if (!(id && client)) > > > > return NULL; > > > > @@ -110,10 +110,30 @@ const struct i2c_device_id > > > > *i2c_match_id(const > > > struct i2c_device_id *id, > > > > return id; > > > > id++; > > > > } > > > > + > > > > return NULL; > > > > } > > > > + > > > > +const struct i2c_device_id *i2c_match_id(const struct > > > > +i2c_device_id > > > *id, > > > > + const struct i2c_client > > > > +*client) { > > > > + return i2c_match_device_id(id, client); } > > > > EXPORT_SYMBOL_GPL(i2c_match_id); > > > > > > Is there any reason why you're adding a new intermediate? > > > > The same code is shared between below function. > > As discussed below, it is not required. > > > > > > > > > > > > > +const void *i2c_get_match_data(const struct i2c_device_id *id, > > > > + const struct i2c_client *client) > > > > > > I think this should take the id table from the i2c_driver, as > > > pointed to by client->dev.driver, instead of from an explicitly > passed parameter. > > > > You mean const void *i2c_get_match_data(const struct i2c_client > *client)?? > > > > struct i2c_driver *driver = client->dev.driver; > > also needs to_i2c_driver() OK. > > > > > And then call > > > > match = i2c_match_id(driver->id_table, client)?? > > Exactly. > > > > > +{ > > > > + const struct i2c_device_id *match; > > > > + > > > > + match = i2c_match_device_id(id, client); > > > > + if (!match) > > > > + return NULL; > > > > + > > > > + return (const void *)match->driver_data; > > > > > > One can discuss about the returned type: personally, I won't mind > > > "const void *" for consistency with other subsystems (e.g. DT), but > > > as i2c_device_id.driver_data has type "kernel_ulong_t", other people > > > might prefer the latter. > > > > The advantage of void*, is upper layer don't need casting. > > Depending on what the driver wants... > Some drivers want integers, other want pointers... For integer and enums, already most of the drivers are using like this chip_id = i2c_match_id(max20730_id, client)->driver_data; > > > eg: with kernel_ulong_t as return parameter: > > > > isl1208->config = (struct isl1208_config *)i2c_get_match_data; > > I know ;-) > For the isl1208 case, "void *" simplifies the driver... But with return type as "void *" the below drivers can make use of it, as these drivers never checks return type for "i2c_match_id". drivers/iio/potentiometer/mcp4018.c: data->cfg = &mcp4018_cfg[i2c_match_id(mcp4018_id, client)->driver_data]; drivers/iio/accel/adxl313_i2c.c: chip_data = (const struct adxl313_chip_info *)i2c_match_id(adxl313_i2c_id, client)->driver_data; drivers/iio/accel/adxl355_i2c.c: chip_data = (void *)i2c_match_id(adxl355, client)->driver_data; drivers/rtc/rtc-pcf85063.c- config = &pcf85063_cfg[type]; drivers/i2c/muxes/i2c-mux-ltc4306.c: chip = &chips[i2c_match_id(ltc4306_id, client)->driver_data]; drivers/hwmon/sht3x.c- attribute_groups = sts3x_groups; drivers/hwmon/pmbus/pmbus.c: device_info = (struct pmbus_device_info *)i2c_match_id(pmbus_id, client)->driver_data; I will send next version based on return type as void*, Based on wider feedback if needed will change return type. Hope it is ok?? Cheers, Biju
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 3442aa80290f..1ab639b2693c 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -99,8 +99,8 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz) } EXPORT_SYMBOL_GPL(i2c_freq_mode_string); -const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, - const struct i2c_client *client) +static const struct i2c_device_id *i2c_match_device_id(const struct i2c_device_id *id, + const struct i2c_client *client) { if (!(id && client)) return NULL; @@ -110,10 +110,30 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, return id; id++; } + return NULL; } + +const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, + const struct i2c_client *client) +{ + return i2c_match_device_id(id, client); +} EXPORT_SYMBOL_GPL(i2c_match_id); +const void *i2c_get_match_data(const struct i2c_device_id *id, + const struct i2c_client *client) +{ + const struct i2c_device_id *match; + + match = i2c_match_device_id(id, client); + if (!match) + return NULL; + + return (const void *)match->driver_data; +} +EXPORT_SYMBOL(i2c_get_match_data); + static int i2c_device_match(struct device *dev, struct device_driver *drv) { struct i2c_client *client = i2c_verify_client(dev); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 0ce344724209..ddfe842b8f24 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -367,6 +367,9 @@ struct i2c_adapter *i2c_verify_adapter(struct device *dev); const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, const struct i2c_client *client); +const void *i2c_get_match_data(const struct i2c_device_id *id, + const struct i2c_client *client); + static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj) { struct device * const dev = kobj_to_dev(kobj);
Add i2c_get_match_data() similar to of_device_get_match_data(), so that we can optimize the driver code that uses both I2C and DT-based matching. Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- eg: The RTC 1208 driver code [1] can be optimized with [2], which is planned to mainline once [3] is accepted. [1] if (client->dev.of_node) { isl1208->config = of_device_get_match_data(&client->dev); if (!isl1208->config) return -ENODEV; } else { const struct i2c_device_id *id = i2c_match_id(isl1208_id, client); if (!id) return -ENODEV; isl1208->config = (struct isl1208_config *)id->driver_data; } [2] if (client->dev.of_node) isl1208->config = of_device_get_match_data(&client->dev); else isl1208->config = i2c_get_match_data(isl1208_id, client); if (!isl1208->config) return -ENODEV; [3] https://lore.kernel.org/linux-renesas-soc/20230522101849.297499-1-biju.das.jz@bp.renesas.com/T/#t --- drivers/i2c/i2c-core-base.c | 24 ++++++++++++++++++++++-- include/linux/i2c.h | 3 +++ 2 files changed, 25 insertions(+), 2 deletions(-)