Re: [PATCH 2/2] gpio: pcf857x: add support for reset-gpios on (most) PCA967x

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Heiko,

On 2/20/25 11:52 AM, Heiko Stübner wrote:
> Am Donnerstag, 20. Februar 2025, 10:56:52 MEZ schrieb Quentin Schulz:
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> The PCA9670, PCA9671, PCA9672 and PCA9673 all have a RESETN input pin
>> that is used to reset the I2C GPIO expander.
>>
>> One needs to hold this pin low for at least 4us and the reset should be
>> finished after about 100us according to the datasheet[1]. Once the reset
>> is done, the "registers and I2C-bus state machine will be held in their
>> default state until the RESET input is once again HIGH.".
>>
>> Because the logic is reset, the latch values eventually provided in the
>> Device Tree via lines-initial-states property are inapplicable so they
>> are simply ignored if a reset GPIO is provided.
>>
>> [1] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf 8.5 and fig 22.
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>> ---
>>   drivers/gpio/gpio-pcf857x.c | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>> index 7c57eaeb0afeba8953d998d8eec60a65b40efb6d..94077208e24ae99a1e8762e783f0eabc580fa520 100644
>> --- a/drivers/gpio/gpio-pcf857x.c
>> +++ b/drivers/gpio/gpio-pcf857x.c
>> @@ -5,6 +5,7 @@
>>    * Copyright (C) 2007 David Brownell
>>    */
>>   
>> +#include <linux/delay.h>
>>   #include <linux/gpio/driver.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
> 
> this is missing
> #include <linux/gpio/consumer.h>
> 
> because otherwise you end up with
> ../drivers/gpio/gpio-pcf857x.c: In function ‘pcf857x_probe’:
> ../drivers/gpio/gpio-pcf857x.c:300:21: error: implicit declaration of function ‘devm_gpiod_get_optional’; did you mean ‘devm_regulator_get_optional’? [-Wimplicit-function-declaration]
>    300 |         rstn_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>        |                     ^~~~~~~~~~~~~~~~~~~~~~~
>        |                     devm_regulator_get_optional
> ../drivers/gpio/gpio-pcf857x.c:300:68: error: ‘GPIOD_OUT_HIGH’ undeclared (first use in this function)
>    300 |         rstn_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>        |                                                                    ^~~~~~~~~~~~~~
> ../drivers/gpio/gpio-pcf857x.c:300:68: note: each undeclared identifier is reported only once for each function it appears in
> ../drivers/gpio/gpio-pcf857x.c:309:17: error: implicit declaration of function ‘gpiod_set_value’ [-Wimplicit-function-declaration]
>    309 |                 gpiod_set_value(rstn_gpio, 0);
>        |                 ^~~~~~~~~~~~~~~
> 

It compiles just fine on my end, this is all very suspicious.

GPIO_PCF857X symbol depends on GPIOLIB which builds this function.

Now, I have no clue how it finds the declaration for me without this 
include. Any clue?

> 
> 
>> @@ -272,12 +273,11 @@ static const struct irq_chip pcf857x_irq_chip = {
>>   
>>   static int pcf857x_probe(struct i2c_client *client)
>>   {
>> +	struct gpio_desc *rstn_gpio;
>>   	struct pcf857x *gpio;
>> -	unsigned int n_latch = 0;
>> +	unsigned int n_latch;
>>   	int status;
>>   
>> -	device_property_read_u32(&client->dev, "lines-initial-states", &n_latch);
>> -
>>   	/* Allocate, initialize, and register this gpio_chip. */
>>   	gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
>>   	if (!gpio)
>> @@ -297,6 +297,29 @@ static int pcf857x_probe(struct i2c_client *client)
>>   	gpio->chip.direction_output	= pcf857x_output;
>>   	gpio->chip.ngpio		= (uintptr_t)i2c_get_match_data(client);
>>   
>> +	rstn_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(rstn_gpio)) {
>> +		return dev_err_probe(&client->dev, PTR_ERR(rstn_gpio),
>> +				     "failed to get reset GPIO\n");
>> +	}
>> +
>> +	if (rstn_gpio) {
>> +		/* Reset already held with devm_gpiod_get_optional with GPIOD_OUT_HIGH */
>> +		usleep_range(4, 8); /* tw(rst) > 4us */
>> +		gpiod_set_value(rstn_gpio, 0);
>> +		usleep_range(100, 200); /* trst > 100uS */
>> +
>> +		/*
>> +		 * Reset "will initialize to their default states of all I/Os to
>> +		 * inputs with weak current source to VDD", which is the same as
>> +		 * writing 1 for all I/Os which is 0 in n_latch.
>> +		 */
>> +		n_latch = 0;
>> +	} else {
>> +		device_property_read_u32(&client->dev, "lines-initial-states",
>> +					 &n_latch);
> 
> device_property_read_u32 will not fill n_latch if the property is missing.
> Before n_latch was always set to 0 at the declaration point above.
> I guess that should be kept, because we want 0, except if
> device_property_read_u32 provides a different value.
> 

Yes, this was an oversight from me, will restore n_latch = 0 at the top 
of the function. Thanks for catching that.

Cheers,
Quentin