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
