i2c: Use ID table to detect ACPI I2C devices

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 <[email protected]>

---
 drivers/i2c/i2c-core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/i2c/i2c-core.c +0 -0

index fec0e0d..c9b30b7 100644

drivers/i2c/i2c-core.c +6 -1

+		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++;
 	}

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.

Quoted 5 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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?

Quoted 3 lines · Robert Dolca

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?

Quoted 7 lines · Mika Westerberg

Quoted 3 lines · Robert Dolca

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.

Quoted 2 lines

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.

Quoted 19 lines · Robert Dolca

Quoted 7 lines · Mika Westerberg

Quoted 3 lines · Robert Dolca

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:

Quoted 8 lines

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.

Quoted 16 lines

Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 1 line

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.

Quoted 53 lines · Mika Westerberg

Quoted 19 lines · Robert Dolca

Quoted 7 lines · Mika Westerberg

Quoted 3 lines · Robert Dolca

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:

Quoted 8 lines

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.

Quoted 21 lines

Quoted 2 lines

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;

Quoted 2 lines

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 54 lines · Robert Dolca

Quoted 53 lines · Mika Westerberg

Quoted 19 lines · Robert Dolca

Quoted 7 lines · Mika Westerberg

Quoted 3 lines · Robert Dolca

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:

Quoted 8 lines

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.

Quoted 21 lines

Quoted 2 lines

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;

Quoted 4 lines

Quoted 2 lines

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:

Quoted 28 lines

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.

Quoted 5 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 1 line

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.