| Message ID | 20250319161117.1780-2-sergio@pereznus.es (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/2] dt-bindings: iio: light: bh1750: Add reset-gpios property | expand |
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
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
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
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 >
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)
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(+)