| Message ID | 20230606130519.382304-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | Superseded |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | [v3] i2c: Add i2c_get_match_data() | expand |
Hi Wolfram/Geert,
Do we need to enhance the logic to use device_get_match_data
to support OF/ACPI/I2C match like below [1].
Or
Are we happy with the current one?
+ Mark, Linux-clk
If I am correct, the new enhancement[1] will add I2C match support [2]
drivers. Currently this driver support only OF/ACPI match-data even though
driver has I2C matching table.
[2] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-renesas-pcie.c#L282
The current patch will be useful for these drivers as well for I2C match support
as these drivers have I2C matching table.
[3] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-versaclock5.c#L956
[4] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-versaclock7.c#L1111
[1]
+const void *i2c_get_match_data(const struct i2c_client *client) {
+ struct device_driver *drv = client->dev.driver;
+ struct i2c_driver *driver = to_i2c_driver(drv);
+ const struct i2c_device_id *match;
+ const void *data;
+
+ data = device_get_match_data(&client->dev);
+ if (!data) {
+ match = i2c_match_id(driver->id_table, client);
+ if (!match)
+ return NULL;
+
+ data = (const void *)match->driver_data;
+ }
+
> + return data;
> +}
> +EXPORT_SYMBOL(i2c_get_match_data);
Cheers,
Biju
> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Tuesday, June 6, 2023 2:05 PM
> To: Wolfram Sang <wsa@kernel.org>; Alessandro Zummo
> <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; linux-i2c@vger.kernel.org;
> linux-rtc@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-
> renesas-soc@vger.kernel.org
> Subject: [PATCH v3] i2c: Add i2c_get_match_data()
>
> Add i2c_get_match_data() to get match data for both I2C and DT-based
> matching, so that we can optimize the driver code that uses both.
>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2->v3:
> * Added support for getting match data for both I2C and DT-based
> matching.
> * Added Rb tag from Geert and retained the Rb tag as change is trivial.
> v1->v2:
> * Dropped parameter const struct i2c_device_id *id and the helper
> function.
>
> eg: The RTC pcf85063/isl1208 driver code can be optimized with this
> patch.
> - if (client->dev.of_node) {
> - config = of_device_get_match_data(&client->dev);
> - if (!config)
> - return -ENODEV;
> - } else {
> - enum pcf85063_type type =
> - i2c_match_id(pcf85063_ids, client)->driver_data;
> - if (type >= PCF85063_LAST_ID)
> - return -ENODEV;
> - config = &pcf85063_cfg[type];
> - }
> + config = i2c_get_match_data(client);
> + if (!config)
> + return -ENODEV;
> ---
> drivers/i2c/i2c-core-base.c | 21 +++++++++++++++++++++
> include/linux/i2c.h | 2 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ae3af738b03f..15a49f4ba668 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -114,6 +114,27 @@ const struct i2c_device_id *i2c_match_id(const
> struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id);
>
> +const void *i2c_get_match_data(const struct i2c_client *client) {
> + struct device_driver *drv = client->dev.driver;
> + struct i2c_driver *driver = to_i2c_driver(drv);
> + const struct i2c_device_id *match;
> + const void *data;
> +
> + if (client->dev.of_node) {
> + data = of_device_get_match_data(&client->dev);
> + } else {
> + match = i2c_match_id(driver->id_table, client);
> + if (!match)
> + return NULL;
> +
> + data = (const void *)match->driver_data;
> + }
> +
> + return 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
> 13a1ce38cb0c..3430cc2b05a6 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -367,6 +367,8 @@ 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_client *client);
> +
> static inline struct i2c_client *kobj_to_i2c_client(struct kobject
> *kobj) {
> struct device * const dev = kobj_to_dev(kobj);
> --
> 2.25.1
On Tue, Jun 06, 2023 at 02:05:19PM +0100, Biju Das wrote: > Add i2c_get_match_data() to get match data for both I2C and > DT-based matching, so that we can optimize the driver code that > uses both. > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2->v3: > * Added support for getting match data for both I2C and DT-based > matching. > * Added Rb tag from Geert and retained the Rb tag as change is trivial. Not so trivial, I think. Codewise yes, but the scope of the function changed as it includes DT now. So, in deed, I would appreciate a comment from Geert if he is okay with this change. I personally think it looks good.
Hi Wolfram, Thanks for the feedback. > Subject: Re: [PATCH v3] i2c: Add i2c_get_match_data() > > On Tue, Jun 06, 2023 at 02:05:19PM +0100, Biju Das wrote: > > Add i2c_get_match_data() to get match data for both I2C and DT-based > > matching, so that we can optimize the driver code that uses both. > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > v2->v3: > > * Added support for getting match data for both I2C and DT-based > > matching. > > * Added Rb tag from Geert and retained the Rb tag as change is > trivial. > > Not so trivial, I think. Codewise yes, but the scope of the function > changed as it includes DT now. So, in deed, I would appreciate a comment > from Geert if he is okay with this change. OK, Will wait for Geert's comment. Cheers, Biju
Hi Biju, On Wed, Jun 7, 2023 at 9:21 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Do we need to enhance the logic to use device_get_match_data > to support OF/ACPI/I2C match like below [1]. Why not? > > Or > > Are we happy with the current one? I don't mind. > +const void *i2c_get_match_data(const struct i2c_client *client) { > + struct device_driver *drv = client->dev.driver; > + struct i2c_driver *driver = to_i2c_driver(drv); > + const struct i2c_device_id *match; > + const void *data; > + > + data = device_get_match_data(&client->dev); if (data) return data; > + if (!data) { > + match = i2c_match_id(driver->id_table, client); > + if (!match) > + return NULL; > + > + data = (const void *)match->driver_data; > + } > + > > + return data; > > +} > > +EXPORT_SYMBOL(i2c_get_match_data); > > From: Biju Das <biju.das.jz@bp.renesas.com> > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -114,6 +114,27 @@ const struct i2c_device_id *i2c_match_id(const > > struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); > > > > +const void *i2c_get_match_data(const struct i2c_client *client) { > > + struct device_driver *drv = client->dev.driver; > > + struct i2c_driver *driver = to_i2c_driver(drv); > > + const struct i2c_device_id *match; > > + const void *data; > > + > > + if (client->dev.of_node) { > > + data = of_device_get_match_data(&client->dev); return of_device_get_match_data(&client->dev); > > + } else { > > + match = i2c_match_id(driver->id_table, client); > > + if (!match) > > + return NULL; > > + > > + data = (const void *)match->driver_data; return ... > > + } > > + > > + return data; > > +} > > +EXPORT_SYMBOL(i2c_get_match_data); Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v3] i2c: Add i2c_get_match_data() > > Hi Biju, > > On Wed, Jun 7, 2023 at 9:21 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: Wolfram, What is your preference in supporting OF/ACPI/I2C match-data as in [A]? Or Just support OF/i2C match-data as in [B]? > > Do we need to enhance the logic to use device_get_match_data to > > support OF/ACPI/I2C match like below [1]. > > Why not? > > > > > Or > > > > Are we happy with the current one? > > I don't mind. > [A] > > +const void *i2c_get_match_data(const struct i2c_client *client) { > > + struct device_driver *drv = client->dev.driver; > > + struct i2c_driver *driver = to_i2c_driver(drv); > > + const struct i2c_device_id *match; > > + const void *data; > > + > > + data = device_get_match_data(&client->dev); > > if (data) > return data; > > > + if (!data) { > > + match = i2c_match_id(driver->id_table, client); > > + if (!match) > > + return NULL; > > + > > + data = (const void *)match->driver_data; > > + } > > + > > > + return data; > > > +} > > > +EXPORT_SYMBOL(i2c_get_match_data); > > > > From: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- a/drivers/i2c/i2c-core-base.c > > > +++ b/drivers/i2c/i2c-core-base.c > > > @@ -114,6 +114,27 @@ const struct i2c_device_id *i2c_match_id(const > > > struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); > > > [B] > > > +const void *i2c_get_match_data(const struct i2c_client *client) { > > > + struct device_driver *drv = client->dev.driver; > > > + struct i2c_driver *driver = to_i2c_driver(drv); > > > + const struct i2c_device_id *match; > > > + const void *data; > > > + > > > + if (client->dev.of_node) { > > > + data = of_device_get_match_data(&client->dev); > > return of_device_get_match_data(&client->dev); > > > > + } else { > > > + match = i2c_match_id(driver->id_table, client); > > > + if (!match) > > > + return NULL; > > > + > > > + data = (const void *)match->driver_data; > > return ... > > > > + } > > > + > > > + return data; > > > +} > > > +EXPORT_SYMBOL(i2c_get_match_data); > Cheers, Biju
Hi Biju, > Do we need to enhance the logic to use device_get_match_data > to support OF/ACPI/I2C match like below [1]. Sure, the more the better. With your original patch, I thought we can add it incrementally when someone needs it and can test it. But using device_get_match_data() we already have a well-tested helper we can use right away. So, let's do that IMO. Happy hacking, Wolfram
> > +const void *i2c_get_match_data(const struct i2c_client *client) { > > + struct device_driver *drv = client->dev.driver; > > + struct i2c_driver *driver = to_i2c_driver(drv); > > + const struct i2c_device_id *match; > > + const void *data; > > + > > + data = device_get_match_data(&client->dev); > > if (data) > return data; I like Biju's version a tad more. Except for errors, and especially within small functions, I think single exit points are easier to understand.
Hi Wolfram, > Subject: Re: [PATCH v3] i2c: Add i2c_get_match_data() > > > > > +const void *i2c_get_match_data(const struct i2c_client *client) { > > > + struct device_driver *drv = client->dev.driver; > > > + struct i2c_driver *driver = to_i2c_driver(drv); > > > + const struct i2c_device_id *match; > > > + const void *data; > > > + > > > + data = device_get_match_data(&client->dev); > > > > if (data) > > return data; > > I like Biju's version a tad more. Except for errors, and especially > within small functions, I think single exit points are easier to > understand. OK, will send V4. Cheers, Biju
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index ae3af738b03f..15a49f4ba668 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -114,6 +114,27 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); +const void *i2c_get_match_data(const struct i2c_client *client) +{ + struct device_driver *drv = client->dev.driver; + struct i2c_driver *driver = to_i2c_driver(drv); + const struct i2c_device_id *match; + const void *data; + + if (client->dev.of_node) { + data = of_device_get_match_data(&client->dev); + } else { + match = i2c_match_id(driver->id_table, client); + if (!match) + return NULL; + + data = (const void *)match->driver_data; + } + + return 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 13a1ce38cb0c..3430cc2b05a6 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -367,6 +367,8 @@ 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_client *client); + static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj) { struct device * const dev = kobj_to_dev(kobj);