On Sun, 16 Mar 2025 15:55:13 +0100
Sergio Perez <sergio@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> .../devicetree/bindings/iio/light/bh1750.yaml | 20 +++-
> drivers/iio/light/bh1750.c | 113 ++++++++++++------
> 2 files changed, 95 insertions(+), 38 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/bh1750.yaml b/Documentation/devicetree/bindings/iio/light/bh1750.yaml
> index 1a88b3c253d5..d53b221eb84b 100644
> --- a/Documentation/devicetree/bindings/iio/light/bh1750.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/bh1750.yaml
> @@ -11,6 +11,9 @@ maintainers:
>
> description: |
> Ambient light sensor with an i2c interface.
> +
> + Some BH1750 sensors require a hardware reset before being properly detected
> + on the I2C bus. This can be done using the optional reset-gpios property.
I don't think this detail belongs here. In general we are going to reset
them all if we have the GPIO.
>
> properties:
> compatible:
> @@ -23,6 +26,10 @@ properties:
>
> reg:
> maxItems: 1
> +
> + reset-gpios:
> + description: GPIO connected to the sensor's reset line (active low)
> + maxItems: 1
>
> required:
> - compatible
> @@ -41,5 +48,16 @@ examples:
> reg = <0x23>;
> };
> };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + light-sensor@23 {
> + compatible = "rohm,bh1750";
> + reg = <0x23>;
> + reset-gpios = <&gpio2 17 GPIO_ACTIVE_HIGH>;
Add the GPIO to the existing example rather than having a new one.
> + };
> + };
>
> -...
> +...
> \ No newline at end of file
> diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> index 4b869fa9e5b1..53d64b70c03f 100644
> --- a/drivers/iio/light/bh1750.c
> +++ b/drivers/iio/light/bh1750.c
> @@ -22,11 +22,16 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
As already pointed out, there is a lot of accidental stuff in here.
I'll review again once that is sorted out. For now I'll ignore it.
If I weren't on a train and bored, I'd probably just have waited for v2.
>
> -#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 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,
> @@ -40,6 +45,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 {
> @@ -62,11 +68,26 @@ struct bh1750_chip_info {
>
> +static int bh1750_reset(struct bh1750_data *data)
> +{
> + if (!data->reset_gpio)
No need to check outside and in here.
> + return 0; /* No GPIO configured for reset, continue */
> +
> + /* Perform reset sequence: low-high */
> + gpiod_set_value_cansleep(data->reset_gpio, 0);
> + usleep_range(BH1750_RESET_DELAY_US, BH1750_RESET_DELAY_US + 1000);
fsleep for cases like this where is approximately but greater than X usecs.
> + gpiod_set_value_cansleep(data->reset_gpio, 1);
> + usleep_range(BH1750_RESET_DELAY_US, BH1750_RESET_DELAY_US + 1000);
fsleep
> +
> + dev_info(&data->client->dev, "BH1750 reset completed via GPIO\n");
Too noisy. dev_dbg at most for something like this.
> + return 0;
> +}
> @@ -248,6 +266,19 @@ static int bh1750_probe(struct i2c_client *client)
> data->client = client;
> data->chip_info = &bh1750_chip_info_tbl[id->driver_data];
>
> + data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(data->reset_gpio)) {
> + ret = PTR_ERR(data->reset_gpio);
> + dev_err(&client->dev, "Failed to get reset GPIO: %d\n", ret);
> + return ret;
Use return dev_err_probe(). In general good to have because of pretty printing
errors messages etc, but in this case you might get a deferral request and
that call adds a bunch of debug info for probe deferal.
> + }
> +
> + if (data->reset_gpio) {
> + ret = bh1750_reset(data);
There isn't a lot going on in that function, so I'd pull all the code down
here and not bother with a function at all.
> + if (ret < 0)
> + return ret;
> + }
> +
