| Message ID | 20230802112317.252745-4-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 |
Hi Niju, On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote: > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches, > > return NULL; > } > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs); This is private to i2c core, you do not need to export it to modules. Thanks.
On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote: > Extend i2c_device_get_match_data() to i2c sysfs interface by changing > i2c_of_match_device_sysfs() to non-static and call this function as a > fallback for i2c_get_match_data_helper(). ... > /* TODO: use i2c_verify_client() when it accepts const pointer */ > const struct i2c_client *client = (dev->type == &i2c_client_type) ? > to_i2c_client(dev) : NULL; > + const void *data; Oh, yeah, definitely, please follow advice for previous patch I have given. > if (!client || !dev->driver) > return NULL; > > - return i2c_get_match_data_helper(client); > + data = i2c_get_match_data_helper(client); > + if (data) > + return data; > + > + if (dev->driver->of_match_table) { > + const struct of_device_id *match; > + > + match = i2c_of_match_device_sysfs(dev->driver->of_match_table, > + client); So, with this it makes sense to have strucct device_driver *drv = dev->driver; ... if (!drv) return NULL; The above for the first patch even. And here match = i2c_of_match_device_sysfs(drv->of_match_table, client); > + if (match) > + return match->data; > + } > + return data; This actually should be return NULL; to avoid (potentially) stale pointers. > } ... > -static const struct of_device_id* > +const struct of_device_id * > i2c_of_match_device_sysfs(const struct of_device_id *matches, > - struct i2c_client *client) > + const struct i2c_client *client) > { > const char *name; > > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches, > > return NULL; > } > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs); > > const struct of_device_id > *i2c_of_match_device(const struct of_device_id *matches, > diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h > index 1247e6e6e975..e4d397b67989 100644 > --- a/drivers/i2c/i2c-core.h > +++ b/drivers/i2c/i2c-core.h > @@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) { > > #ifdef CONFIG_OF > void of_i2c_register_devices(struct i2c_adapter *adap); > +const struct of_device_id * > +i2c_of_match_device_sysfs(const struct of_device_id *matches, > + const struct i2c_client *client); > #else > static inline void of_i2c_register_devices(struct i2c_adapter *adap) { } > +static inline const struct of_device_id * > +i2c_of_match_device_sysfs(const struct of_device_id *matches, > + const struct i2c_client *client) > +{ > + return NULL; > +} > #endif > extern struct notifier_block i2c_of_notifier; That's what above I meant to see as a separate change.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to > support i2c sysfs > > On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote: > > Extend i2c_device_get_match_data() to i2c sysfs interface by changing > > i2c_of_match_device_sysfs() to non-static and call this function as a > > fallback for i2c_get_match_data_helper(). > > ... > > > /* TODO: use i2c_verify_client() when it accepts const pointer */ > > const struct i2c_client *client = (dev->type == > &i2c_client_type) ? > > to_i2c_client(dev) : NULL; > > + const void *data; > > Oh, yeah, definitely, please follow advice for previous patch I have > given. OK. > > > if (!client || !dev->driver) > > return NULL; > > > > - return i2c_get_match_data_helper(client); > > + data = i2c_get_match_data_helper(client); > > + if (data) > > + return data; > > + > > + if (dev->driver->of_match_table) { > > + const struct of_device_id *match; > > + > > + match = i2c_of_match_device_sysfs(dev->driver- > >of_match_table, > > + client); > > So, with this it makes sense to have > > strucct device_driver *drv = dev->driver; > ... > if (!drv) > return NULL; > > The above for the first patch even. And here Agreed. > > match = i2c_of_match_device_sysfs(drv->of_match_table, > client); > > > + if (match) > > + return match->data; > > + } > > > + return data; > > This actually should be > > return NULL; > > to avoid (potentially) stale pointers. OK. > > > } > > ... > > > -static const struct of_device_id* > > +const struct of_device_id * > > i2c_of_match_device_sysfs(const struct of_device_id *matches, > > - struct i2c_client *client) > > + const struct i2c_client *client) > > { > > const char *name; > > > > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct > > of_device_id *matches, > > > > return NULL; > > } > > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs); > > > > const struct of_device_id > > *i2c_of_match_device(const struct of_device_id *matches, diff --git > > a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index > > 1247e6e6e975..e4d397b67989 100644 > > --- a/drivers/i2c/i2c-core.h > > +++ b/drivers/i2c/i2c-core.h > > @@ -82,8 +82,17 @@ static inline void > > i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) { > > > > #ifdef CONFIG_OF > > void of_i2c_register_devices(struct i2c_adapter *adap); > > +const struct of_device_id * > > +i2c_of_match_device_sysfs(const struct of_device_id *matches, > > + const struct i2c_client *client); > > #else > > static inline void of_i2c_register_devices(struct i2c_adapter *adap) > > { } > > +static inline const struct of_device_id * > > +i2c_of_match_device_sysfs(const struct of_device_id *matches, > > + const struct i2c_client *client) { > > + return NULL; > > +} > > #endif > > extern struct notifier_block i2c_of_notifier; > > That's what above I meant to see as a separate change. OK will create a separate patch for this change. Cheers, Biju
Hi Dmitry Torokhov, Thanks for the feedback. > Subject: Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to > support i2c sysfs > > Hi Niju, > > On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote: > > @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct > of_device_id *matches, > > > > return NULL; > > } > > +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs); > > This is private to i2c core, you do not need to export it to modules. Agreed. Drop this export in next version. Cheers, Biju
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 2436f23e63af..d002c5a1cc84 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -131,11 +131,25 @@ static const void *i2c_device_get_match_data(const struct device *dev) /* TODO: use i2c_verify_client() when it accepts const pointer */ const struct i2c_client *client = (dev->type == &i2c_client_type) ? to_i2c_client(dev) : NULL; + const void *data; if (!client || !dev->driver) return NULL; - return i2c_get_match_data_helper(client); + data = i2c_get_match_data_helper(client); + if (data) + return data; + + if (dev->driver->of_match_table) { + const struct of_device_id *match; + + match = i2c_of_match_device_sysfs(dev->driver->of_match_table, + client); + if (match) + return match->data; + } + + return data; } const void *i2c_get_match_data(const struct i2c_client *client) diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index a6c407d36800..179b3486904a 100644 --- a/drivers/i2c/i2c-core-of.c +++ b/drivers/i2c/i2c-core-of.c @@ -113,9 +113,9 @@ void of_i2c_register_devices(struct i2c_adapter *adap) of_node_put(bus); } -static const struct of_device_id* +const struct of_device_id * i2c_of_match_device_sysfs(const struct of_device_id *matches, - struct i2c_client *client) + const struct i2c_client *client) { const char *name; @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches, return NULL; } +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs); const struct of_device_id *i2c_of_match_device(const struct of_device_id *matches, diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index 1247e6e6e975..e4d397b67989 100644 --- a/drivers/i2c/i2c-core.h +++ b/drivers/i2c/i2c-core.h @@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) { #ifdef CONFIG_OF void of_i2c_register_devices(struct i2c_adapter *adap); +const struct of_device_id * +i2c_of_match_device_sysfs(const struct of_device_id *matches, + const struct i2c_client *client); #else static inline void of_i2c_register_devices(struct i2c_adapter *adap) { } +static inline const struct of_device_id * +i2c_of_match_device_sysfs(const struct of_device_id *matches, + const struct i2c_client *client) +{ + return NULL; +} #endif extern struct notifier_block i2c_of_notifier;
Extend i2c_device_get_match_data() to i2c sysfs interface by changing i2c_of_match_device_sysfs() to non-static and call this function as a fallback for i2c_get_match_data_helper(). Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v4: * split from patch #2 * 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 --- drivers/i2c/i2c-core-base.c | 16 +++++++++++++++- drivers/i2c/i2c-core-of.c | 5 +++-- drivers/i2c/i2c-core.h | 9 +++++++++ 3 files changed, 27 insertions(+), 3 deletions(-)