linux-kernel - Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via GPIO

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e4ffd13f-1452-4f18-8d80-f63b391f2545@pereznus.es>
Date: Wed, 19 Mar 2025 23:40:27 +0100
From: Sergio Pérez <sergio@...eznus.es>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Tomasz Duszynski <tduszyns@...il.com>, Jonathan Cameron <jic23@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: light: bh1750: Add hardware reset support via
 GPIO


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