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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >  * 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(-)
> >





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux