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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PN3P287MB2845E6B3A390BE3741CFBF13FF0D2@PN3P287MB2845.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 26 Dec 2024 06:14:59 +0000
From: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "jic23@...nel.org" <jic23@...nel.org>, Lars-Peter Clausen
	<lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Emil Gedenryd
	<emil.gedenryd@...s.com>, Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Arthur Becker <arthur.becker@...tec.com>, Mudit Sharma
	<muditsharma.info@...il.com>, Subhajit Ghosh <subhajit.ghosh@...aklogic.com>,
	Julien Stephan <jstephan@...libre.com>, Uwe Kleine-König
	<u.kleine-koenig@...gutronix.de>, Andreas Dannenberg <dannenberg@...com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: light: opt3001: Add Support for opt3004 light
 sensor

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