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

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

[Quentin Schulz]

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.
This is eventually enforced by the Device Tree binding by making sure
both cannot be present at the same time.

Finally, the reset-gpios property is specific to PCA9670, PCA9671,
PCA9672 and PCA9673 so make it specific to those chips.

[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>
---
Quentin Schulz (2):
      dt-bindings: gpio: nxp,pcf8575: add reset GPIO
      gpio: pcf857x: add support for reset-gpios on (most) PCA967x

 .../devicetree/bindings/gpio/nxp,pcf8575.yaml      | 33 ++++++++++++++++++++++
 drivers/gpio/gpio-pcf857x.c                        | 29 +++++++++++++++++--
 2 files changed, 59 insertions(+), 3 deletions(-)
---
base-commit: 6537cfb395f352782918d8ee7b7f10ba2cc3cbf2
change-id: 20250219-pca976x-reset-driver-c9aa95869426

Best regards,
-- 
Quentin Schulz <quentin.schulz@cherry.de>

[PATCH 1/2] dt-bindings: gpio: nxp,pcf8575: add reset GPIO

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

A few of the I2C GPIO expander chips supported by this binding have a
RESETN pin to be able to reset the chip. The chip is held in reset while
the pin is low, therefore the polarity of reset-gpios is expected to
reflect that, i.e. a GPIO_ACTIVE_HIGH means the GPIO will be held low
for reset and released high, GPIO_ACTIVE_LOW means the GPIO will be held
high for reset and released low.

Out of the supported chips, only PCA9670, PCA9671, PCA9672 and PCA9673
show a RESETN pin in their datasheets. They all share the same reset
timings, that is 4+us reset pulse[0] and 100+us reset time[0].

When performing a reset, "The PCA9670 registers and I2C-bus state
machine will be held in their default state until the RESET input is
once again HIGH."[1] meaning we now know the state of each line
controlled by the GPIO expander. Therefore, setting lines-initial-states
and reset-gpios both does not make sense and their presence is XOR'ed.

[0] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf Fig 22.
[1] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf 8.5

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 .../devicetree/bindings/gpio/nxp,pcf8575.yaml      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
index 3718103e966a13e1d77f73335ff73c18a3199469..d08d3f848f82e74de949da16d26a810dc52a74e5 100644
--- a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
+++ b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
@@ -73,6 +73,39 @@ properties:
 
   wakeup-source: true
 
+  reset-gpios:
+    maxItems: 1
+    description:
+      GPIO controlling the (reset active LOW) RESET# pin.
+
+      Performing a reset makes all lines initialized to their input (pulled-up)
+      state.
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - nxp,pca9670
+                - nxp,pca9671
+                - nxp,pca9672
+                - nxp,pca9673
+    then:
+      properties:
+        reset-gpios: false
+
+  # lines-initial-states XOR reset-gpios
+  # Performing a reset reinitializes all lines to a known state which
+  # may not match passed lines-initial-states
+  - if:
+      required:
+        - lines-initial-states
+    then:
+      properties:
+        reset-gpios: false
+
 patternProperties:
   "^(.+-hog(-[0-9]+)?)$":
     type: object

-- 
2.48.1

Re: [PATCH 1/2] dt-bindings: gpio: nxp,pcf8575: add reset GPIO

[Laurent Pinchart]

Hi Quentin,

Thank you for the patch.

On Thu, Feb 20, 2025 at 10:56:51AM +0100, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> A few of the I2C GPIO expander chips supported by this binding have a
> RESETN pin to be able to reset the chip. The chip is held in reset while
> the pin is low, therefore the polarity of reset-gpios is expected to
> reflect that, i.e. a GPIO_ACTIVE_HIGH means the GPIO will be held low
> for reset and released high, GPIO_ACTIVE_LOW means the GPIO will be held
> high for reset and released low.

I think the convention in DT is the opposite. The DT property is
"reset-gpios", no "resetn-gpio", so the polarity should indicate how to
drive the GPIO to assert a logical "reset". GPIO_ACTIVE_LOW should mean
that the chip will be in reset when the physical GPIO is 0.

Could DT maintainers confirm this ?

> Out of the supported chips, only PCA9670, PCA9671, PCA9672 and PCA9673
> show a RESETN pin in their datasheets. They all share the same reset
> timings, that is 4+us reset pulse[0] and 100+us reset time[0].
> 
> When performing a reset, "The PCA9670 registers and I2C-bus state
> machine will be held in their default state until the RESET input is
> once again HIGH."[1] meaning we now know the state of each line
> controlled by the GPIO expander. Therefore, setting lines-initial-states
> and reset-gpios both does not make sense and their presence is XOR'ed.
> 
> [0] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf Fig 22.
> [1] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf 8.5
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  .../devicetree/bindings/gpio/nxp,pcf8575.yaml      | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
> index 3718103e966a13e1d77f73335ff73c18a3199469..d08d3f848f82e74de949da16d26a810dc52a74e5 100644
> --- a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
> +++ b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
> @@ -73,6 +73,39 @@ properties:
>  
>    wakeup-source: true
>  
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO controlling the (reset active LOW) RESET# pin.
> +
> +      Performing a reset makes all lines initialized to their input (pulled-up)
> +      state.
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              enum:
> +                - nxp,pca9670
> +                - nxp,pca9671
> +                - nxp,pca9672
> +                - nxp,pca9673
> +    then:
> +      properties:
> +        reset-gpios: false
> +
> +  # lines-initial-states XOR reset-gpios
> +  # Performing a reset reinitializes all lines to a known state which
> +  # may not match passed lines-initial-states
> +  - if:
> +      required:
> +        - lines-initial-states
> +    then:
> +      properties:
> +        reset-gpios: false
> +
>  patternProperties:
>    "^(.+-hog(-[0-9]+)?)$":
>      type: object
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: gpio: nxp,pcf8575: add reset GPIO

[Quentin Schulz]

Hi Laurent,

On 2/20/25 1:24 PM, Laurent Pinchart wrote:
> Hi Quentin,
> 
> Thank you for the patch.
> 
> On Thu, Feb 20, 2025 at 10:56:51AM +0100, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> A few of the I2C GPIO expander chips supported by this binding have a
>> RESETN pin to be able to reset the chip. The chip is held in reset while
>> the pin is low, therefore the polarity of reset-gpios is expected to
>> reflect that, i.e. a GPIO_ACTIVE_HIGH means the GPIO will be held low
>> for reset and released high, GPIO_ACTIVE_LOW means the GPIO will be held
>> high for reset and released low.
> 
> I think the convention in DT is the opposite. The DT property is
> "reset-gpios", no "resetn-gpio", so the polarity should indicate how to
> drive the GPIO to assert a logical "reset". GPIO_ACTIVE_LOW should mean
> that the chip will be in reset when the physical GPIO is 0.
> 

Oh boy. I actually meant the opposite. What a brain fart. You can see 
the implementation in the driver too, if I am not having a second brain 
fart, it should follow what you're saying. I activate/assert 
(GPIOD_OUT_HIGH) then release/deassert (gpiod_set_value(x, 0)), so if 
you have a line straight from the SoC to the RESETN pin, you'd need 
GPIO_ACTIVE_LOW in DT to model that.

The polarity of the line should match the reset state. I.e. if 
GPIO_ACTIVE_LOW for reset-gpio, it means the chip is in reset when the 
line is low. It exits reset when high.

I got confused by the GPIOD_OUT_HIGH flag I used in the driver to 
*assert* the reset, which is putting the line in logical high (or rather 
"active"), which is "drive low" for me on all my devices that'll make 
use of it (no inverter on the line, so RESETN meaning is kept, 0V = 
reset; I have GPIO_ACTIVE_LOW for my reset GPIO and it does reflect 
that, c.f. 
https://lore.kernel.org/linux-rockchip/20250220-ringneck-dtbos-v1-4-25c97f2385e6@cherry.de/T/#u).

Cheers,
Quentin

Re: [PATCH 1/2] dt-bindings: gpio: nxp,pcf8575: add reset GPIO

[Heiko Stübner]

Am Donnerstag, 20. Februar 2025, 10:56:51 MEZ schrieb Quentin Schulz:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> A few of the I2C GPIO expander chips supported by this binding have a
> RESETN pin to be able to reset the chip. The chip is held in reset while
> the pin is low, therefore the polarity of reset-gpios is expected to
> reflect that, i.e. a GPIO_ACTIVE_HIGH means the GPIO will be held low
> for reset and released high, GPIO_ACTIVE_LOW means the GPIO will be held
> high for reset and released low.
> 
> Out of the supported chips, only PCA9670, PCA9671, PCA9672 and PCA9673
> show a RESETN pin in their datasheets. They all share the same reset
> timings, that is 4+us reset pulse[0] and 100+us reset time[0].
> 
> When performing a reset, "The PCA9670 registers and I2C-bus state
> machine will be held in their default state until the RESET input is
> once again HIGH."[1] meaning we now know the state of each line
> controlled by the GPIO expander. Therefore, setting lines-initial-states
> and reset-gpios both does not make sense and their presence is XOR'ed.
> 
> [0] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf Fig 22.
> [1] https://www.nxp.com/docs/en/data-sheet/PCA9670.pdf 8.5
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  .../devicetree/bindings/gpio/nxp,pcf8575.yaml      | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
> index 3718103e966a13e1d77f73335ff73c18a3199469..d08d3f848f82e74de949da16d26a810dc52a74e5 100644
> --- a/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
> +++ b/Documentation/devicetree/bindings/gpio/nxp,pcf8575.yaml
[...]
> +  # lines-initial-states XOR reset-gpios
> +  # Performing a reset reinitializes all lines to a known state which
> +  # may not match passed lines-initial-states
> +  - if:
> +      required:
> +        - lines-initial-states
> +    then:
> +      properties:
> +        reset-gpios: false
> +

exclusion logic
Tested-by: Heiko Stuebner <heiko@sntech.de>

dtbscheck is happy when either reset-gpios or lines-initial-states is
present, but when both are present complains like

    rk3588-tiger-haikou-video-demo.dtb: gpio@27: reset-gpios: False schema does not allow [[205, 17, 1]]

as expected.

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

[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>
@@ -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);
+	}
+
 	/* NOTE:  the OnSemi jlc1562b is also largely compatible with
 	 * these parts, notably for output.  It has a low-resolution
 	 * DAC instead of pin change IRQs; and its inputs can be the

-- 
2.48.1

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

[Heiko Stübner]

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);
      |                 ^~~~~~~~~~~~~~~



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

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

[Quentin Schulz]

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

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

[Laurent Pinchart]

On Thu, Feb 20, 2025 at 01:13:06PM +0100, Quentin Schulz wrote:
> 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?

Possibly indirect includes that depend on your kernel config ? The above
functions and macros are declared and defined in linux/gpio/consumer.h,
so you should include it regardless.

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

I'd call it reset_gpio as in drivers we deal with logical signals. Up to
you.

> >>   	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");
> >> +	}

No need for curly braces.

> >> +
> >> +	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 */

Maybe use fsleep() for both ?

> >> +
> >> +		/*
> >> +		 * 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.

-- 
Regards,

Laurent Pinchart

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

[Bartosz Golaszewski]

On Thu, Feb 20, 2025 at 10:57 AM Quentin Schulz <foss+kernel@0leil.net> wrote:
>
> 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>
> @@ -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);

Should probably be gpiod_set_value_cansleep().

Bartosz

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

[Heiko Stübner]

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>

With the gpio-consumer fixed, reset-gpio handling works nicely
on my rk3588-tiger-haikou with the DSI display overlay, so

Tested-by: Heiko Stuebner <heiko@sntech.de>