iio: light: ltrf216a: Document the device name

[PATCH] iio: light: ltrf216a: Document the device name

Posted by Shreeya Patel 1 year, 5 months ago

Compatible 'ltr,ltrf216a' is used by Valve's Steamdeck device
via the ACPI + PRP0001 mechanism.
Document the device name alognside the compatible.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/light/ltrf216a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
index 68dc48420a88..e97ff8f7ecd0 100644
--- a/drivers/iio/light/ltrf216a.c
+++ b/drivers/iio/light/ltrf216a.c
@@ -528,7 +528,7 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
 
 static const struct of_device_id ltrf216a_of_match[] = {
 	{ .compatible = "liteon,ltrf216a" },
-	{ .compatible = "ltr,ltrf216a" },
+	{ .compatible = "ltr,ltrf216a" }, /* For Valve's Steamdeck device */
 	{}
 };
 MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
-- 
2.39.2

Re: [PATCH] iio: light: ltrf216a: Document the device name

Posted by Jonathan Cameron 1 year, 4 months ago

On Mon, 15 Jul 2024 01:07:09 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Compatible 'ltr,ltrf216a' is used by Valve's Steamdeck device
> via the ACPI + PRP0001 mechanism.
> Document the device name alognside the compatible.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Hi Shreeya,

I'm guessing you used get_maintainers for this.  Always check that there
isn't anyone else who should be added.

Given the root of this was the discussion about dropping this undocumented
binding the DT maintainers and list should definitely be CC'd + Marek
who was part of the earlier discussion.

The fact that this isn't a typical ID from a Windows consumer device does
change things a little, but I'd imagine a firmware update for the steamdeck
is equally unlikely to happen in order to fix the vendor-id.
(I was surprised to discover they provide windows drivers for it but
not seeing the light sensor in the list)

So my opinion is that we should keep this ID in place with an enhanced
description.

The other suggestion from the Krzysztof's thread was the risk of this getting
abused.  Perhaps we should specifically fail to probe if that ID is used and
it's not an ACPI firmware?

> ---
>  drivers/iio/light/ltrf216a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> index 68dc48420a88..e97ff8f7ecd0 100644
> --- a/drivers/iio/light/ltrf216a.c
> +++ b/drivers/iio/light/ltrf216a.c
> @@ -528,7 +528,7 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>  
>  static const struct of_device_id ltrf216a_of_match[] = {
>  	{ .compatible = "liteon,ltrf216a" },
> -	{ .compatible = "ltr,ltrf216a" },
> +	{ .compatible = "ltr,ltrf216a" }, /* For Valve's Steamdeck device */

Add that the steamdeck is an ACPI platform using PRP0001 as that's
the key part that is different from a normal 'wrong' of_device_id.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ltrf216a_of_match);

Re: [PATCH] iio: light: ltrf216a: Document the device name

Posted by Shreeya Patel 1 year, 4 months ago

On Saturday, July 20, 2024 18:52 IST, Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 15 Jul 2024 01:07:09 +0530
> Shreeya Patel <shreeya.patel@collabora.com> wrote:
> 
> > Compatible 'ltr,ltrf216a' is used by Valve's Steamdeck device
> > via the ACPI + PRP0001 mechanism.
> > Document the device name alognside the compatible.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Hi Shreeya,
> 
> I'm guessing you used get_maintainers for this.  Always check that there
> isn't anyone else who should be added.
> 
> Given the root of this was the discussion about dropping this undocumented
> binding the DT maintainers and list should definitely be CC'd + Marek
> who was part of the earlier discussion.
> 

Sorry about that, I used get_maintainers like you said but I'll keep this in
mind next time.

> The fact that this isn't a typical ID from a Windows consumer device does
> change things a little, but I'd imagine a firmware update for the steamdeck
> is equally unlikely to happen in order to fix the vendor-id.
> (I was surprised to discover they provide windows drivers for it but
> not seeing the light sensor in the list)
> 
> So my opinion is that we should keep this ID in place with an enhanced
> description.
> 
> The other suggestion from the Krzysztof's thread was the risk of this getting
> abused.  Perhaps we should specifically fail to probe if that ID is used and
> it's not an ACPI firmware?
> 

Rob said in another thread that use of it can now not pass the validation
with dtschema. So I'm guessing we don't need to add any extra checks anymore?

However, I'll send a v2 with enhanced description alongside the ID.

Thanks,
Shreeya Patel

> 
> > ---
> >  drivers/iio/light/ltrf216a.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> > index 68dc48420a88..e97ff8f7ecd0 100644
> > --- a/drivers/iio/light/ltrf216a.c
> > +++ b/drivers/iio/light/ltrf216a.c
> > @@ -528,7 +528,7 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> >  
> >  static const struct of_device_id ltrf216a_of_match[] = {
> >  	{ .compatible = "liteon,ltrf216a" },
> > -	{ .compatible = "ltr,ltrf216a" },
> > +	{ .compatible = "ltr,ltrf216a" }, /* For Valve's Steamdeck device */
> 
> Add that the steamdeck is an ACPI platform using PRP0001 as that's
> the key part that is different from a normal 'wrong' of_device_id.
> 
> 
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
>

Re: [PATCH] iio: light: ltrf216a: Document the device name

Posted by Jonathan Cameron 1 year, 4 months ago

On Mon, 22 Jul 2024 15:31:20 +0100
"Shreeya Patel" <shreeya.patel@collabora.com> wrote:

> On Saturday, July 20, 2024 18:52 IST, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Mon, 15 Jul 2024 01:07:09 +0530
> > Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >   
> > > Compatible 'ltr,ltrf216a' is used by Valve's Steamdeck device
> > > via the ACPI + PRP0001 mechanism.
> > > Document the device name alognside the compatible.
> > > 
> > > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>  
> > Hi Shreeya,
> > 
> > I'm guessing you used get_maintainers for this.  Always check that there
> > isn't anyone else who should be added.
> > 
> > Given the root of this was the discussion about dropping this undocumented
> > binding the DT maintainers and list should definitely be CC'd + Marek
> > who was part of the earlier discussion.
> >   
> 
> Sorry about that, I used get_maintainers like you said but I'll keep this in
> mind next time.
> 
> > The fact that this isn't a typical ID from a Windows consumer device does
> > change things a little, but I'd imagine a firmware update for the steamdeck
> > is equally unlikely to happen in order to fix the vendor-id.
> > (I was surprised to discover they provide windows drivers for it but
> > not seeing the light sensor in the list)
> > 
> > So my opinion is that we should keep this ID in place with an enhanced
> > description.
> > 
> > The other suggestion from the Krzysztof's thread was the risk of this getting
> > abused.  Perhaps we should specifically fail to probe if that ID is used and
> > it's not an ACPI firmware?
> >   
> 
> Rob said in another thread that use of it can now not pass the validation
> with dtschema. So I'm guessing we don't need to add any extra checks anymore?
That's how I interpreted Rob's reply as well.

> 
> However, I'll send a v2 with enhanced description alongside the ID.
Great.

J
> 
> Thanks,
> Shreeya Patel
> 
> >   
> > > ---
> > >  drivers/iio/light/ltrf216a.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> > > index 68dc48420a88..e97ff8f7ecd0 100644
> > > --- a/drivers/iio/light/ltrf216a.c
> > > +++ b/drivers/iio/light/ltrf216a.c
> > > @@ -528,7 +528,7 @@ MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> > >  
> > >  static const struct of_device_id ltrf216a_of_match[] = {
> > >  	{ .compatible = "liteon,ltrf216a" },
> > > -	{ .compatible = "ltr,ltrf216a" },
> > > +	{ .compatible = "ltr,ltrf216a" }, /* For Valve's Steamdeck device */  
> > 
> > Add that the steamdeck is an ACPI platform using PRP0001 as that's
> > the key part that is different from a normal 'wrong' of_device_id.
> > 
> >   
> > >  	{}
> > >  };
> > >  MODULE_DEVICE_TABLE(of, ltrf216a_of_match);  
> >  
>

Re: [PATCH] iio: light: ltrf216a: Document the device name

Posted by Rob Herring 1 year, 4 months ago

On Sat, Jul 20, 2024 at 7:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 15 Jul 2024 01:07:09 +0530
> Shreeya Patel <shreeya.patel@collabora.com> wrote:
>
> > Compatible 'ltr,ltrf216a' is used by Valve's Steamdeck device
> > via the ACPI + PRP0001 mechanism.
> > Document the device name alognside the compatible.
> >
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Hi Shreeya,
>
> I'm guessing you used get_maintainers for this.  Always check that there
> isn't anyone else who should be added.
>
> Given the root of this was the discussion about dropping this undocumented
> binding the DT maintainers and list should definitely be CC'd + Marek
> who was part of the earlier discussion.
>
> The fact that this isn't a typical ID from a Windows consumer device does
> change things a little, but I'd imagine a firmware update for the steamdeck
> is equally unlikely to happen in order to fix the vendor-id.
> (I was surprised to discover they provide windows drivers for it but
> not seeing the light sensor in the list)
>
> So my opinion is that we should keep this ID in place with an enhanced
> description.
>
> The other suggestion from the Krzysztof's thread was the risk of this getting
> abused.  Perhaps we should specifically fail to probe if that ID is used and
> it's not an ACPI firmware?

Use of it can now not pass validation with dtschema. IMO, that's enough.

Rob