diff mbox series

[v2,1/2] Input: exc3000 - Simplify probe()

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

Commit Message

Biju Das July 17, 2023, 1:17 p.m. UTC
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.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
 * Added Rb tag from Geert.
---
 drivers/input/touchscreen/exc3000.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Comments

Dmitry Torokhov July 17, 2023, 3:58 p.m. UTC | #1
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.
Biju Das July 17, 2023, 4:35 p.m. UTC | #2
+ 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
Mark Brown July 17, 2023, 6:15 p.m. UTC | #3
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?
Biju Das July 17, 2023, 6:27 p.m. UTC | #4
> -----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
Dmitry Torokhov July 17, 2023, 6:28 p.m. UTC | #5
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.
Biju Das July 17, 2023, 6:45 p.m. UTC | #6
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
Mark Brown July 17, 2023, 6:49 p.m. UTC | #7
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.
Dmitry Torokhov July 18, 2023, 6:36 p.m. UTC | #8
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.
Biju Das July 19, 2023, 6:43 a.m. UTC | #9
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
Dmitry Torokhov July 21, 2023, 10:10 p.m. UTC | #10
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.
Biju Das July 22, 2023, 5:51 p.m. UTC | #11
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
Dmitry Torokhov July 23, 2023, 1:17 a.m. UTC | #12
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
Biju Das July 23, 2023, 6:05 a.m. UTC | #13
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
Biju Das July 23, 2023, 6:50 a.m. UTC | #14
> 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
Dmitry Torokhov July 23, 2023, 8:06 p.m. UTC | #15
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 mbox series

Patch

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);