On Sunday, July 07, 2024 19:38 IST, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Sun, 7 Jul 2024 15:46:26 +0200
> Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> > On 07/07/2024 15:37, Jonathan Cameron wrote:
> > > On Sun, 7 Jul 2024 14:02:39 +0200
> > > Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> > >
> > >> On 05/07/2024 12:42, Shreeya Patel wrote:
> > >>> On Friday, July 05, 2024 15:20 IST, Marek Vasut <marex@xxxxxxx> wrote:
> > >>>
> > >>>> The "ltr,ltrf216a" compatible string is not documented in DT binding
> > >>>> document, remove it.
> > >>>>
> > >>>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > >>>> ---
> > >>>> Cc: Conor Dooley <conor+dt@xxxxxxxxxx>
> > >>>> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> > >>>> Cc: Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>
> > >>>> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > >>>> Cc: Marek Vasut <marex@xxxxxxx>
> > >>>> Cc: Rob Herring <robh@xxxxxxxxxx>
> > >>>> Cc: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
> > >>>> Cc: devicetree@xxxxxxxxxxxxxxx
> > >>>> Cc: linux-iio@xxxxxxxxxxxxxxx
> > >>>> ---
> > >>>> 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@xxxxxxxxxxxxx/
> > >>
> > >> And adding this specific compatible was clearly NAKed:
> > >> https://lore.kernel.org/all/20220516170406.GB2825626-robh@xxxxxxxxxx/
> > >>
> > >> 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@xxxxxxxxxxxxx/#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
> >
>
