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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <40c9c996-94a2-4c1b-ac18-c75bebd1b3f2@pereznus.es>
Date: Thu, 20 Mar 2025 15:32:40 +0100
From: Sergio Pérez <sergio@...eznus.es>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Tomasz Duszynski <tduszyns@...il.com>, Jonathan Cameron
 <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>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: iio: light: bh1750: Add reset-gpios
 property


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@...eznus.es>
>>>> ---
>>>>    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
>