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

[PATCH v7 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.

There is a plan to replace i2c_get_match_data()->device_get_match_data()
later, once this patch hits mainline as it is redundant.

v6->v7:
 * Added ack from Greg Kroah-Hartman for patch#1
 * Swapped patch#2 and patch#3 from v6.
 * Added Rb tag from Andy for patch#2 and patch#4
 * Updated commit description of patch#2 by removing unnecessary wrapping.
 * Updated typo in commit description struct bus_type()->struct bus_type.
v5->v6:
 * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_
   data() and this function become redundant once this patch series hits
   mainline.
 * Added Rb tag from Sakari for patch#1.
 * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4.
 * Added Rb tag from Andy for patch#2
 * Separate patch#3 to prepare for better difference for
   i2c_match_id() changes.
 * Merged patch#4 from v5 with patch#4.
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: Enhance i2c_get_match_data()
  i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static
  i2c: Add i2c_device_get_match_data() callback

 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 v7 1/4] drivers: fwnode: Extend device_get_match_data() to struct bus_type

[Biju Das]

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>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v6->v7:
 * Added ack from Greg Kroah-Hartman.
v5->v6:
 * Added Rb tag from Sakari.
v4->v5:
 * No change
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

[PATCH v7 2/4] i2c: Enhance i2c_get_match_data()

[Biju Das]

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.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v6->v7:
 * Updated commit description by removing unnecessary wrapping.
 * Moved the patch from #3 to #2.
 * Added Rb tag from Andy
v6:
 * Separate patch to prepare for better difference for
   i2c_match_id() changes.
---
 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);
 
-- 
2.25.1

[PATCH v7 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v6->v7:
 * Moved from patch#2 to patch#3.
v5->v6:
 * Added Rb tag from Andy.
 * Moved to patch#2
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 v7 4/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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v6->v7:
 * Updated typo in commit description struct bus_type()->struct bus_type.
 * Added Rb tag from Andy.
v5->v6:
 * Merged with patch#3 from v5.
 * Separate patch#3 to prepare for better difference for
   i2c_match_id() changes.
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 | 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);
 
-- 
2.25.1

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

[Jonathan Cameron]

On Fri,  4 Aug 2023 17:17:24 +0100
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> This patch series extend device_get_match_data() to struct bus_type,
> so that buses like I2C can get matched data.
> 
> There is a plan to replace i2c_get_match_data()->device_get_match_data()
> later, once this patch hits mainline as it is redundant.

Are we sure we don't have any instances left of the pattern that
used to be common (typically for drivers where dt tables were added
later) of

	chip_info = device_get_match_data();
	if (!chip_info) {
		chip_info = arrayofchipinfo[id->driver_data];	
	}

Looks like the first driver I checked, iio/adc/max1363.c does
this still and will I think break with this series.

Or am I missing some reason this isn't a problem?

Clearly this only matters if we get to the bus callback, but
enabling that is the whole point of this series.  Hence
I think a lot of auditing is needed before this can be safely applied.

Jonathan

> v6->v7:
>  * Added ack from Greg Kroah-Hartman for patch#1
>  * Swapped patch#2 and patch#3 from v6.
>  * Added Rb tag from Andy for patch#2 and patch#4
>  * Updated commit description of patch#2 by removing unnecessary wrapping.
>  * Updated typo in commit description struct bus_type()->struct bus_type.
> v5->v6:
>  * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_
>    data() and this function become redundant once this patch series hits
>    mainline.
>  * Added Rb tag from Sakari for patch#1.
>  * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4.
>  * Added Rb tag from Andy for patch#2
>  * Separate patch#3 to prepare for better difference for
>    i2c_match_id() changes.
>  * Merged patch#4 from v5 with patch#4.
> 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: Enhance i2c_get_match_data()
>   i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static
>   i2c: Add i2c_device_get_match_data() callback
> 
>  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(-)
> 

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Jonathan Cameron,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Fri,  4 Aug 2023 17:17:24 +0100
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > This patch series extend device_get_match_data() to struct bus_type,
> > so that buses like I2C can get matched data.
> >
> > There is a plan to replace
> > i2c_get_match_data()->device_get_match_data()
> > later, once this patch hits mainline as it is redundant.
> 
> Are we sure we don't have any instances left of the pattern that used to
> be common (typically for drivers where dt tables were added
> later) of
> 
> 	chip_info = device_get_match_data();
> 	if (!chip_info) {
> 		chip_info = arrayofchipinfo[id->driver_data];
> 	}
> 
> Looks like the first driver I checked, iio/adc/max1363.c does this still
> and will I think break with this series.
> 
> Or am I missing some reason this isn't a problem?

Good catch, this will break.
we need to make I2C table like OF/ACPI tables.

Yes, before adding callback support to i2c_device_get_match_data(),
We need to make similar table for OF/ACPI/I2C for all i2c drivers
that use device_get_match_data()or of_get_device_match_data().

May be first intermediate step is to use i2c_get_match_data()
For such table conversion. Once all table conversion is done
then we can add i2c_device_get_match_data() callback.

The below one is the recommendation from Andy.

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

> 
> Clearly this only matters if we get to the bus callback, but enabling that
> is the whole point of this series.  Hence I think a lot of auditing is
> needed before this can be safely applied.

Sure.

Cheers,
Biju

> 
> Jonathan
> 
> > v6->v7:
> >  * Added ack from Greg Kroah-Hartman for patch#1
> >  * Swapped patch#2 and patch#3 from v6.
> >  * Added Rb tag from Andy for patch#2 and patch#4
> >  * Updated commit description of patch#2 by removing unnecessary
> wrapping.
> >  * Updated typo in commit description struct bus_type()->struct
> bus_type.
> > v5->v6:
> >  * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_
> >    data() and this function become redundant once this patch series hits
> >    mainline.
> >  * Added Rb tag from Sakari for patch#1.
> >  * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4.
> >  * Added Rb tag from Andy for patch#2
> >  * Separate patch#3 to prepare for better difference for
> >    i2c_match_id() changes.
> >  * Merged patch#4 from v5 with patch#4.
> > 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: Enhance i2c_get_match_data()
> >   i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static
> >   i2c: Add i2c_device_get_match_data() callback
> >
> >  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(-)
> >

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

[Jonathan Cameron]

On Sat, 5 Aug 2023 17:42:21 +0000
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Hi Jonathan Cameron,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> > 
> > On Fri,  4 Aug 2023 17:17:24 +0100
> > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >   
> > > This patch series extend device_get_match_data() to struct bus_type,
> > > so that buses like I2C can get matched data.
> > >
> > > There is a plan to replace
> > > i2c_get_match_data()->device_get_match_data()
> > > later, once this patch hits mainline as it is redundant.  
> > 
> > Are we sure we don't have any instances left of the pattern that used to
> > be common (typically for drivers where dt tables were added
> > later) of
> > 
> > 	chip_info = device_get_match_data();
> > 	if (!chip_info) {
> > 		chip_info = arrayofchipinfo[id->driver_data];
> > 	}
> > 
> > Looks like the first driver I checked, iio/adc/max1363.c does this still
> > and will I think break with this series.
> > 
> > Or am I missing some reason this isn't a problem?  
> 
> Good catch, this will break.
> we need to make I2C table like OF/ACPI tables.
> 
> Yes, before adding callback support to i2c_device_get_match_data(),
> We need to make similar table for OF/ACPI/I2C for all i2c drivers
> that use device_get_match_data()or of_get_device_match_data().

To throw another option out there, could we make the I2C subsystem use
the of_device_id table in place of the i2c_device_id table? 
That is perform matches based only on the of_device_id table in all
drivers (with some glue code making that work for the less common paths,
remaining board files etc). The ACPI PRP0001 magic is doing similar
already.

I can't immediately see why that wouldn't work other than being a bit
of a large job to implement in all drivers.

Getting rid of the duplication would be good.  Probably some rough corners
to make it possible to do this in a gradual process. In particular some
of the naming used for i2c_device_id table entries won't be 'valid'
DT compatibles (minus the vendor id)

> 
> May be first intermediate step is to use i2c_get_match_data()
> For such table conversion. Once all table conversion is done
> then we can add i2c_device_get_match_data() callback.
> 
> The below one is the recommendation from Andy.
> 
> + * 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.
> + */

We still need to maintain consistency across the two tables, which
is a stronger requirement than avoiding 0.  Some drivers already do that by
forcing the enum used to start at 1 which doesn't solver the different data
types issue.

Jonathan

> 
> > 
> > Clearly this only matters if we get to the bus callback, but enabling that
> > is the whole point of this series.  Hence I think a lot of auditing is
> > needed before this can be safely applied.  
> 
> Sure.
> 
> Cheers,
> Biju
> 
> > 
> > Jonathan
> >   
> > > v6->v7:
> > >  * Added ack from Greg Kroah-Hartman for patch#1
> > >  * Swapped patch#2 and patch#3 from v6.
> > >  * Added Rb tag from Andy for patch#2 and patch#4
> > >  * Updated commit description of patch#2 by removing unnecessary  
> > wrapping.  
> > >  * Updated typo in commit description struct bus_type()->struct  
> > bus_type.  
> > > v5->v6:
> > >  * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_
> > >    data() and this function become redundant once this patch series hits
> > >    mainline.
> > >  * Added Rb tag from Sakari for patch#1.
> > >  * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4.
> > >  * Added Rb tag from Andy for patch#2
> > >  * Separate patch#3 to prepare for better difference for
> > >    i2c_match_id() changes.
> > >  * Merged patch#4 from v5 with patch#4.
> > > 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: Enhance i2c_get_match_data()
> > >   i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static
> > >   i2c: Add i2c_device_get_match_data() callback
> > >
> > >  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(-)
> > >  
> 

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Jonathan Cameron,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Sat, 5 Aug 2023 17:42:21 +0000
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type
> > >
> > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > > This patch series extend device_get_match_data() to struct
> > > > bus_type, so that buses like I2C can get matched data.
> > > >
> > > > There is a plan to replace
> > > > i2c_get_match_data()->device_get_match_data()
> > > > later, once this patch hits mainline as it is redundant.
> > >
> > > Are we sure we don't have any instances left of the pattern that
> > > used to be common (typically for drivers where dt tables were added
> > > later) of
> > >
> > > 	chip_info = device_get_match_data();
> > > 	if (!chip_info) {
> > > 		chip_info = arrayofchipinfo[id->driver_data];
> > > 	}
> > >
> > > Looks like the first driver I checked, iio/adc/max1363.c does this
> > > still and will I think break with this series.
> > >
> > > Or am I missing some reason this isn't a problem?
> >
> > Good catch, this will break.
> > we need to make I2C table like OF/ACPI tables.
> >
> > Yes, before adding callback support to i2c_device_get_match_data(), We
> > need to make similar table for OF/ACPI/I2C for all i2c drivers that
> > use device_get_match_data()or of_get_device_match_data().
> 
> To throw another option out there, could we make the I2C subsystem use the
> of_device_id table in place of the i2c_device_id table?

+ device tree

Not sure that require bindings to be updated as we are replacing name->compatible, and I guess 'check patch' will complain undocumented compatible as all the names in 'i2c_device_id' are not mapped to compatible in 'of_device_id'. for eg: drivers/rtc/ rtc-isl1208.c[1]??

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/rtc/rtc-isl1208.c?h=next-20230804#n111

Cheers,
Biju

> That is perform matches based only on the of_device_id table in all
> drivers (with some glue code making that work for the less common paths,
> remaining board files etc). The ACPI PRP0001 magic is doing similar
> already.
> 
> I can't immediately see why that wouldn't work other than being a bit of a
> large job to implement in all drivers.
> 
> Getting rid of the duplication would be good.  Probably some rough corners
> to make it possible to do this in a gradual process. In particular some of
> the naming used for i2c_device_id table entries won't be 'valid'
> DT compatibles (minus the vendor id)
> 
> >
> > May be first intermediate step is to use i2c_get_match_data() For such
> > table conversion. Once all table conversion is done then we can add
> > i2c_device_get_match_data() callback.
> >
> > The below one is the recommendation from Andy.
> >
> > + * 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.
> > + */
> 
> We still need to maintain consistency across the two tables, which is a
> stronger requirement than avoiding 0.  Some drivers already do that by
> forcing the enum used to start at 1 which doesn't solver the different
> data types issue.
> 
> Jonathan
> 
> >
> > >
> > > Clearly this only matters if we get to the bus callback, but
> > > enabling that is the whole point of this series.  Hence I think a
> > > lot of auditing is needed before this can be safely applied.
> >
> > Sure.
> >
> > Cheers,
> > Biju
> >
> > >
> > > Jonathan
> > >
> > > > v6->v7:
> > > >  * Added ack from Greg Kroah-Hartman for patch#1
> > > >  * Swapped patch#2 and patch#3 from v6.
> > > >  * Added Rb tag from Andy for patch#2 and patch#4
> > > >  * Updated commit description of patch#2 by removing unnecessary
> > > wrapping.
> > > >  * Updated typo in commit description struct bus_type()->struct
> > > bus_type.
> > > > v5->v6:
> > > >  * Cced linux-rtc and linux-iio as these subsytems uses
> i2c_get_match_
> > > >    data() and this function become redundant once this patch series
> hits
> > > >    mainline.
> > > >  * Added Rb tag from Sakari for patch#1.
> > > >  * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4.
> > > >  * Added Rb tag from Andy for patch#2
> > > >  * Separate patch#3 to prepare for better difference for
> > > >    i2c_match_id() changes.
> > > >  * Merged patch#4 from v5 with patch#4.
> > > > 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: Enhance i2c_get_match_data()
> > > >   i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-
> static
> > > >   i2c: Add i2c_device_get_match_data() callback
> > > >
> > > >  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(-)
> > > >
> >

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

[Andy Shevchenko]

On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> On Sat, 5 Aug 2023 17:42:21 +0000
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > Biju Das <biju.das.jz@bp.renesas.com> wrote:

...

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

> We still need to maintain consistency across the two tables, which
> is a stronger requirement than avoiding 0.

True. Any suggestion how to amend the above comment? Because the documentation
makes sense on its own (may be split from the series?).

> Some drivers already do that by forcing the enum used to start at 1 which
> doesn't solver the different data types issue.

And some maintainers do not want to see non-enum values in i2c ID table.
*Shrug*.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 7 Aug 2023 17:54:07 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > On Sat, 5 Aug 2023 17:42:21 +0000
> > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> 
> ...
> 
> > > + * 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.  
> 
> > We still need to maintain consistency across the two tables, which
> > is a stronger requirement than avoiding 0.  
> 
> True. Any suggestion how to amend the above comment? Because the documentation
> makes sense on its own (may be split from the series?).

For bus ID tables it is fine right now as long as no one checks for NULL.
I guess adding this to the i2c_get_match_data and spi equivalent wrapper
functions might avoid someone shooting themselves in the foot (I've done it
for starters more than once).

> 
> > Some drivers already do that by forcing the enum used to start at 1 which
> > doesn't solver the different data types issue.  
> 
> And some maintainers do not want to see non-enum values in i2c ID table.
> *Shrug*.
> 

That leaves us stuck unless we move to a form where the i2c ID table isn't
used if there is an of_device_id table (or maybe we invent yet another table
and if that is present it is used for dt and were i2c_device_id is used and
hence becomes an opt in?  That will also be tricky however.

Jonathan

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

[Dmitry Torokhov]

On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > On Sat, 5 Aug 2023 17:42:21 +0000
> > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> ...
> 
> > > + * 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.
> 
> > We still need to maintain consistency across the two tables, which
> > is a stronger requirement than avoiding 0.
> 
> True. Any suggestion how to amend the above comment? Because the documentation
> makes sense on its own (may be split from the series?).
> 
> > Some drivers already do that by forcing the enum used to start at 1 which
> > doesn't solver the different data types issue.
> 
> And some maintainers do not want to see non-enum values in i2c ID table.
> *Shrug*.

So in legacy ID lookup path we can safely assume that values below 4096
are scalars and return NULL from the new device_get_match_data(). This
way current drivers using the values as indices or doing direct
comparisons against them can continue doing manual look up and using
them as they see fit. And we can convert the drivers at our leisure.

Thanks.

-- 
Dmitry

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

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 08:45:05PM +0100, Jonathan Cameron wrote:
> On Mon, 7 Aug 2023 17:54:07 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > > On Sat, 5 Aug 2023 17:42:21 +0000
> > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> > > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  

...

> > > > + * 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.  
> > 
> > > We still need to maintain consistency across the two tables, which
> > > is a stronger requirement than avoiding 0.  
> > 
> > True. Any suggestion how to amend the above comment? Because the documentation
> > makes sense on its own (may be split from the series?).
> 
> For bus ID tables it is fine right now as long as no one checks for NULL.
> I guess adding this to the i2c_get_match_data and spi equivalent wrapper
> functions might avoid someone shooting themselves in the foot (I've done it
> for starters more than once).

Ah, okay.

...

> > > Some drivers already do that by forcing the enum used to start at 1 which
> > > doesn't solver the different data types issue.  
> > 
> > And some maintainers do not want to see non-enum values in i2c ID table.
> > *Shrug*.
> 
> That leaves us stuck unless we move to a form where the i2c ID table isn't
> used if there is an of_device_id table (or maybe we invent yet another table
> and if that is present it is used for dt and were i2c_device_id is used and
> hence becomes an opt in?  That will also be tricky however.

This won't ever happen as it will break an ABI (as far as I understand
the current state of affairs). Seems the easiest path is to try to sell
the point that having pointers in I2C ID tables is okay to the maintainers
who don't see it that way.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > > On Sat, 5 Aug 2023 17:42:21 +0000
> > > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:

...

> > > > + * 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.
> > 
> > > We still need to maintain consistency across the two tables, which
> > > is a stronger requirement than avoiding 0.
> > 
> > True. Any suggestion how to amend the above comment? Because the documentation
> > makes sense on its own (may be split from the series?).
> > 
> > > Some drivers already do that by forcing the enum used to start at 1 which
> > > doesn't solver the different data types issue.
> > 
> > And some maintainers do not want to see non-enum values in i2c ID table.
> > *Shrug*.
> 
> So in legacy ID lookup path we can safely assume that values below 4096
> are scalars and return NULL from the new device_get_match_data(). This
> way current drivers using the values as indices or doing direct
> comparisons against them can continue doing manual look up and using
> them as they see fit. And we can convert the drivers at our leisure.

It's a good idea, but I believe will be received as hack.
But why not to try? We indeed have an error pointers for the last page
and NULL (which is only up to 16 IIRC) and reserved space in the first
page. To be more robust I would check all enums that are being used
in I2C ID tables for maximum value and if that is less than 16, use
ZERO_OR_NULL_PTR() instead of custom stuff.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 7 Aug 2023 13:37:12 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:  
> > > On Sat, 5 Aug 2023 17:42:21 +0000
> > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> > > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> > 
> > ...
> >   
> > > > + * 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.  
> >   
> > > We still need to maintain consistency across the two tables, which
> > > is a stronger requirement than avoiding 0.  
> > 
> > True. Any suggestion how to amend the above comment? Because the documentation
> > makes sense on its own (may be split from the series?).
> >   
> > > Some drivers already do that by forcing the enum used to start at 1 which
> > > doesn't solver the different data types issue.  
> > 
> > And some maintainers do not want to see non-enum values in i2c ID table.
> > *Shrug*.  
> 
> So in legacy ID lookup path we can safely assume that values below 4096
> are scalars and return NULL from the new device_get_match_data(). This
> way current drivers using the values as indices or doing direct
> comparisons against them can continue doing manual look up and using
> them as they see fit. And we can convert the drivers at our leisure.

Good idea.  Though I suspect there may still be nasty cases. People have
been known to put chip ID values in these fields so that they can then
match them against a who am I register as a 'detect it's the right part' check.

No idea if we have any drivers doing that but if there are hopefully not too many!

Jonathan

> 
> Thanks.
> 

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi all,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Tue, 8 Aug 2023 15:18:52 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > > > > On Sat, 5 Aug 2023 17:42:21 +0000 Biju Das
> > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > On Fri,  4 Aug 2023 17:17:24 +0100 Biju Das
> > > > > > > <biju.das.jz@bp.renesas.com> wrote:
> >
> > ...
> >
> > > > > > + * 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.
> > > >
> > > > > We still need to maintain consistency across the two tables,
> > > > > which is a stronger requirement than avoiding 0.
> > > >
> > > > True. Any suggestion how to amend the above comment? Because the
> > > > documentation makes sense on its own (may be split from the
> series?).
> > > >
> > > > > Some drivers already do that by forcing the enum used to start
> > > > > at 1 which doesn't solver the different data types issue.
> > > >
> > > > And some maintainers do not want to see non-enum values in i2c ID
> table.
> > > > *Shrug*.
> > >
> > > So in legacy ID lookup path we can safely assume that values below
> > > 4096 are scalars and return NULL from the new
> > > device_get_match_data(). This way current drivers using the values
> > > as indices or doing direct comparisons against them can continue
> > > doing manual look up and using them as they see fit. And we can
> convert the drivers at our leisure.
> >
> > It's a good idea, but I believe will be received as hack.
> > But why not to try? We indeed have an error pointers for the last page
> > and NULL (which is only up to 16 IIRC) and reserved space in the first
> > page. To be more robust I would check all enums that are being used in
> > I2C ID tables for maximum value and if that is less than 16, use
> > ZERO_OR_NULL_PTR() instead of custom stuff.
> >
> See iio/adc/max1363 example that has 37ish.
> 
> Could tidy that one up first and hopefully not find any others that are in
> subsystems not keen on the move away from enums.

If there is no objection, I can fix this using i2c_get_match_data() for iio/adc/max1363

and 

device_match_data() will return ZERO_OR_NULL_PTR() if max enum ID in the ID lookup table is less than 16.

and the drivers that use legacy ID's will fallback to ID lookup.

So that there won't be any regression.

Cheers,
Biju

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

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 06:25:51PM +0100, Jonathan Cameron wrote:
> On Tue, 8 Aug 2023 15:18:52 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:  
> > > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:  
> > > > > On Sat, 5 Aug 2023 17:42:21 +0000
> > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  
> > > > > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:  

...

> > > > > > + * 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.  
> > > >   
> > > > > We still need to maintain consistency across the two tables, which
> > > > > is a stronger requirement than avoiding 0.  
> > > > 
> > > > True. Any suggestion how to amend the above comment? Because the documentation
> > > > makes sense on its own (may be split from the series?).
> > > >   
> > > > > Some drivers already do that by forcing the enum used to start at 1 which
> > > > > doesn't solver the different data types issue.  
> > > > 
> > > > And some maintainers do not want to see non-enum values in i2c ID table.
> > > > *Shrug*.  
> > > 
> > > So in legacy ID lookup path we can safely assume that values below 4096
> > > are scalars and return NULL from the new device_get_match_data(). This
> > > way current drivers using the values as indices or doing direct
> > > comparisons against them can continue doing manual look up and using
> > > them as they see fit. And we can convert the drivers at our leisure.  
> > 
> > It's a good idea, but I believe will be received as hack.
> > But why not to try? We indeed have an error pointers for the last page
> > and NULL (which is only up to 16 IIRC) and reserved space in the first
> > page. To be more robust I would check all enums that are being used
> > in I2C ID tables for maximum value and if that is less than 16, use
> > ZERO_OR_NULL_PTR() instead of custom stuff.
> > 
> See iio/adc/max1363 example that has 37ish.
> 
> Could tidy that one up first and hopefully not find any others that
> are in subsystems not keen on the move away from enums.

Oh, yep, this needs a treewide audit and fixes around before going further.
As you said.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > On Tue, 8 Aug 2023 15:18:52 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:

...

> > > > So in legacy ID lookup path we can safely assume that values below
> > > > 4096 are scalars and return NULL from the new
> > > > device_get_match_data(). This way current drivers using the values
> > > > as indices or doing direct comparisons against them can continue
> > > > doing manual look up and using them as they see fit. And we can
> > convert the drivers at our leisure.
> > >
> > > It's a good idea, but I believe will be received as hack.
> > > But why not to try? We indeed have an error pointers for the last page
> > > and NULL (which is only up to 16 IIRC) and reserved space in the first
> > > page. To be more robust I would check all enums that are being used in
> > > I2C ID tables for maximum value and if that is less than 16, use
> > > ZERO_OR_NULL_PTR() instead of custom stuff.
> > >
> > See iio/adc/max1363 example that has 37ish.
> > 
> > Could tidy that one up first and hopefully not find any others that are in
> > subsystems not keen on the move away from enums.
> 
> If there is no objection, I can fix this using i2c_get_match_data() for
> iio/adc/max1363 and device_match_data() will return ZERO_OR_NULL_PTR()
> if max enum ID in the ID lookup table is less than 16. And the drivers
> that use legacy ID's will fallback to ID lookup. So that there won't be
> any regression.

I'm good with this approach, but make sure you checked the whole kernel source
tree for a such.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > > On Tue, 8 Aug 2023 15:18:52 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > So in legacy ID lookup path we can safely assume that values
> > > > > below
> > > > > 4096 are scalars and return NULL from the new
> > > > > device_get_match_data(). This way current drivers using the
> > > > > values as indices or doing direct comparisons against them can
> > > > > continue doing manual look up and using them as they see fit.
> > > > > And we can
> > > convert the drivers at our leisure.
> > > >
> > > > It's a good idea, but I believe will be received as hack.
> > > > But why not to try? We indeed have an error pointers for the last
> > > > page and NULL (which is only up to 16 IIRC) and reserved space in
> > > > the first page. To be more robust I would check all enums that are
> > > > being used in I2C ID tables for maximum value and if that is less
> > > > than 16, use
> > > > ZERO_OR_NULL_PTR() instead of custom stuff.
> > > >
> > > See iio/adc/max1363 example that has 37ish.
> > >
> > > Could tidy that one up first and hopefully not find any others that
> > > are in subsystems not keen on the move away from enums.
> >
> > If there is no objection, I can fix this using i2c_get_match_data()
> > for
> > iio/adc/max1363 and device_match_data() will return ZERO_OR_NULL_PTR()
> > if max enum ID in the ID lookup table is less than 16. And the drivers
> > that use legacy ID's will fallback to ID lookup. So that there won't
> > be any regression.
> 
> I'm good with this approach, but make sure you checked the whole kernel
> source tree for a such.

Checking against 16 is too short I guess??

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.

/*device enum */
enum inv_devices {
	INV_MPU6050,
	INV_MPU6500,
	INV_MPU6515,
	INV_MPU6880,
	INV_MPU6000,
	INV_MPU9150,
	INV_MPU9250,
	INV_MPU9255,
	INV_ICM20608,
	INV_ICM20608D,
	INV_ICM20609,
	INV_ICM20689,
	INV_ICM20600,
	INV_ICM20602,
	INV_ICM20690,
	INV_IAM20680,
	INV_NUM_PARTS
};

The new helper function

+static bool i2c_is_client_uses_legacy_id_table(const struct i2c_driver *driver)
+{
+	const struct i2c_device_id *id = driver->id_table;
+	kernel_ulong_t max_val = 0;
+
+	if (!id)
+		return FALSE;
+
+	while (id->name[0]) {
+		if (id->driver_data > max_val)
+			max_val = id->driver_data;
+		id++;
+	}
+
+	return ZERO_OR_NULL_PTR(max_val);
+}
+

Cheers,
Biju

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

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:

...

> > I'm good with this approach, but make sure you checked the whole kernel
> > source tree for a such.
> 
> Checking against 16 is too short I guess??
> 
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.

So, what does prevent us from moving that tables to use pointers?

> /*device enum */
> enum inv_devices {
> 	INV_MPU6050,
> 	INV_MPU6500,
> 	INV_MPU6515,
> 	INV_MPU6880,
> 	INV_MPU6000,
> 	INV_MPU9150,
> 	INV_MPU9250,
> 	INV_MPU9255,
> 	INV_ICM20608,
> 	INV_ICM20608D,
> 	INV_ICM20609,
> 	INV_ICM20689,
> 	INV_ICM20600,
> 	INV_ICM20602,
> 	INV_ICM20690,
> 	INV_IAM20680,
> 	INV_NUM_PARTS
> };
> 
> The new helper function

You mean for debugging? We don't need that in production.

I think what you need is a coccinelle script to find these.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> 
> ...
> 
> > > I'm good with this approach, but make sure you checked the whole
> > > kernel source tree for a such.
> >
> > Checking against 16 is too short I guess??
> >
> > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> 
> So, what does prevent us from moving that tables to use pointers?

I think that will lead to ABI breakage(client->name vs id->name)

	match = device_get_match_data(&client->dev);
	if (match) {
		chip_type = (uintptr_t)match;
		name = client->name;
	} else if (id) {
		chip_type = (enum inv_devices)
			id->driver_data;
		name = id->name;
	} else {
		return -ENOSYS;
	}

> 
> > /*device enum */
> > enum inv_devices {
> > 	INV_MPU6050,
> > 	INV_MPU6500,
> > 	INV_MPU6515,
> > 	INV_MPU6880,
> > 	INV_MPU6000,
> > 	INV_MPU9150,
> > 	INV_MPU9250,
> > 	INV_MPU9255,
> > 	INV_ICM20608,
> > 	INV_ICM20608D,
> > 	INV_ICM20609,
> > 	INV_ICM20689,
> > 	INV_ICM20600,
> > 	INV_ICM20602,
> > 	INV_ICM20690,
> > 	INV_IAM20680,
> > 	INV_NUM_PARTS
> > };
> >
> > The new helper function
> 
> You mean for debugging? We don't need that in production.

That is sample code for iterating through id table to find max enum
and check against ZERO_OR_NULL_PTR

> 
> I think what you need is a coccinelle script to find these.

I need to explore using coccinelle script as I have n't tried before.

Cheers,
Biju 

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

[Geert Uytterhoeven]

Hi Andy,

On Mon, Aug 7, 2023 at 4:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > On Sat, 5 Aug 2023 17:42:21 +0000
> > Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > On Fri,  4 Aug 2023 17:17:24 +0100
> > > > Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> ...
>
> > > + * 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.
>
> > We still need to maintain consistency across the two tables, which
> > is a stronger requirement than avoiding 0.
>
> True. Any suggestion how to amend the above comment? Because the documentation
> makes sense on its own (may be split from the series?).

I do have an idea how to handle that (inspired by the macro-heavy
R-Car pin control drivers ;-)

    #define DEVICES(fn) \
        fn("vendor1", "device1", &driver_data1) \
        fn("vendor3", "device2", &driver_data2) \
        fn("vendor3", "device2", &driver_data3)

    OF_MATCH_TABLE(driver_of_match, DEVICES);
    I2C_MATCH_TABLE(driver_i2c_ids, DEVICES);

and in the of resp. i2c headers:

    #define EMIT_OF_ENTRY(_vendor, _device, _data) \
            { .name = _vendor ## "," ## _device, .data = _data) },
    #define EMIT_OF_ENTRIES(_name, _devs) \
            static const struct of_device_id _name[] = { \
                    _devs(EMIT_OF_ENTRY) \
                    { } \
            } \

    #define EMIT_I2C_ENTRY(_vendor, _device, _data) \
            { .name = _device, .driver_data = (void *)_data) },
    #define EMIT_I2C_ENTRIES(_name, _devs) \
            static const struct i2c_device_id _name[] = { \
                    _devs(EMIT_I2C_ENTRY) \
                    { } \
            } \

I didn't try to compile this, so I may have missed something (e.g.
a required intermediate macro to ensure proper expansion).
Unfortunately this would break grep'ability for compatible values...

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 v7 0/4] Extend device_get_match_data() to struct bus_type

[Geert Uytterhoeven]

Hi Biju,

On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> >
> > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> >
> > ...
> >
> > > > I'm good with this approach, but make sure you checked the whole
> > > > kernel source tree for a such.
> > >
> > > Checking against 16 is too short I guess??
> > >
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> >
> > So, what does prevent us from moving that tables to use pointers?
>
> I think that will lead to ABI breakage(client->name vs id->name)
>
>         match = device_get_match_data(&client->dev);
>         if (match) {
>                 chip_type = (uintptr_t)match;
>                 name = client->name;
>         } else if (id) {
>                 chip_type = (enum inv_devices)
>                         id->driver_data;
>                 name = id->name;
>         } else {
>                 return -ENOSYS;
>         }

I don't consider that part of the ABI, as e.g. converting from board files
to DT would change the name.
In addition, using id->name breaks multiple instances of the same device.

I applaud more unification ;-)

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 v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> Hi Biju,
> 
> On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type
> > >
> > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > >
> > > ...
> > >
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > >
> > > So, what does prevent us from moving that tables to use pointers?
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> >         match = device_get_match_data(&client->dev);
> >         if (match) {
> >                 chip_type = (uintptr_t)match;
> >                 name = client->name;
> >         } else if (id) {
> >                 chip_type = (enum inv_devices)
> >                         id->driver_data;
> >                 name = id->name;
> >         } else {
> >                 return -ENOSYS;
> >         }
> 
> I don't consider that part of the ABI, as e.g. converting from board files
> to DT would change the name.
> In addition, using id->name breaks multiple instances of the same device.

OK, then according to you this patch is ok [1]?

https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230807172548.258247-2-biju.das.jz@bp.renesas.com/

Cheers,
Biju

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

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:

...

> > > > I'm good with this approach, but make sure you checked the whole
> > > > kernel source tree for a such.
> > >
> > > Checking against 16 is too short I guess??
> > >
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > 
> > So, what does prevent us from moving that tables to use pointers?
> 
> I think that will lead to ABI breakage(client->name vs id->name)
> 
> 	match = device_get_match_data(&client->dev);
> 	if (match) {
> 		chip_type = (uintptr_t)match;
> 		name = client->name;
> 	} else if (id) {
> 		chip_type = (enum inv_devices)
> 			id->driver_data;
> 		name = id->name;
> 	} else {
> 		return -ENOSYS;
> 	}


It's easy to work around (may be better fix can be found, haven't checked, just
what first comes to my mind):

	match ...
	name = match->name;

	/* If enumerated via firmware node, fix the ABI */
	if (dev_fwnode())
		client->name

> > > /*device enum */
> > > enum inv_devices {
> > > 	INV_MPU6050,
> > > 	INV_MPU6500,
> > > 	INV_MPU6515,
> > > 	INV_MPU6880,
> > > 	INV_MPU6000,
> > > 	INV_MPU9150,
> > > 	INV_MPU9250,
> > > 	INV_MPU9255,
> > > 	INV_ICM20608,
> > > 	INV_ICM20608D,
> > > 	INV_ICM20609,
> > > 	INV_ICM20689,
> > > 	INV_ICM20600,
> > > 	INV_ICM20602,
> > > 	INV_ICM20690,
> > > 	INV_IAM20680,
> > > 	INV_NUM_PARTS
> > > };
> > >
> > > The new helper function
> > 
> > You mean for debugging? We don't need that in production.
> 
> That is sample code for iterating through id table to find max enum
> and check against ZERO_OR_NULL_PTR

Much better with a coccinelle. You will find all or almost all occurrences
without too much effort done.

> > I think what you need is a coccinelle script to find these.
> 
> I need to explore using coccinelle script as I have n't tried before.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> 
> ...
> 
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > >
> > > So, what does prevent us from moving that tables to use pointers?
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> > 	match = device_get_match_data(&client->dev);
> > 	if (match) {
> > 		chip_type = (uintptr_t)match;
> > 		name = client->name;
> > 	} else if (id) {
> > 		chip_type = (enum inv_devices)
> > 			id->driver_data;
> > 		name = id->name;
> > 	} else {
> > 		return -ENOSYS;
> > 	}
> 
> 
> It's easy to work around (may be better fix can be found, haven't checked,
> just what first comes to my mind):
> 
> 	match ...
> 	name = match->name;

The device_get_match_data()API returns matched data, so we cannot use that one here.

Maybe??

/* If enumerated via ID lookup, fix the ABI */
if (!dev_fwnode())
	name = id->name;

Cheers,
Biju

> 
> 	/* If enumerated via firmware node, fix the ABI */
> 	if (dev_fwnode())
> 		client->name
> 
> > > > /*device enum */
> > > > enum inv_devices {
> > > > 	INV_MPU6050,
> > > > 	INV_MPU6500,
> > > > 	INV_MPU6515,
> > > > 	INV_MPU6880,
> > > > 	INV_MPU6000,
> > > > 	INV_MPU9150,
> > > > 	INV_MPU9250,
> > > > 	INV_MPU9255,
> > > > 	INV_ICM20608,
> > > > 	INV_ICM20608D,
> > > > 	INV_ICM20609,
> > > > 	INV_ICM20689,
> > > > 	INV_ICM20600,
> > > > 	INV_ICM20602,
> > > > 	INV_ICM20690,
> > > > 	INV_IAM20680,
> > > > 	INV_NUM_PARTS
> > > > };
> > > >
> > > > The new helper function
> > >
> > > You mean for debugging? We don't need that in production.
> >
> > That is sample code for iterating through id table to find max enum
> > and check against ZERO_OR_NULL_PTR
> 
> Much better with a coccinelle. You will find all or almost all occurrences
> without too much effort done.
> 
> > > I think what you need is a coccinelle script to find these.
> >
> > I need to explore using coccinelle script as I have n't tried before.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

> Subject: RE: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> 
> Hi Andy Shevchenko,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> >
> > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to
> > > > struct bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das
> wrote:
> > > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> >
> > ...
> >
> > > > > > I'm good with this approach, but make sure you checked the
> > > > > > whole kernel source tree for a such.
> > > > >
> > > > > Checking against 16 is too short I guess??
> > > > >
> > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > > >
> > > > So, what does prevent us from moving that tables to use pointers?
> > >
> > > I think that will lead to ABI breakage(client->name vs id->name)
> > >
> > > 	match = device_get_match_data(&client->dev);
> > > 	if (match) {
> > > 		chip_type = (uintptr_t)match;
> > > 		name = client->name;
> > > 	} else if (id) {
> > > 		chip_type = (enum inv_devices)
> > > 			id->driver_data;
> > > 		name = id->name;
> > > 	} else {
> > > 		return -ENOSYS;
> > > 	}
> >
> >
> > It's easy to work around (may be better fix can be found, haven't
> > checked, just what first comes to my mind):
> >
> > 	match ...
> > 	name = match->name;
> 
> The device_get_match_data()API returns matched data, so we cannot use that
> one here.
> 
> Maybe??
> 
> /* If enumerated via ID lookup, fix the ABI */ if (!dev_fwnode())
> 	name = id->name;

Looks this will work.

/* If enumerated via firmware node, fix the ABI */
	if (dev_fwnode())
		name = client->name
	else
		name = id->name

Cheers,
Biju

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

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 06:58:41AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:

...

> > It's easy to work around (may be better fix can be found, haven't checked,
> > just what first comes to my mind):
> > 
> > 	match ...
> > 	name = match->name;
> 
> The device_get_match_data()API returns matched data, so we cannot use that one here.
> 
> Maybe??
> 
> /* If enumerated via ID lookup, fix the ABI */
> if (!dev_fwnode())
> 	name = id->name;

Yeah, you got the idea.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 14 Aug 2023 15:12:24 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Biju,
> 
> On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type
> > >
> > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:  
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:  
> > >
> > > ...
> > >  
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.  
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.  
> > >
> > > So, what does prevent us from moving that tables to use pointers?  
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> >         match = device_get_match_data(&client->dev);
> >         if (match) {
> >                 chip_type = (uintptr_t)match;
> >                 name = client->name;
> >         } else if (id) {
> >                 chip_type = (enum inv_devices)
> >                         id->driver_data;
> >                 name = id->name;
> >         } else {
> >                 return -ENOSYS;
> >         }  
> 
> I don't consider that part of the ABI, as e.g. converting from board files
> to DT would change the name.
> In addition, using id->name breaks multiple instances of the same device.

This has always been a mess as I wasn't paying attention a long time back
and we ended up with some client->name entries being used for iio_dev->name
whereas it should be the part number.

Using id->name is correct choice.  This is supposed to be the same for multiple
instances of the same device.  There is label and a bunch of other options
for differentiating them including their parent devices.

Problem is that is exported to userspace and often used as part of the
matching when a userspace tool is trying to find the device.

We could 'give it a go' by setting the name in teh switch statement in the core code
and hope no one notices but it's not ideal
https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L1597

Jonathan



> 
> I applaud more unification ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>