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

[Jonathan Cameron <jic23@kernel.org>]

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
>