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

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: <6110750.alqRGMn8q6@diego>
Date: Thu, 20 Feb 2025 11:52:29 +0100
From: Heiko Stübner <heiko@...ech.de>
To: Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
 Quentin Schulz <foss+kernel@...il.net>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Quentin Schulz <quentin.schulz@...rry.de>
Subject:
 Re: [PATCH 2/2] gpio: pcf857x: add support for reset-gpios on (most) PCA967x

Am Donnerstag, 20. Februar 2025, 10:56:52 MEZ schrieb Quentin Schulz:
> From: Quentin Schulz <quentin.schulz@...rry.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@...rry.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);
      |                 ^~~~~~~~~~~~~~~



> @@ -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.


Heiko