[PATCH v2 0/2] Extend device_get_match_data() to struct bus_type

[PATCH v2 0/2] Extend device_get_match_data() to struct bus_type

[Biju Das]

This patch series extend device_get_match_data() to struct bus_type,
so that buses like I2C can get matched data.

RFC v1->v2:
 * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry.
 * Documented device_get_match_data().
 * Added multiple returns to make code path for generic fwnode-based
   lookup faster.
 * Fixed build warnings reported by kernel test robot <lkp@intel.com>
 * Added const qualifier to return type and parameter struct i2c_driver
   in i2c_get_match_data_helper().
 * Added const qualifier to struct i2c_driver in i2c_get_match_data()
 * Dropped driver variable from i2c_device_get_match_data()
 * Replaced to_i2c_client with logic for assigning verify_client as it
   returns non const pointer.

Biju Das (2):
  drivers: fwnode: Extend device_get_match_data() to struct bus_type
  i2c: Add i2c_device_get_match_data() callback

 drivers/base/property.c     | 21 ++++++++++++++++++++-
 drivers/i2c/i2c-core-base.c | 36 +++++++++++++++++++++++++++---------
 include/linux/device/bus.h  |  3 +++
 3 files changed, 50 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Add i2c_device_get_match_data() callback to struct bus_type().

While at it, introduced i2c_get_match_data_helper() to avoid code
duplication with i2c_get_match_data().

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
RFC V1->v2:
 * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry.
 * Fixed build warnings reported by kernel test robot <lkp@intel.com>
 * Added const qualifier to return type and parameter struct i2c_driver
   in i2c_get_match_data_helper().
 * Added const qualifier to struct i2c_driver in i2c_get_match_data()
 * Dropped driver variable from i2c_device_get_match_data()
 * Replaced to_i2c_client with logic for assigning verify_client as it
   returns non const pointer.
---
 drivers/i2c/i2c-core-base.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..6183e9e36889 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,20 +114,37 @@ 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)
+static const void *i2c_get_match_data_helper(const struct i2c_driver *driver,
+					     const struct i2c_client *client)
 {
-	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	const struct i2c_device_id *match;
+
+	match = i2c_match_id(driver->id_table, client);
+	if (!match)
+		return NULL;
+
+	return (const void *)match->driver_data;
+}
+
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
+					  to_i2c_client(dev) : NULL;
+
+	if (!dev->driver)
+		return NULL;
+
+	return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
+}
+
+const void *i2c_get_match_data(const struct i2c_client *client)
+{
+	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	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;
-	}
+	if (!data)
+		data = i2c_get_match_data_helper(driver, client);
 
 	return data;
 }
@@ -695,6 +712,7 @@ struct bus_type i2c_bus_type = {
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
+	.get_match_data	= i2c_device_get_match_data,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
 
-- 
2.25.1

Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Andy Shevchenko]

On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
> 
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().

I have not been Cc'ed to this...

...

> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> +					  to_i2c_client(dev) : NULL;

There is an API i2c_verify_client() or something like this, I don't remember
by heart.

> +	if (!dev->driver)
> +		return NULL;
> +
> +	return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> +}

...

> +const void *i2c_get_match_data(const struct i2c_client *client)
> +{
> +	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
>  	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;
> -	}
> +	if (!data)
> +		data = i2c_get_match_data_helper(driver, client);

	if (data)
		return data;

	return i2c_...;

>  
>  	return data;
>  }

...

Side question, what is the idea for i2c_of_match_device()? Shouldn't you also
take it into consideration?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Dmitry Torokhov]

On Wed, Jul 26, 2023 at 07:44:17PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> >
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
>
> I have not been Cc'ed to this...
>
> ...
>
> > +static const void *i2c_device_get_match_data(const struct device *dev)
> > +{
> > +   const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> > +                                     to_i2c_client(dev) : NULL;
>
> There is an API i2c_verify_client() or something like this, I don't remember
> by heart.

It's been discussed in a separate thread. i2c_verify_client() needs a
non-const pointer. It would be nice to clean up i2c_verify_client() to
accept both variants, but that can be done later.

>
> > +   if (!dev->driver)
> > +           return NULL;
> > +
> > +   return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> > +}
>
> ...
>
> > +const void *i2c_get_match_data(const struct i2c_client *client)
> > +{
> > +   const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> >     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;
> > -   }
> > +   if (!data)
> > +           data = i2c_get_match_data_helper(driver, client);
>
>       if (data)
>               return data;
>
>       return i2c_...;
>
> >
> >     return data;
> >  }
>
> ...
>
> Side question, what is the idea for i2c_of_match_device()? Shouldn't you also
> take it into consideration?

Good call. I think we need to add something like

        if (!data && driver->driver.of_match_table) {
                match =
i2c_of_match_device_sysfs(driver->driver.of_match_table, client);
                if (match)
                        data = match->data;
        }

to i2c_device_get_match_data().

Thanks.


--
Dmitry

Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Andy Shevchenko]

On Wed, Jul 26, 2023 at 10:56:41AM -0700, Dmitry Torokhov wrote:
> On Wed, Jul 26, 2023 at 07:44:17PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote:

...

> > > +static const void *i2c_device_get_match_data(const struct device *dev)
> > > +{
> > > +   const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> > > +                                     to_i2c_client(dev) : NULL;
> >
> > There is an API i2c_verify_client() or something like this, I don't remember
> > by heart.
> 
> It's been discussed in a separate thread. i2c_verify_client() needs a
> non-const pointer. It would be nice to clean up i2c_verify_client() to
> accept both variants, but that can be done later.

Then this code needs a TODO comment:

	/* TODO: use i2c_verify_client() when it accepts const pointer */


> > > +   if (!dev->driver)
> > > +           return NULL;
> > > +
> > > +   return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> > > +}

...

> > Side question, what is the idea for i2c_of_match_device()? Shouldn't you also
> > take it into consideration?
> 
> Good call. I think we need to add something like
> 
>         if (!data && driver->driver.of_match_table) {
>                 match =
> i2c_of_match_device_sysfs(driver->driver.of_match_table, client);
>                 if (match)
>                         data = match->data;
>         }
> 
> to i2c_device_get_match_data().

Haven't checked myself, by I trust your suggestion. Let's see it in v3 then.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Wed, Jul 26, 2023 at 10:56:41AM -0700, Dmitry Torokhov wrote:
> > On Wed, Jul 26, 2023 at 07:44:17PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote:
> 
> ...
> 
> > > > +static const void *i2c_device_get_match_data(const struct device
> > > > +*dev) {
> > > > +   const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> > > > +                                     to_i2c_client(dev) : NULL;
> > >
> > > There is an API i2c_verify_client() or something like this, I don't
> > > remember by heart.
> >
> > It's been discussed in a separate thread. i2c_verify_client() needs a
> > non-const pointer. It would be nice to clean up i2c_verify_client() to
> > accept both variants, but that can be done later.
> 
> Then this code needs a TODO comment:
> 
> 	/* TODO: use i2c_verify_client() when it accepts const pointer */

Agreed.

> 
> 
> > > > +   if (!dev->driver)
> > > > +           return NULL;
> > > > +
> > > > +   return i2c_get_match_data_helper(to_i2c_driver(dev->driver),
> > > > +client); }
> 
> ...
> 
> > > Side question, what is the idea for i2c_of_match_device()? Shouldn't
> > > you also take it into consideration?
> >
> > Good call. I think we need to add something like
> >
> >         if (!data && driver->driver.of_match_table) {
> >                 match =
> > i2c_of_match_device_sysfs(driver->driver.of_match_table, client);
> >                 if (match)
> >                         data = match->data;
> >         }
> >
> > to i2c_device_get_match_data().
> 
> Haven't checked myself, by I trust your suggestion. Let's see it in v3
> then.

OK, will send V3 to provide feedback.

Cheers,
Biju

RE: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote:
> > Add i2c_device_get_match_data() callback to struct bus_type().
> >
> > While at it, introduced i2c_get_match_data_helper() to avoid code
> > duplication with i2c_get_match_data().
> 
> I have not been Cc'ed to this...

I execute one script per patch, to pick the recipient list. Somehow it did not pick your name. Next time I will make sure you are on the Cc list.

> 
> ...
> 
> > +static const void *i2c_device_get_match_data(const struct device
> > +*dev) {
> > +	const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> > +					  to_i2c_client(dev) : NULL;
> 
> There is an API i2c_verify_client() or something like this, I don't
> remember by heart.

Dmitry already responded.

> 
> > +	if (!dev->driver)
> > +		return NULL;
> > +
> > +	return i2c_get_match_data_helper(to_i2c_driver(dev->driver),
> > +client); }
> 
> ...
> 
> > +const void *i2c_get_match_data(const struct i2c_client *client) {
> > +	const struct i2c_driver *driver = to_i2c_driver(client-
> >dev.driver);
> >  	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;
> > -	}
> > +	if (!data)
> > +		data = i2c_get_match_data_helper(driver, client);
> 
> 	if (data)
> 		return data;
> 
> 	return i2c_...;

OK.

> 
> >
> >  	return data;
> >  }
> 
> ...
> 
> Side question, what is the idea for i2c_of_match_device()? Shouldn't you
> also take it into consideration?

I guess you are ok with Dmitry's suggestion.

Cheers,
Biju