[PATCH 0/2] iio: light: opt3001: Add Support for OPT3004 Light Sensor

[PATCH 0/2] iio: light: opt3001: Add Support for OPT3004 Light Sensor

[Hardevsinh Palaniya]

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

Hardevsinh Palaniya (2):
  dt-bindings: iio: light: opt3001: add compatible for opt3004
  iio: light: opt3001: Add Support for opt3004 light sensor

 Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml | 1 +
 drivers/iio/light/Kconfig                                   | 3 ++-
 drivers/iio/light/opt3001.c                                 | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] 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. This Adds the compatible string for OPT3004.

Datasheet: https://www.ti.com/lit/gpn/opt3004

Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
 Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
index 67ca8d08256a..05b9a4f510bd 100644
--- a/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt3001.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - ti,opt3001
       - ti,opt3002
+      - ti,opt3004
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: iio: light: opt3001: add compatible for opt3004

[Krzysztof Kozlowski]

On 24/12/2024 07:13, 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. This Adds the compatible string for OPT3004.


Make the devices compatible (use fallback).

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


Best regards,
Krzysztof

[PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[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. This Adds the compatible string for OPT3004, enabling
the driver to support it without any functional changes.

Datasheet: https://www.ti.com/lit/gpn/opt3004

Tested-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
 drivers/iio/light/Kconfig   | 3 ++-
 drivers/iio/light/opt3001.c | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 29ffa8491927..748c8c2cd3e7 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -475,7 +475,8 @@ config OPT3001
 	depends on I2C
 	help
 	  If you say Y or M here, you get support for Texas Instruments
-	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor.
+	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor,
+	  OPT3004 Digital ambient light sensor.
 
 	  If built as a dynamically linked module, it will be called
 	  opt3001.
diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 65b295877b41..542af8612d34 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -949,6 +949,7 @@ static const struct opt3001_chip_info opt3002_chip_information = {
 static const struct i2c_device_id opt3001_id[] = {
 	{ "opt3001", (kernel_ulong_t)&opt3001_chip_information },
 	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
+	{ "opt3004", (kernel_ulong_t)&opt3001_chip_information },
 	{ } /* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(i2c, opt3001_id);
@@ -956,6 +957,7 @@ MODULE_DEVICE_TABLE(i2c, opt3001_id);
 static const struct of_device_id opt3001_of_match[] = {
 	{ .compatible = "ti,opt3001", .data = &opt3001_chip_information },
 	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
+	{ .compatible = "ti,opt3004", .data = &opt3001_chip_information },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, opt3001_of_match);
-- 
2.34.1

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Andy Shevchenko]

On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:
> Add Support for OPT3004 Digital ambient light sensor (ALS) with
> increased angular IR rejection

Missing period here.

> The OPT3004 sensor shares the same functionality and scale range as
> the OPT3001. This Adds the compatible string for OPT3004, enabling
> the driver to support it without any functional changes.
> 
> Datasheet: https://www.ti.com/lit/gpn/opt3004

> 

This blank line is not needed.

> Tested-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>

This tag is superfluous, it's assumed that author testing their code.

> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>

...

>  	help
>  	  If you say Y or M here, you get support for Texas Instruments
> -	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor.
> +	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor,
> +	  OPT3004 Digital ambient light sensor.

Can you rather convert this to a list (one item per line)?

	  - OPT3001 Ambient Light Sensor
	  - OPT3002 Light-to-Digital Sensor
	  - OPT3004 Digital ambient light sensor

...

>  static const struct of_device_id opt3001_of_match[] = {
>  	{ .compatible = "ti,opt3001", .data = &opt3001_chip_information },
>  	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> +	{ .compatible = "ti,opt3004", .data = &opt3001_chip_information },
>  	{ }
>  };

I'm always puzzled why do we need a new compatible for the existing driver
data? Is this hardware has an additional feature that driver does not (yet)
implement?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Hardevsinh Palaniya]

Hi Andy,

Thanks for the reviews 

> On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:
> > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > increased angular IR rejection
>
> Missing period here.
> 
> > The OPT3004 sensor shares the same functionality and scale range as
> > the OPT3001. This Adds the compatible string for OPT3004, enabling
> > the driver to support it without any functional changes.
> >
> > Datasheet: https://www.ti.com/lit/gpn/opt3004
>
> >
> 
> This blank line is not needed.
>
> > Tested-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
>
> This tag is superfluous, it's assumed that author testing their code.

okay , i will remove 

> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
>
> ...
>
> >       help
> >         If you say Y or M here, you get support for Texas Instruments
> > -       OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor.
> > +       OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor,
> > +       OPT3004 Digital ambient light sensor.
> 
> Can you rather convert this to a list (one item per line)?
> 
>          - OPT3001 Ambient Light Sensor
>          - OPT3002 Light-to-Digital Sensor
>          - OPT3004 Digital ambient light sensor

Sure , i will 

> >  static const struct of_device_id opt3001_of_match[] = {
> >       { .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> >       { .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> > +     { .compatible = "ti,opt3004", .data = &opt3001_chip_information },
> >       { }
> >  };
> 
> I'm always puzzled why do we need a new compatible for the existing driver
> data? Is this hardware has an additional feature that driver does not (yet)
> implement?

OPT3001 and OPT3004 sensors are functionally identical, and there are no
additional features in the OPT3004 that require separate handling in the driver.

The new compatible string for the OPT3004 is being added, which will allow the
driver to recognize and support this sensor in the same way it handles the OPT3001.

Best Regards,
Hardev

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Andy Shevchenko]

On Wed, Dec 25, 2024 at 09:56:36AM +0000, Hardevsinh Palaniya wrote:
> > On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:

...

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

> > > The OPT3004 sensor shares the same functionality and scale range as
> > > the OPT3001. This Adds the compatible string for OPT3004, enabling
> > > the driver to support it without any functional changes.
> > >
> > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> >
> > >
> > 
> > This blank line is not needed.

You left two above comments unanswered while Acking the rest, it's a bit confusing.
Are you agree on them or not?

...

> > >  static const struct of_device_id opt3001_of_match[] = {
> > >       { .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> > >       { .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> > > +     { .compatible = "ti,opt3004", .data = &opt3001_chip_information },
> > >       { }
> > >  };
> > 
> > I'm always puzzled why do we need a new compatible for the existing driver
> > data? Is this hardware has an additional feature that driver does not (yet)
> > implement?
> 
> OPT3001 and OPT3004 sensors are functionally identical, and there are no
> additional features in the OPT3004 that require separate handling in the driver.
> 
> The new compatible string for the OPT3004 is being added, which will allow the
> driver to recognize and support this sensor in the same way it handles the OPT3001.

But why? I understand if you put two compatible strings into the DT to make it
explicit in case of the future developments of the driver, but new compatible
in the driver makes only sense when you have either quirk(s) or feature(s) that
are different to the existing code. Since you haven't added either, what's the
point?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Hardevsinh Palaniya]

Hi Andy

> On Wed, Dec 25, 2024 at 09:56:36AM +0000, Hardevsinh Palaniya wrote:
> > > On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:
>

...

> > > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > > increased angular IR rejection
> > >
> > > Missing period here.
>
> > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > the OPT3001. This Adds the compatible string for OPT3004, enabling
> > > > the driver to support it without any functional changes.
> > > >
> > > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > >
> > > >
> > >
> > > This blank line is not needed.
> 
> You left two above comments unanswered while Acking the rest, it's a bit confusing.
> Are you agree on them or not?

Apologies for overlooking those comments. They seemed straightforward, so I
assumed your review was accurate, and I planned to address them directly in the
next version without explicitly responding.

Regarding the second comment:
The blank line was added to differentiate between the commit message and the
SoB tag. Are you sure it should be removed?

...

> > > >  static const struct of_device_id opt3001_of_match[] = {
> > > >       { .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> > > >       { .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> > > > +     { .compatible = "ti,opt3004", .data = &opt3001_chip_information },
> > > >       { }
> > > >  };
> > >
> > > I'm always puzzled why do we need a new compatible for the existing driver
> > > data? Is this hardware has an additional feature that driver does not (yet)
> > > implement?
> >
> > OPT3001 and OPT3004 sensors are functionally identical, and there are no
> > additional features in the OPT3004 that require separate handling in the driver.
> >
> > The new compatible string for the OPT3004 is being added, which will allow the
> > driver to recognize and support this sensor in the same way it handles the OPT3001.
> But why? I understand if you put two compatible strings into the DT to make it
> explicit in case of the future developments of the driver, but new compatible
> in the driver makes only sense when you have either quirk(s) or feature(s) that
> are different to the existing code. Since you haven't added either, what's the
> point?

Understood.

I also found a similar case with the ADXL346, which is identical to the ADXL345.
In the mainline kernel, a compatible string was added as a fallback in the bindings
but was not added to the driver itself.

Thanks for the insight.

In the next version, I will drop this patch and only submit the bindings for the OPT3004.
using the fallback mechanism.

Best Regards,
Hardev

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Andy Shevchenko]

On Thu, Dec 26, 2024 at 06:14:59AM +0000, Hardevsinh Palaniya wrote:
> > On Wed, Dec 25, 2024 at 09:56:36AM +0000, Hardevsinh Palaniya wrote:
> > > > On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:

...

> > > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > > the OPT3001. This Adds the compatible string for OPT3004, enabling
> > > > > the driver to support it without any functional changes.
> > > > >
> > > > > Datasheet: https://www.ti.com/lit/gpn/opt3004
> > > >
> > > > >
> > > >
> > > > This blank line is not needed.

> Regarding the second comment:
> The blank line was added to differentiate between the commit message and the
> SoB tag. Are you sure it should be removed?

If I understood the intention properly the Datasheet is supposed to be a tag,
so there shouldn't be blank lines among the tags in the tag block. With that
I truly think you should remove that blank line.

...

> > > > >  static const struct of_device_id opt3001_of_match[] = {
> > > > >       { .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> > > > >       { .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> > > > > +     { .compatible = "ti,opt3004", .data = &opt3001_chip_information },
> > > > >       { }
> > > > >  };
> > > >
> > > > I'm always puzzled why do we need a new compatible for the existing driver
> > > > data? Is this hardware has an additional feature that driver does not (yet)
> > > > implement?
> > >
> > > OPT3001 and OPT3004 sensors are functionally identical, and there are no
> > > additional features in the OPT3004 that require separate handling in the driver.
> > >
> > > The new compatible string for the OPT3004 is being added, which will allow the
> > > driver to recognize and support this sensor in the same way it handles the OPT3001.
> > But why? I understand if you put two compatible strings into the DT to make it
> > explicit in case of the future developments of the driver, but new compatible
> > in the driver makes only sense when you have either quirk(s) or feature(s) that
> > are different to the existing code. Since you haven't added either, what's the
> > point?
> 
> Understood.
> 
> I also found a similar case with the ADXL346, which is identical to the ADXL345.
> In the mainline kernel, a compatible string was added as a fallback in the bindings
> but was not added to the driver itself.
> 
> Thanks for the insight.
> 
> In the next version, I will drop this patch and only submit the bindings for the OPT3004.
> using the fallback mechanism.

But the kernel help test is good to have been updated, as well as having the
Datasheet link to be in the Git history (kinda documented).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light sensor

[Jonathan Cameron]

On Thu, 26 Dec 2024 06:14:59 +0000
Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io> wrote:

> Hi Andy
> 
> > On Wed, Dec 25, 2024 at 09:56:36AM +0000, Hardevsinh Palaniya wrote:  
> > > > On Tue, Dec 24, 2024 at 11:43:16AM +0530, Hardevsinh Palaniya wrote:  
> >  
> 
> ...
> 
> > > > > Add Support for OPT3004 Digital ambient light sensor (ALS) with
> > > > > increased angular IR rejection  
> > > >
> > > > Missing period here.  
> >  
> > > > > The OPT3004 sensor shares the same functionality and scale range as
> > > > > the OPT3001. This Adds the compatible string for OPT3004, enabling
> > > > > the driver to support it without any functional changes.
> > > > >
> > > > > Datasheet: https://www.ti.com/lit/gpn/opt3004  
> > > >  
> > > > >  
> > > >
> > > > This blank line is not needed.  
> > 
> > You left two above comments unanswered while Acking the rest, it's a bit confusing.
> > Are you agree on them or not?  
> 
> Apologies for overlooking those comments. They seemed straightforward, so I
> assumed your review was accurate, and I planned to address them directly in the
> next version without explicitly responding.
> 
> Regarding the second comment:
> The blank line was added to differentiate between the commit message and the
> SoB tag. Are you sure it should be removed?
> 
> ...
> 
> > > > >  static const struct of_device_id opt3001_of_match[] = {
> > > > >       { .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> > > > >       { .compatible = "ti,opt3002", .data = &opt3002_chip_information },
> > > > > +     { .compatible = "ti,opt3004", .data = &opt3001_chip_information },
> > > > >       { }
> > > > >  };  
> > > >
> > > > I'm always puzzled why do we need a new compatible for the existing driver
> > > > data? Is this hardware has an additional feature that driver does not (yet)
> > > > implement?  
> > >
> > > OPT3001 and OPT3004 sensors are functionally identical, and there are no
> > > additional features in the OPT3004 that require separate handling in the driver.
> > >
> > > The new compatible string for the OPT3004 is being added, which will allow the
> > > driver to recognize and support this sensor in the same way it handles the OPT3001.  
> > But why? I understand if you put two compatible strings into the DT to make it
> > explicit in case of the future developments of the driver, but new compatible
> > in the driver makes only sense when you have either quirk(s) or feature(s) that
> > are different to the existing code. Since you haven't added either, what's the
> > point?  
> 
> Understood.
> 
> I also found a similar case with the ADXL346, which is identical to the ADXL345.
> In the mainline kernel, a compatible string was added as a fallback in the bindings
> but was not added to the driver itself.

There is a small difference. The ADXL346 at least has a different ID from the ADXL345.
The driver may sanity check that. It shouldn't reject an ID it doesn't recognise but
it may well print a message as sometimes this indicates a wrong DT.

The ADXL346 also has additional registers, so whilst it is backwards compatible with
the ADXL345 additional features may be support in future that need the distinction
to be in DT.

Jonathan


Jonathan

> 
> Thanks for the insight.
> 
> In the next version, I will drop this patch and only submit the bindings for the OPT3004.
> using the fallback mechanism.
> 
> Best Regards,
> Hardev