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

[Dmitry Torokhov]

On Tue, Nov 01, 2016 at 01:14:16PM +0700, Phong Vo wrote:
> >From: Mika Westerberg <mika.westerb...@linux.intel.com>
> >Subject: Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when
> using DT ids through ACPI
> >Date: Monday 13th June 2016 09:26:55 UTC (5 months ago)
> >
> >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(_list);
> >>
> >> -   strlcpy(info->type, dev_name(>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(>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.
> 
> All,
> 
> Is there a conclusion on this? We have been tackling the same issue and
> incidentally arrived at a
> similar solution as like Lenard proposed  in the patch above.

FWIW I agree with Mika (I think?) and I do not like that we mangle OF
data. This causes issues with module loading (where every OF ID has to
be stripped and added to the legacy table), and so forth. Drivers can
either use of_device_get_match_data() to get "real" OF id, or maybe we
could temporarily transform of_device_id into i2c_device_id in i2c bus
code and pass it to probe(), and do the same with ACPI match data.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Tue, Nov 01, 2016 at 01:14:16PM +0700, Phong Vo wrote:
> >From: Mika Westerberg 
> >Subject: Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when
> using DT ids through ACPI
> >Date: Monday 13th June 2016 09:26:55 UTC (5 months ago)
> >
> >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(_list);
> >>
> >> -   strlcpy(info->type, dev_name(>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(>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.
> 
> All,
> 
> Is there a conclusion on this? We have been tackling the same issue and
> incidentally arrived at a
> similar solution as like Lenard proposed  in the patch above.

FWIW I agree with Mika (I think?) and I do not like that we mangle OF
data. This causes issues with module loading (where every OF ID has to
be stripped and added to the legacy table), and so forth. Drivers can
either use of_device_get_match_data() to get "real" OF id, or maybe we
could temporarily transform of_device_id into i2c_device_id in i2c bus
code and pass it to probe(), and do the same with ACPI match data.

Thanks.

-- 
Dmitry

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

[Phong Vo]

>From: Mika Westerberg <mika.westerb...@linux.intel.com>
>Subject: Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when
using DT ids through ACPI
>Date: Monday 13th June 2016 09:26:55 UTC (5 months ago)
>
>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(_list);
>>
>> -   strlcpy(info->type, dev_name(>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(>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.

All,

Is there a conclusion on this? We have been tackling the same issue and
incidentally arrived at a
similar solution as like Lenard proposed  in the patch above.

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

[Phong Vo]

>From: Mika Westerberg 
>Subject: Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when
using DT ids through ACPI
>Date: Monday 13th June 2016 09:26:55 UTC (5 months ago)
>
>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(_list);
>>
>> -   strlcpy(info->type, dev_name(>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(>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.

All,

Is there a conclusion on this? We have been tackling the same issue and
incidentally arrived at a
similar solution as like Lenard proposed  in the patch above.

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

[Mika Westerberg]

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(_list);
> 
> -   strlcpy(info->type, dev_name(>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(>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.

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

[Mika Westerberg]

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(_list);
> 
> -   strlcpy(info->type, dev_name(>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(>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.

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(_list);

-   strlcpy(info->type, dev_name(>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(>dev),
sizeof(info->type));

return 0;
 }

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

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(_list);

-   strlcpy(info->type, dev_name(>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(>dev),
sizeof(info->type));

return 0;
 }

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

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

[Wolfram Sang]

> Looking at that series it seems that the intention is to eventually
> remove the i2c_device_id argument from probe completely? That would
> cause a lot of code churn. It would also require every driver that needs
> to differentiate between models to pretty much duplicate the matching
> logic performed by the core.

I am with you on the "duplicated matching" issue which I personally
don't like at all.

That being said, it is the de-facto standard way of doing it currently.
If this is going to be changed, it should be done on a BIG SCALE.

Currently, I2C has a special way of passing matches. Because I see more
important topics to work on, I personally could leave it as is. But if
people want I2C to behave as the rest of the kernel, this is fine with
me if they are committed to do it 100%. Replacing the current I2C
special way with another I2C special way is no option. The correct path
IMO is to bring I2C in line with the rest of the kernel and fix the
kernel.



signature.asc
Description: PGP signature

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

[Wolfram Sang]

> Looking at that series it seems that the intention is to eventually
> remove the i2c_device_id argument from probe completely? That would
> cause a lot of code churn. It would also require every driver that needs
> to differentiate between models to pretty much duplicate the matching
> logic performed by the core.

I am with you on the "duplicated matching" issue which I personally
don't like at all.

That being said, it is the de-facto standard way of doing it currently.
If this is going to be changed, it should be done on a BIG SCALE.

Currently, I2C has a special way of passing matches. Because I see more
important topics to work on, I personally could leave it as is. But if
people want I2C to behave as the rest of the kernel, this is fine with
me if they are committed to do it 100%. Replacing the current I2C
special way with another I2C special way is no option. The correct path
IMO is to bring I2C in line with the rest of the kernel and fix the
kernel.



signature.asc
Description: PGP signature

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 10:04 AM, Wolfram Sang 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.
> 
> Note that this DT behaviour is about to be dropped to match I2C with
> the "generic" behaviour".
> 
> https://lkml.org/lkml/2016/5/4/534

Looking at that series it seems that the intention is to eventually
remove the i2c_device_id argument from probe completely? That would
cause a lot of code churn. It would also require every driver that needs
to differentiate between models to pretty much duplicate the matching
logic performed by the core.

This does seem better than receiving an i2c_device_id parameter argument
which may or may not be NULL. Still, in order to support multiple models
using ACPI DT ids every driver would have to attempt some sort of
acpi_of_match_device, right?

Have you considered adding .probe_of(dev, of_device_id) and
.probe_acpi(dev, acpi_device_id) instead, with arguments which are
always guaranteed to be non-NULL? The main advantage would be that
drivers don't need to do their own matching and all rules are only
present ever in the core. Then ACPI with DT ids could be made to "just
work" without per-driver support.

-- 
Regards,
Leonard

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 10:04 AM, Wolfram Sang 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.
> 
> Note that this DT behaviour is about to be dropped to match I2C with
> the "generic" behaviour".
> 
> https://lkml.org/lkml/2016/5/4/534

Looking at that series it seems that the intention is to eventually
remove the i2c_device_id argument from probe completely? That would
cause a lot of code churn. It would also require every driver that needs
to differentiate between models to pretty much duplicate the matching
logic performed by the core.

This does seem better than receiving an i2c_device_id parameter argument
which may or may not be NULL. Still, in order to support multiple models
using ACPI DT ids every driver would have to attempt some sort of
acpi_of_match_device, right?

Have you considered adding .probe_of(dev, of_device_id) and
.probe_acpi(dev, acpi_device_id) instead, with arguments which are
always guaranteed to be non-NULL? The main advantage would be that
drivers don't need to do their own matching and all rules are only
present ever in the core. Then ACPI with DT ids could be made to "just
work" without per-driver support.

-- 
Regards,
Leonard

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

[Wolfram Sang]

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.

Note that this DT behaviour is about to be dropped to match I2C with
the "generic" behaviour".

https://lkml.org/lkml/2016/5/4/534



signature.asc
Description: PGP signature

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

[Wolfram Sang]

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.

Note that this DT behaviour is about to be dropped to match I2C with
the "generic" behaviour".

https://lkml.org/lkml/2016/5/4/534



signature.asc
Description: PGP signature

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

[Mika Westerberg]

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.

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

[Mika Westerberg]

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.

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

[Crestez Dan Leonard]

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.

Signed-off-by: Crestez Dan Leonard 
---
 drivers/i2c/i2c-core.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3ffeb6c..911052d 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -581,17 +581,23 @@ 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)
+static const struct i2c_device_id *i2c_match_id_name(const struct 
i2c_device_id *id,
+const char *id_name)
 {
while (id->name[0]) {
-   if (strcmp(client->name, id->name) == 0)
+   if (strcmp(id_name, id->name) == 0)
return id;
id++;
}
return NULL;
 }
 
+static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+   const struct i2c_client *client)
+{
+   return i2c_match_id_name(id, client->name);
+}
+
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
struct i2c_client   *client = i2c_verify_client(dev);
@@ -767,6 +773,7 @@ static int i2c_device_probe(struct device *dev)
 {
struct i2c_client   *client = i2c_verify_client(dev);
struct i2c_driver   *driver;
+   const struct i2c_device_id *i2c_device_id;
int status;
 
if (!client)
@@ -826,7 +833,22 @@ static int i2c_device_probe(struct device *dev)
if (status == -EPROBE_DEFER)
goto err_clear_wakeup_irq;
 
-   status = driver->probe(client, i2c_match_id(driver->id_table, client));
+   i2c_device_id = i2c_match_id(driver->id_table, client);
+#ifdef CONFIG_ACPI
+   if (!i2c_device_id) {
+   const char *id_name;
+   const struct of_device_id *ofid;
+
+   ofid = acpi_of_match_device(ACPI_COMPANION(>dev),
+   driver->driver.of_match_table);
+   if (ofid) {
+   id_name = strchr(ofid->compatible, ',');
+   id_name = id_name ? id_name + 1 : ofid->compatible;
+   i2c_device_id = i2c_match_id_name(driver->id_table, 
id_name);
+   }
+   }
+#endif
+   status = driver->probe(client, i2c_device_id);
if (status)
goto err_detach_pm_domain;
 
-- 
2.5.5

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

[Crestez Dan Leonard]

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.

Signed-off-by: Crestez Dan Leonard 
---
 drivers/i2c/i2c-core.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3ffeb6c..911052d 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -581,17 +581,23 @@ 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)
+static const struct i2c_device_id *i2c_match_id_name(const struct 
i2c_device_id *id,
+const char *id_name)
 {
while (id->name[0]) {
-   if (strcmp(client->name, id->name) == 0)
+   if (strcmp(id_name, id->name) == 0)
return id;
id++;
}
return NULL;
 }
 
+static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+   const struct i2c_client *client)
+{
+   return i2c_match_id_name(id, client->name);
+}
+
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
struct i2c_client   *client = i2c_verify_client(dev);
@@ -767,6 +773,7 @@ static int i2c_device_probe(struct device *dev)
 {
struct i2c_client   *client = i2c_verify_client(dev);
struct i2c_driver   *driver;
+   const struct i2c_device_id *i2c_device_id;
int status;
 
if (!client)
@@ -826,7 +833,22 @@ static int i2c_device_probe(struct device *dev)
if (status == -EPROBE_DEFER)
goto err_clear_wakeup_irq;
 
-   status = driver->probe(client, i2c_match_id(driver->id_table, client));
+   i2c_device_id = i2c_match_id(driver->id_table, client);
+#ifdef CONFIG_ACPI
+   if (!i2c_device_id) {
+   const char *id_name;
+   const struct of_device_id *ofid;
+
+   ofid = acpi_of_match_device(ACPI_COMPANION(>dev),
+   driver->driver.of_match_table);
+   if (ofid) {
+   id_name = strchr(ofid->compatible, ',');
+   id_name = id_name ? id_name + 1 : ofid->compatible;
+   i2c_device_id = i2c_match_id_name(driver->id_table, 
id_name);
+   }
+   }
+#endif
+   status = driver->probe(client, i2c_device_id);
if (status)
goto err_detach_pm_domain;
 
-- 
2.5.5