* [PATCH v2 0/2] exc3000 driver enhancements
@ 2023-07-17 13:17 Biju Das
2023-07-17 13:17 ` [PATCH v2 1/2] Input: exc3000 - Simplify probe() Biju Das
2023-07-17 13:17 ` [PATCH v2 2/2] Input: exc3000 - Drop enum eeti_dev_id and split exc3000_info[] Biju Das
0 siblings, 2 replies; 18+ messages in thread
From: Biju Das @ 2023-07-17 13:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Andreas Helbech Kleist, Uwe Kleine-König,
Geert Uytterhoeven, Mike Looijmans, linux-input,
Prabhakar Mahadev Lad, linux-renesas-soc
This patch series is based on the code improvement work done for
rtc-isl1208 driver and is just compile tested.
Biju Das (2):
Input: exc3000 - Simplify probe()
Input: exc3000 - Drop enum eeti_dev_id and split exc3000_info[]
v1->v2:
* Added Rb tag from Geert for patch#1
* Added patch#2 for dropping enum eeti_dev_id and split the array
exc3000_info[] as individual variables, and make lines shorter by
referring to e.g. &exc3000_info instead of &exc3000_info[EETI_EXC3000].
drivers/input/touchscreen/exc3000.c | 54 ++++++++++++-----------------
1 file changed, 22 insertions(+), 32 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 13:17 [PATCH v2 0/2] exc3000 driver enhancements Biju Das
@ 2023-07-17 13:17 ` Biju Das
2023-07-17 15:58 ` Dmitry Torokhov
2023-07-17 13:17 ` [PATCH v2 2/2] Input: exc3000 - Drop enum eeti_dev_id and split exc3000_info[] Biju Das
1 sibling, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-17 13:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König, linux-input,
Prabhakar Mahadev Lad, linux-renesas-soc
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(-)
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] Input: exc3000 - Drop enum eeti_dev_id and split exc3000_info[]
2023-07-17 13:17 [PATCH v2 0/2] exc3000 driver enhancements Biju Das
2023-07-17 13:17 ` [PATCH v2 1/2] Input: exc3000 - Simplify probe() Biju Das
@ 2023-07-17 13:17 ` Biju Das
1 sibling, 0 replies; 18+ messages in thread
From: Biju Das @ 2023-07-17 13:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Andreas Helbech Kleist, Uwe Kleine-König,
Geert Uytterhoeven, Mike Looijmans, linux-input,
Prabhakar Mahadev Lad, linux-renesas-soc
Drop enum eeti_dev_id and split the array exc3000_info[] as individual
variables, and make lines shorter by referring to e.g. &exc3000_info
instead of &exc3000_info[EETI_EXC3000].
Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
* New patch.
---
drivers/input/touchscreen/exc3000.c | 42 +++++++++++++----------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index 8b65b4e2aa50..b124a64f8164 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -47,25 +47,19 @@ struct eeti_dev_info {
int max_xy;
};
-enum eeti_dev_id {
- EETI_EXC3000,
- EETI_EXC80H60,
- EETI_EXC80H84,
+static const struct eeti_dev_info exc3000_info = {
+ .name = "EETI EXC3000 Touch Screen",
+ .max_xy = SZ_4K - 1
};
-static struct eeti_dev_info exc3000_info[] = {
- [EETI_EXC3000] = {
- .name = "EETI EXC3000 Touch Screen",
- .max_xy = SZ_4K - 1,
- },
- [EETI_EXC80H60] = {
- .name = "EETI EXC80H60 Touch Screen",
- .max_xy = SZ_16K - 1,
- },
- [EETI_EXC80H84] = {
- .name = "EETI EXC80H84 Touch Screen",
- .max_xy = SZ_16K - 1,
- },
+static const struct eeti_dev_info exc80h60_info = {
+ .name = "EETI EXC80H60 Touch Screen",
+ .max_xy = SZ_16K - 1
+};
+
+static const struct eeti_dev_info exc80h84_info = {
+ .name = "EETI EXC80H84 Touch Screen",
+ .max_xy = SZ_16K - 1
};
struct exc3000_data {
@@ -441,18 +435,18 @@ static int exc3000_probe(struct i2c_client *client)
}
static const struct i2c_device_id exc3000_id[] = {
- { "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] },
+ { "exc3000", .driver_data = (kernel_ulong_t)&exc3000_info },
+ { "exc80h60", .driver_data = (kernel_ulong_t)&exc80h60_info },
+ { "exc80h84", .driver_data = (kernel_ulong_t)&exc80h84_info },
{ }
};
MODULE_DEVICE_TABLE(i2c, exc3000_id);
#ifdef CONFIG_OF
static const struct of_device_id exc3000_of_match[] = {
- { .compatible = "eeti,exc3000", .data = &exc3000_info[EETI_EXC3000] },
- { .compatible = "eeti,exc80h60", .data = &exc3000_info[EETI_EXC80H60] },
- { .compatible = "eeti,exc80h84", .data = &exc3000_info[EETI_EXC80H84] },
+ { .compatible = "eeti,exc3000", .data = &exc3000_info },
+ { .compatible = "eeti,exc80h60", .data = &exc80h60_info },
+ { .compatible = "eeti,exc80h84", .data = &exc80h84_info },
{ }
};
MODULE_DEVICE_TABLE(of, exc3000_of_match);
@@ -460,7 +454,7 @@ MODULE_DEVICE_TABLE(of, exc3000_of_match);
#ifdef CONFIG_ACPI
static const struct acpi_device_id exc3000_acpi_match[] = {
- { "EGA00001", .driver_data = (kernel_ulong_t)&exc3000_info[EETI_EXC80H60] },
+ { "EGA00001", .driver_data = (kernel_ulong_t)&exc80h60_info },
{ }
};
MODULE_DEVICE_TABLE(acpi, exc3000_acpi_match);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 13:17 ` [PATCH v2 1/2] Input: exc3000 - Simplify probe() Biju Das
@ 2023-07-17 15:58 ` Dmitry Torokhov
2023-07-17 16:35 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-17 15:58 UTC (permalink / raw)
To: Biju Das
Cc: Mike Looijmans, Andreas Helbech Kleist, Geert Uytterhoeven,
Uwe Kleine-König, linux-input, Prabhakar Mahadev Lad,
linux-renesas-soc
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.
--
Dmitry
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 15:58 ` Dmitry Torokhov
@ 2023-07-17 16:35 ` Biju Das
2023-07-17 18:15 ` Mark Brown
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-17 16:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mike Looijmans, Andreas Helbech Kleist, Geert Uytterhoeven,
Uwe Kleine-König, linux-input@vger.kernel.org,
Prabhakar Mahadev Lad, linux-renesas-soc@vger.kernel.org,
Mark Brown, Wolfram Sang
+ 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 16:35 ` Biju Das
@ 2023-07-17 18:15 ` Mark Brown
2023-07-17 18:27 ` Biju Das
2023-07-17 18:28 ` Dmitry Torokhov
0 siblings, 2 replies; 18+ messages in thread
From: Mark Brown @ 2023-07-17 18:15 UTC (permalink / raw)
To: Biju Das
Cc: Dmitry Torokhov, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 18:15 ` Mark Brown
@ 2023-07-17 18:27 ` Biju Das
2023-07-17 18:28 ` Dmitry Torokhov
1 sibling, 0 replies; 18+ messages in thread
From: Biju Das @ 2023-07-17 18:27 UTC (permalink / raw)
To: Mark Brown
Cc: Dmitry Torokhov, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko
> -----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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 18:15 ` Mark Brown
2023-07-17 18:27 ` Biju Das
@ 2023-07-17 18:28 ` Dmitry Torokhov
2023-07-17 18:45 ` Biju Das
2023-07-17 18:49 ` Mark Brown
1 sibling, 2 replies; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-17 18:28 UTC (permalink / raw)
To: Mark Brown
Cc: Biju Das, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang
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.
--
Dmitry
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 18:28 ` Dmitry Torokhov
@ 2023-07-17 18:45 ` Biju Das
2023-07-18 18:36 ` Dmitry Torokhov
2023-07-17 18:49 ` Mark Brown
1 sibling, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-17 18:45 UTC (permalink / raw)
To: Dmitry Torokhov, Mark Brown
Cc: Mike Looijmans, Andreas Helbech Kleist, Geert Uytterhoeven,
Uwe Kleine-König, linux-input@vger.kernel.org,
Prabhakar Mahadev Lad, linux-renesas-soc@vger.kernel.org,
Wolfram Sang, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 18:28 ` Dmitry Torokhov
2023-07-17 18:45 ` Biju Das
@ 2023-07-17 18:49 ` Mark Brown
1 sibling, 0 replies; 18+ messages in thread
From: Mark Brown @ 2023-07-17 18:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-17 18:45 ` Biju Das
@ 2023-07-18 18:36 ` Dmitry Torokhov
2023-07-19 6:43 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-18 18:36 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko
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.
--
Dmitry
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-18 18:36 ` Dmitry Torokhov
@ 2023-07-19 6:43 ` Biju Das
2023-07-21 22:10 ` Dmitry Torokhov
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-19 6:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-19 6:43 ` Biju Das
@ 2023-07-21 22:10 ` Dmitry Torokhov
2023-07-22 17:51 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-21 22:10 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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.
--
Dmitry
^ permalink raw reply related [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-21 22:10 ` Dmitry Torokhov
@ 2023-07-22 17:51 ` Biju Das
2023-07-23 1:17 ` Dmitry Torokhov
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-22 17:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-22 17:51 ` Biju Das
@ 2023-07-23 1:17 ` Dmitry Torokhov
2023-07-23 6:05 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-23 1:17 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-23 1:17 ` Dmitry Torokhov
@ 2023-07-23 6:05 ` Biju Das
2023-07-23 6:50 ` Biju Das
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-23 6:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-23 6:05 ` Biju Das
@ 2023-07-23 6:50 ` Biju Das
2023-07-23 20:06 ` Dmitry Torokhov
0 siblings, 1 reply; 18+ messages in thread
From: Biju Das @ 2023-07-23 6:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
2023-07-23 6:50 ` Biju Das
@ 2023-07-23 20:06 ` Dmitry Torokhov
0 siblings, 0 replies; 18+ messages in thread
From: Dmitry Torokhov @ 2023-07-23 20:06 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
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.
--
Dmitry
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-07-23 20:07 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-17 13:17 [PATCH v2 0/2] exc3000 driver enhancements Biju Das
2023-07-17 13:17 ` [PATCH v2 1/2] Input: exc3000 - Simplify probe() Biju Das
2023-07-17 15:58 ` Dmitry Torokhov
2023-07-17 16:35 ` Biju Das
2023-07-17 18:15 ` Mark Brown
2023-07-17 18:27 ` Biju Das
2023-07-17 18:28 ` Dmitry Torokhov
2023-07-17 18:45 ` Biju Das
2023-07-18 18:36 ` Dmitry Torokhov
2023-07-19 6:43 ` Biju Das
2023-07-21 22:10 ` Dmitry Torokhov
2023-07-22 17:51 ` Biju Das
2023-07-23 1:17 ` Dmitry Torokhov
2023-07-23 6:05 ` Biju Das
2023-07-23 6:50 ` Biju Das
2023-07-23 20:06 ` Dmitry Torokhov
2023-07-17 18:49 ` Mark Brown
2023-07-17 13:17 ` [PATCH v2 2/2] Input: exc3000 - Drop enum eeti_dev_id and split exc3000_info[] Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).