[PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Hardevsinh Palaniya]

Add Support for OPT3004 Digital ambient light sensor (ALS) with
increased angular IR rejection.

The OPT3004 sensor shares the same functionality and scale range as
the OPT3001. The compatible string is added with fallback support to
ensure compatibility.

Datasheet: https://www.ti.com/lit/gpn/opt3004
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---

v2 -> v3:

- Reverse the order of compatible string

v1 -> v2:                                                                       
                                                                                
- Use fallback mechanism for the OPT3004.                                       
- Drop 2/2 patch from the patch series[1] as per feedback.                      
                                                                                
Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t

---
 .../devicetree/bindings/iio/light/ti,opt3001.yaml      | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
index 67ca8d08256a..015860bfaf12 100644
--- a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
@@ -15,9 +15,13 @@ description: |
 
 properties:
   compatible:
-    enum:
-      - ti,opt3001
-      - ti,opt3002
+    oneOf:
+      - enum:
+          - ti,opt3001
+          - ti,opt3002
+      - items:
+          - const: ti,opt3004
+          - const: ti,opt3001
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Krzysztof Kozlowski]

On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:
> Add Support for OPT3004 Digital ambient light sensor (ALS) with
> increased angular IR rejection.
> 
> The OPT3004 sensor shares the same functionality and scale range as
> the OPT3001. The compatible string is added with fallback support to
> ensure compatibility.
> 
> Datasheet: https://www.ti.com/lit/gpn/opt3004
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> ---
> 
> v2 -> v3:
> 
> - Reverse the order of compatible string
> 
> v1 -> v2:                                                                       
>                                                                                 
> - Use fallback mechanism for the OPT3004.                                       
> - Drop 2/2 patch from the patch series[1] as per feedback.                      
>                                                                                 
> Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t


And where is any user of this, the DTS? We don't take bindings just
because there is such device out there.

I looked and nothing:
https://lore.kernel.org/all/?q=ti%2Copt3004

Best regards,
Krzysztof

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Hardevsinh Palaniya]

Hi Krzysztof,
 
> On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:
> > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > increased angular IR rejection.
> >
> > The OPT3004 sensor shares the same functionality and scale range as
> > the OPT3001. The compatible string is added with fallback support to
> > ensure compatibility.
> >
> > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > ---
> >
> > v2 -> v3:
> >
> > - Reverse the order of compatible string
> >
> > v1 -> v2:
> >
> > - Use fallback mechanism for the OPT3004.
> > - Drop 2/2 patch from the patch series[1] as per feedback.
> >
> > Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t
> 
>
> And where is any user of this, the DTS? We don't take bindings just
> because there is such device out there.
>
> I looked and nothing:
> https://lore.kernel.org/all/?q=ti%2Copt3004
 
I added compatibility for the OPT3004 into the driver. However, 
based on Andy's feedback, it seems there might not be a need 
to include this directly in the driver. 
(Refer to Link [1] from the changelog.)
 
we could follow a similar approach to how we handled the ADXL346,
by just adding it to the bindings, since the ADXL346 is similar to the 
ADXL345.
 
If I misunderstood then please let me know.
 
Best Regards,
Hardev

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Jonathan Cameron]

On Sat, 28 Dec 2024 10:54:33 +0000
Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:

> Hi Krzysztof,
>  
> > On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:  
> > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > increased angular IR rejection.
> > >
> > > The OPT3004 sensor shares the same functionality and scale range as
> > > the OPT3001. The compatible string is added with fallback support to
> > > ensure compatibility.
> > >
> > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > > ---
> > >
> > > v2 -> v3:
> > >
> > > - Reverse the order of compatible string
> > >
> > > v1 -> v2:
> > >
> > > - Use fallback mechanism for the OPT3004.
> > > - Drop 2/2 patch from the patch series[1] as per feedback.
> > >
> > > Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t  
> > 
> >
> > And where is any user of this, the DTS? We don't take bindings just
> > because there is such device out there.
> >
> > I looked and nothing:
> > https://lore.kernel.org/all/?q=ti%2Copt3004  
>  
> I added compatibility for the OPT3004 into the driver. However, 
> based on Andy's feedback, it seems there might not be a need 
> to include this directly in the driver. 
> (Refer to Link [1] from the changelog.)
>  
> we could follow a similar approach to how we handled the ADXL346,
> by just adding it to the bindings, since the ADXL346 is similar to the 
> ADXL345.
>  
> If I misunderstood then please let me know.

Perhaps give some more information on the device in which this is found?
If that's a board that you plan to support upstream in the longer term
then that would provide more justification for this patch.

The note on the opt3001 page does give a hint as to how the parts are different
but saying the opt3004 has better IR rejection.  They also have a somewhat different
sensitivity curves. However, those are details we don't expose in the ABI and the
devices unhelpfully report the same ID register value, so it is not obvious that
we need to treat them differently.

Jonathan

>  
> Best Regards,
> Hardev

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Krzysztof Kozlowski]

On 28/12/2024 11:54, Hardevsinh Palaniya wrote:
>>> - Reverse the order of compatible string
>>>
>>> v1 -> v2:
>>>
>>> - Use fallback mechanism for the OPT3004.
>>> - Drop 2/2 patch from the patch series[1] as per feedback.
>>>
>>> Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t
>>  
>>
>> And where is any user of this, the DTS? We don't take bindings just
>> because there is such device out there.
>>
>> I looked and nothing:
>> https://lore.kernel.org/all/?q=ti%2Copt3004
>  
> I added compatibility for the OPT3004 into the driver. However, 
> based on Andy's feedback, it seems there might not be a need 
> to include this directly in the driver. 
> (Refer to Link [1] from the changelog.)
>  
> we could follow a similar approach to how we handled the ADXL346,
> by just adding it to the bindings, since the ADXL346 is similar to the 
> ADXL345.

Sure, I am not talking about this. I am looking for a user of the
binding. Where is one?

User is any piece of *upstream* project which uses the binding.



Best regards,
Krzysztof

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Hardevsinh Palaniya]

Hi Jonathan, Krzysztof
 
Thanks for your input.
 
> On Sat, 28 Dec 2024 10:54:33 +0000
> Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:
> 
> > Hi Krzysztof,
> >
> > > On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:
> > > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > > increased angular IR rejection.
> > > >
> > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > the OPT3001. The compatible string is added with fallback support to
> > > > ensure compatibility.
> > > >
> > > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > > > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > > > ---
> > > >
> > > > v2 -> v3:
> > > >
> > > > - Reverse the order of compatible string
> > > >
> > > > v1 -> v2:
> > > >
> > > > - Use fallback mechanism for the OPT3004.
> > > > - Drop 2/2 patch from the patch series[1] as per feedback.
> > > >
> > > > Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t
> > > 
> > >
> > > And where is any user of this, the DTS? We don't take bindings just
> > > because there is such device out there.
> > >
> > > I looked and nothing:
> > > https://lore.kernel.org/all/?q=ti%2Copt3004
> > >
> > I added compatibility for the OPT3004 into the driver. However,
> > based on Andy's feedback, it seems there might not be a need
> > to include this directly in the driver.
> > (Refer to Link [1] from the changelog.)
> >
> > we could follow a similar approach to how we handled the ADXL346,
> > by just adding it to the bindings, since the ADXL346 is similar to the
> > ADXL345.
> >
> > If I misunderstood then please let me know.
>
> Perhaps give some more information on the device in which this is found?
> If that's a board that you plan to support upstream in the longer term
> then that would provide more justification for this patch.
> 
> The note on the opt3001 page does give a hint as to how the parts are different
> but saying the opt3004 has better IR rejection.  They also have a somewhat different
> sensitivity curves. However, those are details we don't expose in the ABI and the
> devices unhelpfully report the same ID register value, so it is not obvious that
> we need to treat them differently.
 
In our customer project, this sensor is utilized in the product. However, 
I acknowledge that, at present, there is no upstream user of this binding
in the mainline.
 
Additionally, it is unlikely that the custom board using this sensor will be 
upstreamed in the future. The primary motivation for this patch is the 
absence of existing support for the OPT3004 in the mainline kernel.
 
I fully understand your concerns regarding the addition of bindings without 
an immediate upstream use case. I leave the decision to you regarding whether
it would be appropriate to include this binding. I am open to your guidance and
will respect the outcome either way.
 
Best Regards,
Hardev

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Rob Herring]

On Mon, Dec 30, 2024 at 07:34:27AM +0000, Hardevsinh Palaniya wrote:
> Hi Jonathan, Krzysztof
>  
> Thanks for your input.
>  
> > On Sat, 28 Dec 2024 10:54:33 +0000
> > Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:
> > 
> > > Hi Krzysztof,
> > >
> > > > On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:
> > > > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > > > increased angular IR rejection.
> > > > >
> > > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > > the OPT3001. The compatible string is added with fallback support to
> > > > > ensure compatibility.
> > > > >
> > > > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > > > > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > > > > ---
> > > > >
> > > > > v2 -> v3:
> > > > >
> > > > > - Reverse the order of compatible string
> > > > >
> > > > > v1 -> v2:
> > > > >
> > > > > - Use fallback mechanism for the OPT3004.
> > > > > - Drop 2/2 patch from the patch series[1] as per feedback.
> > > > >
> > > > > Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t
> > > > 
> > > >
> > > > And where is any user of this, the DTS? We don't take bindings just
> > > > because there is such device out there.
> > > >
> > > > I looked and nothing:
> > > > https://lore.kernel.org/all/?q=ti%2Copt3004
> > > >
> > > I added compatibility for the OPT3004 into the driver. However,
> > > based on Andy's feedback, it seems there might not be a need
> > > to include this directly in the driver.
> > > (Refer to Link [1] from the changelog.)
> > >
> > > we could follow a similar approach to how we handled the ADXL346,
> > > by just adding it to the bindings, since the ADXL346 is similar to the
> > > ADXL345.
> > >
> > > If I misunderstood then please let me know.
> >
> > Perhaps give some more information on the device in which this is found?
> > If that's a board that you plan to support upstream in the longer term
> > then that would provide more justification for this patch.
> > 
> > The note on the opt3001 page does give a hint as to how the parts are different
> > but saying the opt3004 has better IR rejection.  They also have a somewhat different
> > sensitivity curves. However, those are details we don't expose in the ABI and the
> > devices unhelpfully report the same ID register value, so it is not obvious that
> > we need to treat them differently.
>  
> In our customer project, this sensor is utilized in the product. However, 
> I acknowledge that, at present, there is no upstream user of this binding
> in the mainline.
>  
> Additionally, it is unlikely that the custom board using this sensor will be 
> upstreamed in the future. The primary motivation for this patch is the 
> absence of existing support for the OPT3004 in the mainline kernel.
>  
> I fully understand your concerns regarding the addition of bindings without 
> an immediate upstream use case. I leave the decision to you regarding whether
> it would be appropriate to include this binding. I am open to your guidance and
> will respect the outcome either way.

I'm confused. Jonathan seems to say the OPT3004 needs different 
handling. The binding says it doesn't at least for some subset of 
functionality matching OPT3001.

If you need driver changes, then submit this with the driver changes. If 
you don't, then make it clear the device works with the existing driver.

Rob

Re: [PATCH v3] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Jonathan Cameron]

On Thu, 2 Jan 2025 15:34:36 -0600
Rob Herring <robh@kernel.org> wrote:

> On Mon, Dec 30, 2024 at 07:34:27AM +0000, Hardevsinh Palaniya wrote:
> > Hi Jonathan, Krzysztof
> >  
> > Thanks for your input.
> >    
> > > On Sat, 28 Dec 2024 10:54:33 +0000
> > > Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:
> > >   
> > > > Hi Krzysztof,
> > > >  
> > > > > On Fri, Dec 27, 2024 at 06:41:32PM +0530, Hardevsinh Palaniya wrote:  
> > > > > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > > > > increased angular IR rejection.
> > > > > >
> > > > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > > > the OPT3001. The compatible string is added with fallback support to
> > > > > > ensure compatibility.
> > > > > >
> > > > > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > > > > > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > > > > > ---
> > > > > >
> > > > > > v2 -> v3:
> > > > > >
> > > > > > - Reverse the order of compatible string
> > > > > >
> > > > > > v1 -> v2:
> > > > > >
> > > > > > - Use fallback mechanism for the OPT3004.
> > > > > > - Drop 2/2 patch from the patch series[1] as per feedback.
> > > > > >
> > > > > > Link[1]: https://lore.kernel.org/linux-iio/20241224061321.6048-1-hardevsinh.palaniya@siliconsignals.io/T/#t  
> > > > > 
> > > > >
> > > > > And where is any user of this, the DTS? We don't take bindings just
> > > > > because there is such device out there.
> > > > >
> > > > > I looked and nothing:
> > > > > https://lore.kernel.org/all/?q=ti%2Copt3004
> > > > >  
> > > > I added compatibility for the OPT3004 into the driver. However,
> > > > based on Andy's feedback, it seems there might not be a need
> > > > to include this directly in the driver.
> > > > (Refer to Link [1] from the changelog.)
> > > >
> > > > we could follow a similar approach to how we handled the ADXL346,
> > > > by just adding it to the bindings, since the ADXL346 is similar to the
> > > > ADXL345.
> > > >
> > > > If I misunderstood then please let me know.  
> > >
> > > Perhaps give some more information on the device in which this is found?
> > > If that's a board that you plan to support upstream in the longer term
> > > then that would provide more justification for this patch.
> > > 
> > > The note on the opt3001 page does give a hint as to how the parts are different
> > > but saying the opt3004 has better IR rejection.  They also have a somewhat different
> > > sensitivity curves. However, those are details we don't expose in the ABI and the
> > > devices unhelpfully report the same ID register value, so it is not obvious that
> > > we need to treat them differently.  
> >  
> > In our customer project, this sensor is utilized in the product. However, 
> > I acknowledge that, at present, there is no upstream user of this binding
> > in the mainline.
> >  
> > Additionally, it is unlikely that the custom board using this sensor will be 
> > upstreamed in the future. The primary motivation for this patch is the 
> > absence of existing support for the OPT3004 in the mainline kernel.
> >  
> > I fully understand your concerns regarding the addition of bindings without 
> > an immediate upstream use case. I leave the decision to you regarding whether
> > it would be appropriate to include this binding. I am open to your guidance and
> > will respect the outcome either way.  
> 
> I'm confused. Jonathan seems to say the OPT3004 needs different 
> handling. The binding says it doesn't at least for some subset of 
> functionality matching OPT3001.
> 
> If you need driver changes, then submit this with the driver changes. If 
> you don't, then make it clear the device works with the existing driver.

It would need different handling only if we exposed extremely detailed
information on the light sensitivity.  We've looked at how to design an ABI
for that in the past, but there has never been sufficient demand for that
information to justify what would be a complex bit of ABI.

Datasheet graphs aren't easy to convey from kernel drivers to userspace :(
I don't see us closing that gap in the near future and even then I don't think
any user would care about the small differences between these two sensors.

Hence I'd suggest we don't explicitly support the OPT3004 in the binding
until we need to. In meantime any out of tree board can just use a
fallback compatible from opt3004 to opt3001 thus future proofing against
the potential for the subtle differences making a difference in the future.

Conclusion: Don't do anything upstream now.

Jonathan


Jonathan


> 
> Rob