| Message ID | 20240714193709.543509-1-shreeya.patel@collabora.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: ltrf216a: Document the device name | expand |
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);
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
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); >
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); > > >
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);
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(-)