| Message ID | 20230803103102.323987-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Extend device_get_match_data() to struct bus_type | expand |
On Thu, Aug 03, 2023 at 11:31:00AM +0100, Biju Das wrote: > Add i2c_device_get_match_data() callback to struct bus_type(). > > While at it, introduced i2c_get_match_data_helper() to avoid code > duplication with i2c_get_match_data(). It seems you are missing to Cc Andi for all these... (not your fault, rather unfortunately). Yes, while he is not directly involved into core changes the drivers are pretty much should consider this change. ... > data = device_get_match_data(&client->dev); > - if (!data) { > - match = i2c_match_id(driver->id_table, client); > - if (!match) > - return NULL; > + if (data) > + return data; > > - data = (const void *)match->driver_data; > - } > - > - return data; Looking at this, it _might_ make sense to split another patch to prepare for better difference here. - if (!data) { - match = i2c_match_id(driver->id_table, client); - if (!match) - return NULL; + if (data) + return data; + + match = i2c_match_id(driver->id_table, client); + if (!match) + return NULL; + + return (const void *)match->driver_data; Just play with this idea.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data() > callback > > On Thu, Aug 03, 2023 at 11:31:00AM +0100, Biju Das wrote: > > Add i2c_device_get_match_data() callback to struct bus_type(). > > > > While at it, introduced i2c_get_match_data_helper() to avoid code > > duplication with i2c_get_match_data(). > > It seems you are missing to Cc Andi for all these... (not your fault, > rather unfortunately). > > Yes, while he is not directly involved into core changes the drivers are > pretty much should consider this change. Sure. > > ... > > > data = device_get_match_data(&client->dev); > > - if (!data) { > > - match = i2c_match_id(driver->id_table, client); > > - if (!match) > > - return NULL; > > + if (data) > > + return data; > > > > - data = (const void *)match->driver_data; > > - } > > - > > - return data; > > Looking at this, it _might_ make sense to split another patch to prepare > for better difference here. OK. > > - if (!data) { > - match = i2c_match_id(driver->id_table, client); > - if (!match) > - return NULL; > + if (data) > + return data; > + > + match = i2c_match_id(driver->id_table, client); > + if (!match) > + return NULL; > + > + return (const void *)match->driver_data; > > Just play with this idea. Does these below 2 patches ok? PATCH x: ------- Subject: [PATCH 1/2] i2c: Enhance i2c_get_match_data() 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. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- 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); Patch x+1: --------- Subject: [PATCH 2/2] i2c: Add i2c_device_get_match_data() callback 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> --- 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); Cheers, Biju
On Thu, Aug 03, 2023 at 01:52:05PM +0000, Biju Das wrote: > > On Thu, Aug 03, 2023 at 11:31:00AM +0100, Biju Das wrote: > Does these below 2 patches ok? Looks awesome!
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 60746652fd52..e738cfba9c3b 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -114,22 +114,48 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); -const void *i2c_get_match_data(const struct i2c_client *client) +static const void *i2c_get_match_data_helper(const struct i2c_client *client) { - struct i2c_driver *driver = to_i2c_driver(client->dev.driver); + const struct i2c_driver *driver = to_i2c_driver(client->dev.driver); const struct i2c_device_id *match; + + match = i2c_match_id(driver->id_table, client); + if (!match) + return NULL; + + return (const void *)match->driver_data; +} + +static const void *i2c_device_get_match_data(const struct device *dev) +{ + const struct device_driver *drv = dev->driver; + const struct i2c_client *client; + + /* + * 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; + + return i2c_get_match_data_helper(client); +} + +const void *i2c_get_match_data(const struct i2c_client *client) +{ const void *data; data = device_get_match_data(&client->dev); - if (!data) { - match = i2c_match_id(driver->id_table, client); - if (!match) - return NULL; + if (data) + return data; - data = (const void *)match->driver_data; - } - - return data; + return i2c_get_match_data_helper(client); } EXPORT_SYMBOL(i2c_get_match_data); @@ -695,6 +721,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);
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> --- 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 | 47 +++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 10 deletions(-)