| Message ID | 6721468a-753d-fb52-5bc5-8738312b7bd0@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jun 10, 2016 at 06:57:36PM +0300, Crestez Dan Leonard wrote: > 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; > } Yes, that's what I mean. > The biggest concern is that this would change the i2c device name > between kernel versions. Is that acceptable? I don't think that is a problem since I still have not seen a single system using ACPI _DSD so I would not expect anything to break. However, I'm still not convinced it is good idea to return i2c_device_id from a completely different table if we match using ACPI/DT table. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));