| Message ID | 20230723083721.35384-3-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 |
On Sun, Jul 23, 2023 at 09:37:21AM +0100, Biju Das 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> > --- > drivers/i2c/i2c-core-base.c | 38 +++++++++++++++++++++++++++++-------- > 1 file changed, 30 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 60746652fd52..80259111355b 100644 > --- 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, > + 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; Not sure if this helper is needed given that in the end (hopefully) we can simply remove i2c_get_match_data() and leave only i2c_device_get_match_data(). i2c_get_match_data() and leave only i2c_device_get_match_data(). Thanks.
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 * > + 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? > +} > + > +static const void *i2c_device_get_match_data(const struct device *dev) > +{ > + const struct i2c_client *client = to_i2c_client(dev); > + const struct i2c_driver *driver; > + > + if (!dev->driver) > + return NULL; > + > + driver = to_i2c_driver(dev->driver); > + if (!driver) > + return NULL; "driver" can never be NULL here. > + > + return i2c_get_match_data_helper(driver, client); > +} > + Gr{oetje,eeting}s, Geert
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?? > > + const struct i2c_driver *driver; > > + > > + if (!dev->driver) > > + return NULL; > > + > > + driver = to_i2c_driver(dev->driver); > > + if (!driver) > > + return NULL; > > "driver" can never be NULL here. OK, will remove this. Cheers, Biju > > > + > > + return i2c_get_match_data_helper(driver, client); } > > + >
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.
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 Gr{oetje,eeting}s, Geert
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.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 60746652fd52..80259111355b 100644 --- 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, + 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; +} + +static const void *i2c_device_get_match_data(const struct device *dev) +{ + const struct i2c_client *client = to_i2c_client(dev); + const struct i2c_driver *driver; + + if (!dev->driver) + return NULL; + + driver = to_i2c_driver(dev->driver); + if (!driver) + return NULL; + + return i2c_get_match_data_helper(driver, client); +} + const void *i2c_get_match_data(const struct i2c_client *client) { struct i2c_driver *driver = to_i2c_driver(client->dev.driver); - const struct i2c_device_id *match; const void *data; data = device_get_match_data(&client->dev); - if (!data) { - match = i2c_match_id(driver->id_table, client); - if (!match) - return NULL; - - data = (const void *)match->driver_data; - } + if (!data) + data = i2c_get_match_data_helper(driver, client); return data; } @@ -695,6 +716,7 @@ struct bus_type i2c_bus_type = { .probe = i2c_device_probe, .remove = i2c_device_remove, .shutdown = i2c_device_shutdown, + .get_match_data = i2c_device_get_match_data, }; EXPORT_SYMBOL_GPL(i2c_bus_type);