| 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
| ||
|
Message-ID: <a1ee42f8-56c8-4a77-ab94-e0bb88476cbf@pereznus.es>
Date: Tue, 18 Mar 2025 15:35:29 +0100
From: Sergio Pérez <sergio@...eznus.es>
To: Jonathan Cameron <jic23@...nel.org>
Cc: tduszyns@...il.com, lars@...afoo.de, robh@...nel.org,
conor+dt@...nel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, krzk+dt@...nel.org
Subject: Re: [PATCH] iio: light: bh1750: Add hardware reset support via GPIO
El 17/03/2025 a las 12:58, Jonathan Cameron escribió:
> On Sun, 16 Mar 2025 15:55:13 +0100
> Sergio Perez <sergio@...eznus.es> 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@...eznus.es>
>> ---
>> .../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.
Yes, I am going to change it.
>> + };
>> + };
>>
>> -...
>> +...
>> \ 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;
>> + }
>> +
Thanks for the suggestions, I rewrote the code with all these points, I
passed the tests again and everything seems correct.
View attachment "0001-iio-light-bh1750-Add-hardware-reset-support-via-GPIO.patch" of type "text/plain" (3273 bytes)
Powered by blists - more mailing lists