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

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

[Sergio Perez]

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@pereznus.es>
---
 Documentation/devicetree/bindings/iio/light/bh1750.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/bh1750.yaml b/Documentation/devicetree/bindings/iio/light/bh1750.yaml
index 1a88b3c253d5..f7a8dcd7d2a1 100644
--- a/Documentation/devicetree/bindings/iio/light/bh1750.yaml
+++ b/Documentation/devicetree/bindings/iio/light/bh1750.yaml
@@ -24,6 +24,10 @@ properties:
   reg:
     maxItems: 1
 
+  reset-gpios:
+    description: GPIO connected to the sensor's reset line (active low)
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -39,6 +43,7 @@ examples:
       light-sensor@23 {
         compatible = "rohm,bh1750";
         reg = <0x23>;
+        reset-gpios = <&gpio2 17 0>;
       };
     };
 
-- 
2.43.0

[PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

[Sergio Perez]

Some BH1750 sensors require a hardware reset before they can be
detected on the I2C bus. This implementation adds support for an
optional reset GPIO that can be specified in the device tree.

The reset sequence pulls the GPIO low and then high before initializing
the sensor, which enables proper detection with tools like i2cdetect.
This is particularly important for sensors that power on in an
undefined state.

Signed-off-by: Sergio Perez <sergio@pereznus.es>
---
 drivers/iio/light/bh1750.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
index 4b869fa9e5b1..1852467e96cf 100644
--- a/drivers/iio/light/bh1750.c
+++ b/drivers/iio/light/bh1750.c
@@ -22,12 +22,16 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
 
 #define BH1750_POWER_DOWN		0x00
 #define BH1750_ONE_TIME_H_RES_MODE	0x20 /* auto-mode for BH1721 */
 #define BH1750_CHANGE_INT_TIME_H_BIT	0x40
 #define BH1750_CHANGE_INT_TIME_L_BIT	0x60
 
+/* Define the reset delay time in microseconds */
+#define BH1750_RESET_DELAY_US 10000 /* 10ms */
+
 enum {
 	BH1710,
 	BH1721,
@@ -40,6 +44,7 @@ struct bh1750_data {
 	struct mutex lock;
 	const struct bh1750_chip_info *chip_info;
 	u16 mtreg;
+	struct gpio_desc *reset_gpio;
 };
 
 struct bh1750_chip_info {
@@ -248,6 +253,24 @@ static int bh1750_probe(struct i2c_client *client)
 	data->client = client;
 	data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
 
+	/* Get reset GPIO from device tree */
+	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
+									"reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpio))
+		return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
+							"Failed to get reset GPIO\n");
+
+	/* Perform hardware reset if GPIO is provided */
+	if (data->reset_gpio) {
+		/* Perform reset sequence: low-high */
+		gpiod_set_value_cansleep(data->reset_gpio, 0);
+		fsleep(BH1750_RESET_DELAY_US);
+		gpiod_set_value_cansleep(data->reset_gpio, 1);
+		fsleep(BH1750_RESET_DELAY_US);
+
+		dev_dbg(&client->dev, "BH1750 reset completed via GPIO\n");
+	}
+
 	usec = data->chip_info->mtreg_to_usec * data->chip_info->mtreg_default;
 	ret = bh1750_change_int_time(data, usec);
 	if (ret < 0)
-- 
2.43.0

Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

[Krzysztof Kozlowski]

On 19/03/2025 17:11, Sergio Perez wrote:
>  struct bh1750_chip_info {
> @@ -248,6 +253,24 @@ static int bh1750_probe(struct i2c_client *client)
>  	data->client = client;
>  	data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
>  
> +	/* Get reset GPIO from device tree */
> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
> +									"reset", GPIOD_OUT_HIGH);


Mess indentation.

> +	if (IS_ERR(data->reset_gpio))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
> +							"Failed to get reset GPIO\n");
> +
> +	/* Perform hardware reset if GPIO is provided */
> +	if (data->reset_gpio) {
> +		/* Perform reset sequence: low-high */
> +		gpiod_set_value_cansleep(data->reset_gpio, 0);
> +		fsleep(BH1750_RESET_DELAY_US);
> +		gpiod_set_value_cansleep(data->reset_gpio, 1);


So you keep device at reset state. This wasn't tested or your DTS is wrong.

I expect to acknowledge/respond to each of this comments above. Next
version, which is supposed to be v5, should fix them.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

[Sergio Pérez]

El 19/03/2025 a las 20:15, Krzysztof Kozlowski escribió:
> On 19/03/2025 17:11, Sergio Perez wrote:
>>   struct bh1750_chip_info {
>> @@ -248,6 +253,24 @@ static int bh1750_probe(struct i2c_client *client)
>>   	data->client = client;
>>   	data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
>>   
>> +	/* Get reset GPIO from device tree */
>> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
>> +									"reset", GPIOD_OUT_HIGH);
>
> Mess indentation.
Regarding indentation, I'll fix it in the next version to ensure 
consistency with kernel style guidelines.
>
>> +	if (IS_ERR(data->reset_gpio))
>> +		return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
>> +							"Failed to get reset GPIO\n");
>> +
>> +	/* Perform hardware reset if GPIO is provided */
>> +	if (data->reset_gpio) {
>> +		/* Perform reset sequence: low-high */
>> +		gpiod_set_value_cansleep(data->reset_gpio, 0);
>> +		fsleep(BH1750_RESET_DELAY_US);
>> +		gpiod_set_value_cansleep(data->reset_gpio, 1);
>
> So you keep device at reset state. This wasn't tested or your DTS is wrong.
The BH1750 reset pin (DVI) is "active low", meaning the device is in 
reset state when the pin is at 0V. When the pin is at high level, the 
device exits reset and operates normally.

According to the datasheet (can provide upon request), the reset 
sequence should:
1. Pull the reset pin low to enter reset state
2. Wait (minimum 1µs, I use 10ms to be safe)
3. Pull the reset pin high to exit reset state
4. Leave the pin high for normal operation

My implementation follows this exact sequence, so the device is NOT left 
in reset state. The initialization code:
1. Sets the pin to 0 (device enters reset)
2. Waits
3. Sets the pin to 1 (device exits reset)
4. Leaves it at 1, which is the normal operating state

I've modified the YAML description to remove "active low" to avoid 
confusion, as the implementation is correct for this hardware.
>
> I expect to acknowledge/respond to each of this comments above. Next
> version, which is supposed to be v5, should fix them.
>
> Best regards,
> Krzysztof

Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

[Krzysztof Kozlowski]

On Wed, Mar 19, 2025 at 11:40:27PM +0100, Sergio Pérez wrote:
> 
> El 19/03/2025 a las 20:15, Krzysztof Kozlowski escribió:
> > On 19/03/2025 17:11, Sergio Perez wrote:
> > >   struct bh1750_chip_info {
> > > @@ -248,6 +253,24 @@ static int bh1750_probe(struct i2c_client *client)
> > >   	data->client = client;
> > >   	data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
> > > +	/* Get reset GPIO from device tree */
> > > +	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
> > > +									"reset", GPIOD_OUT_HIGH);
> > 
> > Mess indentation.
> Regarding indentation, I'll fix it in the next version to ensure consistency
> with kernel style guidelines.
> > 
> > > +	if (IS_ERR(data->reset_gpio))
> > > +		return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
> > > +							"Failed to get reset GPIO\n");
> > > +
> > > +	/* Perform hardware reset if GPIO is provided */
> > > +	if (data->reset_gpio) {
> > > +		/* Perform reset sequence: low-high */
> > > +		gpiod_set_value_cansleep(data->reset_gpio, 0);
> > > +		fsleep(BH1750_RESET_DELAY_US);
> > > +		gpiod_set_value_cansleep(data->reset_gpio, 1);
> > 
> > So you keep device at reset state. This wasn't tested or your DTS is wrong.
> The BH1750 reset pin (DVI) is "active low", meaning the device is in reset
> state when the pin is at 0V. When the pin is at high level, the device exits
> reset and operates normally.

I read this after responding to your binding change, so this confirms
what I saw in datasheet and is contradictory to your response to the
binding.

First,  your binding should say which pin it is in the description.

Second, it is active low...

> 
> According to the datasheet (can provide upon request), the reset sequence
> should:
> 1. Pull the reset pin low to enter reset state
> 2. Wait (minimum 1µs, I use 10ms to be safe)
> 3. Pull the reset pin high to exit reset state
> 4. Leave the pin high for normal operation
> 
> My implementation follows this exact sequence, so the device is NOT left in
> reset state. The initialization code:
> 1. Sets the pin to 0 (device enters reset)

I don't think you get how GPIOs work. 0 means logical zero, so GPIO is
not active, not the actual signal level.

> 2. Waits
> 3. Sets the pin to 1 (device exits reset)
> 4. Leaves it at 1, which is the normal operating state
> 
> I've modified the YAML description to remove "active low" to avoid
> confusion, as the implementation is correct for this hardware.

You have wrong implementation.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

[Sergio Pérez]

El 20/03/2025 a las 9:55, Krzysztof Kozlowski escribió:
> On Wed, Mar 19, 2025 at 11:40:27PM +0100, Sergio Pérez wrote:
>> El 19/03/2025 a las 20:15, Krzysztof Kozlowski escribió:
>>> On 19/03/2025 17:11, Sergio Perez wrote:
>>>>    struct bh1750_chip_info {
>>>> @@ -248,6 +253,24 @@ static int bh1750_probe(struct i2c_client *client)
>>>>    	data->client = client;
>>>>    	data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
>>>> +	/* Get reset GPIO from device tree */
>>>> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
>>>> +									"reset", GPIOD_OUT_HIGH);
>>> Mess indentation.
>> Regarding indentation, I'll fix it in the next version to ensure consistency
>> with kernel style guidelines.
>>>> +	if (IS_ERR(data->reset_gpio))
>>>> +		return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
>>>> +							"Failed to get reset GPIO\n");
>>>> +
>>>> +	/* Perform hardware reset if GPIO is provided */
>>>> +	if (data->reset_gpio) {
>>>> +		/* Perform reset sequence: low-high */
>>>> +		gpiod_set_value_cansleep(data->reset_gpio, 0);
>>>> +		fsleep(BH1750_RESET_DELAY_US);
>>>> +		gpiod_set_value_cansleep(data->reset_gpio, 1);
>>> So you keep device at reset state. This wasn't tested or your DTS is wrong.
>> The BH1750 reset pin (DVI) is "active low", meaning the device is in reset
>> state when the pin is at 0V. When the pin is at high level, the device exits
>> reset and operates normally.
> I read this after responding to your binding change, so this confirms
> what I saw in datasheet and is contradictory to your response to the
> binding.
>
> First,  your binding should say which pin it is in the description.
>
> Second, it is active low...
That's right, it's commented on in the binding patch.
>> According to the datasheet (can provide upon request), the reset sequence
>> should:
>> 1. Pull the reset pin low to enter reset state
>> 2. Wait (minimum 1µs, I use 10ms to be safe)
>> 3. Pull the reset pin high to exit reset state
>> 4. Leave the pin high for normal operation
>>
>> My implementation follows this exact sequence, so the device is NOT left in
>> reset state. The initialization code:
>> 1. Sets the pin to 0 (device enters reset)
> I don't think you get how GPIOs work. 0 means logical zero, so GPIO is
> not active, not the actual signal level.
True, it's commented on in the binding patch.
>
>> 2. Waits
>> 3. Sets the pin to 1 (device exits reset)
>> 4. Leaves it at 1, which is the normal operating state
>>
>> I've modified the YAML description to remove "active low" to avoid
>> confusion, as the implementation is correct for this hardware.
> You have wrong implementation.
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

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@pereznus.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.

Best regards,
Krzysztof

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

[Sergio Pérez]

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@pereznus.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 *"

Regarding the binding I've modified for next v5 the YAML description to 
remove "active low" to avoid confusion and modified the example to:

examples:
   - |
     i2c {
       #address-cells = <1>;
       #size-cells = <0>;

       light-sensor@23 {
         compatible = "rohm,bh1750";
         reg = <0x23>;
         reset-gpios = <&gpio2 17 GPIO_ACTIVE_HIGH>;
       };
     };

That is the original version and is the configuration running on my machine.

> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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@pereznus.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.

Best regards,
Krzysztof

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@pereznus.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
>