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

["Sergio Pérez" <sergio@pereznus.es>]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]


El 18/03/2025 a las 17:23, Krzysztof Kozlowski escribió:
> On 18/03/2025 17:21, Krzysztof Kozlowski wrote:
>> On 18/03/2025 17:06, Sergio Pérez wrote:
>>> El 18/03/2025 a las 16:16, Krzysztof Kozlowski escribió:
>>>> On 18/03/2025 15:16, Sergio Pérez wrote:
>>>>> Hello,
>>>>>
>>>>> El 17/03/2025 a las 8:24, Krzysztof Kozlowski escribió:
>>>>>> On 16/03/2025 15:55, Sergio Perez wrote:
>>>>>>> Some BH1750 sensors require a hardware reset before they can be
>>>>>>> detected on the I2C bus. This patch 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.
>>>>>>>
>>>>>>> Update the devicetree binding documentation to include the new
>>>>>>> reset-gpios property with examples.
>>>>>>>
>>>>>>> Signed-off-by: Sergio Perez <sergio@pereznus.es>
>>>>>> Please run scripts/checkpatch.pl and fix reported warnings. After that,
>>>>>> run also `scripts/checkpatch.pl --strict` and (probably) fix more
>>>>>> warnings. Some warnings can be ignored, especially from --strict run,
>>>>>> but the code here looks like it needs a fix. Feel free to get in touch
>>>>>> if the warning is not clear.
>>>> You keep ignoring paragraphs. Did you read this?
>>> I pass this check several times and every time I do any step to make
>>> sure I am well.
>>>
>>> scripts/checkpatch.pl -f drivers/iio/light/bh1750.c
>>> total: 0 errors, 0 warnings, 354 lines checked
>>
>> That's not how you run checkpatch. Read the submitting patches. Just
>> like the name tells you, check the patch, you run it on the patch.
> BTW, I wonder which guideline told you to run it on the file? Because
> checkpatch description and submitting patches tell about running it on
> the patches, so I wonder where did you get suggestion to run it like that?
You're absolutely right. I misunderstood how to use checkpatch.pl and 
was incorrectly running it on the source files instead of the patch 
file. Thank you for pointing this out.

$ scripts/checkpatch.pl --strict -f 
0001-iio-light-bh1750-Add-hardware-reset-support-via-GPIO.patch
total: 0 errors, 0 warnings, 0 checks, 102 lines checked

0001-iio-light-bh1750-Add-hardware-reset-support-via-GPIO.patch has no 
obvious style problems and is ready for submission.

I've now run the tool correctly on my patch file and have fixed the 
identified issues:
- Removed trailing whitespace
- Fixed lines exceeding 79 characters
- Fixed the inconsistency between the description and example for 
reset-gpios
- Modified the existing example instead of adding a new one
- Ensured proper line endings and formatting
- Used proper get_maintainers.pl to include all recipients

Thank you for your patience as I learn the proper kernel contribution 
process.
>
> Best regards,
> Krzysztof

[-- Attachment #2: 0001-iio-light-bh1750-Add-hardware-reset-support-via-GPIO.patch --]
[-- Type: text/plain, Size: 3267 bytes --]

From 0b0f8f0f7988eccb6b4ed42984c5bde092b1d5fe Mon Sep 17 00:00:00 2001
From: Sergio Perez <sergio@pereznus.es>
Date: Tue, 18 Mar 2025 01:12:05 +0100
Subject: [PATCH] iio: light: bh1750: Add hardware reset support via GPIO

Some BH1750 sensors require a hardware reset before they can be
detected on the I2C bus. This patch 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.

Update the devicetree binding documentation to include the new
reset-gpios property with examples.

Signed-off-by: Sergio Perez <sergio@pereznus.es>
---
 .../devicetree/bindings/iio/light/bh1750.yaml |  5 ++++
 drivers/iio/light/bh1750.c                    | 23 +++++++++++++++++++
 2 files changed, 28 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>;
       };
     };

diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
index 4b869fa9e5b1..cfcce0c73aa2 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