Re: [PATCH 2/2] gpio: pcf857x: add support for reset-gpios on (most) PCA967x
From: Quentin Schulz
Date: Thu Feb 20 2025 - 07:13:31 EST
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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