| Message ID | 20250316145514.627-1-sergio@pereznus.es (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: bh1750: Add hardware reset support via GPIO | expand |
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. <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. </form letter> > --- > .../devicetree/bindings/iio/light/bh1750.yaml | 20 +++- > drivers/iio/light/bh1750.c | 113 ++++++++++++------ ... and please go through your patch and see what happened there. > 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. > > 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>; > + }; > + }; > > -... > +... > \ No newline at end of file You have unrelated changed all over the place. Best regards, Krzysztof
On Sun, 16 Mar 2025 15:55:13 +0100 Sergio Perez <sergio@pereznus.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@pereznus.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. > + }; > + }; > > -... > +... > \ 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; > + } > +
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. 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>; + }; + }; -... +... \ 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> -#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 const struct bh1750_chip_info bh1750_chip_info_tbl[] = { - [BH1710] = { 140, 1022, 300, 400, 250000000, 2, 0x001F, 0x03E0 }, - [BH1721] = { 140, 1020, 300, 400, 250000000, 2, 0x0010, 0x03E0 }, - [BH1750] = { 31, 254, 69, 1740, 57500000, 1, 0x001F, 0x00E0 }, + [BH1710] = {140, 1022, 300, 400, 250000000, 2, 0x001F, 0x03E0}, + [BH1721] = {140, 1020, 300, 400, 250000000, 2, 0x0010, 0x03E0}, + [BH1750] = {31, 254, 69, 1740, 57500000, 1, 0x001F, 0x00E0}, }; +static int bh1750_reset(struct bh1750_data *data) +{ + if (!data->reset_gpio) + 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); + gpiod_set_value_cansleep(data->reset_gpio, 1); + usleep_range(BH1750_RESET_DELAY_US, BH1750_RESET_DELAY_US + 1000); + + dev_info(&data->client->dev, "BH1750 reset completed via GPIO\n"); + return 0; +} + static int bh1750_change_int_time(struct bh1750_data *data, int usec) { int ret; @@ -87,13 +108,13 @@ static int bh1750_change_int_time(struct bh1750_data *data, int usec) regval = (val & chip_info->int_time_high_mask) >> 5; ret = i2c_smbus_write_byte(data->client, - BH1750_CHANGE_INT_TIME_H_BIT | regval); + BH1750_CHANGE_INT_TIME_H_BIT | regval); if (ret < 0) return ret; regval = val & chip_info->int_time_low_mask; ret = i2c_smbus_write_byte(data->client, - BH1750_CHANGE_INT_TIME_L_BIT | regval); + BH1750_CHANGE_INT_TIME_L_BIT | regval); if (ret < 0) return ret; @@ -129,8 +150,8 @@ static int bh1750_read(struct bh1750_data *data, int *val) } static int bh1750_read_raw(struct iio_dev *indio_dev, - struct iio_chan_spec const *chan, - int *val, int *val2, long mask) + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) { int ret, tmp; struct bh1750_data *data = iio_priv(indio_dev); @@ -165,8 +186,8 @@ static int bh1750_read_raw(struct iio_dev *indio_dev, } static int bh1750_write_raw(struct iio_dev *indio_dev, - struct iio_chan_spec const *chan, - int val, int val2, long mask) + struct iio_chan_spec const *chan, + int val, int val2, long mask) { int ret; struct bh1750_data *data = iio_priv(indio_dev); @@ -186,7 +207,7 @@ static int bh1750_write_raw(struct iio_dev *indio_dev, } static ssize_t bh1750_show_int_time_available(struct device *dev, - struct device_attribute *attr, char *buf) + struct device_attribute *attr, char *buf) { int i; size_t len = 0; @@ -195,7 +216,7 @@ static ssize_t bh1750_show_int_time_available(struct device *dev, for (i = chip_info->mtreg_min; i <= chip_info->mtreg_max; i += chip_info->inc) len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06d ", - chip_info->mtreg_to_usec * i); + chip_info->mtreg_to_usec * i); buf[len - 1] = '\n'; @@ -220,13 +241,10 @@ static const struct iio_info bh1750_info = { }; static const struct iio_chan_spec bh1750_channels[] = { - { - .type = IIO_LIGHT, - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | - BIT(IIO_CHAN_INFO_SCALE) | - BIT(IIO_CHAN_INFO_INT_TIME) - } -}; + {.type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_INT_TIME)}}; static int bh1750_probe(struct i2c_client *client) { @@ -236,7 +254,7 @@ static int bh1750_probe(struct i2c_client *client) struct iio_dev *indio_dev; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C | - I2C_FUNC_SMBUS_WRITE_BYTE)) + I2C_FUNC_SMBUS_WRITE_BYTE)) return -EOPNOTSUPP; indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); @@ -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; + } + + if (data->reset_gpio) { + ret = bh1750_reset(data); + if (ret < 0) + return ret; + } + usec = data->chip_info->mtreg_to_usec * data->chip_info->mtreg_default; ret = bh1750_change_int_time(data, usec); if (ret < 0) @@ -295,23 +326,31 @@ static int bh1750_suspend(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(bh1750_pm_ops, bh1750_suspend, NULL); static const struct i2c_device_id bh1750_id[] = { - { "bh1710", BH1710 }, - { "bh1715", BH1750 }, - { "bh1721", BH1721 }, - { "bh1750", BH1750 }, - { "bh1751", BH1750 }, - { } -}; + {"bh1710", BH1710}, + {"bh1715", BH1750}, + {"bh1721", BH1721}, + {"bh1750", BH1750}, + {"bh1751", BH1750}, + {}}; MODULE_DEVICE_TABLE(i2c, bh1750_id); static const struct of_device_id bh1750_of_match[] = { - { .compatible = "rohm,bh1710", }, - { .compatible = "rohm,bh1715", }, - { .compatible = "rohm,bh1721", }, - { .compatible = "rohm,bh1750", }, - { .compatible = "rohm,bh1751", }, - { } -}; + { + .compatible = "rohm,bh1710", + }, + { + .compatible = "rohm,bh1715", + }, + { + .compatible = "rohm,bh1721", + }, + { + .compatible = "rohm,bh1750", + }, + { + .compatible = "rohm,bh1751", + }, + {}}; MODULE_DEVICE_TABLE(of, bh1750_of_match); static struct i2c_driver bh1750_driver = {
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 | 20 +++- drivers/iio/light/bh1750.c | 113 ++++++++++++------ 2 files changed, 95 insertions(+), 38 deletions(-)