Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when using DT ids through ACPI

[Crestez Dan Leonard]

On 06/10/2016 09:32 AM, Mika Westerberg wrote:
> On Thu, Jun 09, 2016 at 04:06:03PM +0300, Crestez Dan Leonard wrote:
>> When devices are instatiated through devicetree the i2c_client->name is
>> set to the compatible string with company name stripped out. This is
>> then matched to the i2c_device_id table to pass the device_id to the
>> probe function. This id parameter is used by some device drivers to
>> differentiate between model numbers.
>>
>> When using ACPI this id parameter is NULL and the driver usually needs
>> to do ACPI-specific differentiation.
>>
>> This patch attempts to find a valid i2c_device_id when using ACPI with
>> DT-like compatible strings.
> 
> So I don't really understand why it would be good idea to pass
> i2c_device_id for devices which are matched against their ACPI/DT
> tables. Apparently DT is already doing that so maybe there is some
> reason.
> 
> Anyway, why not fill in the device name when it is first enumerated
> if it uses DT compatible property? Just like DT does.
> 
This automatic matching of i2c_device_id works for devicetree because
of_i2c_register_device sets i2c_board_info.type to the compatible string
with the vendor prefix removed. For I2C devices described via ACPI the
i2c_board_info.type string is set to the ACPI device name. This ends up
something like "PRP0001:00".

This could be changed in acpi_i2c_get_info to use the of_compatible
string from DSD if present. Is that what you mean? That would work and
it would be cleaner than my patch. Something like this:

diff --git drivers/i2c/i2c-core.c drivers/i2c/i2c-core.c
index 1e0ef9b..ba2fe7f 100644
--- drivers/i2c/i2c-core.c
+++ drivers/i2c/i2c-core.c
@@ -181,7 +181,24 @@ static int acpi_i2c_get_info(struct acpi_device *adev,

        acpi_dev_free_resource_list(&resource_list);

-       strlcpy(info->type, dev_name(&adev->dev), sizeof(info->type));
+       /*
+        * If we have a DT id set info.type to the first compatible
string with
+        * the vendor prefix stripped. This is similar to of_modalias_node
+        */
+       if (adev->data.of_compatible) {
+               const union acpi_object *obj;
+               const char *str, *chr;
+
+               obj = adev->data.of_compatible;
+               if (obj->type == ACPI_TYPE_PACKAGE)
+                       obj = obj->package.elements;
+               str = obj->string.pointer;
+               chr = strchr(str, ',');
+               if (chr)
+                       str = chr + 1;
+               strlcpy(info->type, str, sizeof(info->type));
+       } else
+               strlcpy(info->type, dev_name(&adev->dev),
sizeof(info->type));

        return 0;
 }

The biggest concern is that this would change the i2c device name
between kernel versions. Is that acceptable?