On Mon, Jul 24, 2023 at 03:06:50PM +0000, Biju Das wrote:
> Hi Geert,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> > callback
> >
> > Hi Biju,
> >
> > On Sun, Jul 23, 2023 at 10:37 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > 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().
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -114,20 +114,41 @@ const struct i2c_device_id *i2c_match_id(const
> > > struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id);
> > >
> > > +static void *i2c_get_match_data_helper(struct i2c_driver *driver,
> >
> > static const void *
>
> I missed this.
>
> >
> > > + const struct i2c_client
> > > +*client) {
> > > + const struct i2c_device_id *match;
> > > +
> > > + match = i2c_match_id(driver->id_table, client);
> > > + if (!match)
> > > + return NULL;
> > > +
> > > + return (const void *)match->driver_data;
> >
> > I guess your compiler didn't complain about the const/non-const
> > conversion when returning because it inlined the function?
>
> It complained. Somehow, I didn't notice that warning before sending the patch.
>
> >
> > > +}
> > > +
> > > +static const void *i2c_device_get_match_data(const struct device
> > > +*dev) {
> > > + const struct i2c_client *client = to_i2c_client(dev);
>
> Not sure, non-const i2c_verify_client(dev)to be used here??
Good call, it actually should, as i2c bus contains instances of both
i2c_client and i2c_adapter.
Unfortunately i2c_verify_client() right now is a function, we might need
to turn it into a macro to allow transparently handle const/non-const
device argument... If this is too hard at the moment we could open-code
i2c_verify_client() in i2c_device_get_match_data() and first check on
the device type before doing to_i2c_client() conversion.
Also I see i2c_device_remove() uses to_i2c_client(). I guess it only
works because i2c adapters never have drivers assigned to them, but it
would be nice to use i2c_verify_client() and maybe put a big fat warning
if we get wrong type of device there.
Thanks.
--
Dmitry
