* [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
@ 2024-07-05 9:50 Marek Vasut
2024-07-05 9:55 ` Krzysztof Kozlowski
2024-07-05 10:42 ` Shreeya Patel
0 siblings, 2 replies; 18+ messages in thread
From: Marek Vasut @ 2024-07-05 9:50 UTC (permalink / raw)
To: linux-iio
Cc: Marek Vasut, Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, Shreeya Patel, devicetree
The "ltr,ltrf216a" compatible string is not documented in DT binding
document, remove it.
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Shreeya Patel <shreeya.patel@collabora.com>
Cc: devicetree@vger.kernel.org
Cc: linux-iio@vger.kernel.org
---
drivers/iio/light/ltrf216a.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
index 68dc48420a886..78fc910fcb18c 100644
--- a/drivers/iio/light/ltrf216a.c
+++ b/drivers/iio/light/ltrf216a.c
@@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
static const struct of_device_id ltrf216a_of_match[] = {
{ .compatible = "liteon,ltrf216a" },
- { .compatible = "ltr,ltrf216a" },
{}
};
MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 9:50 [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string Marek Vasut
@ 2024-07-05 9:55 ` Krzysztof Kozlowski
2024-07-05 10:42 ` Shreeya Patel
1 sibling, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-05 9:55 UTC (permalink / raw)
To: Marek Vasut, linux-iio
Cc: Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, Shreeya Patel, devicetree
On 05/07/2024 11:50, Marek Vasut wrote:
> The "ltr,ltrf216a" compatible string is not documented in DT binding
> document, remove it.
Also could be useful:
"The compatible was rejected during review by Rob Herring but still made
it to the kernel."
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Ehh, so the original author just half-understood comment from Rob [1]
and totally ignored checkpatch warning :/
If only people actually run and read the output of the tools instead of
pushing their code...
Thanks for cleanup.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
[1] https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 9:50 [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string Marek Vasut
2024-07-05 9:55 ` Krzysztof Kozlowski
@ 2024-07-05 10:42 ` Shreeya Patel
2024-07-05 14:52 ` Marek Vasut
2024-07-07 12:02 ` Krzysztof Kozlowski
1 sibling, 2 replies; 18+ messages in thread
From: Shreeya Patel @ 2024-07-05 10:42 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> The "ltr,ltrf216a" compatible string is not documented in DT binding
> document, remove it.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> ---
> drivers/iio/light/ltrf216a.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> index 68dc48420a886..78fc910fcb18c 100644
> --- a/drivers/iio/light/ltrf216a.c
> +++ b/drivers/iio/light/ltrf216a.c
> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>
> static const struct of_device_id ltrf216a_of_match[] = {
> { .compatible = "liteon,ltrf216a" },
> - { .compatible = "ltr,ltrf216a" },
> {}
This compatible string with a different vendor prefix was added for a specific reason.
Please see the commit message of the following patch :-
https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
We were very well aware that not documenting this was going to generate a warning so
we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
from his last message was that it wasn't necessary to fix the DT warning.
Thanks,
Shreeya Patel
> };
> MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 10:42 ` Shreeya Patel
@ 2024-07-05 14:52 ` Marek Vasut
2024-07-05 18:03 ` Shreeya Patel
2024-07-07 12:02 ` Krzysztof Kozlowski
1 sibling, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2024-07-05 14:52 UTC (permalink / raw)
To: Shreeya Patel
Cc: linux-iio, Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On 7/5/24 12:42 PM, Shreeya Patel wrote:
> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
>
>> The "ltr,ltrf216a" compatible string is not documented in DT binding
>> document, remove it.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-iio@vger.kernel.org
>> ---
>> drivers/iio/light/ltrf216a.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
>> index 68dc48420a886..78fc910fcb18c 100644
>> --- a/drivers/iio/light/ltrf216a.c
>> +++ b/drivers/iio/light/ltrf216a.c
>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>>
>> static const struct of_device_id ltrf216a_of_match[] = {
>> { .compatible = "liteon,ltrf216a" },
>> - { .compatible = "ltr,ltrf216a" },
>> {}
>
> This compatible string with a different vendor prefix was added for a specific reason.
> Please see the commit message of the following patch :-
> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
>
> We were very well aware that not documenting this was going to generate a warning so
> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> from his last message was that it wasn't necessary to fix the DT warning.
From what I read in the aforementioned discussion thread, it seems Rob
was very much opposed to this compatible string, so this shouldn't have
gone in in the first place.
But it did ... so the question is, what now ?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 14:52 ` Marek Vasut
@ 2024-07-05 18:03 ` Shreeya Patel
2024-07-07 11:26 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2024-07-05 18:03 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On Friday, July 05, 2024 20:22 IST, Marek Vasut <marex@denx.de> wrote:
> On 7/5/24 12:42 PM, Shreeya Patel wrote:
> > On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> >
> >> The "ltr,ltrf216a" compatible string is not documented in DT binding
> >> document, remove it.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> ---
> >> Cc: Conor Dooley <conor+dt@kernel.org>
> >> Cc: Jonathan Cameron <jic23@kernel.org>
> >> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> >> Cc: devicetree@vger.kernel.org
> >> Cc: linux-iio@vger.kernel.org
> >> ---
> >> drivers/iio/light/ltrf216a.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> >> index 68dc48420a886..78fc910fcb18c 100644
> >> --- a/drivers/iio/light/ltrf216a.c
> >> +++ b/drivers/iio/light/ltrf216a.c
> >> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> >>
> >> static const struct of_device_id ltrf216a_of_match[] = {
> >> { .compatible = "liteon,ltrf216a" },
> >> - { .compatible = "ltr,ltrf216a" },
> >> {}
> >
> > This compatible string with a different vendor prefix was added for a specific reason.
> > Please see the commit message of the following patch :-
> > https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> >
> > We were very well aware that not documenting this was going to generate a warning so
> > we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> > from his last message was that it wasn't necessary to fix the DT warning.
>
> From what I read in the aforementioned discussion thread, it seems Rob
> was very much opposed to this compatible string, so this shouldn't have
> gone in in the first place.
>
> But it did ... so the question is, what now ?
There were multiple versions sent for adding LTRF216A light sensor driver
and this compatible string wasn't something that was accepted by mistake.
Most of the versions of the patch series made it very clear that it generates a warning
which you can check here :-
https://lore.kernel.org/lkml/20220731173446.7400bfa8@jic23-huawei/T/#me55be502302d70424a85368c2645c89f860b7b40
I would just go with whatever Jonathan decides to do here :)
Thanks,
Shreeya Patel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 18:03 ` Shreeya Patel
@ 2024-07-07 11:26 ` Jonathan Cameron
2024-07-07 12:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2024-07-07 11:26 UTC (permalink / raw)
To: Shreeya Patel
Cc: Marek Vasut, linux-iio, Conor Dooley, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On Fri, 05 Jul 2024 19:03:04 +0100
"Shreeya Patel" <shreeya.patel@collabora.com> wrote:
> On Friday, July 05, 2024 20:22 IST, Marek Vasut <marex@denx.de> wrote:
>
> > On 7/5/24 12:42 PM, Shreeya Patel wrote:
> > > On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> > >
> > >> The "ltr,ltrf216a" compatible string is not documented in DT binding
> > >> document, remove it.
> > >>
> > >> Signed-off-by: Marek Vasut <marex@denx.de>
> > >> ---
> > >> Cc: Conor Dooley <conor+dt@kernel.org>
> > >> Cc: Jonathan Cameron <jic23@kernel.org>
> > >> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > >> Cc: Lars-Peter Clausen <lars@metafoo.de>
> > >> Cc: Marek Vasut <marex@denx.de>
> > >> Cc: Rob Herring <robh@kernel.org>
> > >> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> > >> Cc: devicetree@vger.kernel.org
> > >> Cc: linux-iio@vger.kernel.org
> > >> ---
> > >> drivers/iio/light/ltrf216a.c | 1 -
> > >> 1 file changed, 1 deletion(-)
> > >>
> > >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> > >> index 68dc48420a886..78fc910fcb18c 100644
> > >> --- a/drivers/iio/light/ltrf216a.c
> > >> +++ b/drivers/iio/light/ltrf216a.c
> > >> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> > >>
> > >> static const struct of_device_id ltrf216a_of_match[] = {
> > >> { .compatible = "liteon,ltrf216a" },
> > >> - { .compatible = "ltr,ltrf216a" },
> > >> {}
> > >
> > > This compatible string with a different vendor prefix was added for a specific reason.
> > > Please see the commit message of the following patch :-
> > > https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> > >
> > > We were very well aware that not documenting this was going to generate a warning so
> > > we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> > > from his last message was that it wasn't necessary to fix the DT warning.
> >
> > From what I read in the aforementioned discussion thread, it seems Rob
> > was very much opposed to this compatible string, so this shouldn't have
> > gone in in the first place.
> >
> > But it did ... so the question is, what now ?
>
> There were multiple versions sent for adding LTRF216A light sensor driver
> and this compatible string wasn't something that was accepted by mistake.
> Most of the versions of the patch series made it very clear that it generates a warning
> which you can check here :-
> https://lore.kernel.org/lkml/20220731173446.7400bfa8@jic23-huawei/T/#me55be502302d70424a85368c2645c89f860b7b40
>
> I would just go with whatever Jonathan decides to do here :)
If it's needed for a released device (which is what Shreeya's linked thread suggests)
that we can't get the manufacturer to fix, we are stuck with that entry existing for ever.
No regressions rule applies.
It would be helpful to have a specific reference to what that device is though.
When we've had this mess for horribly broken ACPI IDs that have gotten into devices
we try to add a comment on where they are known to exist. I'd ideally like such
a comment added here.
Jonathan
>
> Thanks,
> Shreeya Patel
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-05 10:42 ` Shreeya Patel
2024-07-05 14:52 ` Marek Vasut
@ 2024-07-07 12:02 ` Krzysztof Kozlowski
2024-07-07 13:37 ` Jonathan Cameron
1 sibling, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-07 12:02 UTC (permalink / raw)
To: Shreeya Patel, Marek Vasut
Cc: linux-iio, Conor Dooley, Jonathan Cameron, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On 05/07/2024 12:42, Shreeya Patel wrote:
> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
>
>> The "ltr,ltrf216a" compatible string is not documented in DT binding
>> document, remove it.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-iio@vger.kernel.org
>> ---
>> drivers/iio/light/ltrf216a.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
>> index 68dc48420a886..78fc910fcb18c 100644
>> --- a/drivers/iio/light/ltrf216a.c
>> +++ b/drivers/iio/light/ltrf216a.c
>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>>
>> static const struct of_device_id ltrf216a_of_match[] = {
>> { .compatible = "liteon,ltrf216a" },
>> - { .compatible = "ltr,ltrf216a" },
>> {}
>
> This compatible string with a different vendor prefix was added for a specific reason.
> Please see the commit message of the following patch :-
> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
And adding this specific compatible was clearly NAKed:
https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
yet you still added it. That's a deliberate going around maintainer's
decision.
>
> We were very well aware that not documenting this was going to generate a warning so
You *CANNOT* have undocumented compatibles.
> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
Because the driver was NAKed obviously as well.
> from his last message was that it wasn't necessary to fix the DT warning.
I am quite angry that maintainer tells you something, but you push your
patch through because apparently you need to fulfill your project
requirements.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 11:26 ` Jonathan Cameron
@ 2024-07-07 12:06 ` Krzysztof Kozlowski
2024-07-07 13:43 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-07 12:06 UTC (permalink / raw)
To: Jonathan Cameron, Shreeya Patel
Cc: Marek Vasut, linux-iio, Conor Dooley, Krzysztof Kozlowski,
Lars-Peter Clausen, Rob Herring, devicetree, kernel
On 07/07/2024 13:26, Jonathan Cameron wrote:
> On Fri, 05 Jul 2024 19:03:04 +0100
> "Shreeya Patel" <shreeya.patel@collabora.com> wrote:
>
>> On Friday, July 05, 2024 20:22 IST, Marek Vasut <marex@denx.de> wrote:
>>
>>> On 7/5/24 12:42 PM, Shreeya Patel wrote:
>>>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
>>>>> document, remove it.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>> Cc: Jonathan Cameron <jic23@kernel.org>
>>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> Cc: linux-iio@vger.kernel.org
>>>>> ---
>>>>> drivers/iio/light/ltrf216a.c | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
>>>>> index 68dc48420a886..78fc910fcb18c 100644
>>>>> --- a/drivers/iio/light/ltrf216a.c
>>>>> +++ b/drivers/iio/light/ltrf216a.c
>>>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>>>>>
>>>>> static const struct of_device_id ltrf216a_of_match[] = {
>>>>> { .compatible = "liteon,ltrf216a" },
>>>>> - { .compatible = "ltr,ltrf216a" },
>>>>> {}
>>>>
>>>> This compatible string with a different vendor prefix was added for a specific reason.
>>>> Please see the commit message of the following patch :-
>>>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
>>>>
>>>> We were very well aware that not documenting this was going to generate a warning so
>>>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
>>>> from his last message was that it wasn't necessary to fix the DT warning.
>>>
>>> From what I read in the aforementioned discussion thread, it seems Rob
>>> was very much opposed to this compatible string, so this shouldn't have
>>> gone in in the first place.
>>>
>>> But it did ... so the question is, what now ?
>>
>> There were multiple versions sent for adding LTRF216A light sensor driver
>> and this compatible string wasn't something that was accepted by mistake.
>> Most of the versions of the patch series made it very clear that it generates a warning
>> which you can check here :-
>> https://lore.kernel.org/lkml/20220731173446.7400bfa8@jic23-huawei/T/#me55be502302d70424a85368c2645c89f860b7b40
>>
>> I would just go with whatever Jonathan decides to do here :)
>
> If it's needed for a released device (which is what Shreeya's linked thread suggests)
Not entirely. The device was released EARLIER and they wanted to add
support for ACPI or out-of-tree kernel for EARLIER releases.
Regression rule does not work like that.
> that we can't get the manufacturer to fix, we are stuck with that entry existing for ever.
> No regressions rule applies.
At the moment of posting their patch regression rule did not apply. Only
now you could claim that Collabora's broken code is being part of ABI,
even though it was explicitly NAKed.
So what does it mean for us? Collabora wants to add ABI, we NAK it, ABI
sneaks in (happens) and now we are going to support that ABI?
So what incentive any company has to follow maintainers decision if they
can sneak such stuff in and get away with it?
>
> It would be helpful to have a specific reference to what that device is though.
> When we've had this mess for horribly broken ACPI IDs that have gotten into devices
> we try to add a comment on where they are known to exist. I'd ideally like such
> a comment added here.
Sorry, I stand by decision from May 2022: this was NAKed by Rob and
should have never been supported by kernel. We did not agree to support
it. Will it affect users? Sure, Collabora's fault.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 12:02 ` Krzysztof Kozlowski
@ 2024-07-07 13:37 ` Jonathan Cameron
2024-07-07 13:46 ` Krzysztof Kozlowski
2024-08-12 13:24 ` Andy Shevchenko
0 siblings, 2 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-07-07 13:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Shreeya Patel, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel, Andy Shevchenko
On Sun, 7 Jul 2024 14:02:39 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 05/07/2024 12:42, Shreeya Patel wrote:
> > On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> >
> >> The "ltr,ltrf216a" compatible string is not documented in DT binding
> >> document, remove it.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> ---
> >> Cc: Conor Dooley <conor+dt@kernel.org>
> >> Cc: Jonathan Cameron <jic23@kernel.org>
> >> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> >> Cc: devicetree@vger.kernel.org
> >> Cc: linux-iio@vger.kernel.org
> >> ---
> >> drivers/iio/light/ltrf216a.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> >> index 68dc48420a886..78fc910fcb18c 100644
> >> --- a/drivers/iio/light/ltrf216a.c
> >> +++ b/drivers/iio/light/ltrf216a.c
> >> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> >>
> >> static const struct of_device_id ltrf216a_of_match[] = {
> >> { .compatible = "liteon,ltrf216a" },
> >> - { .compatible = "ltr,ltrf216a" },
> >> {}
> >
> > This compatible string with a different vendor prefix was added for a specific reason.
> > Please see the commit message of the following patch :-
> > https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
>
> And adding this specific compatible was clearly NAKed:
> https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
>
> yet you still added it. That's a deliberate going around maintainer's
> decision.
>
The statement from Rob was very specific. The schema is not applicable to ACPI bindings
- that's the basis on which he doesn't want this in the schema. Specifically
because "There's not really any point in having this in schema as you can't
use that schema with ACPI".
That is true (though arguably you could with sufficient tooling apply the schema
to the relevant part of DSDT).
The compatible is usable, via PRP0001 ACPI IDs.
> >
> > We were very well aware that not documenting this was going to generate a warning so
>
> You *CANNOT* have undocumented compatibles.
Why not? This corner case is a valid reason for that to be allowed.
You cannot use that compatible with DT bindings. Absolutely. The compatible
has other uses...
>
> > we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
>
> Because the driver was NAKed obviously as well.
>
> > from his last message was that it wasn't necessary to fix the DT warning.
>
> I am quite angry that maintainer tells you something, but you push your
> patch through because apparently you need to fulfill your project
> requirements.
I think this is a fundamental misunderstanding of the situation and probably
at least partly my fault for not clarifying my reading of the situation more
fully at the time.
As far as I am concerned. The situation is:
1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
that exist just to deal with garbage firmware's. The folk involved in
reviewing these have pushed back hard for a long time, but sadly there
is still a lot of garbage shipping because Windows lets it through and
Linux support comes second. It's made even worse by Microsoft defining
their own standards that aren't compliant with ACPI as they don't
even bother with reserving the methods IDs. ROTM for example.
2) This is an ACPI binding, it just happens to use a DT compatible via the
PRP0001 mechanism. Yes, we strongly discourage people doing that in
shipping products but there have been other cases of it.
3) Shreeya read a distinction (that I also agree with) between the schema
and the compatible list. The schema does not apply to this situation
(because we can't actually check it today for DSDT) hence Rob's Nack
was making the point it was inappropriate to carry it there.
So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
I'd love to be in a position to say no on ACPI bindings that are garbage
(there are a lot of them) but Windows is dominant in that space so
we get stuck with their mess. On server's it is a different game
and the kernel community regularly gets significant changes made.
Jonathan
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 12:06 ` Krzysztof Kozlowski
@ 2024-07-07 13:43 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-07-07 13:43 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Shreeya Patel, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel
On Sun, 7 Jul 2024 14:06:10 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 07/07/2024 13:26, Jonathan Cameron wrote:
> > On Fri, 05 Jul 2024 19:03:04 +0100
> > "Shreeya Patel" <shreeya.patel@collabora.com> wrote:
> >
> >> On Friday, July 05, 2024 20:22 IST, Marek Vasut <marex@denx.de> wrote:
> >>
> >>> On 7/5/24 12:42 PM, Shreeya Patel wrote:
> >>>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
> >>>>> document, remove it.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>> ---
> >>>>> Cc: Conor Dooley <conor+dt@kernel.org>
> >>>>> Cc: Jonathan Cameron <jic23@kernel.org>
> >>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> >>>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >>>>> Cc: Marek Vasut <marex@denx.de>
> >>>>> Cc: Rob Herring <robh@kernel.org>
> >>>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> >>>>> Cc: devicetree@vger.kernel.org
> >>>>> Cc: linux-iio@vger.kernel.org
> >>>>> ---
> >>>>> drivers/iio/light/ltrf216a.c | 1 -
> >>>>> 1 file changed, 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> >>>>> index 68dc48420a886..78fc910fcb18c 100644
> >>>>> --- a/drivers/iio/light/ltrf216a.c
> >>>>> +++ b/drivers/iio/light/ltrf216a.c
> >>>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> >>>>>
> >>>>> static const struct of_device_id ltrf216a_of_match[] = {
> >>>>> { .compatible = "liteon,ltrf216a" },
> >>>>> - { .compatible = "ltr,ltrf216a" },
> >>>>> {}
> >>>>
> >>>> This compatible string with a different vendor prefix was added for a specific reason.
> >>>> Please see the commit message of the following patch :-
> >>>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> >>>>
> >>>> We were very well aware that not documenting this was going to generate a warning so
> >>>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> >>>> from his last message was that it wasn't necessary to fix the DT warning.
> >>>
> >>> From what I read in the aforementioned discussion thread, it seems Rob
> >>> was very much opposed to this compatible string, so this shouldn't have
> >>> gone in in the first place.
> >>>
> >>> But it did ... so the question is, what now ?
> >>
> >> There were multiple versions sent for adding LTRF216A light sensor driver
> >> and this compatible string wasn't something that was accepted by mistake.
> >> Most of the versions of the patch series made it very clear that it generates a warning
> >> which you can check here :-
> >> https://lore.kernel.org/lkml/20220731173446.7400bfa8@jic23-huawei/T/#me55be502302d70424a85368c2645c89f860b7b40
> >>
> >> I would just go with whatever Jonathan decides to do here :)
> >
> > If it's needed for a released device (which is what Shreeya's linked thread suggests)
>
> Not entirely. The device was released EARLIER and they wanted to add
> support for ACPI or out-of-tree kernel for EARLIER releases.
>
> Regression rule does not work like that.
>
> > that we can't get the manufacturer to fix, we are stuck with that entry existing for ever.
> > No regressions rule applies.
>
> At the moment of posting their patch regression rule did not apply. Only
> now you could claim that Collabora's broken code is being part of ABI,
> even though it was explicitly NAKed.
>
The problem is that I did take this patch. So now it's a regression.
> So what does it mean for us? Collabora wants to add ABI, we NAK it, ABI
> sneaks in (happens) and now we are going to support that ABI?
I disagree and laid out why in the other branch of this thread so won't
repeat here.
>
> So what incentive any company has to follow maintainers decision if they
> can sneak such stuff in and get away with it?
I may be misremembering but that wasn't how I interpreted things at the time.
I interpreted Rob's response in a much narrower way than you have.
>
> >
> > It would be helpful to have a specific reference to what that device is though.
> > When we've had this mess for horribly broken ACPI IDs that have gotten into devices
> > we try to add a comment on where they are known to exist. I'd ideally like such
> > a comment added here.
>
> Sorry, I stand by decision from May 2022: this was NAKed by Rob and
> should have never been supported by kernel. We did not agree to support
> it. Will it affect users? Sure, Collabora's fault.
Fine, unless a consensus is clearly reached on this I'm not going to merge
this patch because I think it is an unfortunate fact of life that we should
endeavour to support consumer devices with the garbage we get handed in
consumer device ACPI tables because that's not a place we can force changes.
If I'm missing something about the background to this, then maybe it's a different
question. Particularly if perhaps circumstances have changed and perhaps
the devices with this odd ACPI table are no longer relevant and we can drop
this as unnecessary. I'd love that to be the case!
Jonathan
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 13:37 ` Jonathan Cameron
@ 2024-07-07 13:46 ` Krzysztof Kozlowski
2024-07-07 14:08 ` Jonathan Cameron
2024-08-12 13:24 ` Andy Shevchenko
1 sibling, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-07 13:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Shreeya Patel, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel, Andy Shevchenko
On 07/07/2024 15:37, Jonathan Cameron wrote:
> On Sun, 7 Jul 2024 14:02:39 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
>> On 05/07/2024 12:42, Shreeya Patel wrote:
>>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
>>>
>>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
>>>> document, remove it.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>> Cc: Jonathan Cameron <jic23@kernel.org>
>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: linux-iio@vger.kernel.org
>>>> ---
>>>> drivers/iio/light/ltrf216a.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
>>>> index 68dc48420a886..78fc910fcb18c 100644
>>>> --- a/drivers/iio/light/ltrf216a.c
>>>> +++ b/drivers/iio/light/ltrf216a.c
>>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>>>>
>>>> static const struct of_device_id ltrf216a_of_match[] = {
>>>> { .compatible = "liteon,ltrf216a" },
>>>> - { .compatible = "ltr,ltrf216a" },
>>>> {}
>>>
>>> This compatible string with a different vendor prefix was added for a specific reason.
>>> Please see the commit message of the following patch :-
>>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
>>
>> And adding this specific compatible was clearly NAKed:
>> https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
>>
>> yet you still added it. That's a deliberate going around maintainer's
>> decision.
>>
>
> The statement from Rob was very specific. The schema is not applicable to ACPI bindings
> - that's the basis on which he doesn't want this in the schema. Specifically
> because "There's not really any point in having this in schema as you can't
> use that schema with ACPI".
>
> That is true (though arguably you could with sufficient tooling apply the schema
> to the relevant part of DSDT).
>
> The compatible is usable, via PRP0001 ACPI IDs.
Uh, that's sounds like a slippery slope. To my understanding, PRP0001
allows to create ACPI IDs from OF IDs, so it requires to have a valid OF
ID. Valid OF ID requires bindings, doesn't it?
If it does not, then anyone can add any Devicetree properties, claiming
it is for ACPI ID thus not providing bindings (or bypassing bindings
review / NAK).
>
>>>
>>> We were very well aware that not documenting this was going to generate a warning so
>>
>> You *CANNOT* have undocumented compatibles.
>
> Why not? This corner case is a valid reason for that to be allowed.
> You cannot use that compatible with DT bindings. Absolutely. The compatible
> has other uses...
Okay. With that approach what stops anyone from submitting DTS using
that compatible (claiming there is a driver for that compatible)?
>
>
>>
>>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
>>
>> Because the driver was NAKed obviously as well.
>>
>>> from his last message was that it wasn't necessary to fix the DT warning.
>>
>> I am quite angry that maintainer tells you something, but you push your
>> patch through because apparently you need to fulfill your project
>> requirements.
>
> I think this is a fundamental misunderstanding of the situation and probably
> at least partly my fault for not clarifying my reading of the situation more
> fully at the time.
>
> As far as I am concerned. The situation is:
> 1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
> that exist just to deal with garbage firmware's. The folk involved in
> reviewing these have pushed back hard for a long time, but sadly there
> is still a lot of garbage shipping because Windows lets it through and
> Linux support comes second. It's made even worse by Microsoft defining
> their own standards that aren't compliant with ACPI as they don't
> even bother with reserving the methods IDs. ROTM for example.
Hm, and these devices do not provide normal ACPI IDs? They use Of-like
ones? I don't know that much about ACPI, but aren't they coming without
vendor prefix thus "ltr,ltrf216a" is just wrong and should be "lTRF216A"
alone?
>
> 2) This is an ACPI binding, it just happens to use a DT compatible via the
> PRP0001 mechanism. Yes, we strongly discourage people doing that in
> shipping products but there have been other cases of it.
OK, is this the case here?
>
> 3) Shreeya read a distinction (that I also agree with) between the schema
> and the compatible list. The schema does not apply to this situation
> (because we can't actually check it today for DSDT) hence Rob's Nack
> was making the point it was inappropriate to carry it there.
>
> So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
> I'd love to be in a position to say no on ACPI bindings that are garbage
> (there are a lot of them) but Windows is dominant in that space so
> we get stuck with their mess. On server's it is a different game
> and the kernel community regularly gets significant changes made.
Original discussion had only vague statement of "vendor prefix name as
'ltr' through ACPI". But what does it even mean? What ACPI ID is
reported by these devices?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 13:46 ` Krzysztof Kozlowski
@ 2024-07-07 14:08 ` Jonathan Cameron
2024-07-08 11:25 ` Krzysztof Kozlowski
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-07-07 14:08 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Shreeya Patel, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel, Andy Shevchenko
On Sun, 7 Jul 2024 15:46:26 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 07/07/2024 15:37, Jonathan Cameron wrote:
> > On Sun, 7 Jul 2024 14:02:39 +0200
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> >> On 05/07/2024 12:42, Shreeya Patel wrote:
> >>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> >>>
> >>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
> >>>> document, remove it.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> ---
> >>>> Cc: Conor Dooley <conor+dt@kernel.org>
> >>>> Cc: Jonathan Cameron <jic23@kernel.org>
> >>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> >>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >>>> Cc: Marek Vasut <marex@denx.de>
> >>>> Cc: Rob Herring <robh@kernel.org>
> >>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> >>>> Cc: devicetree@vger.kernel.org
> >>>> Cc: linux-iio@vger.kernel.org
> >>>> ---
> >>>> drivers/iio/light/ltrf216a.c | 1 -
> >>>> 1 file changed, 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> >>>> index 68dc48420a886..78fc910fcb18c 100644
> >>>> --- a/drivers/iio/light/ltrf216a.c
> >>>> +++ b/drivers/iio/light/ltrf216a.c
> >>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> >>>>
> >>>> static const struct of_device_id ltrf216a_of_match[] = {
> >>>> { .compatible = "liteon,ltrf216a" },
> >>>> - { .compatible = "ltr,ltrf216a" },
> >>>> {}
> >>>
> >>> This compatible string with a different vendor prefix was added for a specific reason.
> >>> Please see the commit message of the following patch :-
> >>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> >>
> >> And adding this specific compatible was clearly NAKed:
> >> https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
> >>
> >> yet you still added it. That's a deliberate going around maintainer's
> >> decision.
> >>
> >
> > The statement from Rob was very specific. The schema is not applicable to ACPI bindings
> > - that's the basis on which he doesn't want this in the schema. Specifically
> > because "There's not really any point in having this in schema as you can't
> > use that schema with ACPI".
> >
> > That is true (though arguably you could with sufficient tooling apply the schema
> > to the relevant part of DSDT).
> >
> > The compatible is usable, via PRP0001 ACPI IDs.
>
> Uh, that's sounds like a slippery slope. To my understanding, PRP0001
> allows to create ACPI IDs from OF IDs, so it requires to have a valid OF
> ID. Valid OF ID requires bindings, doesn't it?
>
> If it does not, then anyone can add any Devicetree properties, claiming
> it is for ACPI ID thus not providing bindings (or bypassing bindings
> review / NAK).
True, but in a similar fashion to ACPI bindings (which Andy in particular
keeps a close eye on!) we should ask for very specific device reference
and document the broken part. I've gotten a lot stricter on this over
the last few years so new cases of this in IIO at least require such
a comment alongside the ID table entry.
>
> >
> >>>
> >>> We were very well aware that not documenting this was going to generate a warning so
> >>
> >> You *CANNOT* have undocumented compatibles.
> >
> > Why not? This corner case is a valid reason for that to be allowed.
> > You cannot use that compatible with DT bindings. Absolutely. The compatible
> > has other uses...
>
> Okay. With that approach what stops anyone from submitting DTS using
> that compatible (claiming there is a driver for that compatible)?
That's a good point. Perhaps we should just add a check for this?
Easy to add a check on the firmware type. This is a rare enough case that
just doing it in the driver seems fine to me (rather than more general
infrastructure).
>
> >
> >
> >>
> >>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> >>
> >> Because the driver was NAKed obviously as well.
> >>
> >>> from his last message was that it wasn't necessary to fix the DT warning.
> >>
> >> I am quite angry that maintainer tells you something, but you push your
> >> patch through because apparently you need to fulfill your project
> >> requirements.
> >
> > I think this is a fundamental misunderstanding of the situation and probably
> > at least partly my fault for not clarifying my reading of the situation more
> > fully at the time.
> >
> > As far as I am concerned. The situation is:
> > 1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
> > that exist just to deal with garbage firmware's. The folk involved in
> > reviewing these have pushed back hard for a long time, but sadly there
> > is still a lot of garbage shipping because Windows lets it through and
> > Linux support comes second. It's made even worse by Microsoft defining
> > their own standards that aren't compliant with ACPI as they don't
> > even bother with reserving the methods IDs. ROTM for example.
>
> Hm, and these devices do not provide normal ACPI IDs? They use Of-like
> ones? I don't know that much about ACPI, but aren't they coming without
> vendor prefix thus "ltr,ltrf216a" is just wrong and should be "lTRF216A"
> alone?
Yes, they come with the ID that is matched on by the ACPI core as PRP0001
which basically means use the DT compatible.
Then a device specific property that provides 'compatible' to look up against.
The intent being to allow use of existing drivers without needing to modify
them to add ACPI IDs to match against.
LTRF216A is worse than using PRP0001 and DT vendor ID
ACPI has it's own equivalent of vendor IDs and you have to apply for one from
relevant committee in the UEFI forum (ASWG)
https://uefi.org/ACPI_ID_List
(there is a 3 letter form as well).
It's easy to get an ID (takes a few weeks though) but many sensor companies
etc don't bother. Sometimes they say it's because the OEMs should do this
and sometimes those OEMs do, so the binding is under their vendor not the
device manufacturer. That's when you see what looks like completely unrelated
IDs being used.
It would be good it liteon got a proper ID and started issuing device numbers
to go with it though.
There are a lot of old bindings that make IDs up. Some are based on cut and paste
and we've been trying to scrub those, others are based on what Windows drivers
bind against and so we are stuck with that set.
For extra fun we have examples of hardware with a common ID for incompatible
devices for which we have different drivers. That's a real pain when it happens
but a few sensor manufacturers have 'one windows driver' for many years worth
of unrelated devices and use horrible matching routines to figure out what is
actually there...).
>
> >
> > 2) This is an ACPI binding, it just happens to use a DT compatible via the
> > PRP0001 mechanism. Yes, we strongly discourage people doing that in
> > shipping products but there have been other cases of it.
>
> OK, is this the case here?
Shreeya, can you check this. If we can get an example of such a device
that would help. (This is the same request we've made when removing
potentially false ACPI IDs). If we can't actually pin it down to a device
I don't mind dropping the ID and seeing if anyone shouts.
>
> >
> > 3) Shreeya read a distinction (that I also agree with) between the schema
> > and the compatible list. The schema does not apply to this situation
> > (because we can't actually check it today for DSDT) hence Rob's Nack
> > was making the point it was inappropriate to carry it there.
> >
> > So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
> > I'd love to be in a position to say no on ACPI bindings that are garbage
> > (there are a lot of them) but Windows is dominant in that space so
> > we get stuck with their mess. On server's it is a different game
> > and the kernel community regularly gets significant changes made.
>
> Original discussion had only vague statement of "vendor prefix name as
> 'ltr' through ACPI". But what does it even mean? What ACPI ID is
> reported by these devices?
PRP0001 is the only way it can be done that I know of so I read the
original thread with that in mind. I might be wrong though and
that would indeed change this discussion!
Thanks,
Jonathan
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 14:08 ` Jonathan Cameron
@ 2024-07-08 11:25 ` Krzysztof Kozlowski
2024-07-08 12:29 ` Sebastian Reichel
2024-07-08 12:13 ` Sebastian Reichel
2024-07-08 13:20 ` Shreeya Patel
2 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-08 11:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Shreeya Patel, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel, Andy Shevchenko
On 07/07/2024 16:08, Jonathan Cameron wrote:
>>
>>>
>>>>>
>>>>> We were very well aware that not documenting this was going to generate a warning so
>>>>
>>>> You *CANNOT* have undocumented compatibles.
>>>
>>> Why not? This corner case is a valid reason for that to be allowed.
>>> You cannot use that compatible with DT bindings. Absolutely. The compatible
>>> has other uses...
>>
>> Okay. With that approach what stops anyone from submitting DTS using
>> that compatible (claiming there is a driver for that compatible)?
>
> That's a good point. Perhaps we should just add a check for this?
> Easy to add a check on the firmware type. This is a rare enough case that
> just doing it in the driver seems fine to me (rather than more general
> infrastructure).
Another point of slippery slope:
1. We accept such undocumented compatible in OF device id for ACPI
(PRP0001).
2. Out-of-tree DTS uses it.
3. Whatever we decide to do now with that compatible, we have
undocumented ABI exposed and used by users.
That's the answer why we cannot have undocumented compatibles: because
we do not want to have implicit ABI. We want explicit ABI, which is:
1. Clearly documented,
2. Reviewed/accepted explicitly.
>
>>
>>>
>>>
>>>>
>>>>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
>>>>
>>>> Because the driver was NAKed obviously as well.
>>>>
>>>>> from his last message was that it wasn't necessary to fix the DT warning.
>>>>
>>>> I am quite angry that maintainer tells you something, but you push your
>>>> patch through because apparently you need to fulfill your project
>>>> requirements.
>>>
>>> I think this is a fundamental misunderstanding of the situation and probably
>>> at least partly my fault for not clarifying my reading of the situation more
>>> fully at the time.
>>>
>>> As far as I am concerned. The situation is:
>>> 1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
>>> that exist just to deal with garbage firmware's. The folk involved in
>>> reviewing these have pushed back hard for a long time, but sadly there
>>> is still a lot of garbage shipping because Windows lets it through and
>>> Linux support comes second. It's made even worse by Microsoft defining
>>> their own standards that aren't compliant with ACPI as they don't
>>> even bother with reserving the methods IDs. ROTM for example.
>>
>> Hm, and these devices do not provide normal ACPI IDs? They use Of-like
>> ones? I don't know that much about ACPI, but aren't they coming without
>> vendor prefix thus "ltr,ltrf216a" is just wrong and should be "lTRF216A"
>> alone?
>
> Yes, they come with the ID that is matched on by the ACPI core as PRP0001
> which basically means use the DT compatible.
> Then a device specific property that provides 'compatible' to look up against.
> The intent being to allow use of existing drivers without needing to modify
> them to add ACPI IDs to match against.
>
> LTRF216A is worse than using PRP0001 and DT vendor ID
> ACPI has it's own equivalent of vendor IDs and you have to apply for one from
> relevant committee in the UEFI forum (ASWG)
> https://uefi.org/ACPI_ID_List
> (there is a 3 letter form as well).
> It's easy to get an ID (takes a few weeks though) but many sensor companies
> etc don't bother. Sometimes they say it's because the OEMs should do this
> and sometimes those OEMs do, so the binding is under their vendor not the
> device manufacturer. That's when you see what looks like completely unrelated
> IDs being used.
>
> It would be good it liteon got a proper ID and started issuing device numbers
> to go with it though.
>
> There are a lot of old bindings that make IDs up. Some are based on cut and paste
> and we've been trying to scrub those, others are based on what Windows drivers
> bind against and so we are stuck with that set.
> For extra fun we have examples of hardware with a common ID for incompatible
> devices for which we have different drivers. That's a real pain when it happens
> but a few sensor manufacturers have 'one windows driver' for many years worth
> of unrelated devices and use horrible matching routines to figure out what is
> actually there...).
BTW, I really miss the information what sort of users they have for that
compatible. The explanation was vague. What are the "released devices"?
What ACPI tables do they have?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 14:08 ` Jonathan Cameron
2024-07-08 11:25 ` Krzysztof Kozlowski
@ 2024-07-08 12:13 ` Sebastian Reichel
2024-07-08 13:20 ` Shreeya Patel
2 siblings, 0 replies; 18+ messages in thread
From: Sebastian Reichel @ 2024-07-08 12:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Krzysztof Kozlowski, Shreeya Patel, Marek Vasut, linux-iio,
Conor Dooley, Krzysztof Kozlowski, Lars-Peter Clausen,
Rob Herring, devicetree, kernel, Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 525 bytes --]
Hi,
On Sun, Jul 07, 2024 at 03:08:35PM GMT, Jonathan Cameron wrote:
> > > 2) This is an ACPI binding, it just happens to use a DT compatible via the
> > > PRP0001 mechanism. Yes, we strongly discourage people doing that in
> > > shipping products but there have been other cases of it.
> >
> > OK, is this the case here?
>
> [...] If we can get an example of such a device that would help. [...]
The ALS is used by the Valve Steamdeck via ACPI + PRP0001 with the
bad compatible string.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-08 11:25 ` Krzysztof Kozlowski
@ 2024-07-08 12:29 ` Sebastian Reichel
0 siblings, 0 replies; 18+ messages in thread
From: Sebastian Reichel @ 2024-07-08 12:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jonathan Cameron, Shreeya Patel, Marek Vasut, linux-iio,
Conor Dooley, Krzysztof Kozlowski, Lars-Peter Clausen,
Rob Herring, devicetree, kernel, Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 2191 bytes --]
Hi,
On Mon, Jul 08, 2024 at 01:25:10PM GMT, Krzysztof Kozlowski wrote:
> On 07/07/2024 16:08, Jonathan Cameron wrote:
> >>>>> We were very well aware that not documenting this was going
> >>>>> to generate a warning so
> >>>>
> >>>> You *CANNOT* have undocumented compatibles.
> >>>
> >>> Why not? This corner case is a valid reason for that to be allowed.
> >>> You cannot use that compatible with DT bindings. Absolutely. The
> >>> compatible has other uses...
> >>
> >> Okay. With that approach what stops anyone from submitting DTS using
> >> that compatible (claiming there is a driver for that compatible)?
> >
> > That's a good point. Perhaps we should just add a check for this?
> > Easy to add a check on the firmware type. This is a rare enough case that
> > just doing it in the driver seems fine to me (rather than more general
> > infrastructure).
>
> Another point of slippery slope:
> 1. We accept such undocumented compatible in OF device id for ACPI
> (PRP0001).
> 2. Out-of-tree DTS uses it.
> 3. Whatever we decide to do now with that compatible, we have
> undocumented ABI exposed and used by users.
>
> That's the answer why we cannot have undocumented compatibles: because
> we do not want to have implicit ABI. We want explicit ABI, which is:
> 1. Clearly documented,
> 2. Reviewed/accepted explicitly.
Regarding implicit ABI, I would be much more worried about things
like this commit: 7b458a4c5d7302947556e12c83cfe4da769665d0 ("usb:
typec: Add typec_port_register_altmodes()"). It's not referenced
in the commit message, but IIRC Rob Herring ACK'd the approach
taken by Hans. I understand that it will become quite hard to get
things upstream when having to synchronize ACPI and DT first. But
it does mean vendors can (and actually did in this case) start
(ab)using the properties with DT making use of the implcit ABI.
Note that in case of Type-C there is now a proper upstream DT ABI. I
just wanted to point out that I totally understand the line of
thought Jonathan followed and demonstrate how one can have
undocumented properties (admittedly not a compatible in this case).
Greetings,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 14:08 ` Jonathan Cameron
2024-07-08 11:25 ` Krzysztof Kozlowski
2024-07-08 12:13 ` Sebastian Reichel
@ 2024-07-08 13:20 ` Shreeya Patel
2024-07-08 16:01 ` Jonathan Cameron
2 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2024-07-08 13:20 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Krzysztof Kozlowski, Marek Vasut, linux-iio, Conor Dooley,
Krzysztof Kozlowski, Lars-Peter Clausen, Rob Herring, devicetree,
kernel, Andy Shevchenko
On Sunday, July 07, 2024 19:38 IST, Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 7 Jul 2024 15:46:26 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> > On 07/07/2024 15:37, Jonathan Cameron wrote:
> > > On Sun, 7 Jul 2024 14:02:39 +0200
> > > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > >> On 05/07/2024 12:42, Shreeya Patel wrote:
> > >>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> > >>>
> > >>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
> > >>>> document, remove it.
> > >>>>
> > >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> > >>>> ---
> > >>>> Cc: Conor Dooley <conor+dt@kernel.org>
> > >>>> Cc: Jonathan Cameron <jic23@kernel.org>
> > >>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > >>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
> > >>>> Cc: Marek Vasut <marex@denx.de>
> > >>>> Cc: Rob Herring <robh@kernel.org>
> > >>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> > >>>> Cc: devicetree@vger.kernel.org
> > >>>> Cc: linux-iio@vger.kernel.org
> > >>>> ---
> > >>>> drivers/iio/light/ltrf216a.c | 1 -
> > >>>> 1 file changed, 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> > >>>> index 68dc48420a886..78fc910fcb18c 100644
> > >>>> --- a/drivers/iio/light/ltrf216a.c
> > >>>> +++ b/drivers/iio/light/ltrf216a.c
> > >>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> > >>>>
> > >>>> static const struct of_device_id ltrf216a_of_match[] = {
> > >>>> { .compatible = "liteon,ltrf216a" },
> > >>>> - { .compatible = "ltr,ltrf216a" },
> > >>>> {}
> > >>>
> > >>> This compatible string with a different vendor prefix was added for a specific reason.
> > >>> Please see the commit message of the following patch :-
> > >>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> > >>
> > >> And adding this specific compatible was clearly NAKed:
> > >> https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
> > >>
> > >> yet you still added it. That's a deliberate going around maintainer's
> > >> decision.
> > >>
> > >
> > > The statement from Rob was very specific. The schema is not applicable to ACPI bindings
> > > - that's the basis on which he doesn't want this in the schema. Specifically
> > > because "There's not really any point in having this in schema as you can't
> > > use that schema with ACPI".
> > >
> > > That is true (though arguably you could with sufficient tooling apply the schema
> > > to the relevant part of DSDT).
> > >
> > > The compatible is usable, via PRP0001 ACPI IDs.
> >
> > Uh, that's sounds like a slippery slope. To my understanding, PRP0001
> > allows to create ACPI IDs from OF IDs, so it requires to have a valid OF
> > ID. Valid OF ID requires bindings, doesn't it?
> >
> > If it does not, then anyone can add any Devicetree properties, claiming
> > it is for ACPI ID thus not providing bindings (or bypassing bindings
> > review / NAK).
>
> True, but in a similar fashion to ACPI bindings (which Andy in particular
> keeps a close eye on!) we should ask for very specific device reference
> and document the broken part. I've gotten a lot stricter on this over
> the last few years so new cases of this in IIO at least require such
> a comment alongside the ID table entry.
>
> >
> > >
> > >>>
> > >>> We were very well aware that not documenting this was going to generate a warning so
> > >>
> > >> You *CANNOT* have undocumented compatibles.
> > >
> > > Why not? This corner case is a valid reason for that to be allowed.
> > > You cannot use that compatible with DT bindings. Absolutely. The compatible
> > > has other uses...
> >
> > Okay. With that approach what stops anyone from submitting DTS using
> > that compatible (claiming there is a driver for that compatible)?
>
> That's a good point. Perhaps we should just add a check for this?
> Easy to add a check on the firmware type. This is a rare enough case that
> just doing it in the driver seems fine to me (rather than more general
> infrastructure).
>
> >
> > >
> > >
> > >>
> > >>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> > >>
> > >> Because the driver was NAKed obviously as well.
> > >>
> > >>> from his last message was that it wasn't necessary to fix the DT warning.
> > >>
> > >> I am quite angry that maintainer tells you something, but you push your
> > >> patch through because apparently you need to fulfill your project
> > >> requirements.
> > >
> > > I think this is a fundamental misunderstanding of the situation and probably
> > > at least partly my fault for not clarifying my reading of the situation more
> > > fully at the time.
> > >
> > > As far as I am concerned. The situation is:
> > > 1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
> > > that exist just to deal with garbage firmware's. The folk involved in
> > > reviewing these have pushed back hard for a long time, but sadly there
> > > is still a lot of garbage shipping because Windows lets it through and
> > > Linux support comes second. It's made even worse by Microsoft defining
> > > their own standards that aren't compliant with ACPI as they don't
> > > even bother with reserving the methods IDs. ROTM for example.
> >
> > Hm, and these devices do not provide normal ACPI IDs? They use Of-like
> > ones? I don't know that much about ACPI, but aren't they coming without
> > vendor prefix thus "ltr,ltrf216a" is just wrong and should be "lTRF216A"
> > alone?
>
> Yes, they come with the ID that is matched on by the ACPI core as PRP0001
> which basically means use the DT compatible.
> Then a device specific property that provides 'compatible' to look up against.
> The intent being to allow use of existing drivers without needing to modify
> them to add ACPI IDs to match against.
>
> LTRF216A is worse than using PRP0001 and DT vendor ID
> ACPI has it's own equivalent of vendor IDs and you have to apply for one from
> relevant committee in the UEFI forum (ASWG)
> https://uefi.org/ACPI_ID_List
> (there is a 3 letter form as well).
> It's easy to get an ID (takes a few weeks though) but many sensor companies
> etc don't bother. Sometimes they say it's because the OEMs should do this
> and sometimes those OEMs do, so the binding is under their vendor not the
> device manufacturer. That's when you see what looks like completely unrelated
> IDs being used.
>
> It would be good it liteon got a proper ID and started issuing device numbers
> to go with it though.
>
> There are a lot of old bindings that make IDs up. Some are based on cut and paste
> and we've been trying to scrub those, others are based on what Windows drivers
> bind against and so we are stuck with that set.
> For extra fun we have examples of hardware with a common ID for incompatible
> devices for which we have different drivers. That's a real pain when it happens
> but a few sensor manufacturers have 'one windows driver' for many years worth
> of unrelated devices and use horrible matching routines to figure out what is
> actually there...).
>
> >
> > >
> > > 2) This is an ACPI binding, it just happens to use a DT compatible via the
> > > PRP0001 mechanism. Yes, we strongly discourage people doing that in
> > > shipping products but there have been other cases of it.
> >
> > OK, is this the case here?
>
> Shreeya, can you check this. If we can get an example of such a device
> that would help. (This is the same request we've made when removing
> potentially false ACPI IDs). If we can't actually pin it down to a device
> I don't mind dropping the ID and seeing if anyone shouts.
>
This is exactly the case here. Thank you for putting it in better words.
(B+)(root@linux iio:device0)# cat /sys/bus/i2c/devices/i2c-PRP0001\:01/modalias
of:NltrfTCltr,ltrf216a
Above is the output from the steam deck device which I had also shared with Rob
during the discussion of adding the string with a deprecated tag [1]
I understand that it was NAK'd by Rob but what I understood from his last
email in the thread is that it is okay to add this compatible in the driver but
shouldn't be documented in the bindings.
https://lore.kernel.org/all/f37bccaf-233c-a244-3d81-849a988b1a92@collabora.com/#t
> >
> > >
> > > 3) Shreeya read a distinction (that I also agree with) between the schema
> > > and the compatible list. The schema does not apply to this situation
> > > (because we can't actually check it today for DSDT) hence Rob's Nack
> > > was making the point it was inappropriate to carry it there.
> > >
Exactly! This was my understanding at that time.
Thanks,
Shreeya Patel
> > > So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
> > > I'd love to be in a position to say no on ACPI bindings that are garbage
> > > (there are a lot of them) but Windows is dominant in that space so
> > > we get stuck with their mess. On server's it is a different game
> > > and the kernel community regularly gets significant changes made.
> >
> > Original discussion had only vague statement of "vendor prefix name as
> > 'ltr' through ACPI". But what does it even mean? What ACPI ID is
> > reported by these devices?
>
> PRP0001 is the only way it can be done that I know of so I read the
> original thread with that in mind. I might be wrong though and
> that would indeed change this discussion!
>
> Thanks,
>
> Jonathan
>
>
> >
> > Best regards,
> > Krzysztof
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-08 13:20 ` Shreeya Patel
@ 2024-07-08 16:01 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-07-08 16:01 UTC (permalink / raw)
To: Shreeya Patel
Cc: Jonathan Cameron, Krzysztof Kozlowski, Marek Vasut, linux-iio,
Conor Dooley, Krzysztof Kozlowski, Lars-Peter Clausen,
Rob Herring, devicetree, kernel, Andy Shevchenko
On Mon, 8 Jul 2024 14:20:54 +0100
Shreeya Patel <shreeya.patel@collabora.com> wrote:
> On Sunday, July 07, 2024 19:38 IST, Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Sun, 7 Jul 2024 15:46:26 +0200
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > > On 07/07/2024 15:37, Jonathan Cameron wrote:
> > > > On Sun, 7 Jul 2024 14:02:39 +0200
> > > > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > >> On 05/07/2024 12:42, Shreeya Patel wrote:
> > > >>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@denx.de> wrote:
> > > >>>
> > > >>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
> > > >>>> document, remove it.
> > > >>>>
> > > >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> > > >>>> ---
> > > >>>> Cc: Conor Dooley <conor+dt@kernel.org>
> > > >>>> Cc: Jonathan Cameron <jic23@kernel.org>
> > > >>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > > >>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
> > > >>>> Cc: Marek Vasut <marex@denx.de>
> > > >>>> Cc: Rob Herring <robh@kernel.org>
> > > >>>> Cc: Shreeya Patel <shreeya.patel@collabora.com>
> > > >>>> Cc: devicetree@vger.kernel.org
> > > >>>> Cc: linux-iio@vger.kernel.org
> > > >>>> ---
> > > >>>> drivers/iio/light/ltrf216a.c | 1 -
> > > >>>> 1 file changed, 1 deletion(-)
> > > >>>>
> > > >>>> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> > > >>>> index 68dc48420a886..78fc910fcb18c 100644
> > > >>>> --- a/drivers/iio/light/ltrf216a.c
> > > >>>> +++ b/drivers/iio/light/ltrf216a.c
> > > >>>> @@ -528,7 +528,6 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> > > >>>>
> > > >>>> static const struct of_device_id ltrf216a_of_match[] = {
> > > >>>> { .compatible = "liteon,ltrf216a" },
> > > >>>> - { .compatible = "ltr,ltrf216a" },
> > > >>>> {}
> > > >>>
> > > >>> This compatible string with a different vendor prefix was added for a specific reason.
> > > >>> Please see the commit message of the following patch :-
> > > >>> https://lore.kernel.org/all/20220511094024.175994-2-shreeya.patel@collabora.com/
> > > >>
> > > >> And adding this specific compatible was clearly NAKed:
> > > >> https://lore.kernel.org/all/20220516170406.GB2825626-robh@kernel.org/
> > > >>
> > > >> yet you still added it. That's a deliberate going around maintainer's
> > > >> decision.
> > > >>
> > > >
> > > > The statement from Rob was very specific. The schema is not applicable to ACPI bindings
> > > > - that's the basis on which he doesn't want this in the schema. Specifically
> > > > because "There's not really any point in having this in schema as you can't
> > > > use that schema with ACPI".
> > > >
> > > > That is true (though arguably you could with sufficient tooling apply the schema
> > > > to the relevant part of DSDT).
> > > >
> > > > The compatible is usable, via PRP0001 ACPI IDs.
> > >
> > > Uh, that's sounds like a slippery slope. To my understanding, PRP0001
> > > allows to create ACPI IDs from OF IDs, so it requires to have a valid OF
> > > ID. Valid OF ID requires bindings, doesn't it?
> > >
> > > If it does not, then anyone can add any Devicetree properties, claiming
> > > it is for ACPI ID thus not providing bindings (or bypassing bindings
> > > review / NAK).
> >
> > True, but in a similar fashion to ACPI bindings (which Andy in particular
> > keeps a close eye on!) we should ask for very specific device reference
> > and document the broken part. I've gotten a lot stricter on this over
> > the last few years so new cases of this in IIO at least require such
> > a comment alongside the ID table entry.
> >
> > >
> > > >
> > > >>>
> > > >>> We were very well aware that not documenting this was going to generate a warning so
> > > >>
> > > >> You *CANNOT* have undocumented compatibles.
> > > >
> > > > Why not? This corner case is a valid reason for that to be allowed.
> > > > You cannot use that compatible with DT bindings. Absolutely. The compatible
> > > > has other uses...
> > >
> > > Okay. With that approach what stops anyone from submitting DTS using
> > > that compatible (claiming there is a driver for that compatible)?
> >
> > That's a good point. Perhaps we should just add a check for this?
> > Easy to add a check on the firmware type. This is a rare enough case that
> > just doing it in the driver seems fine to me (rather than more general
> > infrastructure).
> >
> > >
> > > >
> > > >
> > > >>
> > > >>> we tried to fix that with a deprecated tag but it was NAKd by Rob. What we understood
> > > >>
> > > >> Because the driver was NAKed obviously as well.
> > > >>
> > > >>> from his last message was that it wasn't necessary to fix the DT warning.
> > > >>
> > > >> I am quite angry that maintainer tells you something, but you push your
> > > >> patch through because apparently you need to fulfill your project
> > > >> requirements.
> > > >
> > > > I think this is a fundamental misunderstanding of the situation and probably
> > > > at least partly my fault for not clarifying my reading of the situation more
> > > > fully at the time.
> > > >
> > > > As far as I am concerned. The situation is:
> > > > 1) Existing shipping consumer device. We have 100s of cases of ACPI bindings
> > > > that exist just to deal with garbage firmware's. The folk involved in
> > > > reviewing these have pushed back hard for a long time, but sadly there
> > > > is still a lot of garbage shipping because Windows lets it through and
> > > > Linux support comes second. It's made even worse by Microsoft defining
> > > > their own standards that aren't compliant with ACPI as they don't
> > > > even bother with reserving the methods IDs. ROTM for example.
> > >
> > > Hm, and these devices do not provide normal ACPI IDs? They use Of-like
> > > ones? I don't know that much about ACPI, but aren't they coming without
> > > vendor prefix thus "ltr,ltrf216a" is just wrong and should be "lTRF216A"
> > > alone?
> >
> > Yes, they come with the ID that is matched on by the ACPI core as PRP0001
> > which basically means use the DT compatible.
> > Then a device specific property that provides 'compatible' to look up against.
> > The intent being to allow use of existing drivers without needing to modify
> > them to add ACPI IDs to match against.
> >
> > LTRF216A is worse than using PRP0001 and DT vendor ID
> > ACPI has it's own equivalent of vendor IDs and you have to apply for one from
> > relevant committee in the UEFI forum (ASWG)
> > https://uefi.org/ACPI_ID_List
> > (there is a 3 letter form as well).
> > It's easy to get an ID (takes a few weeks though) but many sensor companies
> > etc don't bother. Sometimes they say it's because the OEMs should do this
> > and sometimes those OEMs do, so the binding is under their vendor not the
> > device manufacturer. That's when you see what looks like completely unrelated
> > IDs being used.
> >
> > It would be good it liteon got a proper ID and started issuing device numbers
> > to go with it though.
> >
> > There are a lot of old bindings that make IDs up. Some are based on cut and paste
> > and we've been trying to scrub those, others are based on what Windows drivers
> > bind against and so we are stuck with that set.
> > For extra fun we have examples of hardware with a common ID for incompatible
> > devices for which we have different drivers. That's a real pain when it happens
> > but a few sensor manufacturers have 'one windows driver' for many years worth
> > of unrelated devices and use horrible matching routines to figure out what is
> > actually there...).
> >
> > >
> > > >
> > > > 2) This is an ACPI binding, it just happens to use a DT compatible via the
> > > > PRP0001 mechanism. Yes, we strongly discourage people doing that in
> > > > shipping products but there have been other cases of it.
> > >
> > > OK, is this the case here?
> >
> > Shreeya, can you check this. If we can get an example of such a device
> > that would help. (This is the same request we've made when removing
> > potentially false ACPI IDs). If we can't actually pin it down to a device
> > I don't mind dropping the ID and seeing if anyone shouts.
> >
>
> This is exactly the case here. Thank you for putting it in better words.
>
> (B+)(root@linux iio:device0)# cat /sys/bus/i2c/devices/i2c-PRP0001\:01/modalias
> of:NltrfTCltr,ltrf216a
>
> Above is the output from the steam deck device which I had also shared with Rob
> during the discussion of adding the string with a deprecated tag [1]
>
> I understand that it was NAK'd by Rob but what I understood from his last
> email in the thread is that it is okay to add this compatible in the driver but
> shouldn't be documented in the bindings.
>
> https://lore.kernel.org/all/f37bccaf-233c-a244-3d81-849a988b1a92@collabora.com/#t
Ok. If we don't make any other changes as a result of this discussion (not sure
where we will end up yet). I'd like a patch adding a comment about the device
that is in the wild alongside the compatible.
Jonathan
>
> > >
> > > >
> > > > 3) Shreeya read a distinction (that I also agree with) between the schema
> > > > and the compatible list. The schema does not apply to this situation
> > > > (because we can't actually check it today for DSDT) hence Rob's Nack
> > > > was making the point it was inappropriate to carry it there.
> > > >
>
> Exactly! This was my understanding at that time.
>
>
> Thanks,
> Shreeya Patel
>
> > > > So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
> > > > I'd love to be in a position to say no on ACPI bindings that are garbage
> > > > (there are a lot of them) but Windows is dominant in that space so
> > > > we get stuck with their mess. On server's it is a different game
> > > > and the kernel community regularly gets significant changes made.
> > >
> > > Original discussion had only vague statement of "vendor prefix name as
> > > 'ltr' through ACPI". But what does it even mean? What ACPI ID is
> > > reported by these devices?
> >
> > PRP0001 is the only way it can be done that I know of so I read the
> > original thread with that in mind. I might be wrong though and
> > that would indeed change this discussion!
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string
2024-07-07 13:37 ` Jonathan Cameron
2024-07-07 13:46 ` Krzysztof Kozlowski
@ 2024-08-12 13:24 ` Andy Shevchenko
1 sibling, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2024-08-12 13:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Krzysztof Kozlowski, Shreeya Patel, Marek Vasut, linux-iio,
Conor Dooley, Krzysztof Kozlowski, Lars-Peter Clausen,
Rob Herring, devicetree, kernel
On Sun, Jul 07, 2024 at 02:37:59PM +0100, Jonathan Cameron wrote:
> On Sun, 7 Jul 2024 14:02:39 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
...
> So, I don't see this as a deliberate attempt to bypass a maintainer Nack.
> I'd love to be in a position to say no on ACPI bindings that are garbage
> (there are a lot of them) but Windows is dominant in that space so
> we get stuck with their mess.
...and we are trying hard to avoid this mess in the future (e.g., what is
happening to MIPI I3C case nowadays).
Unfortunately Linux world seems not being so interested in this topic either
(as my proposal to discuss the ACPI ID topic on LPC has been rejected).
> On server's it is a different game
> and the kernel community regularly gets significant changes made.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-08-12 13:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-05 9:50 [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string Marek Vasut
2024-07-05 9:55 ` Krzysztof Kozlowski
2024-07-05 10:42 ` Shreeya Patel
2024-07-05 14:52 ` Marek Vasut
2024-07-05 18:03 ` Shreeya Patel
2024-07-07 11:26 ` Jonathan Cameron
2024-07-07 12:06 ` Krzysztof Kozlowski
2024-07-07 13:43 ` Jonathan Cameron
2024-07-07 12:02 ` Krzysztof Kozlowski
2024-07-07 13:37 ` Jonathan Cameron
2024-07-07 13:46 ` Krzysztof Kozlowski
2024-07-07 14:08 ` Jonathan Cameron
2024-07-08 11:25 ` Krzysztof Kozlowski
2024-07-08 12:29 ` Sebastian Reichel
2024-07-08 12:13 ` Sebastian Reichel
2024-07-08 13:20 ` Shreeya Patel
2024-07-08 16:01 ` Jonathan Cameron
2024-08-12 13:24 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).