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
