[PATCH v4 0/3] Extend device_get_match_data() to struct bus_type

[PATCH v4 0/3] 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.

v3->v4:
 * Documented corner case for device_get_match_data()
 * Dropped struct i2c_driver parameter from i2c_get_match_data_helper()
 * Split I2C sysfs handling in seperate patch(patch#3)
 * Added space after of_device_id for i2c_of_match_device_sysfs()
 * Added const parameter for struct i2c_client, to prevent overriding it's
   pointer.
 * Moved declaration from public i2c.h->i2c-core.h
v2->v3:
 * Added Rb tag from Andy for patch#1.
 * Extended to support i2c_of_match_device() as suggested by Andy.
 * Changed i2c_of_match_device_sysfs() as non-static function as it is
   needed for i2c_device_get_match_data().
 * Added a TODO comment to use i2c_verify_client() when it accepts const
   pointer.
 * Added multiple returns to make code path for device_get_match_data()
   faster in i2c_get_match_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 (3):
  drivers: fwnode: Extend device_get_match_data() to struct bus_type
  i2c: Add i2c_device_get_match_data() callback
  i2c: Extend i2c_device_get_match_data() to support i2c sysfs

 drivers/base/property.c     | 27 ++++++++++++++++++++-
 drivers/i2c/i2c-core-base.c | 48 ++++++++++++++++++++++++++++++-------
 drivers/i2c/i2c-core-of.c   |  5 ++--
 drivers/i2c/i2c-core.h      |  9 +++++++
 include/linux/device/bus.h  |  3 +++
 5 files changed, 81 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v4 2/3] 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>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Dropped struct i2c_driver parameter from i2c_get_match_data_helper()
 * Split I2C sysfs handling in seperate patch.
v2->v3:
 * Extended to support i2c_of_match_device() as suggested by Andy.
 * Changed i2c_of_match_device_sysfs() as non-static function as it is
   needed for i2c_device_get_match_data().
 * Added a TODO comment to use i2c_verify_client() when it accepts const
   pointer.
 * Added multiple returns to make code path for device_get_match_data()
   faster in i2c_get_match_data().
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 | 38 +++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..2436f23e63af 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,22 +114,39 @@ 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_client *client)
 {
-	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
+	const 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)
+{
+	/* TODO: use i2c_verify_client() when it accepts const pointer */
+	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
+					  to_i2c_client(dev) : NULL;
+
+	if (!client || !dev->driver)
+		return NULL;
+
+	return i2c_get_match_data_helper(client);
+}
+
+const void *i2c_get_match_data(const struct i2c_client *client)
+{
 	const void *data;
 
 	data = device_get_match_data(&client->dev);
-	if (!data) {
-		match = i2c_match_id(driver->id_table, client);
-		if (!match)
-			return NULL;
+	if (data)
+		return data;
 
-		data = (const void *)match->driver_data;
-	}
-
-	return data;
+	return i2c_get_match_data_helper(client);
 }
 EXPORT_SYMBOL(i2c_get_match_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 v4 2/3] i2c: Add i2c_device_get_match_data() callback

[Andy Shevchenko]

On Wed, Aug 02, 2023 at 12:23:16PM +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().

...

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

Yes, perfect!

...

> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> +	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
> +					  to_i2c_client(dev) : NULL;
> +
> +	if (!client || !dev->driver)
> +		return NULL;
> +
> +	return i2c_get_match_data_helper(client);


I believe below looks better from readability and maintenance perspectives.

	const struct i2c_client *client;

	/* ...comment as in Dmitry's reply in v3 thread on why we need this check... */
	if (!dev->driver)
		return NULL;

	/* TODO: use i2c_verify_client() when it accepts const pointer */
	client = (dev->type == &i2c_client_type) ? to_i2c_client(dev) : NULL;
	if (!client)
		return NULL;

> +	return i2c_get_match_data_helper(client);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v4 2/3] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Wed, Aug 02, 2023 at 12:23:16PM +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().
> 
> ...
> 
> > +static const void *i2c_get_match_data_helper(const struct i2c_client
> > +*client)
> >  {
> > -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> > +	const 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; }
> 
> Yes, perfect!
> 
> ...
> 
> > +static const void *i2c_device_get_match_data(const struct device
> > +*dev) {
> > +	/* TODO: use i2c_verify_client() when it accepts const pointer */
> > +	const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> > +					  to_i2c_client(dev) : NULL;
> > +
> > +	if (!client || !dev->driver)
> > +		return NULL;
> > +
> > +	return i2c_get_match_data_helper(client);
> 
> 
> I believe below looks better from readability and maintenance
> perspectives.

Agreed. Will Incorporate latest comment from v4 as well.

Cheers,
Biju

> 
> 	const struct i2c_client *client;
> 
> 	/* ...comment as in Dmitry's reply in v3 thread on why we need
> this check... */
> 	if (!dev->driver)
> 		return NULL;
> 
> 	/* TODO: use i2c_verify_client() when it accepts const pointer */
> 	client = (dev->type == &i2c_client_type) ? to_i2c_client(dev) :
> NULL;
> 	if (!client)
> 		return NULL;
> 
> > +	return i2c_get_match_data_helper(client);
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

[PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Biju Das]

Extend i2c_device_get_match_data() to i2c sysfs interface by changing
i2c_of_match_device_sysfs() to non-static and call this function as a
fallback for i2c_get_match_data_helper().

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * split from patch #2
 * Added space after of_device_id for i2c_of_match_device_sysfs()
 * Added const parameter for struct i2c_client, to prevent overriding it's
   pointer.
 * Moved declaration from public i2c.h->i2c-core.h
---
 drivers/i2c/i2c-core-base.c | 16 +++++++++++++++-
 drivers/i2c/i2c-core-of.c   |  5 +++--
 drivers/i2c/i2c-core.h      |  9 +++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2436f23e63af..d002c5a1cc84 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -131,11 +131,25 @@ static const void *i2c_device_get_match_data(const struct device *dev)
 	/* TODO: use i2c_verify_client() when it accepts const pointer */
 	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
 					  to_i2c_client(dev) : NULL;
+	const void *data;
 
 	if (!client || !dev->driver)
 		return NULL;
 
-	return i2c_get_match_data_helper(client);
+	data = i2c_get_match_data_helper(client);
+	if (data)
+		return data;
+
+	if (dev->driver->of_match_table) {
+		const struct of_device_id *match;
+
+		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
+						  client);
+		if (match)
+			return match->data;
+	}
+
+	return data;
 }
 
 const void *i2c_get_match_data(const struct i2c_client *client)
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..179b3486904a 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -113,9 +113,9 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 	of_node_put(bus);
 }
 
-static const struct of_device_id*
+const struct of_device_id *
 i2c_of_match_device_sysfs(const struct of_device_id *matches,
-				  struct i2c_client *client)
+			  const struct i2c_client *client)
 {
 	const char *name;
 
@@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
 
 const struct of_device_id
 *i2c_of_match_device(const struct of_device_id *matches,
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 1247e6e6e975..e4d397b67989 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) {
 
 #ifdef CONFIG_OF
 void of_i2c_register_devices(struct i2c_adapter *adap);
+const struct of_device_id *
+i2c_of_match_device_sysfs(const struct of_device_id *matches,
+			  const struct i2c_client *client);
 #else
 static inline void of_i2c_register_devices(struct i2c_adapter *adap) { }
+static inline const struct of_device_id *
+i2c_of_match_device_sysfs(const struct of_device_id *matches,
+			  const struct i2c_client *client)
+{
+	return NULL;
+}
 #endif
 extern struct notifier_block i2c_of_notifier;
 
-- 
2.25.1

Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Dmitry Torokhov]

Hi Niju,

On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote:
> @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches,
>  
>  	return NULL;
>  }
> +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);

This is private to i2c core, you do not need to export it to modules.

Thanks.

-- 
Dmitry

RE: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Biju Das]

Hi Dmitry Torokhov,

Thanks for the feedback.

> Subject: Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to
> support i2c sysfs
> 
> Hi Niju,
> 
> On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote:
> > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct
> of_device_id *matches,
> >
> >  	return NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
> 
> This is private to i2c core, you do not need to export it to modules.

Agreed. Drop this export in next version.

Cheers,
Biju

Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Andy Shevchenko]

On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote:
> Extend i2c_device_get_match_data() to i2c sysfs interface by changing
> i2c_of_match_device_sysfs() to non-static and call this function as a
> fallback for i2c_get_match_data_helper().

...

>  	/* TODO: use i2c_verify_client() when it accepts const pointer */
>  	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
>  					  to_i2c_client(dev) : NULL;
> +	const void *data;

Oh, yeah, definitely, please follow advice for previous patch I have given.

>  	if (!client || !dev->driver)
>  		return NULL;
>  
> -	return i2c_get_match_data_helper(client);
> +	data = i2c_get_match_data_helper(client);
> +	if (data)
> +		return data;
> +
> +	if (dev->driver->of_match_table) {
> +		const struct of_device_id *match;
> +
> +		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
> +						  client);

So, with this it makes sense to have

	strucct device_driver *drv = dev->driver;
	...
	if (!drv)
		return NULL;

The above for the first patch even. And here

		match = i2c_of_match_device_sysfs(drv->of_match_table, client);

> +		if (match)
> +			return match->data;
> +	}

> +	return data;

This actually should be

	return NULL;

to avoid (potentially) stale pointers.

>  }

...

> -static const struct of_device_id*
> +const struct of_device_id *
>  i2c_of_match_device_sysfs(const struct of_device_id *matches,
> -				  struct i2c_client *client)
> +			  const struct i2c_client *client)
>  {
>  	const char *name;
>  
> @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches,
>  
>  	return NULL;
>  }
> +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
>  
>  const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 1247e6e6e975..e4d397b67989 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) {
>  
>  #ifdef CONFIG_OF
>  void of_i2c_register_devices(struct i2c_adapter *adap);
> +const struct of_device_id *
> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  const struct i2c_client *client);
>  #else
>  static inline void of_i2c_register_devices(struct i2c_adapter *adap) { }
> +static inline const struct of_device_id *
> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  const struct i2c_client *client)
> +{
> +	return NULL;
> +}
>  #endif
>  extern struct notifier_block i2c_of_notifier;

That's what above I meant to see as a separate change.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to
> support i2c sysfs
> 
> On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote:
> > Extend i2c_device_get_match_data() to i2c sysfs interface by changing
> > i2c_of_match_device_sysfs() to non-static and call this function as a
> > fallback for i2c_get_match_data_helper().
> 
> ...
> 
> >  	/* TODO: use i2c_verify_client() when it accepts const pointer */
> >  	const struct i2c_client *client = (dev->type ==
> &i2c_client_type) ?
> >  					  to_i2c_client(dev) : NULL;
> > +	const void *data;
> 
> Oh, yeah, definitely, please follow advice for previous patch I have
> given.

OK.

> 
> >  	if (!client || !dev->driver)
> >  		return NULL;
> >
> > -	return i2c_get_match_data_helper(client);
> > +	data = i2c_get_match_data_helper(client);
> > +	if (data)
> > +		return data;
> > +
> > +	if (dev->driver->of_match_table) {
> > +		const struct of_device_id *match;
> > +
> > +		match = i2c_of_match_device_sysfs(dev->driver-
> >of_match_table,
> > +						  client);
> 
> So, with this it makes sense to have
> 
> 	strucct device_driver *drv = dev->driver;
> 	...
> 	if (!drv)
> 		return NULL;
> 
> The above for the first patch even. And here

Agreed.

> 
> 		match = i2c_of_match_device_sysfs(drv->of_match_table,
> client);
> 
> > +		if (match)
> > +			return match->data;
> > +	}
> 
> > +	return data;
> 
> This actually should be
> 
> 	return NULL;
> 
> to avoid (potentially) stale pointers.

OK.

> 
> >  }
> 
> ...
> 
> > -static const struct of_device_id*
> > +const struct of_device_id *
> >  i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > -				  struct i2c_client *client)
> > +			  const struct i2c_client *client)
> >  {
> >  	const char *name;
> >
> > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct
> > of_device_id *matches,
> >
> >  	return NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
> >
> >  const struct of_device_id
> >  *i2c_of_match_device(const struct of_device_id *matches, diff --git
> > a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index
> > 1247e6e6e975..e4d397b67989 100644
> > --- a/drivers/i2c/i2c-core.h
> > +++ b/drivers/i2c/i2c-core.h
> > @@ -82,8 +82,17 @@ static inline void
> > i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) {
> >
> >  #ifdef CONFIG_OF
> >  void of_i2c_register_devices(struct i2c_adapter *adap);
> > +const struct of_device_id *
> > +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > +			  const struct i2c_client *client);
> >  #else
> >  static inline void of_i2c_register_devices(struct i2c_adapter *adap)
> > { }
> > +static inline const struct of_device_id *
> > +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> > +			  const struct i2c_client *client) {
> > +	return NULL;
> > +}
> >  #endif
> >  extern struct notifier_block i2c_of_notifier;
> 
> That's what above I meant to see as a separate change.

OK will create a separate patch for this change.

Cheers,
Biju