[PATCH v5 RESEND 0/4] Extend device_get_match_data() to struct bus_type

[PATCH v5 RESEND 0/4] 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.

v4->v5:
 * Added const struct device_driver variable 'drv' in i2c_device_get_match
   _data().
 * For code readability and maintenance perspective, added separate NULL
   check for drv and client variable and added comment for NULL check for
   drv variable.
 * Created separate patch for converting i2c_of_match_device_sysfs() to
   non-static.
 * Removed export symbol for i2c_of_match_device_sysfs().
 * Replaced 'dev->driver'->'drv'.
 * Replaced return value data->NULL to avoid (potentially) stale pointers,
   if there is no match.

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 separate 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 (4):
  drivers: fwnode: Extend device_get_match_data() to struct bus_type
  i2c: Add i2c_device_get_match_data() callback
  i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static
  i2c: Extend i2c_device_get_match_data() to support i2c sysfs

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

-- 
2.25.1

[PATCH v5 RESEND 2/4] 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>
---
v4->v5:
 * Added const struct device_driver variable 'drv' in i2c_device_get_match
   _data().
 * For code readability and maintenance perspective, added separate NULL
   check for drv and client variable and added comment for NULL check for
   drv variable.
v3->v4:
 * Dropped struct i2c_driver parameter from i2c_get_match_data_helper()
 * Split I2C sysfs handling in separate 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 | 47 +++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..e738cfba9c3b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,22 +114,48 @@ 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)
+{
+	const struct device_driver *drv = dev->driver;
+	const struct i2c_client *client;
+
+	/*
+	 * It is not guaranteed that the function is always called on a device
+	 * bound to a driver (even though we normally expect this to be the
+	 * case).
+	 */
+	if (!drv)
+		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);
+}
+
+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 +721,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

[PATCH v5 RESEND 3/4] i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static

[Biju Das]

Currently i2c_of_match_device_sysfs() is used by i2c_of_match_device().
Convert this to non-static function for finding match data for the I2C
sysfs interface using i2c_device_get_match_data() for code reuse.

While at it, fix the below issues:
 1) Replace 'of_device_id*'->'of_device_id *' in function definition.
 2) Fix the alignment in the function definition.
 3) Change the struct i2c_client parameter as const to avoid overriding
    the client pointer.

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>
---
v5:
 * Split from patch #3
 * Removed export symbol
---
 drivers/i2c/i2c-core-of.c | 4 ++--
 drivers/i2c/i2c-core.h    | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..33832622f436 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;
 
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

[PATCH v5 RESEND 4/4] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Biju Das]

Extend i2c_device_get_match_data() to i2c sysfs interface for
retrieving match data from the match table.

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>
---
v5:
 * Separated non-static function change as separate patch#3
 * Replaced 'dev->driver'->'drv'.
 * Replaced return value data->NULL to avoid (potentially) stale pointers,
   if there is no match.
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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index e738cfba9c3b..d543460e47c2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -130,6 +130,7 @@ static const void *i2c_device_get_match_data(const struct device *dev)
 {
 	const struct device_driver *drv = dev->driver;
 	const struct i2c_client *client;
+	const void *data;
 
 	/*
 	 * It is not guaranteed that the function is always called on a device
@@ -144,7 +145,19 @@ static const void *i2c_device_get_match_data(const struct device *dev)
 	if (!client)
 		return NULL;
 
-	return i2c_get_match_data_helper(client);
+	data = i2c_get_match_data_helper(client);
+	if (data)
+		return data;
+
+	if (drv->of_match_table) {
+		const struct of_device_id *match;
+
+		match = i2c_of_match_device_sysfs(drv->of_match_table, client);
+		if (match)
+			return match->data;
+	}
+
+	return NULL;
 }
 
 const void *i2c_get_match_data(const struct i2c_client *client)
-- 
2.25.1

Re: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data() callback

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:31:00AM +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().

It seems you are missing to Cc Andi for all these... (not your fault,
rather unfortunately).

Yes, while he is not directly involved into core changes the drivers
are pretty much should consider this change.

...

>  	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;

Looking at this, it _might_ make sense to split another patch to prepare for
better difference here.

 -	if (!data) {
 -		match = i2c_match_id(driver->id_table, client);
 -		if (!match)
 -			return NULL;
 +	if (data)
 +		return data;
 +
 +	match = i2c_match_id(driver->id_table, client);
 +	if (!match)
 +		return NULL;
 +
 +	return (const void *)match->driver_data;

 Just play with this idea.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 3/4] i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:31:01AM +0100, Biju Das wrote:
> Currently i2c_of_match_device_sysfs() is used by i2c_of_match_device().
> Convert this to non-static function for finding match data for the I2C
> sysfs interface using i2c_device_get_match_data() for code reuse.
> 
> While at it, fix the below issues:
>  1) Replace 'of_device_id*'->'of_device_id *' in function definition.
>  2) Fix the alignment in the function definition.
>  3) Change the struct i2c_client parameter as const to avoid overriding
>     the client pointer.

All makes sense
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 4/4] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:31:02AM +0100, Biju Das wrote:
> Extend i2c_device_get_match_data() to i2c sysfs interface for
> retrieving match data from the match table.

...

> +	const void *data;

> -	return i2c_get_match_data_helper(client);
> +	data = i2c_get_match_data_helper(client);
> +	if (data)
> +		return data;

> +	return NULL;
>  }

These may be incorporated to the previous patch that introduces the helper.
This patch after that will look much better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 0/4] Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:30:58AM +0100, Biju Das wrote:
> This patch series extend device_get_match_data() to struct bus_type,
> so that buses like I2C can get matched data.

Thank you! We are getting closer, I believe v6 will be the one against which
I give my Rb tag.

Nice job!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data() callback

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Thu, Aug 03, 2023 at 11:31:00AM +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().
> 
> It seems you are missing to Cc Andi for all these... (not your fault,
> rather unfortunately).
> 
> Yes, while he is not directly involved into core changes the drivers are
> pretty much should consider this change.

Sure.

> 
> ...
> 
> >  	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;
> 
> Looking at this, it _might_ make sense to split another patch to prepare
> for better difference here.

OK.

> 
>  -	if (!data) {
>  -		match = i2c_match_id(driver->id_table, client);
>  -		if (!match)
>  -			return NULL;
>  +	if (data)
>  +		return data;
>  +
>  +	match = i2c_match_id(driver->id_table, client);
>  +	if (!match)
>  +		return NULL;
>  +
>  +	return (const void *)match->driver_data;
> 
>  Just play with this idea.

Does these below 2 patches ok?

PATCH x:
-------
Subject: [PATCH 1/2] i2c: Enhance i2c_get_match_data()

Enhance i2c_get_match_data() for a faster path for device_get_
match_data().

While at it, add const to struct i2c_driver to prevent overriding
the driver pointer.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/i2c-core-base.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..7005dfe64066 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -116,20 +116,19 @@ EXPORT_SYMBOL_GPL(i2c_match_id);
 
 const void *i2c_get_match_data(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;
 	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;
-	}
+	match = i2c_match_id(driver->id_table, client);
+	if (!match)
+		return NULL;
 
-	return data;
+	return (const void *)match->driver_data;
 }
 EXPORT_SYMBOL(i2c_get_match_data);

Patch x+1:
---------

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

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>
---
 drivers/i2c/i2c-core-base.c | 53 ++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7005dfe64066..d543460e47c2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,15 +114,10 @@ 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)
 {
 	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	const struct i2c_device_id *match;
-	const void *data;
-
-	data = device_get_match_data(&client->dev);
-	if (data)
-		return data;
 
 	match = i2c_match_id(driver->id_table, client);
 	if (!match)
@@ -130,6 +125,51 @@ const void *i2c_get_match_data(const struct i2c_client *client)
 
 	return (const void *)match->driver_data;
 }
+
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+	const struct device_driver *drv = dev->driver;
+	const struct i2c_client *client;
+	const void *data;
+
+	/*
+	 * It is not guaranteed that the function is always called on a device
+	 * bound to a driver (even though we normally expect this to be the
+	 * case).
+	 */
+	if (!drv)
+		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;
+
+	data = i2c_get_match_data_helper(client);
+	if (data)
+		return data;
+
+	if (drv->of_match_table) {
+		const struct of_device_id *match;
+
+		match = i2c_of_match_device_sysfs(drv->of_match_table, client);
+		if (match)
+			return match->data;
+	}
+
+	return NULL;
+}
+
+const void *i2c_get_match_data(const struct i2c_client *client)
+{
+	const void *data;
+
+	data = device_get_match_data(&client->dev);
+	if (data)
+		return data;
+
+	return i2c_get_match_data_helper(client);
+}
 EXPORT_SYMBOL(i2c_get_match_data);
 
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
@@ -694,6 +734,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);

Cheers,
Biju

Re: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data() callback

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 01:52:05PM +0000, Biju Das wrote:
> > On Thu, Aug 03, 2023 at 11:31:00AM +0100, Biju Das wrote:

> Does these below 2 patches ok?

Looks awesome!

-- 
With Best Regards,
Andy Shevchenko