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

[Biju Das <biju.das.jz@bp.renesas.com>]

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