Re: [PATCH v3 1/2] dt-bindings: iio: light: bh1750: Add reset-gpios property

[Sergio Pérez]

El 20/03/2025 a las 9:52, Krzysztof Kozlowski escribió:
> On Wed, Mar 19, 2025 at 11:38:09PM +0100, Sergio Pérez wrote:
> > El 19/03/2025 a las 20:12, Krzysztof Kozlowski escribió:
> > > On 19/03/2025 17:11, Sergio Perez wrote:
> > > > Some BH1750 sensors require a hardware reset via GPIO before they can
> > > > be properly detected on the I2C bus. Add a new reset-gpios property
> > > > to the binding to support this functionality.
> > > >
> > > > The reset-gpios property allows specifying a GPIO that will be toggled
> > > > during driver initialization to reset the sensor.
> > > >
> > > > Signed-off-by: Sergio Perez <sergio@xxxxxxxxxxx>
> > > > ---
> > > >    Documentation/devicetree/bindings/iio/light/bh1750.yaml | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > You just sent v3, while v4 was already on the lists, without improving
> > > and without responding to review.
> > >
> > > NAK.
> > >
> > > You keep repeating the same mistakes: not reading and responding
> > > feedback and it is getting tiresome.
> > I apologize for the confusion with patch versions. You're right that I sent
> > v3
> > after v4 was already on the list. I was trying to follow your exact
> > instructions from:
> > "git add ...
> > git commit --signed-off
> > git format-patch -v3 -2
> > scripts/chekpatch.pl v3*
> > scripts/get_maintainers.pl --no-git-fallback v3*
> > git send-email *"
> v3 stands for version of the patch, so my instruction shuld be here
> adjusted.
>
> > Regarding the binding I've modified for next v5 the YAML description to
> > remove "active low" to avoid confusion and modified the example to:
> So the signal is not active low? Are you really sure?
>
> Looking at BH1750FVI there is no reset signal in the first place...
> unless you mean this is DVI, but the description should then mention it.
>
> If this is DVI, then it is active low.

I apologize for the confusion. You're completely right, and I 
misunderstood how the GPIO flags work in the kernel. I've now corrected 
my implementation to properly handle the active-low reset pin.

Changes for v5:

1. In the binding YAML:
   - Updated description: "GPIO connected to the DVI reset pin (active 
low)"
   - Changed example to use GPIO_ACTIVE_LOW flag:
     reset-gpios = <&gpio2 17 GPIO_ACTIVE_LOW>;

2. In the driver code:
   - Corrected the reset sequence to properly handle active-low:
     ```
     /* Perform reset sequence: active-deactive */
     gpiod_set_value_cansleep(data->reset_gpio, 1); // Active reset 
(pin low)
     fsleep(BH1750_RESET_DELAY_US);
     gpiod_set_value_cansleep(data->reset_gpio, 0); // Deactivate reset 
(pin high)
     fsleep(BH1750_RESET_DELAY_US);
     ```
   - Fixed indentation issues

With these changes, the reset sequence correctly follows the datasheet 
requirements: pull the DVI pin low to reset, wait, then pull it high to 
resume normal operation.

Thank you for your patience and guidance on this.

> Best regards,
> Krzysztof