| Message ID | 20230726130804.186313-3-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Extend device_get_match_data() to struct bus_type | expand |
On Wed, Jul 26, 2023 at 02:08:04PM +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(). I have not been Cc'ed to this... ... > +static const void *i2c_device_get_match_data(const struct device *dev) > +{ > + const struct i2c_client *client = (dev->type == &i2c_client_type) ? > + to_i2c_client(dev) : NULL; There is an API i2c_verify_client() or something like this, I don't remember by heart. > + if (!dev->driver) > + return NULL; > + > + return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client); > +} ... > +const void *i2c_get_match_data(const struct i2c_client *client) > +{ > + const struct i2c_driver *driver = to_i2c_driver(client->dev.driver); > const void *data; > > data = device_get_match_data(&client->dev); > - if (!data) { > - match = i2c_match_id(driver->id_table, client); > - if (!match) > - return NULL; > - > - data = (const void *)match->driver_data; > - } > + if (!data) > + data = i2c_get_match_data_helper(driver, client); if (data) return data; return i2c_...; > > return data; > } ... Side question, what is the idea for i2c_of_match_device()? Shouldn't you also take it into consideration?
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() > callback > > On Wed, Jul 26, 2023 at 02:08:04PM +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(). > > I have not been Cc'ed to this... I execute one script per patch, to pick the recipient list. Somehow it did not pick your name. Next time I will make sure you are on the Cc list. > > ... > > > +static const void *i2c_device_get_match_data(const struct device > > +*dev) { > > + const struct i2c_client *client = (dev->type == > &i2c_client_type) ? > > + to_i2c_client(dev) : NULL; > > There is an API i2c_verify_client() or something like this, I don't > remember by heart. Dmitry already responded. > > > + if (!dev->driver) > > + return NULL; > > + > > + return i2c_get_match_data_helper(to_i2c_driver(dev->driver), > > +client); } > > ... > > > +const void *i2c_get_match_data(const struct i2c_client *client) { > > + const struct i2c_driver *driver = to_i2c_driver(client- > >dev.driver); > > const void *data; > > > > data = device_get_match_data(&client->dev); > > - if (!data) { > > - match = i2c_match_id(driver->id_table, client); > > - if (!match) > > - return NULL; > > - > > - data = (const void *)match->driver_data; > > - } > > + if (!data) > > + data = i2c_get_match_data_helper(driver, client); > > if (data) > return data; > > return i2c_...; OK. > > > > > return data; > > } > > ... > > Side question, what is the idea for i2c_of_match_device()? Shouldn't you > also take it into consideration? I guess you are ok with Dmitry's suggestion. Cheers, Biju
On Wed, Jul 26, 2023 at 10:56:41AM -0700, Dmitry Torokhov wrote: > On Wed, Jul 26, 2023 at 07:44:17PM +0300, Andy Shevchenko wrote: > > On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote: ... > > > +static const void *i2c_device_get_match_data(const struct device *dev) > > > +{ > > > + const struct i2c_client *client = (dev->type == &i2c_client_type) ? > > > + to_i2c_client(dev) : NULL; > > > > There is an API i2c_verify_client() or something like this, I don't remember > > by heart. > > It's been discussed in a separate thread. i2c_verify_client() needs a > non-const pointer. It would be nice to clean up i2c_verify_client() to > accept both variants, but that can be done later. Then this code needs a TODO comment: /* TODO: use i2c_verify_client() when it accepts const pointer */ > > > + if (!dev->driver) > > > + return NULL; > > > + > > > + return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client); > > > +} ... > > Side question, what is the idea for i2c_of_match_device()? Shouldn't you also > > take it into consideration? > > Good call. I think we need to add something like > > if (!data && driver->driver.of_match_table) { > match = > i2c_of_match_device_sysfs(driver->driver.of_match_table, client); > if (match) > data = match->data; > } > > to i2c_device_get_match_data(). Haven't checked myself, by I trust your suggestion. Let's see it in v3 then.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v2 2/2] i2c: Add i2c_device_get_match_data() > callback > > On Wed, Jul 26, 2023 at 10:56:41AM -0700, Dmitry Torokhov wrote: > > On Wed, Jul 26, 2023 at 07:44:17PM +0300, Andy Shevchenko wrote: > > > On Wed, Jul 26, 2023 at 02:08:04PM +0100, Biju Das wrote: > > ... > > > > > +static const void *i2c_device_get_match_data(const struct device > > > > +*dev) { > > > > + const struct i2c_client *client = (dev->type == > &i2c_client_type) ? > > > > + to_i2c_client(dev) : NULL; > > > > > > There is an API i2c_verify_client() or something like this, I don't > > > remember by heart. > > > > It's been discussed in a separate thread. i2c_verify_client() needs a > > non-const pointer. It would be nice to clean up i2c_verify_client() to > > accept both variants, but that can be done later. > > Then this code needs a TODO comment: > > /* TODO: use i2c_verify_client() when it accepts const pointer */ Agreed. > > > > > > + if (!dev->driver) > > > > + return NULL; > > > > + > > > > + return i2c_get_match_data_helper(to_i2c_driver(dev->driver), > > > > +client); } > > ... > > > > Side question, what is the idea for i2c_of_match_device()? Shouldn't > > > you also take it into consideration? > > > > Good call. I think we need to add something like > > > > if (!data && driver->driver.of_match_table) { > > match = > > i2c_of_match_device_sysfs(driver->driver.of_match_table, client); > > if (match) > > data = match->data; > > } > > > > to i2c_device_get_match_data(). > > Haven't checked myself, by I trust your suggestion. Let's see it in v3 > then. OK, will send V3 to provide feedback. Cheers, Biju
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 60746652fd52..6183e9e36889 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -114,20 +114,37 @@ 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_driver *driver, + const struct i2c_client *client) { - 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 i2c_client *client = (dev->type == &i2c_client_type) ? + to_i2c_client(dev) : NULL; + + if (!dev->driver) + return NULL; + + return i2c_get_match_data_helper(to_i2c_driver(dev->driver), client); +} + +const void *i2c_get_match_data(const struct i2c_client *client) +{ + const struct i2c_driver *driver = to_i2c_driver(client->dev.driver); const void *data; data = device_get_match_data(&client->dev); - if (!data) { - match = i2c_match_id(driver->id_table, client); - if (!match) - return NULL; - - data = (const void *)match->driver_data; - } + if (!data) + data = i2c_get_match_data_helper(driver, client); return data; } @@ -695,6 +712,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> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- 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 | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)