RE: [PATCH v3] i2c: Add i2c_get_match_data()

RE: [PATCH v3] i2c: Add i2c_get_match_data()

[Biju Das]

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

Re: [PATCH v3] i2c: Add i2c_get_match_data()

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3] i2c: Add i2c_get_match_data()

[Biju Das]

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

Re: [PATCH v3] i2c: Add i2c_get_match_data()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]


> > +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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3] i2c: Add i2c_get_match_data()

[Biju Das]

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

Re: [PATCH v3] i2c: Add i2c_get_match_data()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]