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 06:43:30PM +0200, Geert Uytterhoeven wrote:
> Hi Dmitry,
> 
> On Mon, Jul 24, 2023 at 6:35 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Jul 24, 2023 at 03:06:50PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH RFC 2/2] i2c: Add i2c_device_get_match_data()
> > > > callback
> > > > 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.
> 
> Tadah, we have _Generic()! See container_of_const():
> https://elixir.bootlin.com/linux/latest/source/include/linux/container_of.h#L25

I think we might want to go through all buses and switch to_XXX()
implementations there to use container_of_const() instead of old
container_of() there.

This still does not help with i2c_verify_client() being currently a
function, so it still needs to be converted to a macro to be able to
return const and nont-const pointers, depending on the argument type.

Thanks.

-- 
Dmitry