On Mon, 8 Jul 2024 14:20:54 +0100
Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote:
> 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
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
> > >
> >
>
>
