Re: [PATCH] iio: light: ltrf216a: Drop undocumented ltr,ltrf216a compatible string

["Shreeya Patel" <shreeya.patel@collabora.com>]

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
> > 
>