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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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