All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Extend device_get_match_data() to struct bus_type
@ 2023-08-02 11:23 Biju Das
  2023-08-02 11:23 ` [PATCH v4 1/3] drivers: fwnode: " Biju Das
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Biju Das @ 2023-08-02 11:23 UTC (permalink / raw)
  To: Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
	Greg Kroah-Hartman, Rafael J. Wysocki
  Cc: Biju Das, linux-acpi, Dmitry Torokhov, Wolfram Sang,
	Geert Uytterhoeven, linux-i2c, linux-renesas-soc

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 1/3] drivers: fwnode: Extend device_get_match_data() to struct bus_type
  2023-08-02 11:23 [PATCH v4 0/3] Extend device_get_match_data() to struct bus_type Biju Das
@ 2023-08-02 11:23 ` Biju Das
  2023-08-02 11:23 ` [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback Biju Das
  2023-08-02 11:23 ` [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs Biju Das
  2 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-02 11:23 UTC (permalink / raw)
  To: Andy Shevchenko, Daniel Scally, Heikki Krogerus, Sakari Ailus,
	Greg Kroah-Hartman, Rafael J. Wysocki
  Cc: Biju Das, linux-acpi, Dmitry Torokhov, Wolfram Sang,
	Geert Uytterhoeven, linux-renesas-soc

Extend device_get_match_data() to buses (for eg: I2C) by adding a
callback device_get_match_data() to struct bus_type() and call this method
as a fallback for generic fwnode based device_get_match_data().

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3->v4:
 * Documented corner case.
v2->v3:
 * Added Rb tag from Andy.
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.
---
 drivers/base/property.c    | 27 ++++++++++++++++++++++++++-
 include/linux/device/bus.h |  3 +++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8c40abed7852..a3c188cf68bb 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1275,9 +1275,34 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
+/**
+ * device_get_match_data - get match data from OF/ACPI/Bus match tables
+ * @dev: device to find the match data
+ *
+ * Find match data using generic fwnode-based lookup and if there is no
+ * match, call the bus->get_match_data() for finding match data.
+ *
+ * Return: a match data pointer or NULL if there is no match in the matching
+ * table.
+ *
+ * Besides the fact that some drivers abuse the device ID driver_data type
+ * and claim it to be integer, for the bus specific ID tables the driver_data
+ * may be defined as kernel_ulong_t. For these tables 0 is a valid response,
+ * but not for this function. It's recommended to convert those either to avoid
+ * 0 or use a real pointer to the predefined driver data.
+ */
 const void *device_get_match_data(const struct device *dev)
 {
-	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+	const void *data;
+
+	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+	if (data)
+		return data;
+
+	if (dev->bus && dev->bus->get_match_data)
+		return dev->bus->get_match_data(dev);
+
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
 
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index ae10c4322754..2e15b0ae5384 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -60,6 +60,7 @@ struct fwnode_handle;
  *			this bus.
  * @dma_cleanup:	Called to cleanup DMA configuration on a device on
  *			this bus.
+ * @get_match_data:	Called to get match data on a device on this bus.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
  * @iommu_ops:  IOMMU specific operations for this bus, used to attach IOMMU
@@ -102,6 +103,8 @@ struct bus_type {
 	int (*dma_configure)(struct device *dev);
 	void (*dma_cleanup)(struct device *dev);
 
+	const void *(*get_match_data)(const struct device *dev);
+
 	const struct dev_pm_ops *pm;
 
 	const struct iommu_ops *iommu_ops;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback
  2023-08-02 11:23 [PATCH v4 0/3] Extend device_get_match_data() to struct bus_type Biju Das
  2023-08-02 11:23 ` [PATCH v4 1/3] drivers: fwnode: " Biju Das
@ 2023-08-02 11:23 ` Biju Das
  2023-08-02 15:08   ` Andy Shevchenko
  2023-08-02 11:23 ` [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs Biju Das
  2 siblings, 1 reply; 10+ messages in thread
From: Biju Das @ 2023-08-02 11:23 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Biju Das, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	Andy Shevchenko, linux-renesas-soc

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs
  2023-08-02 11:23 [PATCH v4 0/3] Extend device_get_match_data() to struct bus_type Biju Das
  2023-08-02 11:23 ` [PATCH v4 1/3] drivers: fwnode: " Biju Das
  2023-08-02 11:23 ` [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback Biju Das
@ 2023-08-02 11:23 ` Biju Das
  2023-08-02 18:00   ` Dmitry Torokhov
  2023-08-02 21:39   ` Andy Shevchenko
  2 siblings, 2 replies; 10+ messages in thread
From: Biju Das @ 2023-08-02 11:23 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Biju Das, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	Andy Shevchenko, linux-renesas-soc

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback
  2023-08-02 11:23 ` [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback Biju Das
@ 2023-08-02 15:08   ` Andy Shevchenko
  2023-08-03  6:01     ` Biju Das
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2023-08-02 15:08 UTC (permalink / raw)
  To: Biju Das
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	linux-renesas-soc

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs
  2023-08-02 11:23 ` [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs Biju Das
@ 2023-08-02 18:00   ` Dmitry Torokhov
  2023-08-03  6:05     ` Biju Das
  2023-08-02 21:39   ` Andy Shevchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2023-08-02 18:00 UTC (permalink / raw)
  To: Biju Das
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Andy Shevchenko,
	linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs
  2023-08-02 11:23 ` [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs Biju Das
  2023-08-02 18:00   ` Dmitry Torokhov
@ 2023-08-02 21:39   ` Andy Shevchenko
  2023-08-03  6:04     ` Biju Das
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2023-08-02 21:39 UTC (permalink / raw)
  To: Biju Das
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	linux-renesas-soc

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback
  2023-08-02 15:08   ` Andy Shevchenko
@ 2023-08-03  6:01     ` Biju Das
  0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-03  6:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	linux-renesas-soc

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
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs
  2023-08-02 21:39   ` Andy Shevchenko
@ 2023-08-03  6:04     ` Biju Das
  0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-03  6:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Dmitry Torokhov,
	linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs
  2023-08-02 18:00   ` Dmitry Torokhov
@ 2023-08-03  6:05     ` Biju Das
  0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-03  6:05 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Wolfram Sang, linux-i2c, Geert Uytterhoeven, Andy Shevchenko,
	linux-renesas-soc


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-08-03  6:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-02 11:23 [PATCH v4 0/3] Extend device_get_match_data() to struct bus_type Biju Das
2023-08-02 11:23 ` [PATCH v4 1/3] drivers: fwnode: " Biju Das
2023-08-02 11:23 ` [PATCH v4 2/3] i2c: Add i2c_device_get_match_data() callback Biju Das
2023-08-02 15:08   ` Andy Shevchenko
2023-08-03  6:01     ` Biju Das
2023-08-02 11:23 ` [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs Biju Das
2023-08-02 18:00   ` Dmitry Torokhov
2023-08-03  6:05     ` Biju Das
2023-08-02 21:39   ` Andy Shevchenko
2023-08-03  6:04     ` Biju Das

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.