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
