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
