When trying to instantiate a st_accel_i2c device from an ACPI based
system, I ran into some problems:
For my device, there is no ACPI match table entry, so rather than
creating /allocating a new ACPI HID for the device, I wanted to use an
existing DT table compatible entry via creating an ACPI_DT_NAMESPACE_HID
/PRP0001 HID ACPI entry (see Documentation/acpi/enumeration.txt).
This did not work because st_accel_i2c.c bails out if there is a ACPI
node but no ACPI table match. Use device_get_match_data API to get the
right driver data.
acpi_device_get_match_data() currently doesn't return DT compatible
matches. A separate patch is submitted to fix this.
v2:
- use device_get_match_data API as suggested by Andy Shevchenko
- removed syncing DT/I2C table names as Jonathan Cameron pointed out
this would be an ABI change
- converting from probe() to probe_new() is moved into a separate
patch (as suggested by Jonathan Cameron)
Nikolaus Voss (2):
IIO: st_accel_i2c.c: Simplify access to driver data
IIO: st_accel_i2c.c: Use probe_new() instead of probe()
drivers/iio/accel/st_accel_i2c.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
--
2.17.1
Use device_get_match_data API to simplify access to driver data.
Let acpi_device_id table entries point to the same driver data as
of_device_id table entries and uniquify access to driver data by using
device_get_match_data API.
Signed-off-by: Nikolaus Voss <[email protected]>
---
drivers/iio/accel/st_accel_i2c.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
index 6bdec8c451e0..163f7ec189b0 100644
--- a/drivers/iio/accel/st_accel_i2c.c
+++ b/drivers/iio/accel/st_accel_i2c.c
@@ -14,8 +14,8 @@
#include <linux/acpi.h>
#include <linux/i2c.h>
#include <linux/iio/iio.h>
+#include <linux/of_device.h>
-#include <linux/iio/common/st_sensors.h>
#include <linux/iio/common/st_sensors_i2c.h>
#include "st_accel.h"
@@ -107,7 +107,7 @@ MODULE_DEVICE_TABLE(of, st_accel_of_match);
#ifdef CONFIG_ACPI
static const struct acpi_device_id st_accel_acpi_match[] = {
- {"SMO8A90", LNG2DM},
+ {"SMO8A90", (kernel_ulong_t)LNG2DM_ACCEL_DEV_NAME},
{ },
};
MODULE_DEVICE_TABLE(acpi, st_accel_acpi_match);
@@ -143,6 +143,7 @@ static int st_accel_i2c_probe(struct i2c_client *client,
{
struct iio_dev *indio_dev;
struct st_sensor_data *adata;
+ const char *match;
int ret;
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*adata));
@@ -151,19 +152,13 @@ static int st_accel_i2c_probe(struct i2c_client *client,
adata = iio_priv(indio_dev);
- if (client->dev.of_node) {
- st_sensors_of_name_probe(&client->dev, st_accel_of_match,
- client->name, sizeof(client->name));
- } else if (ACPI_HANDLE(&client->dev)) {
- ret = st_sensors_match_acpi_device(&client->dev);
- if ((ret < 0) || (ret >= ST_ACCEL_MAX))
- return -ENODEV;
+ match = of_device_get_match_data(&client->dev);
- strlcpy(client->name, st_accel_id_table[ret].name,
- sizeof(client->name));
- } else if (!id)
- return -ENODEV;
+ if (!match)
+ match = acpi_device_get_match_data(&client->dev);
+ if (match)
+ strlcpy(client->name, match, sizeof(client->name));
st_sensors_i2c_configure(indio_dev, client, adata);
--
2.17.1
struct i2c_device_id argument of probe() is not used, so use probe_new()
instead.
Signed-off-by: Nikolaus Voss <[email protected]>
---
drivers/iio/accel/st_accel_i2c.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
index 163f7ec189b0..2bc54a5a2c96 100644
--- a/drivers/iio/accel/st_accel_i2c.c
+++ b/drivers/iio/accel/st_accel_i2c.c
@@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
-static int st_accel_i2c_probe(struct i2c_client *client,
- const struct i2c_device_id *id)
+static int st_accel_i2c_probe(struct i2c_client *client)
{
struct iio_dev *indio_dev;
struct st_sensor_data *adata;
@@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = {
.of_match_table = of_match_ptr(st_accel_of_match),
.acpi_match_table = ACPI_PTR(st_accel_acpi_match),
},
- .probe = st_accel_i2c_probe,
+ .probe_new = st_accel_i2c_probe,
.remove = st_accel_i2c_remove,
.id_table = st_accel_id_table,
};
--
2.17.1
On Tue, Jul 3, 2018 at 8:41 AM, Nikolaus Voss
<[email protected]> wrote:
> Use device_get_match_data API to simplify access to driver data.
..._data()
But. You actually don't use it below.
> Let acpi_device_id table entries point to the same driver data as
> of_device_id table entries and uniquify access to driver data by using
> device_get_match_data API.
> #include <linux/acpi.h>
> #include <linux/i2c.h>
> #include <linux/iio/iio.h>
> +#include <linux/of_device.h>
(linux/property.h)
> + match = of_device_get_match_data(&client->dev);
> + if (!match)
> + match = acpi_device_get_match_data(&client->dev);
What I meant is to simply call
match = device_get_match_data(...);
> + if (match)
> + strlcpy(client->name, match, sizeof(client->name));
--
With Best Regards,
Andy Shevchenko
On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
<[email protected]> wrote:
> struct i2c_device_id argument of probe() is not used, so use probe_new()
> instead.
>
This makes...
> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
...this table obsolete IIUC. At least that's what I did when switched
to ->probe_new() in some drivers.
If I'm mistaken (again? :-) ) I would hear from someone to point me
how it can be used after a switch.
>
> -static int st_accel_i2c_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int st_accel_i2c_probe(struct i2c_client *client)
> {
> struct iio_dev *indio_dev;
> struct st_sensor_data *adata;
> @@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = {
> .of_match_table = of_match_ptr(st_accel_of_match),
> .acpi_match_table = ACPI_PTR(st_accel_acpi_match),
> },
> - .probe = st_accel_i2c_probe,
> + .probe_new = st_accel_i2c_probe,
> .remove = st_accel_i2c_remove,
> .id_table = st_accel_id_table,
> };
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
On Wed, 4 Jul 2018, Andy Shevchenko wrote:
> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
> <[email protected]> wrote:
>> struct i2c_device_id argument of probe() is not used, so use probe_new()
>> instead.
>>
>
> This makes...
>
>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>
> ...this table obsolete IIUC. At least that's what I did when switched
> to ->probe_new() in some drivers.
>
> If I'm mistaken (again? :-) ) I would hear from someone to point me
> how it can be used after a switch.
It is still used by the i2c-core in i2c_device_match() if DT and ACPI
matching fails. And it is used to create the corresponding modaliases for
driver loading. So it is necessary for non-DT/ non-ACPI systems and used
for fallback matching if no match is found in of_device_ids.
>
>>
>> -static int st_accel_i2c_probe(struct i2c_client *client,
>> - const struct i2c_device_id *id)
>> +static int st_accel_i2c_probe(struct i2c_client *client)
>> {
>> struct iio_dev *indio_dev;
>> struct st_sensor_data *adata;
>> @@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = {
>> .of_match_table = of_match_ptr(st_accel_of_match),
>> .acpi_match_table = ACPI_PTR(st_accel_acpi_match),
>> },
>> - .probe = st_accel_i2c_probe,
>> + .probe_new = st_accel_i2c_probe,
>> .remove = st_accel_i2c_remove,
>> .id_table = st_accel_id_table,
>> };
>> --
>> 2.17.1
>>
>
>
>
>
On Wed, 4 Jul 2018, Andy Shevchenko wrote:
> On Tue, Jul 3, 2018 at 8:41 AM, Nikolaus Voss
> <[email protected]> wrote:
>> Use device_get_match_data API to simplify access to driver data.
>
> ..._data()
>
> But. You actually don't use it below.
It is used, see below.
>
>> Let acpi_device_id table entries point to the same driver data as
>> of_device_id table entries and uniquify access to driver data by using
>> device_get_match_data API.
>
>> #include <linux/acpi.h>
>> #include <linux/i2c.h>
>> #include <linux/iio/iio.h>
>
>> +#include <linux/of_device.h>
>
> (linux/property.h)
>
>> + match = of_device_get_match_data(&client->dev);
>> + if (!match)
>> + match = acpi_device_get_match_data(&client->dev);
>
> What I meant is to simply call
>
> match = device_get_match_data(...);
Ok, this works, thank you. I will prepare a new patch version.
This is where the match data is used:
>
>> + if (match)
>> + strlcpy(client->name, match, sizeof(client->name));
>
>
In this driver, match data is used to map DT compatible strings/ ACPI CIDs
to the key strings (.._ACCEL_DEV_NAME) which are used to identify the
actual device (and are also used in i2c_device_id table).
Niko
On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>
>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
>> <[email protected]> wrote:
>>>
>>> struct i2c_device_id argument of probe() is not used, so use probe_new()
>>> instead.
>>>
>>
>> This makes...
>>
>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>
>>
>> ...this table obsolete IIUC. At least that's what I did when switched
>> to ->probe_new() in some drivers.
>>
>> If I'm mistaken (again? :-) ) I would hear from someone to point me
>> how it can be used after a switch.
>
>
> It is still used by the i2c-core in i2c_device_match() if DT and ACPI
> matching fails.
> And it is used to create the corresponding modaliases for
> driver loading.
My question is "How?!"
I don't really see any points to match against it after switching to
->probe_new().
Could you point me to the code path in i2c (or OF?) core for that?
> So it is necessary for non-DT/ non-ACPI systems and used for
> fallback matching if no match is found in of_device_ids.
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 4, 2018 at 9:56 AM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>
>> On Tue, Jul 3, 2018 at 8:41 AM, Nikolaus Voss
>> <[email protected]> wrote:
>>>
>>> Use device_get_match_data API to simplify access to driver data.
>>
>>
>> ..._data()
>>
>> But. You actually don't use it below.
> It is used, see below.
I meant the API call you mentioned in the commit message is not used
in this version and below you agree to use it eventually.
>>> Let acpi_device_id table entries point to the same driver data as
>>> of_device_id table entries and uniquify access to driver data by using
>>> device_get_match_data API.
>>
>>
>>> #include <linux/acpi.h>
>>> #include <linux/i2c.h>
>>> #include <linux/iio/iio.h>
>>
>>
>>> +#include <linux/of_device.h>
>>
>>
>> (linux/property.h)
>>
>>> + match = of_device_get_match_data(&client->dev);
>>> + if (!match)
>>> + match = acpi_device_get_match_data(&client->dev);
>>
>>
>> What I meant is to simply call
>>
>> match = device_get_match_data(...);
>
>
> Ok, this works, thank you. I will prepare a new patch version.
>
> This is where the match data is used:
>
>>
>>> + if (match)
>>> + strlcpy(client->name, match, sizeof(client->name));
> In this driver, match data is used to map DT compatible strings/ ACPI CIDs
> to the key strings (.._ACCEL_DEV_NAME) which are used to identify the actual
> device (and are also used in i2c_device_id table).
--
With Best Regards,
Andy Shevchenko
On Wed, 4 Jul 2018, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss
> <[email protected]> wrote:
>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>
>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
>>> <[email protected]> wrote:
>>>>
>>>> struct i2c_device_id argument of probe() is not used, so use probe_new()
>>>> instead.
>>>>
>>>
>>> This makes...
>>>
>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>>
>>>
>>> ...this table obsolete IIUC. At least that's what I did when switched
>>> to ->probe_new() in some drivers.
>>>
>>> If I'm mistaken (again? :-) ) I would hear from someone to point me
>>> how it can be used after a switch.
>>
>>
>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI
>> matching fails.
>
>> And it is used to create the corresponding modaliases for
>> driver loading.
>
> My question is "How?!"
> I don't really see any points to match against it after switching to
> ->probe_new().
>
> Could you point me to the code path in i2c (or OF?) core for that?
As written above in i2c-core-base.c: i2c_device_match() ->
i2c_match_id(driver->id_table,...
This is used for driver matching before probe() or probe_new() of the
device driver can be called. probe_new() actually is a function signature
change only.
Niko
On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>
>> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss
>> <[email protected]> wrote:
>>>
>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>>
>>>>
>>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
>>>> <[email protected]> wrote:
>>>>>
>>>>>
>>>>> struct i2c_device_id argument of probe() is not used, so use
>>>>> probe_new()
>>>>> instead.
>>>>>
>>>>
>>>> This makes...
>>>>
>>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>>>
>>>>
>>>>
>>>> ...this table obsolete IIUC. At least that's what I did when switched
>>>> to ->probe_new() in some drivers.
>>>>
>>>> If I'm mistaken (again? :-) ) I would hear from someone to point me
>>>> how it can be used after a switch.
>>>
>>>
>>>
>>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI
>>> matching fails.
>>
>>
>>> And it is used to create the corresponding modaliases for
>>> driver loading.
>>
>>
>> My question is "How?!"
>> I don't really see any points to match against it after switching to
>> ->probe_new().
>>
>> Could you point me to the code path in i2c (or OF?) core for that?
>
>
> As written above in i2c-core-base.c: i2c_device_match() ->
> i2c_match_id(driver->id_table,...
>
> This is used for driver matching before probe() or probe_new() of the device
> driver can be called. probe_new() actually is a function signature change
> only.
Okay, IIUC we got a match. What should we do with it? The table is not
used in ->probe_new() (in i2c core), so, you can't say which line
matched there.
--
With Best Regards,
Andy Shevchenko
On Wed, 4 Jul 2018, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss
> <[email protected]> wrote:
>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>
>>> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss
>>> <[email protected]> wrote:
>>>>
>>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>>>
>>>>>
>>>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>> struct i2c_device_id argument of probe() is not used, so use
>>>>>> probe_new()
>>>>>> instead.
>>>>>>
>>>>>
>>>>> This makes...
>>>>>
>>>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>>>>
>>>>>
>>>>>
>>>>> ...this table obsolete IIUC. At least that's what I did when switched
>>>>> to ->probe_new() in some drivers.
>>>>>
>>>>> If I'm mistaken (again? :-) ) I would hear from someone to point me
>>>>> how it can be used after a switch.
>>>>
>>>>
>>>>
>>>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI
>>>> matching fails.
>>>
>>>
>>>> And it is used to create the corresponding modaliases for
>>>> driver loading.
>>>
>>>
>>> My question is "How?!"
>>> I don't really see any points to match against it after switching to
>>> ->probe_new().
>>>
>>> Could you point me to the code path in i2c (or OF?) core for that?
>>
>>
>> As written above in i2c-core-base.c: i2c_device_match() ->
>> i2c_match_id(driver->id_table,...
>>
>> This is used for driver matching before probe() or probe_new() of the device
>> driver can be called. probe_new() actually is a function signature change
>> only.
>
> Okay, IIUC we got a match. What should we do with it? The table is not
> used in ->probe_new() (in i2c core), so, you can't say which line
> matched there.
The table is not used by the driver, but is necessary to
a) bind an i2c device declared via i2c_board_info with type field set
to one of the names of the i2c_device_id table to this driver
b) bind an i2c device declared via DT or ACPI but with no match in of_id/
acpi_id table but an i2c_device_id table match to this driver (fallback
matching)
c) create the right modaliases at compile time for this driver to make
module auto-loading work in case of a) and b)
Niko
Summon Javier to the discussion.
For my opinion he is an expert in this topic.
On Wed, Jul 4, 2018 at 12:42 PM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>> On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss
>> <[email protected]> wrote:
>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss
>>>> <[email protected]> wrote:
>>>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote:
>>>>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss
>>>>>> <[email protected]> wrote:
>>>>>>> struct i2c_device_id argument of probe() is not used, so use
>>>>>>> probe_new()
>>>>>>> instead.
>>>>>> This makes...
>>>>>>
>>>>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>>>>> ...this table obsolete IIUC. At least that's what I did when switched
>>>>>> to ->probe_new() in some drivers.
>>>>>>
>>>>>> If I'm mistaken (again? :-) ) I would hear from someone to point me
>>>>>> how it can be used after a switch.
>>>>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI
>>>>> matching fails.
>>>>> And it is used to create the corresponding modaliases for
>>>>> driver loading.
>>>> My question is "How?!"
>>>> I don't really see any points to match against it after switching to
>>>> ->probe_new().
>>>>
>>>> Could you point me to the code path in i2c (or OF?) core for that?
>>> As written above in i2c-core-base.c: i2c_device_match() ->
>>> i2c_match_id(driver->id_table,...
>>>
>>> This is used for driver matching before probe() or probe_new() of the
>>> device
>>> driver can be called. probe_new() actually is a function signature change
>>> only.
>> Okay, IIUC we got a match. What should we do with it? The table is not
>> used in ->probe_new() (in i2c core), so, you can't say which line
>> matched there.
> The table is not used by the driver, but is necessary to
>
> a) bind an i2c device declared via i2c_board_info with type field set
> to one of the names of the i2c_device_id table to this driver
> b) bind an i2c device declared via DT or ACPI but with no match in of_id/
> acpi_id table but an i2c_device_id table match to this driver (fallback
> matching)
> c) create the right modaliases at compile time for this driver to make
> module auto-loading work in case of a) and b)
Javier, just a summary of the above. Nikolaus switched one driver to
use ->probe_new() hook and left i2c ID table at the same time.
My understanding that this table is not anymore in use.
But I have to admit I didn't see entire picture of this. Can you shed a light?
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 07/04/2018 12:19 PM, Andy Shevchenko wrote:
> Summon Javier to the discussion.
> For my opinion he is an expert in this topic.
I wouldn't call myself an expert in anything to be honest :)
[snip]
>
>> The table is not used by the driver, but is necessary to
>>
>> a) bind an i2c device declared via i2c_board_info with type field set
>> to one of the names of the i2c_device_id table to this driver
>> b) bind an i2c device declared via DT or ACPI but with no match in of_id/
>> acpi_id table but an i2c_device_id table match to this driver (fallback
>> matching)
>> c) create the right modaliases at compile time for this driver to make
>> module auto-loading work in case of a) and b)
>
I think Nikolaus is correct, assuming that the driver can be used on legacy
platforms that register the I2C devices using board files / platform data.
In that case you still need a I2C device ID table for (a) and (c) as he said.
I don't buy on (b) though, that's a bug in my opinion. If you register an I2C
device via DT then you must have a OF device ID entry that matches the device
and the same for ACPI. You can't rely on the I2C device table to do the match.
I would also remove the struct i2c_device_id .driver_data fields from the I2C
device ID table, since are not used and just makes reading the code confusing
(only the struct i2c_device_id .name is used as far as I can see).
> Javier, just a summary of the above. Nikolaus switched one driver to
> use ->probe_new() hook and left i2c ID table at the same time.
> My understanding that this table is not anymore in use.
>
> But I have to admit I didn't see entire picture of this. Can you shed a light?
>
So to shed some light, in the past even {OF,ACPI}-only drivers needed an I2C ID
table because: 1) the .probe callback had a struct i2c_device_id * parameter
and 2) the I2C core always reported a modalias of the form i2c:<foo> even for
devices registered via OF.
The .probe_new callbacks solves (1) and the I2C core now reports of:N*T*Cfoo*
solving (2). So the I2C device table isn't required anymore for {OF,ACPI}-only
drivers, but it's still required for drivers that support legacy board files
that calls i2c_register_board_info() directly. Same for the old .probe callback,
it's needed if struct i2c_device_id .driver_data is used in the probe function.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
> Hi Andy,
>
> On 07/04/2018 12:19 PM, Andy Shevchenko wrote:
>> Summon Javier to the discussion.
>> For my opinion he is an expert in this topic.
>
> I wouldn't call myself an expert in anything to be honest :)
>
> [snip]
>
>>
>>> The table is not used by the driver, but is necessary to
>>>
>>> a) bind an i2c device declared via i2c_board_info with type field set
>>> to one of the names of the i2c_device_id table to this driver
>>> b) bind an i2c device declared via DT or ACPI but with no match in of_id/
>>> acpi_id table but an i2c_device_id table match to this driver (fallback
>>> matching)
>>> c) create the right modaliases at compile time for this driver to make
>>> module auto-loading work in case of a) and b)
>>
>
> I think Nikolaus is correct, assuming that the driver can be used on legacy
> platforms that register the I2C devices using board files / platform data.
> In that case you still need a I2C device ID table for (a) and (c) as he said.
>
> I don't buy on (b) though, that's a bug in my opinion. If you register an I2C
> device via DT then you must have a OF device ID entry that matches the device
> and the same for ACPI. You can't rely on the I2C device table to do the match.
Ok, in my opinion it is an elegant way of not bloating the driver when no
explicit handling (e.g. reading DT properties) is needed. Just adding an
of_device_id doesn't change any driver functionality then but only maps to
an already existing i2c_table_id.
>
> I would also remove the struct i2c_device_id .driver_data fields from the I2C
> device ID table, since are not used and just makes reading the code confusing
> (only the struct i2c_device_id .name is used as far as I can see).
Valid point, thanks. I will change that.
>
>> Javier, just a summary of the above. Nikolaus switched one driver to
>> use ->probe_new() hook and left i2c ID table at the same time.
>> My understanding that this table is not anymore in use.
>>
>> But I have to admit I didn't see entire picture of this. Can you shed a light?
>>
>
> So to shed some light, in the past even {OF,ACPI}-only drivers needed an I2C ID
> table because: 1) the .probe callback had a struct i2c_device_id * parameter
> and 2) the I2C core always reported a modalias of the form i2c:<foo> even for
> devices registered via OF.
It could have been a null pointer and device driver binding (and
auto-loading) done just via driver.name.
Niko
Hi Nikolaus,
On Wed, Jul 4, 2018 at 1:15 PM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
[snip]
>> I think Nikolaus is correct, assuming that the driver can be used on
>> legacy
>> platforms that register the I2C devices using board files / platform data.
>> In that case you still need a I2C device ID table for (a) and (c) as he
>> said.
>>
>> I don't buy on (b) though, that's a bug in my opinion. If you register an
>> I2C
>> device via DT then you must have a OF device ID entry that matches the
>> device
>> and the same for ACPI. You can't rely on the I2C device table to do the
>> match.
>
>
> Ok, in my opinion it is an elegant way of not bloating the driver when no
> explicit handling (e.g. reading DT properties) is needed. Just adding an
> of_device_id doesn't change any driver functionality then but only maps to
> an already existing i2c_table_id.
>
I disagree, in the case of OF for example a compatible string is
composed of both a vendor a device, the complete tuple is what should
be matched.
But with the fallback, only the device portion would be used so both
<foo,bar> and <baz,bar> will match against the i2c device with id
"bar". It may or may not be correct but the vendor portion is very
important to disambiguate.
>>
>> I would also remove the struct i2c_device_id .driver_data fields from the
>> I2C
>> device ID table, since are not used and just makes reading the code
>> confusing
>> (only the struct i2c_device_id .name is used as far as I can see).
>
>
> Valid point, thanks. I will change that.
>
>>
>>> Javier, just a summary of the above. Nikolaus switched one driver to
>>> use ->probe_new() hook and left i2c ID table at the same time.
>>> My understanding that this table is not anymore in use.
>>>
>>> But I have to admit I didn't see entire picture of this. Can you shed a
>>> light?
>>>
>>
>> So to shed some light, in the past even {OF,ACPI}-only drivers needed an
>> I2C ID
>> table because: 1) the .probe callback had a struct i2c_device_id *
>> parameter
>> and 2) the I2C core always reported a modalias of the form i2c:<foo> even
>> for
>> devices registered via OF.
>
>
> It could have been a null pointer and device driver binding (and
> auto-loading) done just via driver.name.
>
I'm not sure I understood this comment.
> Niko
>
Best regards,
Javier
On Wed, Jul 4, 2018 at 1:26 PM, Javier Martinez Canillas
<[email protected]> wrote:
[snip]
>> Ok, in my opinion it is an elegant way of not bloating the driver when no
>> explicit handling (e.g. reading DT properties) is needed. Just adding an
>> of_device_id doesn't change any driver functionality then but only maps to
>> an already existing i2c_table_id.
>>
>
> I disagree, in the case of OF for example a compatible string is
> composed of both a vendor a device, the complete tuple is what should
> be matched.
>
> But with the fallback, only the device portion would be used so both
> <foo,bar> and <baz,bar> will match against the i2c device with id
> "bar". It may or may not be correct but the vendor portion is very
> important to disambiguate.
>
And having a compatible = "bar" is also wrong since the compatible
string would lack the vendor prefix. The fact that the I2C (and SPI)
core allowed this was what caused the problem in the first place IMO.
Best regards,
Javier
Hi Javier,
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
> Hi Nikolaus,
>
> On Wed, Jul 4, 2018 at 1:15 PM, Nikolaus Voss
> <[email protected]> wrote:
>> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
>
> [snip]
>
>>> I think Nikolaus is correct, assuming that the driver can be used on
>>> legacy
>>> platforms that register the I2C devices using board files / platform data.
>>> In that case you still need a I2C device ID table for (a) and (c) as he
>>> said.
>>>
>>> I don't buy on (b) though, that's a bug in my opinion. If you register an
>>> I2C
>>> device via DT then you must have a OF device ID entry that matches the
>>> device
>>> and the same for ACPI. You can't rely on the I2C device table to do the
>>> match.
>>
>>
>> Ok, in my opinion it is an elegant way of not bloating the driver when no
>> explicit handling (e.g. reading DT properties) is needed. Just adding an
>> of_device_id doesn't change any driver functionality then but only maps to
>> an already existing i2c_table_id.
>>
>
> I disagree, in the case of OF for example a compatible string is
> composed of both a vendor a device, the complete tuple is what should
> be matched.
>
> But with the fallback, only the device portion would be used so both
> <foo,bar> and <baz,bar> will match against the i2c device with id
> "bar". It may or may not be correct but the vendor portion is very
> important to disambiguate.
Disambiguation via DT implies there is already a name collision in i2c
modalias name space, so adding an of_id would work around this collision
for DT enumeration. I2c_board_info driver binding would still be broken.
The right fix would be to remove the name collision.
>>>
>>> I would also remove the struct i2c_device_id .driver_data fields from the
>>> I2C
>>> device ID table, since are not used and just makes reading the code
>>> confusing
>>> (only the struct i2c_device_id .name is used as far as I can see).
>>
>>
>> Valid point, thanks. I will change that.
>>
>>>
>>>> Javier, just a summary of the above. Nikolaus switched one driver to
>>>> use ->probe_new() hook and left i2c ID table at the same time.
>>>> My understanding that this table is not anymore in use.
>>>>
>>>> But I have to admit I didn't see entire picture of this. Can you shed a
>>>> light?
>>>>
>>>
>>> So to shed some light, in the past even {OF,ACPI}-only drivers needed an
>>> I2C ID
>>> table because: 1) the .probe callback had a struct i2c_device_id *
>>> parameter
>>> and 2) the I2C core always reported a modalias of the form i2c:<foo> even
>>> for
>>> devices registered via OF.
>>
>>
>> It could have been a null pointer and device driver binding (and
>> auto-loading) done just via driver.name.
>>
>
> I'm not sure I understood this comment.
What I was trying to say is that if the i2c_device_id table information
was not needed (i.d. only one single id), even the old probe() could be
used without defining an i2c_device_id table, the i2c_device_id* argument
to probe() being a nullptr.
Niko
On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
<[email protected]> wrote:
[snip]
>>>
>>> Ok, in my opinion it is an elegant way of not bloating the driver when no
>>> explicit handling (e.g. reading DT properties) is needed. Just adding an
>>> of_device_id doesn't change any driver functionality then but only maps
>>> to
>>> an already existing i2c_table_id.
>>>
>>
>> I disagree, in the case of OF for example a compatible string is
>> composed of both a vendor a device, the complete tuple is what should
>> be matched.
>>
>> But with the fallback, only the device portion would be used so both
>> <foo,bar> and <baz,bar> will match against the i2c device with id
>> "bar". It may or may not be correct but the vendor portion is very
>> important to disambiguate.
>
>
> Disambiguation via DT implies there is already a name collision in i2c
> modalias name space, so adding an of_id would work around this collision for
There wouldn't be a name collision between OF modaliases in that case
since the vendor would be different (of:N*T*Cfoo,bar and
of:N*T*Cbaz,bar). So it wouldn't be a problem for DT-only drivers.
> DT enumeration. I2c_board_info driver binding would still be broken. The
> right fix would be to remove the name collision.
>
Yes, for legacy drivers using board files it would be an issue. But
that's unrelated to what I'm saying. What I don't think is correct is
to ignore the vendor prefix since that's part of the compatible string
semantics.
In general, I think that there should be consistency between the
firmware interface used to register the device, how the module alias
uevent is reported to auto-load a driver and how the driver is matched
with the device. So drivers supporting DT should have a n OF table
(and export it with MODULE_DEVICE_TABLE), drivers supporting ACPI
should have a ACPI table and so on.
But this discussion isn't really related to your patch. I think is
correct but just said that (b) wasn't a justification to leave the I2C
table, points (a) and (c) are though. I won't really be convinced that
the fallback is the correct thing to do or even a good idea.
[snip]
>>>
>>> It could have been a null pointer and device driver binding (and
>>> auto-loading) done just via driver.name.
>>>
>>
>> I'm not sure I understood this comment.
>
>
> What I was trying to say is that if the i2c_device_id table information was
> not needed (i.d. only one single id), even the old probe() could be used
> without defining an i2c_device_id table, the i2c_device_id* argument to
> probe() being a nullptr.
>
I see, yes that would work too. I didn't authored the .probe_new
callback so I don't know if other options were discussed. I also can't
remember if the I2C core attempted to probe even without an I2C device
ID table, I thought it didn't but I can be wrong.
> Niko
Best regards,
Javier
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
> <[email protected]> wrote:
>
[snip]
> But this discussion isn't really related to your patch. I think is
> correct but just said that (b) wasn't a justification to leave the I2C
> table, points (a) and (c) are though. I won't really be convinced that
> the fallback is the correct thing to do or even a good idea.
I didn't want to annoy you, I just wanted to understand why you think
fallback is such a bad thing that you call it a bug. And I see, it has its
drawbacks ;-). Anyway, thanks for taking the time to clarify this,
Niko
[snip]
On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss
<[email protected]> wrote:
> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
>>
>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
>> <[email protected]> wrote:
>>
>
> [snip]
>
>> But this discussion isn't really related to your patch. I think is
>> correct but just said that (b) wasn't a justification to leave the I2C
>> table, points (a) and (c) are though. I won't really be convinced that
>> the fallback is the correct thing to do or even a good idea.
>
>
> I didn't want to annoy you, I just wanted to understand why you think
> fallback is such a bad thing that you call it a bug. And I see, it has its
> drawbacks ;-). Anyway, thanks for taking the time to clarify this,
>
Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say
is that I've a strong opinion on this and won't be convinced otherwise
:)
So for me is a bug because that would mean that either an entry is
missing in an OF device table or a DTS has a node with a compatible
string without a vendor prefix.
> Niko
>
> [snip]
>
Best regards,
Javier
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
> On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss
> <[email protected]> wrote:
>> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
>>>
>>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
>>> <[email protected]> wrote:
>>>
>>
>> [snip]
>>
>>> But this discussion isn't really related to your patch. I think is
>>> correct but just said that (b) wasn't a justification to leave the I2C
>>> table, points (a) and (c) are though. I won't really be convinced that
>>> the fallback is the correct thing to do or even a good idea.
>>
>>
>> I didn't want to annoy you, I just wanted to understand why you think
>> fallback is such a bad thing that you call it a bug. And I see, it has its
>> drawbacks ;-). Anyway, thanks for taking the time to clarify this,
>>
>
> Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say
> is that I've a strong opinion on this and won't be convinced otherwise
> :)
>
> So for me is a bug because that would mean that either an entry is
> missing in an OF device table or a DTS has a node with a compatible
> string without a vendor prefix.
Yes, I see your point (and your strong opinion :-)), but AFAIK vendor
prefix is not mandatory... At least for vendor-agnostic drivers like
"regulator-fixed" (very popular in dts files). My point is not bloating
drivers with large redundant (from a driver-functional view) tables when
one table could be enough for a properly working driver. Having three
different names for exactly the same isn't very beautiful IMO.
I hope you're still not annoyed...
Niko
On 07/04/2018 03:24 PM, Nikolaus Voss wrote:
> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
>> On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss
>> <[email protected]> wrote:
>>> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote:
>>>>
>>>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
>>>> <[email protected]> wrote:
>>>>
>>>
>>> [snip]
>>>
>>>> But this discussion isn't really related to your patch. I think is
>>>> correct but just said that (b) wasn't a justification to leave the I2C
>>>> table, points (a) and (c) are though. I won't really be convinced that
>>>> the fallback is the correct thing to do or even a good idea.
>>>
>>>
>>> I didn't want to annoy you, I just wanted to understand why you think
>>> fallback is such a bad thing that you call it a bug. And I see, it has its
>>> drawbacks ;-). Anyway, thanks for taking the time to clarify this,
>>>
>>
>> Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say
>> is that I've a strong opinion on this and won't be convinced otherwise
>> :)
>>
>> So for me is a bug because that would mean that either an entry is
>> missing in an OF device table or a DTS has a node with a compatible
>> string without a vendor prefix.
>
> Yes, I see your point (and your strong opinion :-)), but AFAIK vendor
> prefix is not mandatory... At least for vendor-agnostic drivers like
The latest Device Tree specification [0] says about the compatible string:
"The recommended format is 'manufacturer,model', where manufacturer is a
string describing the name of the manufacturer (such as a stock ticker
symbol), and model specifies the model number".
> "regulator-fixed" (very popular in dts files). My point is not bloating
I don't think the "regulator-fixed" is a good example. Since the Device Tree
should describe the hardware. The "regulator-fixed" is a convenient way to
describe a fixed voltage regulator but I think is more of an exception.
I'm pretty sure that the DT maintainers wouldn't ack a DT binding with a
compatible string that doesn't have a manufacture prefix nowadays.
> drivers with large redundant (from a driver-functional view) tables when
> one table could be enough for a properly working driver. Having three
You need the OF table anyways for module autoload since the I2C core will report
a OF module alias. You can only do the I2C fallback trick if your driver can't
be build as a module. But even in that case you would be ignoring the vendor.
> different names for exactly the same isn't very beautiful IMO.
>
I agree with you on that. But abusing a table used by another firmware interface
isn't beautiful either. So I think the best is to have consistency and always
use the same table for the same firmware interface.
> I hope you're still not annoyed...
>
Don't worry for that, it's very hard to get my annoyed :)
> Niko
>
[0]: https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
On Wed, Jul 4, 2018 at 4:44 PM, Javier Martinez Canillas
<[email protected]> wrote:
> On 07/04/2018 03:24 PM, Nikolaus Voss wrote:
>> I hope you're still not annoyed...
> Don't worry for that, it's very hard to get my annoyed :)
Javier, thanks for your patience and nice explanation!
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 07/04/2018 06:11 PM, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 4:44 PM, Javier Martinez Canillas
> <[email protected]> wrote:
>> On 07/04/2018 03:24 PM, Nikolaus Voss wrote:
>
>>> I hope you're still not annoyed...
>> Don't worry for that, it's very hard to get my annoyed :)
>
> Javier, thanks for your patience and nice explanation!
>
You are welcome. I'm glad you found it useful!
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat