[PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Robert Dolca]

For i2c devices enumerated with ACPI you need to declare both
acpi_match_table and id_table. When using ACPI, the i2c_device_id structure
supplied to the probe function is null and you have to handle this case
in the driver.

The current name for the i2c client when using ACPI is "HID:UID" where the
UID has 7 or 8 characters and the UID has 2 characters. The UID is not
relevant for identifying the chip so it does not have any practical
purpose.

Modifying i2c_match_id we make the comparison by ignoring the UID from the
client name when the device was discovered using ACPI. The comparison is
case insensitive because the ACPI names are uppercase and the DT and ID
table names are lowercase. It would not make sense to have two different
chips with the same name and the only diference being the capitalized
letters.

With these changes the probe function gets a valid i2c_device_id and the
driver doesn't have to declare acpi_match_table.

Signed-off-by: Robert Dolca <robert.dolca@intel.com>
---
 drivers/i2c/i2c-core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index fec0e0d..c9b30b7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -447,8 +447,18 @@ static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
 static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 						const struct i2c_client *client)
 {
+	char name[I2C_NAME_SIZE], *c;
+
+	strlcpy(name, client->name, sizeof(name));
+
+	if (ACPI_HANDLE(&client->dev)) {
+		c = strchr(name, ':');
+		if (c)
+			*c = 0;
+	}
+
 	while (id->name[0]) {
-		if (strcmp(client->name, id->name) == 0)
+		if (strncasecmp(name, id->name, sizeof(id->name)) == 0)
 			return id;
 		id++;
 	}
-- 
1.9.1

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Rafael J. Wysocki]

On Tuesday, May 19, 2015 05:03:29 PM Robert Dolca wrote:
> For i2c devices enumerated with ACPI you need to declare both
> acpi_match_table and id_table. When using ACPI, the i2c_device_id structure
> supplied to the probe function is null and you have to handle this case
> in the driver.
> 
> The current name for the i2c client when using ACPI is "HID:UID" where the
> UID has 7 or 8 characters and the UID has 2 characters. The UID is not
> relevant for identifying the chip so it does not have any practical
> purpose.
> 
> Modifying i2c_match_id we make the comparison by ignoring the UID from the
> client name when the device was discovered using ACPI. The comparison is
> case insensitive because the ACPI names are uppercase and the DT and ID
> table names are lowercase. It would not make sense to have two different
> chips with the same name and the only diference being the capitalized
> letters.
> 
> With these changes the probe function gets a valid i2c_device_id and the
> driver doesn't have to declare acpi_match_table.
> 
> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> ---
>  drivers/i2c/i2c-core.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index fec0e0d..c9b30b7 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -447,8 +447,18 @@ static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
>  static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
>  						const struct i2c_client *client)
>  {
> +	char name[I2C_NAME_SIZE], *c;
> +
> +	strlcpy(name, client->name, sizeof(name));
> +
> +	if (ACPI_HANDLE(&client->dev)) {

If you don't need to use the handle, it is better to call has_acpi_companion() here.

> +		c = strchr(name, ':');
> +		if (c)
> +			*c = 0;
> +	}
> +
>  	while (id->name[0]) {
> -		if (strcmp(client->name, id->name) == 0)
> +		if (strncasecmp(name, id->name, sizeof(id->name)) == 0)
>  			return id;
>  		id++;
>  	}
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Mika Westerberg]

On Tue, May 19, 2015 at 05:03:29PM +0300, Robert Dolca wrote:
> For i2c devices enumerated with ACPI you need to declare both
> acpi_match_table and id_table. When using ACPI, the i2c_device_id structure
> supplied to the probe function is null and you have to handle this case
> in the driver.
> 
> The current name for the i2c client when using ACPI is "HID:UID" where the
> UID has 7 or 8 characters and the UID has 2 characters. The UID is not
> relevant for identifying the chip so it does not have any practical
> purpose.

First of all, it is not "HID:UID" since the number after ":" is actually
increasing number assigned by the ACPI core. Nothing to do with _UID.

Secondly we do not list "_HID:nn" in drivers acpi_match_tables but
instead it is either "HID" or "CID", no ":nn" there.

> Modifying i2c_match_id we make the comparison by ignoring the UID from the
> client name when the device was discovered using ACPI. The comparison is
> case insensitive because the ACPI names are uppercase and the DT and ID
> table names are lowercase. It would not make sense to have two different
> chips with the same name and the only diference being the capitalized
> letters.
> 
> With these changes the probe function gets a valid i2c_device_id and the
> driver doesn't have to declare acpi_match_table.

No. We don't do that for DT and we definitely don't want to mix ACPI
identifiers with arbitrary I2C device names.

You are not supposed to put ACPI identifiers into i2c_device_id table.

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Robert Dolca]

On Wed, May 20, 2015 at 10:47 AM, Mika Westerberg wrote:
> On Tue, May 19, 2015 at 05:03:29PM +0300, Robert Dolca wrote:
>> For i2c devices enumerated with ACPI you need to declare both
>> acpi_match_table and id_table. When using ACPI, the i2c_device_id structure
>> supplied to the probe function is null and you have to handle this case
>> in the driver.
>>
>> The current name for the i2c client when using ACPI is "HID:UID" where the
>> UID has 7 or 8 characters and the UID has 2 characters. The UID is not
>> relevant for identifying the chip so it does not have any practical
>> purpose.
>
> First of all, it is not "HID:UID" since the number after ":" is actually
> increasing number assigned by the ACPI core. Nothing to do with _UID.

You are right. My mistake.

> Secondly we do not list "_HID:nn" in drivers acpi_match_tables but
> instead it is either "HID" or "CID", no ":nn" there.

I didn't say that you do that :)

>
>> Modifying i2c_match_id we make the comparison by ignoring the UID from the
>> client name when the device was discovered using ACPI. The comparison is
>> case insensitive because the ACPI names are uppercase and the DT and ID
>> table names are lowercase. It would not make sense to have two different
>> chips with the same name and the only diference being the capitalized
>> letters.
>>
>> With these changes the probe function gets a valid i2c_device_id and the
>> driver doesn't have to declare acpi_match_table.
>
> No. We don't do that for DT and we definitely don't want to mix ACPI
> identifiers with arbitrary I2C device names.
>
> You are not supposed to put ACPI identifiers into i2c_device_id table.

Currently, if the name used for DT (in dts) matches one of the names
specified in the id table you will have a match. Isn't that an
intended behavior?

Robert

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Mika Westerberg]

On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
> Currently, if the name used for DT (in dts) matches one of the names
> specified in the id table you will have a match. Isn't that an
> intended behavior?

I thought one needs to put IDs to the driver .of_match_table. This is
also what i2c_device_match() is expecting, if I read it right.

BTW, how modules are supposed to be matched if we allow putting ACPI
identifiers to i2c_device_id table?

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Robert Dolca]

On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
> On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
>> Currently, if the name used for DT (in dts) matches one of the names
>> specified in the id table you will have a match. Isn't that an
>> intended behavior?
>
> I thought one needs to put IDs to the driver .of_match_table. This is
> also what i2c_device_match() is expecting, if I read it right.

If you put the DT id in of_match_table it will match here:

i2c_device_match
        /* Attempt an OF style match */
        if (of_driver_match_device(dev, drv))
                return 1;

If you don't specify of_match_table and you put the same ID in
i2c_device_id table it wil match here:

i2c_device_match
        driver = to_i2c_driver(drv);
        /* match on an id table if there is one */
        if (driver->id_table)
                return i2c_match_id(driver->id_table, client) != NULL;

This is happening because the name from dts is used for client->name.
i2c_match_id does the matching based on the client name.

> BTW, how modules are supposed to be matched if we allow putting ACPI
> identifiers to i2c_device_id table?

My aproach was like this: if the driver specifies .acpi_match table it
will work like before.

i2c_device_match
        /* Then ACPI style match */
        if (acpi_driver_match_device(dev, drv))
                return 1;

If the driver does not specify .acpi_match table the i2c core will
atempt to match against the  i2c_match_id table (the same way it does
for DT). In the ACPI case the client->name has that :nn suffix and
what the patch does is to ignore that when i2c_match_id is called.

i2c_device_match
        driver = to_i2c_driver(drv);
        /* match on an id table if there is one */
        if (driver->id_table)
                return i2c_match_id(driver->id_table, client) != NULL;

The final goal is to simplify the driver and remove redundant code.

Robert

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Mika Westerberg]

On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
> On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
> > On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
> >> Currently, if the name used for DT (in dts) matches one of the names
> >> specified in the id table you will have a match. Isn't that an
> >> intended behavior?
> >
> > I thought one needs to put IDs to the driver .of_match_table. This is
> > also what i2c_device_match() is expecting, if I read it right.
> 
> If you put the DT id in of_match_table it will match here:
> 
> i2c_device_match
>         /* Attempt an OF style match */
>         if (of_driver_match_device(dev, drv))
>                 return 1;
> 
> If you don't specify of_match_table and you put the same ID in
> i2c_device_id table it wil match here:
> 
> i2c_device_match
>         driver = to_i2c_driver(drv);
>         /* match on an id table if there is one */
>         if (driver->id_table)
>                 return i2c_match_id(driver->id_table, client) != NULL;
> 
> This is happening because the name from dts is used for client->name.
> i2c_match_id does the matching based on the client name.

OK.

> > BTW, how modules are supposed to be matched if we allow putting ACPI
> > identifiers to i2c_device_id table?
> 
> My aproach was like this: if the driver specifies .acpi_match table it
> will work like before.
> 
> i2c_device_match
>         /* Then ACPI style match */
>         if (acpi_driver_match_device(dev, drv))
>                 return 1;
> 
> If the driver does not specify .acpi_match table the i2c core will
> atempt to match against the  i2c_match_id table (the same way it does
> for DT). In the ACPI case the client->name has that :nn suffix and
> what the patch does is to ignore that when i2c_match_id is called.
> 
> i2c_device_match
>         driver = to_i2c_driver(drv);
>         /* match on an id table if there is one */
>         if (driver->id_table)
>                 return i2c_match_id(driver->id_table, client) != NULL;

Yeah but when you have device with modalias of "acpi:FOO:" how
udev/modprobe is supposed find the correct module?

> The final goal is to simplify the driver and remove redundant code.

IMHO mixing ACPI identifiers with I2C device identifiers does not
simplify anything. And since you need to stick the ACPI ID somewhere
anyway I don't get the point of removing redundant code either.

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Robert Dolca]

On Wed, May 20, 2015 at 1:57 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
>> On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
>> > On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
>> >> Currently, if the name used for DT (in dts) matches one of the names
>> >> specified in the id table you will have a match. Isn't that an
>> >> intended behavior?
>> >
>> > I thought one needs to put IDs to the driver .of_match_table. This is
>> > also what i2c_device_match() is expecting, if I read it right.
>>
>> If you put the DT id in of_match_table it will match here:
>>
>> i2c_device_match
>>         /* Attempt an OF style match */
>>         if (of_driver_match_device(dev, drv))
>>                 return 1;
>>
>> If you don't specify of_match_table and you put the same ID in
>> i2c_device_id table it wil match here:
>>
>> i2c_device_match
>>         driver = to_i2c_driver(drv);
>>         /* match on an id table if there is one */
>>         if (driver->id_table)
>>                 return i2c_match_id(driver->id_table, client) != NULL;
>>
>> This is happening because the name from dts is used for client->name.
>> i2c_match_id does the matching based on the client name.
>
> OK.
>
>> > BTW, how modules are supposed to be matched if we allow putting ACPI
>> > identifiers to i2c_device_id table?
>>
>> My aproach was like this: if the driver specifies .acpi_match table it
>> will work like before.
>>
>> i2c_device_match
>>         /* Then ACPI style match */
>>         if (acpi_driver_match_device(dev, drv))
>>                 return 1;
>>
>> If the driver does not specify .acpi_match table the i2c core will
>> atempt to match against the  i2c_match_id table (the same way it does
>> for DT). In the ACPI case the client->name has that :nn suffix and
>> what the patch does is to ignore that when i2c_match_id is called.
>>
>> i2c_device_match
>>         driver = to_i2c_driver(drv);
>>         /* match on an id table if there is one */
>>         if (driver->id_table)
>>                 return i2c_match_id(driver->id_table, client) != NULL;
>
> Yeah but when you have device with modalias of "acpi:FOO:" how
> udev/modprobe is supposed find the correct module?

The modalias file content exposed by the i2c device in sysfs will be the same.
Currently the alias exposed by the driver is based on the i2c_match_id
table and it does not have any aliases based on
acpi_device_id table. I am new to this are so please correct me if I am wrong.

>> The final goal is to simplify the driver and remove redundant code.
>
> IMHO mixing ACPI identifiers with I2C device identifiers does not
> simplify anything. And since you need to stick the ACPI ID somewhere
> anyway I don't get the point of removing redundant code either.

It will make the i2c_device_id be not NULL when the device is probed.
Here is an example:

static int probe(struct i2c_client *client, const struct i2c_device_id *id)
       /* Get device name from device table or ACPI */
        if (ACPI_HANDLE(dev)) {
                acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
                if (!acpi_id)
                        return -ENODEV;

                sprintf(data->fw_name, "%s.fw", acpi_id->id);

                for (i = 0; i < strlen(data->fw_name); i++)
                        data->fw_name[i] = tolower(data->fw_name[i]);
        } else {
                sprintf(data->fw_name, "%s.fw", id->name);
        }

This will become:
        sprintf(data->fw_name, "%s.fw", id->name);

There are allot more cases like this already in the kernel.

Robert

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Mika Westerberg]

On Wed, May 20, 2015 at 04:07:00PM +0300, Robert Dolca wrote:
> On Wed, May 20, 2015 at 1:57 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
> >> On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
> >> > On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
> >> >> Currently, if the name used for DT (in dts) matches one of the names
> >> >> specified in the id table you will have a match. Isn't that an
> >> >> intended behavior?
> >> >
> >> > I thought one needs to put IDs to the driver .of_match_table. This is
> >> > also what i2c_device_match() is expecting, if I read it right.
> >>
> >> If you put the DT id in of_match_table it will match here:
> >>
> >> i2c_device_match
> >>         /* Attempt an OF style match */
> >>         if (of_driver_match_device(dev, drv))
> >>                 return 1;
> >>
> >> If you don't specify of_match_table and you put the same ID in
> >> i2c_device_id table it wil match here:
> >>
> >> i2c_device_match
> >>         driver = to_i2c_driver(drv);
> >>         /* match on an id table if there is one */
> >>         if (driver->id_table)
> >>                 return i2c_match_id(driver->id_table, client) != NULL;
> >>
> >> This is happening because the name from dts is used for client->name.
> >> i2c_match_id does the matching based on the client name.
> >
> > OK.
> >
> >> > BTW, how modules are supposed to be matched if we allow putting ACPI
> >> > identifiers to i2c_device_id table?
> >>
> >> My aproach was like this: if the driver specifies .acpi_match table it
> >> will work like before.
> >>
> >> i2c_device_match
> >>         /* Then ACPI style match */
> >>         if (acpi_driver_match_device(dev, drv))
> >>                 return 1;
> >>
> >> If the driver does not specify .acpi_match table the i2c core will
> >> atempt to match against the  i2c_match_id table (the same way it does
> >> for DT). In the ACPI case the client->name has that :nn suffix and
> >> what the patch does is to ignore that when i2c_match_id is called.
> >>
> >> i2c_device_match
> >>         driver = to_i2c_driver(drv);
> >>         /* match on an id table if there is one */
> >>         if (driver->id_table)
> >>                 return i2c_match_id(driver->id_table, client) != NULL;
> >
> > Yeah but when you have device with modalias of "acpi:FOO:" how
> > udev/modprobe is supposed find the correct module?
> 
> The modalias file content exposed by the i2c device in sysfs will be the same.

It won't be the same. If it has an ACPI companion it will be different.

Here's one example from a machine with I2C touchscreen connected:

# cat /sys/bus/i2c/devices/i2c-ATML3432\:00/modalias 
acpi:ATML3432:PNP0C50:

> Currently the alias exposed by the driver is based on the i2c_match_id
> table and it does not have any aliases based on
> acpi_device_id table. I am new to this are so please correct me if I am wrong.
> 
> >> The final goal is to simplify the driver and remove redundant code.
> >
> > IMHO mixing ACPI identifiers with I2C device identifiers does not
> > simplify anything. And since you need to stick the ACPI ID somewhere
> > anyway I don't get the point of removing redundant code either.
> 
> It will make the i2c_device_id be not NULL when the device is probed.
> Here is an example:
> 
> static int probe(struct i2c_client *client, const struct i2c_device_id *id)
>        /* Get device name from device table or ACPI */
>         if (ACPI_HANDLE(dev)) {
>                 acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
>                 if (!acpi_id)
>                         return -ENODEV;
> 
>                 sprintf(data->fw_name, "%s.fw", acpi_id->id);
> 
>                 for (i = 0; i < strlen(data->fw_name); i++)
>                         data->fw_name[i] = tolower(data->fw_name[i]);
>         } else {
>                 sprintf(data->fw_name, "%s.fw", id->name);
>         }
> 
> This will become:
>         sprintf(data->fw_name, "%s.fw", id->name);

OK, in that case it shortens the code I give you that. But the normal
case where you only need to match against the ACPI identifier (and
handling modules properly) this does not simplify anything.

> There are allot more cases like this already in the kernel.

I didn't find too many. Most of them are under drivers/iio and they all
do something like:

static const char *kmx61_match_acpi_device(struct device *dev)
{
        const struct acpi_device_id *id;

        id = acpi_match_device(dev->driver->acpi_match_table, dev);
        if (!id)
                return NULL;
        return dev_name(dev);
}

static int kmx61_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
	...
	if (id)
                name = id->name;
        else if (ACPI_HANDLE(&client->dev))
                name = kmx61_match_acpi_device(&client->dev);
        else
                return -ENODEV;

which I think is not needed at all. If you end up having an ACPI handle
for your I2C device and the I2C core has already done the matching, you
don't need to do the match again in the driver. Instead this can be written like:

	if (id)
		name = id->name;
	else if (has_acpi_companion(&client->dev))
		name = dev_name(&client->dev);
	else
		return -ENODEV;

And you probably don't need that 'name' either.

Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

[Robert Dolca]

On Wed, May 20, 2015 at 4:57 PM, Mika Westerberg wrote:
> On Wed, May 20, 2015 at 04:07:00PM +0300, Robert Dolca wrote:
>> On Wed, May 20, 2015 at 1:57 PM, Mika Westerberg wrote:
>> > On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
>> >> On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
>> >> > On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
>> >> >> Currently, if the name used for DT (in dts) matches one of the names
>> >> >> specified in the id table you will have a match. Isn't that an
>> >> >> intended behavior?
>> >> >
>> >> > I thought one needs to put IDs to the driver .of_match_table. This is
>> >> > also what i2c_device_match() is expecting, if I read it right.
>> >>
>> >> If you put the DT id in of_match_table it will match here:
>> >>
>> >> i2c_device_match
>> >>         /* Attempt an OF style match */
>> >>         if (of_driver_match_device(dev, drv))
>> >>                 return 1;
>> >>
>> >> If you don't specify of_match_table and you put the same ID in
>> >> i2c_device_id table it wil match here:
>> >>
>> >> i2c_device_match
>> >>         driver = to_i2c_driver(drv);
>> >>         /* match on an id table if there is one */
>> >>         if (driver->id_table)
>> >>                 return i2c_match_id(driver->id_table, client) != NULL;
>> >>
>> >> This is happening because the name from dts is used for client->name.
>> >> i2c_match_id does the matching based on the client name.
>> >
>> > OK.
>> >
>> >> > BTW, how modules are supposed to be matched if we allow putting ACPI
>> >> > identifiers to i2c_device_id table?
>> >>
>> >> My aproach was like this: if the driver specifies .acpi_match table it
>> >> will work like before.
>> >>
>> >> i2c_device_match
>> >>         /* Then ACPI style match */
>> >>         if (acpi_driver_match_device(dev, drv))
>> >>                 return 1;
>> >>
>> >> If the driver does not specify .acpi_match table the i2c core will
>> >> atempt to match against the  i2c_match_id table (the same way it does
>> >> for DT). In the ACPI case the client->name has that :nn suffix and
>> >> what the patch does is to ignore that when i2c_match_id is called.
>> >>
>> >> i2c_device_match
>> >>         driver = to_i2c_driver(drv);
>> >>         /* match on an id table if there is one */
>> >>         if (driver->id_table)
>> >>                 return i2c_match_id(driver->id_table, client) != NULL;
>> >
>> > Yeah but when you have device with modalias of "acpi:FOO:" how
>> > udev/modprobe is supposed find the correct module?
>>
>> The modalias file content exposed by the i2c device in sysfs will be the same.
>
> It won't be the same. If it has an ACPI companion it will be different.
>
> Here's one example from a machine with I2C touchscreen connected:
>
> # cat /sys/bus/i2c/devices/i2c-ATML3432\:00/modalias
> acpi:ATML3432:PNP0C50:
>
>> Currently the alias exposed by the driver is based on the i2c_match_id
>> table and it does not have any aliases based on
>> acpi_device_id table. I am new to this are so please correct me if I am wrong.
>>
>> >> The final goal is to simplify the driver and remove redundant code.
>> >
>> > IMHO mixing ACPI identifiers with I2C device identifiers does not
>> > simplify anything. And since you need to stick the ACPI ID somewhere
>> > anyway I don't get the point of removing redundant code either.
>>
>> It will make the i2c_device_id be not NULL when the device is probed.
>> Here is an example:
>>
>> static int probe(struct i2c_client *client, const struct i2c_device_id *id)
>>        /* Get device name from device table or ACPI */
>>         if (ACPI_HANDLE(dev)) {
>>                 acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
>>                 if (!acpi_id)
>>                         return -ENODEV;
>>
>>                 sprintf(data->fw_name, "%s.fw", acpi_id->id);
>>
>>                 for (i = 0; i < strlen(data->fw_name); i++)
>>                         data->fw_name[i] = tolower(data->fw_name[i]);
>>         } else {
>>                 sprintf(data->fw_name, "%s.fw", id->name);
>>         }
>>
>> This will become:
>>         sprintf(data->fw_name, "%s.fw", id->name);
>
> OK, in that case it shortens the code I give you that. But the normal
> case where you only need to match against the ACPI identifier (and
> handling modules properly) this does not simplify anything.
>
>> There are allot more cases like this already in the kernel.
>
> I didn't find too many. Most of them are under drivers/iio and they all
> do something like:
>
> static const char *kmx61_match_acpi_device(struct device *dev)
> {
>         const struct acpi_device_id *id;
>
>         id = acpi_match_device(dev->driver->acpi_match_table, dev);
>         if (!id)
>                 return NULL;
>         return dev_name(dev);
> }
>
> static int kmx61_probe(struct i2c_client *client, const struct i2c_device_id *id)
> {
>         ...
>         if (id)
>                 name = id->name;
>         else if (ACPI_HANDLE(&client->dev))
>                 name = kmx61_match_acpi_device(&client->dev);
>         else
>                 return -ENODEV;
>
> which I think is not needed at all. If you end up having an ACPI handle
> for your I2C device and the I2C core has already done the matching, you
> don't need to do the match again in the driver. Instead this can be written like:
>
>         if (id)
>                 name = id->name;
>         else if (has_acpi_companion(&client->dev))
>                 name = dev_name(&client->dev);
>         else
>                 return -ENODEV;
>
> And you probably don't need that 'name' either.

I get your point. The benefits are not worth mixing the i2c_device_id
table with the ACPI ids.

Thanks for your valuable feedback.

Robert