| Message ID | 20230717131756.240645-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | exc3000 driver enhancements | expand |
Hi Biju, On Mon, Jul 17, 2023 at 02:17:55PM +0100, Biju Das wrote: > The exc3000_id.driver_data could store a pointer to the info, > like for ACPI/DT-based matching, making I2C, ACPI and DT-based > matching more similar. > > After that, we can simplify the probe() by replacing device_get_ > match_data() and i2c_match_id() by i2c_get_match_data() as we have > similar I2C, ACPI and DT-based matching table. Have you considered enhancing device_get_match_data() to allow for bus-specific "get_match_data" implementations? This way the drivers would simply call device_get_match_data() and not care if they are I2C, SPI, or something else... Thanks.
+ Mark and Wolfram > -----Original Message----- > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Sent: Monday, July 17, 2023 4:58 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Mike Looijmans <mike.looijmans@topic.nl>; Andreas Helbech Kleist > <andreaskleist@gmail.com>; Geert Uytterhoeven <geert+renesas@glider.be>; > Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux- > input@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > Hi Biju, > > On Mon, Jul 17, 2023 at 02:17:55PM +0100, Biju Das wrote: > > The exc3000_id.driver_data could store a pointer to the info, like for > > ACPI/DT-based matching, making I2C, ACPI and DT-based matching more > > similar. > > > > After that, we can simplify the probe() by replacing device_get_ > > match_data() and i2c_match_id() by i2c_get_match_data() as we have > > similar I2C, ACPI and DT-based matching table. > > Have you considered enhancing device_get_match_data() to allow for bus- > specific "get_match_data" implementations? This way the drivers would > simply call device_get_match_data() and not care if they are I2C, SPI, > or something else... The .device_get_match_data callbacks are missing for I2C and SPI bus subsystems. Can you please throw some lights on this? Cheers, Biju
On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > The .device_get_match_data callbacks are missing for I2C and SPI bus subsystems. > Can you please throw some lights on this? It's the first time I've ever heard of that callback, I don't know why whoever added it wouldn't have done those buses in particular or if it just didn't happen. Try adding it and if it works send the patches?
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Monday, July 17, 2023 7:16 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Mike Looijmans > <mike.looijmans@topic.nl>; Andreas Helbech Kleist > <andreaskleist@gmail.com>; Geert Uytterhoeven <geert+renesas@glider.be>; > Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; linux- > input@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org; Wolfram Sang > <wsa+renesas@sang-engineering.com> > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > The .device_get_match_data callbacks are missing for I2C and SPI bus > subsystems. > > Can you please throw some lights on this? > > It's the first time I've ever heard of that callback, I don't know why > whoever added it wouldn't have done those buses in particular or if it > just didn't happen. Try adding it and if it works send the patches? I need to investigate at some point later, Currently only OF and ACPI have these callbacks. [1] https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/of/property.c#L1507 [2] https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/acpi/property.c#L1587 Cheers, Biju
On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > The .device_get_match_data callbacks are missing for I2C and SPI bus subsystems. > > Can you please throw some lights on this? > > It's the first time I've ever heard of that callback, I don't know why > whoever added it wouldn't have done those buses in particular or if it > just didn't happen. Try adding it and if it works send the patches? I think there is a disconnect. Right now device_get_match_data callbacks are part of fwnode_operations. I was proposing to add another optional device_get_match_data callback to 'struct bus_type' to allow individual buses control how match data is handled, before (or after) jumping into the fwnode-backed device_get_match_data callbacks. Thanks.
Hi Dmitry, > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > The .device_get_match_data callbacks are missing for I2C and SPI bus > subsystems. > > > Can you please throw some lights on this? > > > > It's the first time I've ever heard of that callback, I don't know why > > whoever added it wouldn't have done those buses in particular or if it > > just didn't happen. Try adding it and if it works send the patches? > > I think there is a disconnect. Right now device_get_match_data callbacks > are part of fwnode_operations. I was proposing to add another optional > device_get_match_data callback to 'struct bus_type' to allow individual > buses control how match data is handled, before (or after) jumping into > the fwnode-backed device_get_match_data callbacks. That is what implemented here [1] and [2] right? [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117 [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364 First it check for fwnode-backed device_get_match_data callbacks and Fallback is bus-type based match. Looks like you are proposing to unify [1] and [2] and you want the logic to be other way around. ie, first bus-type match, then fwnode-backed callbacks? Cheers, Biju
On Mon, Jul 17, 2023 at 11:28:12AM -0700, Dmitry Torokhov wrote: > I think there is a disconnect. Right now device_get_match_data callbacks > are part of fwnode_operations. I was proposing to add another optional > device_get_match_data callback to 'struct bus_type' to allow individual > buses control how match data is handled, before (or after) jumping into > the fwnode-backed device_get_match_data callbacks. Ah, that explains why I never heard of it! I can see that being useful for providing a single interface to pull mach data out of in drivers.
On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > Hi Dmitry, > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > The .device_get_match_data callbacks are missing for I2C and SPI bus > > subsystems. > > > > Can you please throw some lights on this? > > > > > > It's the first time I've ever heard of that callback, I don't know why > > > whoever added it wouldn't have done those buses in particular or if it > > > just didn't happen. Try adding it and if it works send the patches? > > > > I think there is a disconnect. Right now device_get_match_data callbacks > > are part of fwnode_operations. I was proposing to add another optional > > device_get_match_data callback to 'struct bus_type' to allow individual > > buses control how match data is handled, before (or after) jumping into > > the fwnode-backed device_get_match_data callbacks. > > That is what implemented here [1] and [2] right? > > [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117 > [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364 > > First it check for fwnode-backed device_get_match_data callbacks and > Fallback is bus-type based match. > > Looks like you are proposing to unify [1] and [2] and you want the > logic to be other way around. ie, first bus-type match, then > fwnode-backed callbacks? > I do not have a strong preference for the ordering, i.e. I think it is perfectly fine to do the generic fwnode-based lookup and if there is no match have bus method called as a fallback, but I do not want driver writers to learn about multiple <bus-prefix>_get_match_data() implementations, I would prefer if they could call device_get_match_data() and the right thing happened in all cases. Thanks.
Hi Dmitry Torokhov, Thanks for the feedback. > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > Hi Dmitry, > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > The .device_get_match_data callbacks are missing for I2C and SPI > > > > > bus > > > subsystems. > > > > > Can you please throw some lights on this? > > > > > > > > It's the first time I've ever heard of that callback, I don't know > > > > why whoever added it wouldn't have done those buses in particular > > > > or if it just didn't happen. Try adding it and if it works send > the patches? > > > > > > I think there is a disconnect. Right now device_get_match_data > > > callbacks are part of fwnode_operations. I was proposing to add > > > another optional device_get_match_data callback to 'struct bus_type' > > > to allow individual buses control how match data is handled, before > > > (or after) jumping into the fwnode-backed device_get_match_data > callbacks. > > > > That is what implemented here [1] and [2] right? [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364 [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117 > > > > > > First it check for fwnode-backed device_get_match_data callbacks and > > Fallback is bus-type based match. > > > > Looks like you are proposing to unify [1] and [2] and you want the > > logic to be other way around. ie, first bus-type match, then > > fwnode-backed callbacks? > > > > I do not have a strong preference for the ordering, i.e. I think it is > perfectly fine to do the generic fwnode-based lookup and if there is no > match have bus method called as a fallback, That involves a bit of work. const void *device_get_match_data(const struct device *dev); const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, const struct i2c_client *client); const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev); Basically, the bus-client driver(such as exc3000) needs to pass struct device and device_get_match_data after generic fwnode-based lookup, needs to find the bus type based on struct device and call a new generic void* bus_get_match_data(void*) callback, so that each bus interface can do a match. I am not sure, is this proposal acceptable to wider people?? > but I do not want driver > writers to learn about multiple <bus-prefix>_get_match_data() > implementations, I would prefer if they could call > device_get_match_data() and the right thing happened in all cases. The driver is bus specific. So I don't know, why you want to be it generic. If it is i2c client, like other I2C api call the bus-subsystem api for match_data. Similarly, if it is spi client, do the same. Cheers, Biju
On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > Hi Dmitry Torokhov, > > Thanks for the feedback. > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > Hi Dmitry, > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > > > The .device_get_match_data callbacks are missing for I2C and SPI > > > > > > bus > > > > subsystems. > > > > > > Can you please throw some lights on this? > > > > > > > > > > It's the first time I've ever heard of that callback, I don't know > > > > > why whoever added it wouldn't have done those buses in particular > > > > > or if it just didn't happen. Try adding it and if it works send > > the patches? > > > > > > > > I think there is a disconnect. Right now device_get_match_data > > > > callbacks are part of fwnode_operations. I was proposing to add > > > > another optional device_get_match_data callback to 'struct bus_type' > > > > to allow individual buses control how match data is handled, before > > > > (or after) jumping into the fwnode-backed device_get_match_data > > callbacks. > > > > > > That is what implemented here [1] and [2] right? > > [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364 > > [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117 > > > > > > > > > > First it check for fwnode-backed device_get_match_data callbacks and > > > Fallback is bus-type based match. > > > > > > Looks like you are proposing to unify [1] and [2] and you want the > > > logic to be other way around. ie, first bus-type match, then > > > fwnode-backed callbacks? > > > > > > > I do not have a strong preference for the ordering, i.e. I think it is > > perfectly fine to do the generic fwnode-based lookup and if there is no > > match have bus method called as a fallback, > > That involves a bit of work. > > const void *device_get_match_data(const struct device *dev); > > const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, > const struct i2c_client *client); > > const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev); > > Basically, the bus-client driver(such as exc3000) needs to pass struct device > and device_get_match_data after generic fwnode-based lookup, > needs to find the bus type based on struct device and call a new generic > void* bus_get_match_data(void*) callback, so that each bus interface > can do a match. Yes, something like this (which does not seem that involved to me...): diff --git a/drivers/base/property.c b/drivers/base/property.c index 8c40abed7852..cc0bf7bb6f3a 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1277,7 +1277,13 @@ EXPORT_SYMBOL(fwnode_graph_parse_endpoint); const void *device_get_match_data(const struct device *dev) { - return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev); + const void *data; + + data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev); + if (!data && dev->bus && dev->bus->get_match_data) + data = dev->bus->get_match_data(dev); + + return data; } EXPORT_SYMBOL_GPL(device_get_match_data); diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 60746652fd52..5fe47bc491a6 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -114,6 +114,26 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); +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; + const struct i2c_device_id *match; + + if (!dev->driver) + return NULL; + + driver = to_i2c_driver(dev->driver); + if (!driver) + return NULL; + + match = i2c_match_id(driver->id_table, client); + if (!match) + return NULL; + + return (const void *)match->driver_data; +} + const void *i2c_get_match_data(const struct i2c_client *client) { struct i2c_driver *driver = to_i2c_driver(client->dev.driver); @@ -695,6 +715,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); diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h index ae10c4322754..3f2cba28a1af 100644 --- a/include/linux/device/bus.h +++ b/include/linux/device/bus.h @@ -102,6 +102,8 @@ struct bus_type { int (*dma_configure)(struct device *dev); void (*dma_cleanup)(struct device *dev); + const void *(*get_match_data)(const struct device *dev); + const struct dev_pm_ops *pm; const struct iommu_ops *iommu_ops; Thanks.
Hi Dmitry Torokhov, Thanks for the feedback. > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > > Hi Dmitry Torokhov, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > > Hi Dmitry, > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > > > > > The .device_get_match_data callbacks are missing for I2C and > > > > > > > SPI bus > > > > > subsystems. > > > > > > > Can you please throw some lights on this? > > > > > > > > > > > > It's the first time I've ever heard of that callback, I don't > > > > > > know why whoever added it wouldn't have done those buses in > > > > > > particular or if it just didn't happen. Try adding it and if > > > > > > it works send > > > the patches? > > > > > > > > > > I think there is a disconnect. Right now device_get_match_data > > > > > callbacks are part of fwnode_operations. I was proposing to add > > > > > another optional device_get_match_data callback to 'struct > bus_type' > > > > > to allow individual buses control how match data is handled, > > > > > before (or after) jumping into the fwnode-backed > > > > > device_get_match_data > > > callbacks. > > > > > > > > That is what implemented here [1] and [2] right? > > > > > > > > > > > > First it check for fwnode-backed device_get_match_data callbacks > > > > and Fallback is bus-type based match. > > > > > > > > Looks like you are proposing to unify [1] and [2] and you want the > > > > logic to be other way around. ie, first bus-type match, then > > > > fwnode-backed callbacks? > > > > > > > > > > I do not have a strong preference for the ordering, i.e. I think it > > > is perfectly fine to do the generic fwnode-based lookup and if there > > > is no match have bus method called as a fallback, > > > > That involves a bit of work. > > > > const void *device_get_match_data(const struct device *dev); > > > > const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > *id, > > const struct i2c_client *client); > > > > const struct spi_device_id *spi_get_device_id(const struct spi_device > > *sdev); > > > > Basically, the bus-client driver(such as exc3000) needs to pass struct > > device and device_get_match_data after generic fwnode-based lookup, > > needs to find the bus type based on struct device and call a new > > generic > > void* bus_get_match_data(void*) callback, so that each bus interface > > can do a match. > > Yes, something like this (which does not seem that involved to me...): Looks it will work. But there is some 2 additional checks in core code, every driver which is not bus type need to go through this checks. Also in Bus specific callback, there are 2 additional checks. So, performance wise [1] is better. Moreover, we need to avoid code duplication with [1] [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L125 What core people thinking about Dmitry's proposal? Cheers, Biju > > diff --git a/drivers/base/property.c b/drivers/base/property.c index > 8c40abed7852..cc0bf7bb6f3a 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -1277,7 +1277,13 @@ EXPORT_SYMBOL(fwnode_graph_parse_endpoint); > > const void *device_get_match_data(const struct device *dev) { > - return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, > dev); > + const void *data; > + > + data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, > dev); > + if (!data && dev->bus && dev->bus->get_match_data) > + data = dev->bus->get_match_data(dev); > + > + return data; > } > EXPORT_SYMBOL_GPL(device_get_match_data); > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 60746652fd52..5fe47bc491a6 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -114,6 +114,26 @@ const struct i2c_device_id *i2c_match_id(const > struct i2c_device_id *id, } EXPORT_SYMBOL_GPL(i2c_match_id); > > +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; > + const struct i2c_device_id *match; > + > + if (!dev->driver) > + return NULL; > + > + driver = to_i2c_driver(dev->driver); > + if (!driver) > + return NULL; > + > + match = i2c_match_id(driver->id_table, client); > + if (!match) > + return NULL; > + > + return (const void *)match->driver_data; } > + > const void *i2c_get_match_data(const struct i2c_client *client) { > struct i2c_driver *driver = to_i2c_driver(client->dev.driver); > @@ -695,6 +715,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); > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index ae10c4322754..3f2cba28a1af 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -102,6 +102,8 @@ struct bus_type { > int (*dma_configure)(struct device *dev); > void (*dma_cleanup)(struct device *dev); > > + const void *(*get_match_data)(const struct device *dev); > + > const struct dev_pm_ops *pm; > > const struct iommu_ops *iommu_ops; > > > Thanks. > > -- > Dmitry
On Sat, Jul 22, 2023 at 05:51:17PM +0000, Biju Das wrote: > Hi Dmitry Torokhov, > > Thanks for the feedback. > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > > > Hi Dmitry Torokhov, > > > > > > Thanks for the feedback. > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > > > Hi Dmitry, > > > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > > > > > > > The .device_get_match_data callbacks are missing for I2C and > > > > > > > > SPI bus > > > > > > subsystems. > > > > > > > > Can you please throw some lights on this? > > > > > > > > > > > > > > It's the first time I've ever heard of that callback, I don't > > > > > > > know why whoever added it wouldn't have done those buses in > > > > > > > particular or if it just didn't happen. Try adding it and if > > > > > > > it works send > > > > the patches? > > > > > > > > > > > > I think there is a disconnect. Right now device_get_match_data > > > > > > callbacks are part of fwnode_operations. I was proposing to add > > > > > > another optional device_get_match_data callback to 'struct > > bus_type' > > > > > > to allow individual buses control how match data is handled, > > > > > > before (or after) jumping into the fwnode-backed > > > > > > device_get_match_data > > > > callbacks. > > > > > > > > > > That is what implemented here [1] and [2] right? > > > > > > > > > > > > > > > First it check for fwnode-backed device_get_match_data callbacks > > > > > and Fallback is bus-type based match. > > > > > > > > > > Looks like you are proposing to unify [1] and [2] and you want the > > > > > logic to be other way around. ie, first bus-type match, then > > > > > fwnode-backed callbacks? > > > > > > > > > > > > > I do not have a strong preference for the ordering, i.e. I think it > > > > is perfectly fine to do the generic fwnode-based lookup and if there > > > > is no match have bus method called as a fallback, > > > > > > That involves a bit of work. > > > > > > const void *device_get_match_data(const struct device *dev); > > > > > > const struct i2c_device_id *i2c_match_id(const struct i2c_device_id > > *id, > > > const struct i2c_client *client); > > > > > > const struct spi_device_id *spi_get_device_id(const struct spi_device > > > *sdev); > > > > > > Basically, the bus-client driver(such as exc3000) needs to pass struct > > > device and device_get_match_data after generic fwnode-based lookup, > > > needs to find the bus type based on struct device and call a new > > > generic > > > void* bus_get_match_data(void*) callback, so that each bus interface > > > can do a match. > > > > Yes, something like this (which does not seem that involved to me...): > > Looks it will work. > > But there is some 2 additional checks in core code, every driver which is not bus type need to go through this checks. > > Also in Bus specific callback, there are 2 additional checks. > > So, performance wise [1] is better. I do not believe this is a concern whatsoever: majority of architectures/boards have been converted to ACPI/DT, which are being matched first as they are now, so the fallback to bus-specific matching against bus-specific device ID tables will be very infrequent. Additionally, device_get_match_data() is predominantly called from driver probe paths, so we need not be concerned with it being used with class devices or other kinds of devices not associated with a bus. > > Moreover, we need to avoid code duplication with [1] > > [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L125 If and when my proposed solution gets into the kernel we can drop i2c_get_match_data() altogether. Thanks. -- Dmitry
Hi Dmitry, Thanks for the feedback. > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > On Sat, Jul 22, 2023 at 05:51:17PM +0000, Biju Das wrote: > > Hi Dmitry Torokhov, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > > > > Hi Dmitry Torokhov, > > > > > > > > Thanks for the feedback. > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > > > > Hi Dmitry, > > > > > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify > > > > > > > probe() > > > > > > > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote: > > > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote: > > > > > > > > > > > > > > > > > The .device_get_match_data callbacks are missing for I2C > > > > > > > > > and SPI bus > > > > > > > subsystems. > > > > > > > > > Can you please throw some lights on this? > > > > > > > > > > > > > > > > It's the first time I've ever heard of that callback, I > > > > > > > > don't know why whoever added it wouldn't have done those > > > > > > > > buses in particular or if it just didn't happen. Try > > > > > > > > adding it and if it works send > > > > > the patches? > > > > > > > > > > > > > > I think there is a disconnect. Right now > > > > > > > device_get_match_data callbacks are part of > > > > > > > fwnode_operations. I was proposing to add another optional > > > > > > > device_get_match_data callback to 'struct > > > bus_type' > > > > > > > to allow individual buses control how match data is handled, > > > > > > > before (or after) jumping into the fwnode-backed > > > > > > > device_get_match_data > > > > > callbacks. > > > > > > > > > > > > That is what implemented here [1] and [2] right? > > > > > > > > > > > > > > > > > > First it check for fwnode-backed device_get_match_data > > > > > > callbacks and Fallback is bus-type based match. > > > > > > > > > > > > Looks like you are proposing to unify [1] and [2] and you want > > > > > > the logic to be other way around. ie, first bus-type match, > > > > > > then fwnode-backed callbacks? > > > > > > > > > > > > > > > > I do not have a strong preference for the ordering, i.e. I think > > > > > it is perfectly fine to do the generic fwnode-based lookup and > > > > > if there is no match have bus method called as a fallback, > > > > > > > > That involves a bit of work. > > > > > > > > const void *device_get_match_data(const struct device *dev); > > > > > > > > const struct i2c_device_id *i2c_match_id(const struct > > > > i2c_device_id > > > *id, > > > > const struct i2c_client > > > > *client); > > > > > > > > const struct spi_device_id *spi_get_device_id(const struct > > > > spi_device *sdev); > > > > > > > > Basically, the bus-client driver(such as exc3000) needs to pass > > > > struct device and device_get_match_data after generic fwnode-based > > > > lookup, needs to find the bus type based on struct device and call > > > > a new generic > > > > void* bus_get_match_data(void*) callback, so that each bus > > > > interface can do a match. > > > > > > Yes, something like this (which does not seem that involved to > me...): > > > > Looks it will work. > > > > But there is some 2 additional checks in core code, every driver which > is not bus type need to go through this checks. > > > > Also in Bus specific callback, there are 2 additional checks. > > > > So, performance wise [1] is better. > > I do not believe this is a concern whatsoever: majority of > architectures/boards have been converted to ACPI/DT, which are being > matched first as they are now, so the fallback to bus-specific matching > against bus-specific device ID tables will be very infrequent. > Additionally, device_get_match_data() is predominantly called from > driver probe paths, so we need not be concerned with it being used with > class devices or other kinds of devices not associated with a bus. Looks like most of the i2c client driver uses similar handling for ACPI/DT and ID tables. If that is the case, it is good to have this proposed change which will simplify most of the drivers listed in [1] [1] https://elixir.bootlin.com/linux/latest/A/ident/i2c_match_id Eg: drivers/hwmon/pmbus/ibm-cffps.c enum versions vs = cffps_unknown; const void *md = of_device_get_match_data(&client->dev); const struct i2c_device_id *id; if (md) { vs = (enum versions)md; } else { id = i2c_match_id(ibm_cffps_id, client); if (id) vs = (enum versions)id->driver_data; } The above code can be converted to vs = (enum versions)device_get_match_data(&client->dev); > > > > > Moreover, we need to avoid code duplication with [1] > > > > [1] > > If and when my proposed solution gets into the kernel we can drop > i2c_get_match_data() altogether. Agreed. Will wait for other people's view on this topic. Cheers, Biju
> Subject: RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > Hi Dmitry, > > Thanks for the feedback. > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > On Sat, Jul 22, 2023 at 05:51:17PM +0000, Biju Das wrote: > > > Hi Dmitry Torokhov, > > > > > > Thanks for the feedback. > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote: > > > > > Hi Dmitry Torokhov, > > > > > > > > > > Thanks for the feedback. > > > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe() > > > > > > > > > > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote: > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify > > > > > > > > probe() > > > > > > > > > > > > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown > wrote: > > > > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das > wrote: > > > > > > > > > > > > > > > > > > > The .device_get_match_data callbacks are missing for > > > > > > > > > > I2C and SPI bus > > > > > > > > subsystems. > > > > > > > > > > Can you please throw some lights on this? > > > > > > > > > > > > > > > > > > It's the first time I've ever heard of that callback, I > > > > > > > > > don't know why whoever added it wouldn't have done those > > > > > > > > > buses in particular or if it just didn't happen. Try > > > > > > > > > adding it and if it works send > > > > > > the patches? > > > > > > > > > > > > > > > > I think there is a disconnect. Right now > > > > > > > > device_get_match_data callbacks are part of > > > > > > > > fwnode_operations. I was proposing to add another optional > > > > > > > > device_get_match_data callback to 'struct > > > > bus_type' > > > > > > > > to allow individual buses control how match data is > > > > > > > > handled, before (or after) jumping into the fwnode-backed > > > > > > > > device_get_match_data > > > > > > callbacks. > > > > > > > > > > > > > > That is what implemented here [1] and [2] right? > > > > > > > > > > > > > > > > > > > > > First it check for fwnode-backed device_get_match_data > > > > > > > callbacks and Fallback is bus-type based match. > > > > > > > > > > > > > > Looks like you are proposing to unify [1] and [2] and you > > > > > > > want the logic to be other way around. ie, first bus-type > > > > > > > match, then fwnode-backed callbacks? > > > > > > > > > > > > > > > > > > > I do not have a strong preference for the ordering, i.e. I > > > > > > think it is perfectly fine to do the generic fwnode-based > > > > > > lookup and if there is no match have bus method called as a > > > > > > fallback, > > > > > > > > > > That involves a bit of work. > > > > > > > > > > const void *device_get_match_data(const struct device *dev); > > > > > > > > > > const struct i2c_device_id *i2c_match_id(const struct > > > > > i2c_device_id > > > > *id, > > > > > const struct i2c_client > > > > > *client); > > > > > > > > > > const struct spi_device_id *spi_get_device_id(const struct > > > > > spi_device *sdev); > > > > > > > > > > Basically, the bus-client driver(such as exc3000) needs to pass > > > > > struct device and device_get_match_data after generic > > > > > fwnode-based lookup, needs to find the bus type based on struct > > > > > device and call a new generic > > > > > void* bus_get_match_data(void*) callback, so that each bus > > > > > interface can do a match. > > > > > > > > Yes, something like this (which does not seem that involved to > > me...): > > > > > > Looks it will work. > > > > > > But there is some 2 additional checks in core code, every driver > > > which > > is not bus type need to go through this checks. > > > > > > Also in Bus specific callback, there are 2 additional checks. > > > > > > So, performance wise [1] is better. > > > > I do not believe this is a concern whatsoever: majority of > > architectures/boards have been converted to ACPI/DT, which are being > > matched first as they are now, so the fallback to bus-specific > > matching against bus-specific device ID tables will be very > infrequent. > > Additionally, device_get_match_data() is predominantly called from > > driver probe paths, so we need not be concerned with it being used > > with class devices or other kinds of devices not associated with a > bus. > > Looks like most of the i2c client driver uses similar handling for > ACPI/DT and ID tables. If that is the case, it is good to have this > proposed change which will simplify most of the drivers listed in [1] > > [1] > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir > .bootlin.com%2Flinux%2Flatest%2FA%2Fident%2Fi2c_match_id&data=05%7C01%7C > biju.das.jz%40bp.renesas.com%7C2a07c353ab7649fdf29a08db8b42cca3%7C53d825 > 71da1947e49cb4625a166a4a2a%7C0%7C0%7C638256891245437404%7CUnknown%7CTWFp > bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0% > 3D%7C3000%7C%7C%7C&sdata=tOxuTgGKc%2FQYFx94rYUJ8TDTWmGKkETzASV3qUjP2vk%3 > D&reserved=0 > > Eg: drivers/hwmon/pmbus/ibm-cffps.c > > enum versions vs = cffps_unknown; > const void *md = of_device_get_match_data(&client->dev); > const struct i2c_device_id *id; > > if (md) { > vs = (enum versions)md; > } else { > id = i2c_match_id(ibm_cffps_id, client); > if (id) > vs = (enum versions)id->driver_data; > } > > The above code can be converted to > vs = (enum versions)device_get_match_data(&client->dev); > > > > > > > > > Moreover, we need to avoid code duplication with [1] > > > > > > [1] > > > > If and when my proposed solution gets into the kernel we can drop > > i2c_get_match_data() altogether. > > Agreed. Will wait for other people's view on this topic. Also remove spi_get_device_match_data and Make i2c_match_id() and spi_get_device_id() as static and Replace all these with device_get_natch_data() from all i2c/spi client drivers. Can you please post a patch based on this? Cheers, Biju
On Sun, Jul 23, 2023 at 06:50:29AM +0000, Biju Das wrote: > > Can you please post a patch based on this? It looks like you are already taking care of this so I'll let you finish. Thanks.
diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c index 4c0d99aae9e0..8b65b4e2aa50 100644 --- a/drivers/input/touchscreen/exc3000.c +++ b/drivers/input/touchscreen/exc3000.c @@ -42,8 +42,6 @@ #define EXC3000_RESET_MS 10 #define EXC3000_READY_MS 100 -static const struct i2c_device_id exc3000_id[]; - struct eeti_dev_info { const char *name; int max_xy; @@ -347,12 +345,10 @@ static int exc3000_probe(struct i2c_client *client) return -ENOMEM; data->client = client; - data->info = device_get_match_data(&client->dev); - if (!data->info) { - enum eeti_dev_id eeti_dev_id = - i2c_match_id(exc3000_id, client)->driver_data; - data->info = &exc3000_info[eeti_dev_id]; - } + data->info = i2c_get_match_data(client); + if (!data->info) + return -ENODEV; + timer_setup(&data->timer, exc3000_timer, 0); init_completion(&data->wait_event); mutex_init(&data->query_lock); @@ -445,9 +441,9 @@ static int exc3000_probe(struct i2c_client *client) } static const struct i2c_device_id exc3000_id[] = { - { "exc3000", EETI_EXC3000 }, - { "exc80h60", EETI_EXC80H60 }, - { "exc80h84", EETI_EXC80H84 }, + { "exc3000", .driver_data = (kernel_ulong_t)&exc3000_info[EETI_EXC3000] }, + { "exc80h60", .driver_data = (kernel_ulong_t)&exc3000_info[EETI_EXC80H60] }, + { "exc80h84", .driver_data = (kernel_ulong_t)&exc3000_info[EETI_EXC80H84] }, { } }; MODULE_DEVICE_TABLE(i2c, exc3000_id);