Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data() callback

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@bp.renesas.com>
> > 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@gmail.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > 
> > 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