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

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 [off-list ref] wrote:

quoted

quoted

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 [off-list ref] wrote:

quoted

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)

quoted

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

quoted

quoted

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

quoted

Jonathan

quoted

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.

quoted

 * Updated typo in commit description struct bus_type()->struct

bus_type.

quoted

v5->v6:
 * Cced linux-rtc and linux-iio as these subsytems uses

i2c_get_match_

quoted

quoted

quoted

   data() and this function become redundant once this patch series

hits

quoted

quoted

quoted

   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

quoted

   _data().
 * For code readability and maintenance perspective, added separate

NULL

quoted

quoted

quoted

   check for drv and client variable and added comment for NULL
check

for

quoted

   drv variable.
 * Created separate patch for converting i2c_of_match_device_sysfs()

to

quoted

quoted

quoted

   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,

quoted

   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

quoted

   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

quoted

quoted

quoted

   needed for i2c_device_get_match_data().
 * Added a TODO comment to use i2c_verify_client() when it accepts

const

quoted

quoted

quoted

   pointer.
 * Added multiple returns to make code path for

device_get_match_data()

quoted

quoted

quoted

   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
[off-list ref]
 * Added const qualifier to return type and parameter struct

i2c_driver

quoted

quoted

quoted

   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

quoted

quoted

quoted

   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

quoted

quoted

quoted

  i2c: Add i2c_device_get_match_data() callback

 drivers/base/property.c     | 27 ++++++++++++++++-
 drivers/i2c/i2c-core-base.c | 60 ++++++++++++++++++++++++++++++----

---

quoted

quoted

quoted

 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(-)