* [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
@ 2026-09-17 7:56 ` Eliav Farber
2026-09-17 9:58 ` Bartosz Golaszewski
2026-09-18 7:50 ` Rodolfo Giometti
2026-09-17 7:56 ` [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
` (5 subsequent siblings)
6 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-17 7:56 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
On the two probe error paths that map and request the interrupt, probe
overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
-EPROBE_DEFER from the caller. The request_threaded_irq() failure message
also logged the IRQ number but not the errno.
Return the actual error code from both paths, and add the errno to the
request_threaded_irq() failure message.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v3:
- New patch, split out of the pinctrl change: while converting the probe
error paths to a goto, Takashi Sakamoto noted that the hardcoded -EINVAL
discards the real gpiod_to_irq()/request_threaded_irq() error, so fix
that separately first
drivers/pps/clients/pps-gpio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 73ec2c7335e5..038c55c5f7d4 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -165,7 +165,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
ret = gpiod_to_irq(data->gpio_pin);
if (ret < 0) {
dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return -EINVAL;
+ return ret;
}
data->irq = ret;
@@ -197,8 +197,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
- return -EINVAL;
+ dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
+ return ret;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes
2026-09-17 7:56 ` [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-17 9:58 ` Bartosz Golaszewski
2026-09-18 7:50 ` Rodolfo Giometti
1 sibling, 0 replies; 57+ messages in thread
From: Bartosz Golaszewski @ 2026-09-17 9:58 UTC (permalink / raw)
To: Eliav Farber
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel,
Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
On Thu, 17 Sep 2026 09:56:09 +0200, Eliav Farber <farbere@amazon.com> said:
> On the two probe error paths that map and request the interrupt, probe
> overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
> hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
> -EPROBE_DEFER from the caller. The request_threaded_irq() failure message
> also logged the IRQ number but not the errno.
>
> Return the actual error code from both paths, and add the errno to the
> request_threaded_irq() failure message.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes
2026-09-17 7:56 ` [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-09-17 9:58 ` Bartosz Golaszewski
@ 2026-09-18 7:50 ` Rodolfo Giometti
1 sibling, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-18 7:50 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Thu, Sep 17, 2026 at 07:56:09AM +0000, Eliav Farber wrote:
> Return the actual error code from both paths, and add the errno to the
> request_threaded_irq() failure message.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 73ec2c7335e5..038c55c5f7d4 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -165,7 +165,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
> ret = gpiod_to_irq(data->gpio_pin);
> if (ret < 0) {
> dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
> - return -EINVAL;
> + return ret;
> }
> data->irq = ret;
>
> @@ -197,8 +197,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
> data->info.name, data);
> if (ret) {
> pps_unregister_source(data->pps);
> - dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
> - return -EINVAL;
> + dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
> + return ret;
> }
>
> dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
Looks good to me.
Since this restores the -EPROBE_DEFER that gpiod_to_irq() can return,
which the hardcoded -EINVAL was turning into a permanent probe failure,
it is probably worth adding:
Fixes: 4461d65176b4 ("pps: descriptor-based gpio")
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
2026-09-17 7:56 ` [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-17 7:56 ` Eliav Farber
2026-09-17 15:35 ` Rob Herring (Arm)
` (2 more replies)
2026-09-17 7:56 ` [PATCH v3 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
` (4 subsequent siblings)
6 siblings, 3 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-17 7:56 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
When the PPS input GPIO is routed through a pin controller, a board may
need to mux those pins to a different function while pps-gpio is not
driving PPS (for example after the driver is unbound or across a kexec).
Document the optional "default" and "inactive" pinctrl-names and show
both in the example. The "default" state selects the PPS/GPIO function
and is applied by the driver core before probe; the optional "inactive"
state, when present, describes the mux to restore when the driver is
unbound or the system is shut down. The driver looks the states up by
name, so "inactive" may appear in any position; it only requires that a
"default" state also exists.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v3:
- Do not constrain pinctrl-names to a fixed ["default", "inactive"]
tuple. The driver looks the states up by name, so "inactive" may
appear in any position and other states may coexist; only require
(via "contains") that a "default" state exists, and reword the
description accordingly
Changes in v2:
- Rename the released state from "idle" to "inactive"
.../devicetree/bindings/pps/pps-gpio.yaml | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
index 383a838744eb..db6ecb17cb54 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
@@ -28,6 +28,19 @@ properties:
description: Indicates a falling edge assert, when present. Rising edge if absent.
type: boolean
+ pinctrl-names:
+ description:
+ When the PPS input is muxed through a pin controller, the standard
+ "default" state selects the PPS/GPIO function and is applied by the
+ driver core before probe. If a state named "inactive" is also present,
+ it is selected when the driver is unbound or the system is shut down,
+ handing the pins back to their alternate function. The "inactive"
+ state, if used, requires a "default" state; its position among the
+ names does not matter.
+ minItems: 1
+ contains:
+ const: default
+
required:
- compatible
- gpios
@@ -40,8 +53,9 @@ examples:
pps {
compatible = "pps-gpio";
- pinctrl-names = "default";
+ pinctrl-names = "default", "inactive";
pinctrl-0 = <&pinctrl_pps>;
+ pinctrl-1 = <&pinctrl_pps_inactive>;
gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
assert-falling-edge;
echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-17 7:56 ` [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-17 15:35 ` Rob Herring (Arm)
2026-09-17 16:13 ` Rob Herring
2026-09-18 7:50 ` Rodolfo Giometti
2 siblings, 0 replies; 57+ messages in thread
From: Rob Herring (Arm) @ 2026-09-17 15:35 UTC (permalink / raw)
To: Eliav Farber
Cc: linux-gpio, Bartosz Golaszewski, devicetree, Andrew Morton,
Linus Walleij, Fabio Estevam, Takashi Sakamoto, linux-kernel,
Conor Dooley, Krzysztof Kozlowski, Rodolfo Giometti
On Thu, 17 Sep 2026 07:56:10 +0000, Eliav Farber wrote:
> When the PPS input GPIO is routed through a pin controller, a board may
> need to mux those pins to a different function while pps-gpio is not
> driving PPS (for example after the driver is unbound or across a kexec).
>
> Document the optional "default" and "inactive" pinctrl-names and show
> both in the example. The "default" state selects the PPS/GPIO function
> and is applied by the driver core before probe; the optional "inactive"
> state, when present, describes the mux to restore when the driver is
> unbound or the system is shut down. The driver looks the states up by
> name, so "inactive" may appear in any position; it only requires that a
> "default" state also exists.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> Changes in v3:
> - Do not constrain pinctrl-names to a fixed ["default", "inactive"]
> tuple. The driver looks the states up by name, so "inactive" may
> appear in any position and other states may coexist; only require
> (via "contains") that a "default" state exists, and reword the
> description accordingly
>
> Changes in v2:
> - Rename the released state from "idle" to "inactive"
>
> .../devicetree/bindings/pps/pps-gpio.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pps/pps-gpio.example.dtb: pps (pps-gpio): pinctrl-names: ['default', 'inactive'] is too long
from schema $id: http://devicetree.org/schemas/pps/pps-gpio.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.kernel.org/project/devicetree/patch/20260917075611.47881-3-farbere@amazon.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-17 7:56 ` [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
2026-09-17 15:35 ` Rob Herring (Arm)
@ 2026-09-17 16:13 ` Rob Herring
2026-09-18 7:50 ` Rodolfo Giometti
2 siblings, 0 replies; 57+ messages in thread
From: Rob Herring @ 2026-09-17 16:13 UTC (permalink / raw)
To: Eliav Farber
Cc: Rodolfo Giometti, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Thu, Sep 17, 2026 at 07:56:10AM +0000, Eliav Farber wrote:
> When the PPS input GPIO is routed through a pin controller, a board may
> need to mux those pins to a different function while pps-gpio is not
> driving PPS (for example after the driver is unbound or across a kexec).
>
> Document the optional "default" and "inactive" pinctrl-names and show
> both in the example. The "default" state selects the PPS/GPIO function
> and is applied by the driver core before probe; the optional "inactive"
> state, when present, describes the mux to restore when the driver is
> unbound or the system is shut down. The driver looks the states up by
> name, so "inactive" may appear in any position; it only requires that a
> "default" state also exists.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> Changes in v3:
> - Do not constrain pinctrl-names to a fixed ["default", "inactive"]
> tuple. The driver looks the states up by name, so "inactive" may
> appear in any position and other states may coexist; only require
> (via "contains") that a "default" state exists, and reword the
> description accordingly
>
> Changes in v2:
> - Rename the released state from "idle" to "inactive"
>
> .../devicetree/bindings/pps/pps-gpio.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> index 383a838744eb..db6ecb17cb54 100644
> --- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> @@ -28,6 +28,19 @@ properties:
> description: Indicates a falling edge assert, when present. Rising edge if absent.
> type: boolean
>
> + pinctrl-names:
> + description:
> + When the PPS input is muxed through a pin controller, the standard
> + "default" state selects the PPS/GPIO function and is applied by the
> + driver core before probe. If a state named "inactive" is also present,
> + it is selected when the driver is unbound or the system is shut down,
> + handing the pins back to their alternate function. The "inactive"
> + state, if used, requires a "default" state; its position among the
> + names does not matter.
Don't express in prose what can be defined in schema.
> + minItems: 1
> + contains:
> + const: default
minItems: 1
items:
- const: default
- const: inactive
And yes, position does matter. pinctrl-0 was already implicitly defined
as 'default'. You can't change it to pinctrl-1 now.
Rob
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-17 7:56 ` [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
2026-09-17 15:35 ` Rob Herring (Arm)
2026-09-17 16:13 ` Rob Herring
@ 2026-09-18 7:50 ` Rodolfo Giometti
2026-09-18 15:43 ` Rob Herring
2 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-18 7:50 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Thu, Sep 17, 2026 at 07:56:10AM +0000, Eliav Farber wrote:
> Document the optional "default" and "inactive" pinctrl-names and show
> both in the example.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> index 383a838744eb..db6ecb17cb54 100644
> --- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> @@ -28,6 +28,19 @@ properties:
> description: Indicates a falling edge assert, when present. Rising edge if absent.
> type: boolean
>
> + pinctrl-names:
> + description:
> + When the PPS input is muxed through a pin controller, the standard
> + "default" state selects the PPS/GPIO function and is applied by the
> + driver core before probe. If a state named "inactive" is also present,
> + it is selected when the driver is unbound or the system is shut down,
> + handing the pins back to their alternate function. The "inactive"
> + state, if used, requires a "default" state; its position among the
> + names does not matter.
> + minItems: 1
> + contains:
> + const: default
> +
> required:
> - compatible
> - gpios
> @@ -40,8 +53,9 @@ examples:
>
> pps {
> compatible = "pps-gpio";
> - pinctrl-names = "default";
> + pinctrl-names = "default", "inactive";
> pinctrl-0 = <&pinctrl_pps>;
> + pinctrl-1 = <&pinctrl_pps_inactive>;
> gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
> assert-falling-edge;
> echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
I tried:
$ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/pps/pps-gpio.yaml \
dt_binding_check
and it gave me:
pps-gpio.example.dtb: pps (pps-gpio): pinctrl-names:
['default', 'inactive'] is too long
from schema $id: http://devicetree.org/schemas/pps/pps-gpio.yaml
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-18 7:50 ` Rodolfo Giometti
@ 2026-09-18 15:43 ` Rob Herring
0 siblings, 0 replies; 57+ messages in thread
From: Rob Herring @ 2026-09-18 15:43 UTC (permalink / raw)
To: Rodolfo Giometti
Cc: Eliav Farber, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Fri, Sep 18, 2026 at 09:50:06AM +0200, Rodolfo Giometti wrote:
> On Thu, Sep 17, 2026 at 07:56:10AM +0000, Eliav Farber wrote:
> > Document the optional "default" and "inactive" pinctrl-names and show
> > both in the example.
> >
> > Signed-off-by: Eliav Farber <farbere@amazon.com>
> > ---
> > diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> > index 383a838744eb..db6ecb17cb54 100644
> > --- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> > +++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
> > @@ -28,6 +28,19 @@ properties:
> > description: Indicates a falling edge assert, when present. Rising edge if absent.
> > type: boolean
> >
> > + pinctrl-names:
> > + description:
> > + When the PPS input is muxed through a pin controller, the standard
> > + "default" state selects the PPS/GPIO function and is applied by the
> > + driver core before probe. If a state named "inactive" is also present,
> > + it is selected when the driver is unbound or the system is shut down,
> > + handing the pins back to their alternate function. The "inactive"
> > + state, if used, requires a "default" state; its position among the
> > + names does not matter.
> > + minItems: 1
> > + contains:
> > + const: default
> > +
> > required:
> > - compatible
> > - gpios
> > @@ -40,8 +53,9 @@ examples:
> >
> > pps {
> > compatible = "pps-gpio";
> > - pinctrl-names = "default";
> > + pinctrl-names = "default", "inactive";
> > pinctrl-0 = <&pinctrl_pps>;
> > + pinctrl-1 = <&pinctrl_pps_inactive>;
> > gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
> > assert-falling-edge;
> > echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
>
> I tried:
>
> $ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/pps/pps-gpio.yaml \
> dt_binding_check
>
> and it gave me:
>
> pps-gpio.example.dtb: pps (pps-gpio): pinctrl-names:
> ['default', 'inactive'] is too long
> from schema $id: http://devicetree.org/schemas/pps/pps-gpio.yaml
I can't help you without the schema you used. Sounds like something
wrong with 'items'.
Rob
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
2026-09-17 7:56 ` [PATCH v3 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-09-17 7:56 ` [PATCH v3 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-17 7:56 ` Eliav Farber
2026-09-19 17:11 ` [PATCH v4 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
` (3 subsequent siblings)
6 siblings, 0 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-17 7:56 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and need to
mux it to another function when pps-gpio is not driving PPS. The driver
core applies the "default" pinctrl state before probe, so the pins are
muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
the pins back when the driver is unbound or the system is shut down, so
they stay stuck in the GPIO function for whatever runs next, kexec
included.
Look up an optional "inactive" pinctrl state in probe via
devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
pinctrl_select_state() in remove() and shutdown(). The state is looked up
and selected by the driver itself rather than reusing the runtime-PM
"idle"/"sleep" states, so its meaning is unambiguous and it does not
depend on CONFIG_PM. Boards that do not describe an "inactive" state are
unaffected.
Since "inactive" is only meaningful as the mux to restore after the
core-applied "default" state, reject an "inactive" state that is not
paired with a "default" one rather than releasing pins that were never
put into a defined PPS state.
The mux must not change while something can still drive the pins. On
shutdown() the requested IRQ and the echo timer would otherwise outlive
the mux change -- device_shutdown() is not the end of the road, the
kernel keeps running to load and start the kexec image -- so a timer
callback or the PPS handler could poke a line that by then belongs to
another function. Tear down in the same order as remove(): free_irq() and
timer_delete_sync() first, and the mux change last. shutdown() does not
unregister the PPS source, which is a remove-time concern.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v3:
- Treat -ENODEV from devm_pinctrl_get() (a DT device without a
"pinctrl-0" property) as "no pinctrl", not a probe failure; keep
propagating everything else, e.g. -EPROBE_DEFER
- Restore the "inactive" mux on probe failure too: route the error
paths after pps_gpio_get_pins() through a new err_release_pins label
so a failed probe does not leave the pins stuck in the "default" state
- Warn if pinctrl_select_state() fails to apply the "inactive" state
rather than silently ignoring the error (pps_gpio_release_pins() now
takes the struct device to log against)
- Trim and de-duplicate the added comments
Changes in v2:
- Rename the released state from "idle" to "inactive"
- Look the state up in the driver with devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state() instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency
- Fix shutdown() to free_irq() and timer_delete_sync() before the mux
change, matching remove(), so no IRQ or timer callback can drive a pin
after it has been handed back
- Require a "default" state whenever "inactive" is present and reject the
mismatch
drivers/pps/clients/pps-gpio.c | 100 ++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 038c55c5f7d4..2ee8fba8520c 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/pps_kernel.h>
#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/list.h>
#include <linux/property.h>
#include <linux/timer.h>
@@ -30,6 +31,8 @@ struct pps_gpio_device_data {
struct gpio_desc *gpio_pin; /* GPIO port descriptors */
struct gpio_desc *echo_pin;
struct timer_list echo_timer; /* timer to reset echo active state */
+ struct pinctrl *pinctrl; /* pin control handle */
+ struct pinctrl_state *pins_inactive; /* pins released when unbound */
bool assert_falling_edge;
unsigned int echo_active_ms; /* PPS echo active duration */
unsigned long echo_timeout; /* timer timeout value in jiffies */
@@ -96,6 +99,66 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
gpiod_set_value(info->echo_pin, 0);
}
+/*
+ * Look up the optional "inactive" pinctrl state. It requires a "default"
+ * state (applied by the driver core before probe) and is rejected without
+ * one. Absent pinctrl, or an absent "inactive" state, is not an error.
+ */
+static int pps_gpio_get_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ struct pinctrl_state *pins_default;
+
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ /*
+ * A DT device without "pinctrl-0" yields -ENODEV, which
+ * is not an error here; propagate anything else.
+ */
+ if (PTR_ERR(data->pinctrl) == -ENODEV) {
+ data->pinctrl = NULL;
+ return 0;
+ }
+ return dev_err_probe(dev, PTR_ERR(data->pinctrl),
+ "failed to get pinctrl\n");
+ }
+
+ /* The "inactive" state is optional. */
+ data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
+ if (IS_ERR(data->pins_inactive)) {
+ data->pins_inactive = NULL;
+ return 0;
+ }
+
+ /* "inactive" requires a "default" state to return from. */
+ pins_default = pinctrl_lookup_state(data->pinctrl, "default");
+ if (IS_ERR(pins_default))
+ return dev_err_probe(dev, PTR_ERR(pins_default),
+ "\"inactive\" pinctrl state requires a \"default\" state\n");
+
+ return 0;
+}
+
+/*
+ * Restore the "inactive" pinctrl state, handing the pins back to whatever
+ * function uses them while pps-gpio is not driving PPS. This undoes the
+ * "default" state the driver core applied before probe. A no-op for boards
+ * that describe no "inactive" state.
+ */
+static void pps_gpio_release_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ if (!data->pins_inactive)
+ return;
+
+ ret = pinctrl_select_state(data->pinctrl, data->pins_inactive);
+ if (ret)
+ dev_warn(dev, "failed to select inactive pinctrl state: %d\n",
+ ret);
+}
+
static int pps_gpio_setup(struct device *dev)
{
struct pps_gpio_device_data *data = dev_get_drvdata(dev);
@@ -161,11 +224,16 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (ret)
return ret;
+ /* pinctrl setup (optional states) */
+ ret = pps_gpio_get_pins(dev);
+ if (ret)
+ return ret;
+
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
if (ret < 0) {
dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return ret;
+ goto err_release_pins;
}
data->irq = ret;
@@ -187,7 +255,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (IS_ERR(data->pps)) {
dev_err(dev, "failed to register IRQ %d as PPS source\n",
data->irq);
- return PTR_ERR(data->pps);
+ ret = PTR_ERR(data->pps);
+ goto err_release_pins;
}
/* register IRQ interrupt handler */
@@ -198,13 +267,18 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (ret) {
pps_unregister_source(data->pps);
dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
- return ret;
+ goto err_release_pins;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
data->irq);
return 0;
+
+err_release_pins:
+ /* Restore the inactive mux on probe failure; safe to do last here. */
+ pps_gpio_release_pins(dev);
+ return ret;
}
static void pps_gpio_remove(struct platform_device *pdev)
@@ -216,9 +290,28 @@ static void pps_gpio_remove(struct platform_device *pdev)
timer_delete_sync(&data->echo_timer);
/* reset echo pin in any case */
gpiod_set_value(data->echo_pin, 0);
+ /* release the pins last, once nothing can drive them */
+ pps_gpio_release_pins(&pdev->dev);
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
+static void pps_gpio_shutdown(struct platform_device *pdev)
+{
+ struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+
+ /*
+ * The kernel keeps running after device_shutdown() (e.g. to load and
+ * start a kexec image), so quiesce the hardware before touching the
+ * mux: free the IRQ and stop the echo timer first, then release the
+ * pins last, so no callback can drive a pin after it is handed back.
+ * The PPS source is left registered; that is a remove-time concern.
+ */
+ free_irq(data->irq, data);
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ pps_gpio_release_pins(&pdev->dev);
+}
+
static const struct of_device_id pps_gpio_dt_ids[] = {
{ .compatible = "pps-gpio", },
{ /* sentinel */ }
@@ -228,6 +321,7 @@ MODULE_DEVICE_TABLE(of, pps_gpio_dt_ids);
static struct platform_driver pps_gpio_driver = {
.probe = pps_gpio_probe,
.remove = pps_gpio_remove,
+ .shutdown = pps_gpio_shutdown,
.driver = {
.name = PPS_GPIO_NAME,
.of_match_table = pps_gpio_dt_ids,
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v4 0/3] pps-gpio: restore pin mux on unbind and shutdown
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
` (2 preceding siblings ...)
2026-09-17 7:56 ` [PATCH v3 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-19 17:11 ` Eliav Farber
2026-09-22 7:40 ` Rodolfo Giometti
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
2026-09-19 17:11 ` [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
` (2 subsequent siblings)
6 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-19 17:11 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and mux the
pins to a different function when the pps-gpio driver is not active.
The driver core already selects the "default" pinctrl state before probe,
so the pins can be muxed for GPIO/PPS use while the driver is bound without
any driver change. Nothing, however, hands the pins back when the driver
is unbound or the system is shut down (for example before kexec), leaving
them stuck in the GPIO mux for the next kernel.
This series lets pps-gpio select an optional "inactive" pinctrl state in
remove() and shutdown(), so a board can describe the alternate mux there
and have it restored. The state is looked up and selected by the driver
itself (devm_pinctrl_get() + pinctrl_lookup_state() + pinctrl_select_
state()), so its meaning is unambiguous and it does not depend on
CONFIG_PM. It is a no-op for boards that do not describe an "inactive"
state.
Patch 1 is a small preparatory fix to propagate the gpiod_to_irq() and
request_threaded_irq() error codes (rather than a hardcoded -EINVAL) on
the probe error paths that patch 3 then converts to gotos. Patch 2
documents the optional "default"/"inactive" pinctrl-names in the binding;
patch 3 implements the driver side.
Tested on an AL11 K2V6 JRD10 board: binding/unbinding each pps-gpio device
toggles the corresponding PBS pin-mux register between the GPIO function
and the alternate ec_ptp_trigger_in function as expected, and re-binding
restores the GPIO function via the core-applied "default" state. Reading
the mux register with the devices left unbound confirms the "inactive"
mux persists. Also tested with a pps-gpio node that describes no pinctrl
at all, where probe, remove and shutdown behave as before.
Open question for the maintainers (patch 3): a probe that fails before
pps_gpio_get_pins() has run -- devm_kzalloc() or pps_gpio_setup() -- returns
without going through err_release_pins, so the pins are left in the
core-applied "default" state rather than "inactive". I did not change this
in v4 as I would like your guidance on the preferred approach; the trade-offs
are laid out at the end of patch 3's changelog. In short:
A. Move pps_gpio_get_pins() to the top of probe and route the
pps_gpio_setup() failure through err_release_pins too, so every path
the driver can act on restores "inactive". (The devm_kzalloc() failure
is inherently before the driver holds any pinctrl handle, so it cannot
be covered by the driver in any option.)
B. Keep v4 as-is and treat "inactive" as a successful-ownership concern:
a probe that never looked up the state never took the pins, so leaving
the core-applied "default" (long-standing behaviour) is acceptable.
C. As A, but keep the release helper's existing NULL guard so it is robust
regardless of ordering (belt and braces).
I lean towards B (the restore is meaningful only once the driver has taken
ownership), but I am happy to implement A/C if you prefer uniform failure
paths.
Changes in v4:
- Patch 1: add "Fixes: 161520451dfa (\"pps: new client driver using
GPIO\")" and Bartosz Golaszewski's Reviewed-by. See the reply on the v3
1/3 thread for why the tag points at the original driver rather than the
descriptor conversion (4461d65176b4) Rodolfo suggested: the hardcoded
-EINVAL on both error paths predates 4461d65176b4, which only switched
gpio_to_irq() to gpiod_to_irq() and left those returns as context
- Patch 2: rework the binding per Rob Herring. Do not express the
ordering in prose; use an ordered "items" list ("default" then
"inactive") with minItems: 1, since pinctrl-0 is already implicitly
"default" and its position is fixed. This also fixes the
"['default', 'inactive'] is too long" dt_binding_check error seen on v3.
Reword the commit message accordingly
- Patch 3: unchanged from v3
Changes in v3 (addressing Sashiko's and Rodolfo Giometti's
review):
- New preparatory patch 1: propagate the gpiod_to_irq() and
request_threaded_irq() error codes instead of overwriting them with
-EINVAL, and log the errno on the request_threaded_irq() failure. The
pinctrl patch only converts those returns into gotos, so fixing the
discarded errors separately keeps them out of the pinctrl change
- Do not constrain pinctrl-names to a fixed ["default", "inactive"] tuple
in the binding. The driver looks the states up by name, so "inactive"
may appear in any position and other states may coexist; the binding
now only requires that a "default" state exists
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl described" rather
than a probe failure. A DT device without a "pinctrl-0" property gets
-ENODEV from the pinctrl core; that is expected, not an error. Real
errors, including -EPROBE_DEFER, are still propagated
- Restore the "inactive" mux on probe failure too. The error paths after
pps_gpio_get_pins() now go through a new err_release_pins label, so a
probe that fails in gpiod_to_irq(), pps_register_source() or
request_threaded_irq() no longer leaves the pins stuck in the
core-applied "default" state
- Warn if applying the "inactive" state fails rather than ignoring the
pinctrl_select_state() return silently
- Trim and de-duplicate the comments added in v2
Changes in v2 (all addressing Rodolfo Giometti's review):
- Rename the released state from "idle" to "inactive". "idle" is the
runtime-PM state in pinctrl-state.h; overloading it for "driver not
active" would clash with any future runtime PM or .suspend() and was
being baked into the binding as ABI. "inactive" is not a well-known
state, so no generic PM helper will ever auto-select it -- the driver
drives it explicitly
- Look the state up in the driver (devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state()) instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency so a
CONFIG_PM=n kernel that describes an "inactive" state now honors it
instead of silently doing nothing
- Fix shutdown(): tear down in the same order as remove() -- free_irq()
and timer_delete_sync() first, the mux change last -- so no IRQ or echo
timer callback can drive a pin after it has been handed back to another
function; shutdown() no longer changes the mux while the hardware is
still live
- Require a "default" state whenever "inactive" is present and reject the
mismatch, rather than releasing pins that were never put into a defined
PPS state
Link: https://lore.kernel.org/all/20260916134744.46354-1-farbere@amazon.com/ [v1]
Link: https://lore.kernel.org/all/20260916182641.9768-1-farbere@amazon.com/ [v2]
Link: https://lore.kernel.org/all/20260917075611.47881-1-farbere@amazon.com/ [v3]
Eliav Farber (3):
pps: clients: gpio: propagate probe error codes
dt-bindings: pps: pps-gpio: document optional pinctrl states
pps: clients: gpio: release pins to an inactive state on remove and
shutdown
.../devicetree/bindings/pps/pps-gpio.yaml | 14 ++-
drivers/pps/clients/pps-gpio.c | 102 +++++++++++++++++-
2 files changed, 111 insertions(+), 5 deletions(-)
base-commit: 4982d3552a3bf94de503acf93433277d08421de6
--
2.47.3
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v4 0/3] pps-gpio: restore pin mux on unbind and shutdown
2026-09-19 17:11 ` [PATCH v4 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
@ 2026-09-22 7:40 ` Rodolfo Giometti
2026-09-22 10:08 ` Farber, Eliav
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
1 sibling, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 7:40 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Sat, Sep 19, 2026 at 05:11:54PM +0000, Eliav Farber wrote:
> Open question for the maintainers (patch 3): a probe that fails before
> pps_gpio_get_pins() has run -- devm_kzalloc() or pps_gpio_setup() -- returns
> without going through err_release_pins, so the pins are left in the
> core-applied "default" state rather than "inactive". I did not change this
> in v4 as I would like your guidance on the preferred approach; the trade-offs
> are laid out at the end of patch 3's changelog. In short:
>
> A. Move pps_gpio_get_pins() to the top of probe and route the
> pps_gpio_setup() failure through err_release_pins too, so every path
> the driver can act on restores "inactive". (The devm_kzalloc() failure
> is inherently before the driver holds any pinctrl handle, so it cannot
> be covered by the driver in any option.)
> B. Keep v4 as-is and treat "inactive" as a successful-ownership concern:
> a probe that never looked up the state never took the pins, so leaving
> the core-applied "default" (long-standing behaviour) is acceptable.
> C. As A, but keep the release helper's existing NULL guard so it is robust
> regardless of ordering (belt and braces).
>
> I lean towards B (the restore is meaningful only once the driver has taken
> ownership), but I am happy to implement A/C if you prefer uniform failure
> paths.
I would choose option A, keeping the NULL check in the release helper;
I believe this corresponds to your option C. In fact, to me, it
represents the symmetrical counterpart to what the core did on the
driver's behalf.
One thing I would verify on your board before proceeding is this: with
option A, an -EPROBE_DEFER error returned by pps_gpio_setup() would
result in the "inactive" state being selected, and the core would
re-apply "default" on the next attempt. I assume this mux switching is
harmless, but the hardware is available to you, not me.
I wouldn't worry too much about the devm_kzalloc() case.
I also have a couple of observations regarding patches 1/3 and 3/3; I
will send them directly in response to the patches themselves.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH v4 0/3] pps-gpio: restore pin mux on unbind and shutdown
2026-09-22 7:40 ` Rodolfo Giometti
@ 2026-09-22 10:08 ` Farber, Eliav
0 siblings, 0 replies; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 10:08 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Mon, Sep 22, 2026 at 09:40:00AM +0200, Rodolfo Giometti wrote:
> On Sat, Sep 19, 2026 at 05:11:54PM +0000, Eliav Farber wrote:
> > Open question for the maintainers (patch 3): a probe that fails before
> > pps_gpio_get_pins() has run -- devm_kzalloc() or pps_gpio_setup() -- returns
> > without going through err_release_pins, so the pins are left in the
> > core-applied "default" state rather than "inactive". I did not change this
> > in v4 as I would like your guidance on the preferred approach; the trade-offs
> > are laid out at the end of patch 3's changelog. In short:
> >
> > A. Move pps_gpio_get_pins() to the top of probe and route the
> > pps_gpio_setup() failure through err_release_pins too, so every path
> > the driver can act on restores "inactive". (The devm_kzalloc() failure
> > is inherently before the driver holds any pinctrl handle, so it cannot
> > be covered by the driver in any option.)
> > B. Keep v4 as-is and treat "inactive" as a successful-ownership concern:
> > a probe that never looked up the state never took the pins, so leaving
> > the core-applied "default" (long-standing behaviour) is acceptable.
> > C. As A, but keep the release helper's existing NULL guard so it is robust
> > regardless of ordering (belt and braces).
> >
> > I lean towards B (the restore is meaningful only once the driver has taken
> > ownership), but I am happy to implement A/C if you prefer uniform failure
> > paths.
>
> I would choose option A, keeping the NULL check in the release helper;
> I believe this corresponds to your option C. In fact, to me, it
> represents the symmetrical counterpart to what the core did on the
> driver's behalf.
Done in v5: pps_gpio_get_pins() now runs first in probe(), the
pps_gpio_setup() failure goes through err_release_pins, and the release
helper keeps its NULL guard (option C).
> One thing I would verify on your board before proceeding is this: with
> option A, an -EPROBE_DEFER error returned by pps_gpio_setup() would
> result in the "inactive" state being selected, and the core would
> re-apply "default" on the next attempt. I assume this mux switching is
> harmless, but the hardware is available to you, not me.
Verified on the AL11 K2V6 JRD10: I forced pps_gpio_setup() to return
-EPROBE_DEFER a few times and watched the pin-mux register. Each failed
attempt releases the pins to "inactive", the core re-applies "default"
before the next, and it settles at "default" once probe succeeds -- no
spurious PPS event or warning across the cycles.
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v5 0/4] pps-gpio: restore pin mux on unbind and shutdown
2026-09-19 17:11 ` [PATCH v4 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
2026-09-22 7:40 ` Rodolfo Giometti
@ 2026-09-22 10:30 ` Eliav Farber
2026-09-22 10:30 ` [PATCH v5 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
` (4 more replies)
1 sibling, 5 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-22 10:30 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and mux the
pins to a different function when the pps-gpio driver is not active.
The driver core already selects the "default" pinctrl state before probe,
so the pins can be muxed for GPIO/PPS use while the driver is bound without
any driver change. Nothing, however, hands the pins back when the driver
is unbound or the system is shut down (for example before kexec), leaving
them stuck in the GPIO mux for the next kernel.
This series lets pps-gpio select an optional "inactive" pinctrl state in
remove() and shutdown(), so a board can describe the alternate mux there
and have it restored. The state is looked up and selected by the driver
itself (devm_pinctrl_get() + pinctrl_lookup_state() + pinctrl_select_
state()), so its meaning is unambiguous and it does not depend on
CONFIG_PM. It is a no-op for boards that do not describe an "inactive"
state.
Patch 1 is a small preparatory fix to propagate the gpiod_to_irq() and
request_threaded_irq() error codes (rather than a hardcoded -EINVAL) on
the probe error paths that patch 4 then converts to gotos. Patch 2 is a
small preparatory fix restoring the echo-GPIO guard in remove() that a
later cleanup dropped, since patch 4 mirrors remove()'s teardown in the
new shutdown() and would otherwise carry the same latent issue. Patch 3
documents the optional "default"/"inactive" pinctrl-names in the binding;
patch 4 implements the driver side.
Tested on an Amazon AL11 K2V6 JRD10 board: binding/unbinding each pps-gpio
device toggles the corresponding pin-mux register between the GPIO
function and the alternate ec_ptp_trigger_in function as expected, and
re-binding restores the GPIO function via the core-applied "default"
state. Reading the mux register with the device left unbound confirms the
"inactive" mux persists. Also tested with a pps-gpio node that describes
no pinctrl at all, where probe, remove and shutdown behave as before.
For the probe-failure path (see below), a forced-defer test confirmed that
a probe failing after pps_gpio_get_pins() releases the pins to "inactive",
the core re-applies "default" before the next attempt, and the mux settles
back at "default" once probe finally succeeds, with no spurious PPS event
or warning across the cycles.
Changes in v5:
- Patch 1: use dev_err_probe() on both error paths instead of a bare
dev_err() + return, so the propagated -EPROBE_DEFER is logged at debug
level rather than spamming the console and the code is emitted
symbolically. Drop Bartosz's Reviewed-by as the patch changed
materially (Rodolfo Giometti)
- New patch 2: restore the data->echo_pin guard around the echo-timer
teardown in remove(), dropped by commit fde046a8c490. timer_delete_sync()
on a never-initialised timer trips debug_assert_init() under
CONFIG_DEBUG_OBJECTS_TIMERS. Split out so patch 4's new shutdown() does
not reintroduce it (Rodolfo Giometti)
- Patch 4: resolve the v4 probe-failure open question by taking option "A
+ keep the NULL guard": look the pinctrl states up first in probe(),
before pps_gpio_setup(), and route the setup() failure through
err_release_pins too, so every path the driver can act on restores
"inactive". Guard the echo-timer teardown in the new shutdown() with
data->echo_pin, matching remove() (Rodolfo Giometti)
Changes in v4:
- Patch 1: add Fixes: 161520451dfa and Bartosz Golaszewski's Reviewed-by.
The hardcoded -EINVAL predates 4461d65176b4, so the tag points at the
original driver rather than the descriptor conversion
- Patch (binding): rework per Rob Herring - drop the prose, use an ordered
"items" list ("default" then "inactive") with minItems: 1, fixing the
"['default', 'inactive'] is too long" dt_binding_check error
Changes in v3 (addressing Sashiko's and Rodolfo Giometti's review):
- New preparatory patch to propagate the gpiod_to_irq() and
request_threaded_irq() error codes instead of overwriting them with
-EINVAL
- Do not constrain pinctrl-names to a fixed ["default", "inactive"] tuple
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl described" rather
than a probe failure; keep propagating everything else incl.
-EPROBE_DEFER
- Restore the "inactive" mux on probe failure via a new err_release_pins
label
- Warn if applying the "inactive" state fails rather than ignoring the
pinctrl_select_state() return
Changes in v2 (all addressing Rodolfo Giometti's review):
- Rename the released state from "idle" to "inactive" ("idle" is the
runtime-PM state in pinctrl-state.h)
- Look the state up in the driver instead of pinctrl_pm_select_idle_state(),
removing the CONFIG_PM dependency
- Fix shutdown(): tear down (free_irq/timer) before the mux change,
matching remove()
- Require a "default" state whenever "inactive" is present and reject the
mismatch
Link: https://lore.kernel.org/all/20260916134744.46354-1-farbere@amazon.com/ [v1]
Link: https://lore.kernel.org/all/20260916182641.9768-1-farbere@amazon.com/ [v2]
Link: https://lore.kernel.org/all/20260917075611.47881-1-farbere@amazon.com/ [v3]
Link: https://lore.kernel.org/all/20260919171157.5502-1-farbere@amazon.com/ [v4]
Eliav Farber (4):
pps: clients: gpio: propagate probe error codes
pps: clients: gpio: only tear down the echo timer when it exists
dt-bindings: pps: pps-gpio: document optional pinctrl states
pps: clients: gpio: release pins to an inactive state on remove and
shutdown
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +-
drivers/pps/clients/pps-gpio.c | 123 ++++++++++++++++--
2 files changed, 127 insertions(+), 10 deletions(-)
base-commit: 93f51579e7df248780214094418f205253383cc5
--
2.47.3
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH v5 1/4] pps: clients: gpio: propagate probe error codes
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
@ 2026-09-22 10:30 ` Eliav Farber
2026-09-22 10:30 ` [PATCH v5 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
` (3 subsequent siblings)
4 siblings, 0 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-22 10:30 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
On the two probe error paths that map and request the interrupt, probe
overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
-EPROBE_DEFER from the caller. The request_threaded_irq() failure message
also logged the IRQ number but not the errno.
Switch both paths to dev_err_probe() so the actual error code is returned
and logged symbolically, and so a repeated -EPROBE_DEFER during boot is
logged at debug level rather than spamming the console. This also matches
pps_gpio_setup() in the same file, which already uses dev_err_probe().
Fixes: 161520451dfa ("pps: new client driver using GPIO")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v5:
- Use dev_err_probe() on both error paths instead of dev_err() + return,
so a propagated -EPROBE_DEFER is logged at debug level (no console spam
on repeated deferral) and the code is emitted symbolically. This also
matches pps_gpio_setup() in the same file. Drop Bartosz Golaszewski's
Reviewed-by as the patch changed materially
Changes in v4:
- Add Fixes: 161520451dfa ("pps: new client driver using GPIO") and
Bartosz Golaszewski's Reviewed-by. The hardcoded -EINVAL on both error
paths predates 4461d65176b4 (which only switched gpio_to_irq() to
gpiod_to_irq() and left those returns as context), so the tag points at
the original driver rather than the descriptor conversion
Changes in v3:
- New patch, split out of the pinctrl change: while converting the probe
error paths to a goto, Takashi Sakamoto noted that the hardcoded -EINVAL
discards the real gpiod_to_irq()/request_threaded_irq() error, so fix
that separately first
drivers/pps/clients/pps-gpio.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 73ec2c7335e5..ccc2fb470b7e 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -163,10 +163,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0) {
- dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return -EINVAL;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -197,8 +195,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
- return -EINVAL;
+ return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v5 2/4] pps: clients: gpio: only tear down the echo timer when it exists
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
2026-09-22 10:30 ` [PATCH v5 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-22 10:30 ` Eliav Farber
2026-09-22 10:30 ` [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
` (2 subsequent siblings)
4 siblings, 0 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-22 10:30 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
remove() calls timer_delete_sync() on data->echo_timer unconditionally,
but the timer is only initialised by timer_setup() in probe() when the
board describes an "echo" GPIO. On a board without echo-gpios the timer is
never set up, so remove() operates on a timer_list that was never
initialised.
The guard used to be there: it was dropped by commit fde046a8c490 ("pps:
clients: gpio: Remove redundant condition in ->remove()") on the grounds
that "the timer along with GPIO API are NULL-aware". That is true for the
GPIO API - gpiod_set_value() is a no-op for a NULL descriptor - but not
for the timer: timer_delete_sync() on a timer that was never timer_setup()
initialised trips the debug_assert_init() check and emits a debugobjects
"not initialized" warning under CONFIG_DEBUG_OBJECTS_TIMERS.
Restore the data->echo_pin guard around the echo teardown, mirroring the
condition under which the timer is set up in probe(). gpiod_set_value() is
kept under the same guard as it only makes sense together with the echo
timer.
Fixes: fde046a8c490 ("pps: clients: gpio: Remove redundant condition in ->remove()")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v5:
- New patch. Split out because patch 4 mirrors remove()'s teardown in the
new shutdown(); guarding the echo teardown here first keeps that latent
issue out of both paths (Rodolfo Giometti)
drivers/pps/clients/pps-gpio.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index ccc2fb470b7e..aec534c246af 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -211,9 +211,11 @@ static void pps_gpio_remove(struct platform_device *pdev)
free_irq(data->irq, data);
pps_unregister_source(data->pps);
- timer_delete_sync(&data->echo_timer);
- /* reset echo pin in any case */
- gpiod_set_value(data->echo_pin, 0);
+ /* reset the echo state, if the board has an echo GPIO */
+ if (data->echo_pin) {
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ }
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
2026-09-22 10:30 ` [PATCH v5 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-09-22 10:30 ` [PATCH v5 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
@ 2026-09-22 10:30 ` Eliav Farber
2026-09-22 12:46 ` Rodolfo Giometti
2026-09-22 10:30 ` [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
4 siblings, 1 reply; 57+ messages in thread
From: Eliav Farber @ 2026-09-22 10:30 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
When the PPS input GPIO is routed through a pin controller, a board may
need to mux those pins to a different function while pps-gpio is not
driving PPS (for example after the driver is unbound or across a kexec).
Document the optional "default" and "inactive" pinctrl-names and show
both in the example. The "default" state selects the PPS/GPIO function
and is applied by the driver core before probe; the optional "inactive"
state, when present, describes the mux to restore when the driver is
unbound or the system is shut down. "default" is pinctrl-0, matching the
implicit ordering the pinctrl core already assigns it, and "inactive" is
pinctrl-1; the driver looks each state up by name.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v4:
- Rework per Rob Herring: do not express the ordering in prose; use an
ordered "items" list ("default" then "inactive") with minItems: 1,
since pinctrl-0 is already implicitly "default" and its position is
fixed. This also fixes the "['default', 'inactive'] is too long"
dt_binding_check error seen on v3. Reword the commit message accordingly
Changes in v3:
- Do not constrain pinctrl-names to a fixed ["default", "inactive"]
tuple. The driver looks the states up by name, so "inactive" may
appear in any position and other states may coexist; only require
(via "contains") that a "default" state exists, and reword the
description accordingly
Changes in v2:
- Rename the released state from "idle" to "inactive"
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
index 383a838744eb..61b5de6724ea 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
@@ -28,6 +28,17 @@ properties:
description: Indicates a falling edge assert, when present. Rising edge if absent.
type: boolean
+ pinctrl-names:
+ description:
+ The "default" state selects the PPS/GPIO function and is applied by the
+ driver core before probe. The optional "inactive" state, when present,
+ is selected when the driver is unbound or the system is shut down,
+ handing the pins back to their alternate function.
+ minItems: 1
+ items:
+ - const: default
+ - const: inactive
+
required:
- compatible
- gpios
@@ -40,8 +51,9 @@ examples:
pps {
compatible = "pps-gpio";
- pinctrl-names = "default";
+ pinctrl-names = "default", "inactive";
pinctrl-0 = <&pinctrl_pps>;
+ pinctrl-1 = <&pinctrl_pps_inactive>;
gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
assert-falling-edge;
echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-22 10:30 ` [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-22 12:46 ` Rodolfo Giometti
2026-09-23 12:36 ` Rob Herring
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 12:46 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Tue, Sep 22, 2026 at 10:30:50AM +0000, Eliav Farber wrote:
> + minItems: 1
> + items:
> + - const: default
> + - const: inactive
One observation while testing the series, entirely a devicetree call:
dtschema derives maxItems from the items list, so this also caps
pinctrl-names at two and fixes the order --
"default", "inactive", "sleep" -> 'is too long'
"default", "sleep" -> 'inactive' was expected
I have no opinion on whether that matters for this binding, I just
wanted it on the record.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-22 12:46 ` Rodolfo Giometti
@ 2026-09-23 12:36 ` Rob Herring
2026-09-23 13:13 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rob Herring @ 2026-09-23 12:36 UTC (permalink / raw)
To: Rodolfo Giometti
Cc: Eliav Farber, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Tue, Sep 22, 2026 at 02:46:40PM +0200, Rodolfo Giometti wrote:
> On Tue, Sep 22, 2026 at 10:30:50AM +0000, Eliav Farber wrote:
> > + minItems: 1
> > + items:
> > + - const: default
> > + - const: inactive
>
> One observation while testing the series, entirely a devicetree call:
> dtschema derives maxItems from the items list, so this also caps
> pinctrl-names at two and fixes the order --
Yes, that's exactly what was said was needed here.
>
> "default", "inactive", "sleep" -> 'is too long'
> "default", "sleep" -> 'inactive' was expected
Well, that's something different.
items:
- const: default
- enum: [ inactive, sleep ]
- const: sleep
The common pinctrl-names schema should require unique names so 'sleep'
can't be repeated.
>
> I have no opinion on whether that matters for this binding, I just
> wanted it on the record.
We don't even require pinctrl properties to be documented. They are
implicitly allowed, but good to document what users (drivers)
expect/require.
Rob
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-23 12:36 ` Rob Herring
@ 2026-09-23 13:13 ` Farber, Eliav
2026-09-23 18:04 ` Rob Herring
0 siblings, 1 reply; 57+ messages in thread
From: Farber, Eliav @ 2026-09-23 13:13 UTC (permalink / raw)
To: Rob Herring, Rodolfo Giometti
Cc: Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Sep 23, 2026 at 07:36:05AM -0500, Rob Herring wrote:
> On Tue, Sep 22, 2026 at 02:46:40PM +0200, Rodolfo Giometti wrote:
> > On Tue, Sep 22, 2026 at 10:30:50AM +0000, Eliav Farber wrote:
> > > + minItems: 1
> > > + items:
> > > + - const: default
> > > + - const: inactive
> >
> > One observation while testing the series, entirely a devicetree call:
> > dtschema derives maxItems from the items list, so this also caps
> > pinctrl-names at two and fixes the order --
>
> Yes, that's exactly what was said was needed here.
>
> > "default", "inactive", "sleep" -> 'is too long'
> > "default", "sleep" -> 'inactive' was expected
>
> Well, that's something different.
>
> items:
> - const: default
> - enum: [ inactive, sleep ]
> - const: sleep
>
> The common pinctrl-names schema should require unique names so 'sleep'
> can't be repeated.
Thanks. Just so I pick the right thing for v6 -- do you want me to
change patch 3 to that form, or is the current
minItems: 1
items:
- const: default
- const: inactive
fine as-is?
This binding only defines "default" and "inactive"; the driver looks
both up by name and does not use a "sleep" state, so I had deliberately
capped the list at those two. Your snippet reads to me as how the common
pinctrl-names schema would accommodate a "sleep" state in general,
rather than something this binding needs -- but I would rather confirm
than guess.
If you would prefer the more permissive form so a board can also declare
"sleep", I will switch to it (with unique-name enforcement) in v6.
Otherwise I will keep the two-entry list.
> We don't even require pinctrl properties to be documented. They are
> implicitly allowed, but good to document what users (drivers)
> expect/require.
Right -- documenting the two names the driver actually consumes was the
intent here.
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-23 13:13 ` Farber, Eliav
@ 2026-09-23 18:04 ` Rob Herring
0 siblings, 0 replies; 57+ messages in thread
From: Rob Herring @ 2026-09-23 18:04 UTC (permalink / raw)
To: Farber, Eliav
Cc: Rodolfo Giometti, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Sep 23, 2026 at 01:13:10PM +0000, Farber, Eliav wrote:
> On Wed, Sep 23, 2026 at 07:36:05AM -0500, Rob Herring wrote:
> > On Tue, Sep 22, 2026 at 02:46:40PM +0200, Rodolfo Giometti wrote:
> > > On Tue, Sep 22, 2026 at 10:30:50AM +0000, Eliav Farber wrote:
> > > > + minItems: 1
> > > > + items:
> > > > + - const: default
> > > > + - const: inactive
> > >
> > > One observation while testing the series, entirely a devicetree call:
> > > dtschema derives maxItems from the items list, so this also caps
> > > pinctrl-names at two and fixes the order --
> >
> > Yes, that's exactly what was said was needed here.
> >
> > > "default", "inactive", "sleep" -> 'is too long'
> > > "default", "sleep" -> 'inactive' was expected
> >
> > Well, that's something different.
> >
> > items:
> > - const: default
> > - enum: [ inactive, sleep ]
> > - const: sleep
> >
> > The common pinctrl-names schema should require unique names so 'sleep'
> > can't be repeated.
>
> Thanks. Just so I pick the right thing for v6 -- do you want me to
> change patch 3 to that form, or is the current
>
> minItems: 1
> items:
> - const: default
> - const: inactive
>
> fine as-is?
Yes.
> This binding only defines "default" and "inactive"; the driver looks
> both up by name and does not use a "sleep" state, so I had deliberately
> capped the list at those two. Your snippet reads to me as how the common
> pinctrl-names schema would accommodate a "sleep" state in general,
> rather than something this binding needs -- but I would rather confirm
> than guess.
>
> If you would prefer the more permissive form so a board can also declare
> "sleep", I will switch to it (with unique-name enforcement) in v6.
> Otherwise I will keep the two-entry list.
>
> > We don't even require pinctrl properties to be documented. They are
> > implicitly allowed, but good to document what users (drivers)
> > expect/require.
>
> Right -- documenting the two names the driver actually consumes was the
> intent here.
I have no clue why "sleep" was added to the discussion...
Rob
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
` (2 preceding siblings ...)
2026-09-22 10:30 ` [PATCH v5 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-22 10:30 ` Eliav Farber
2026-09-22 12:46 ` Rodolfo Giometti
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
4 siblings, 1 reply; 57+ messages in thread
From: Eliav Farber @ 2026-09-22 10:30 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and need to
mux it to another function when pps-gpio is not driving PPS. The driver
core applies the "default" pinctrl state before probe, so the pins are
muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
the pins back when the driver is unbound or the system is shut down, so
they stay stuck in the GPIO function for whatever runs next, kexec
included.
Look up an optional "inactive" pinctrl state in probe via
devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
pinctrl_select_state() in remove() and shutdown(). The state is looked up
and selected by the driver itself rather than reusing the runtime-PM
"idle"/"sleep" states, so its meaning is unambiguous and it does not
depend on CONFIG_PM. Boards that do not describe an "inactive" state are
unaffected.
Since "inactive" is only meaningful as the mux to restore after the
core-applied "default" state, reject an "inactive" state that is not
paired with a "default" one rather than releasing pins that were never
put into a defined PPS state.
Look up the pinctrl states first in probe(), before pps_gpio_setup(), and
route every subsequent failure through a common err_release_pins label.
The driver core applies the "default" mux before probe(), so a probe that
fails after this point would otherwise leave the pins stuck in "default";
releasing them to "inactive" on the error path is the symmetrical
counterpart to what the core did on the driver's behalf. A failure in
pps_gpio_get_pins() itself returns directly, as no state was taken yet;
pps_gpio_release_pins() is a no-op when no "inactive" state was found.
The mux must not change while something can still drive the pins. On
shutdown() the requested IRQ and the echo timer would otherwise outlive
the mux change -- device_shutdown() is not the end of the road, the
kernel keeps running to load and start the kexec image -- so a timer
callback or the PPS handler could poke a line that by then belongs to
another function. Tear down in the same order as remove(): free_irq()
first, then the echo timer (only when the board has an echo GPIO, as in
remove()), and the mux change last. shutdown() does not unregister the
PPS source, which is a remove-time concern.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v5:
- Resolve the v4 probe-failure open question by taking option "A + keep
the NULL guard": look the pinctrl states up first in probe(), before
pps_gpio_setup(), and route the pps_gpio_setup() failure through
err_release_pins too, so every path the driver can act on restores
"inactive". pps_gpio_release_pins() keeps its NULL guard, so it is a
no-op when no "inactive" state was found. Verified on the AL11 K2V6
JRD10 with a forced-defer test: the pins are released to "inactive" on
each failed attempt, the core re-applies "default" before the next, and
the mux settles at "default" on the eventual success, with no spurious
PPS event (Rodolfo Giometti)
- Clear data->pins_inactive when rejecting an "inactive" state that has no
"default", so a board deliberately rejected here can never have its pins
released to "inactive"
- Guard the echo-timer teardown in the new shutdown() with data->echo_pin,
matching remove() after the preceding patch (Rodolfo Giometti)
- Convert the gpiod_to_irq()/request_threaded_irq() error paths to the
log-only dev_err_probe() + goto err_release_pins form, following patch 1
Changes in v4:
- No functional change (probe-failure raised as an open question, now
resolved in v5 above)
Changes in v3:
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl", not a probe
failure; keep propagating everything else, e.g. -EPROBE_DEFER
- Restore the "inactive" mux on probe failure via a new err_release_pins
label
- Warn if pinctrl_select_state() fails to apply the "inactive" state
rather than silently ignoring the error
- Trim and de-duplicate the added comments
Changes in v2:
- Rename the released state from "idle" to "inactive"
- Look the state up in the driver with devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state() instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency
- Fix shutdown() to free_irq() and the echo teardown before the mux
change, matching remove()
- Require a "default" state whenever "inactive" is present and reject the
mismatch
drivers/pps/clients/pps-gpio.c | 117 +++++++++++++++++++++++++++++++--
1 file changed, 111 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index aec534c246af..203bf465ed8b 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/pps_kernel.h>
#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/list.h>
#include <linux/property.h>
#include <linux/timer.h>
@@ -30,6 +31,8 @@ struct pps_gpio_device_data {
struct gpio_desc *gpio_pin; /* GPIO port descriptors */
struct gpio_desc *echo_pin;
struct timer_list echo_timer; /* timer to reset echo active state */
+ struct pinctrl *pinctrl; /* pin control handle */
+ struct pinctrl_state *pins_inactive; /* pins released when unbound */
bool assert_falling_edge;
unsigned int echo_active_ms; /* PPS echo active duration */
unsigned long echo_timeout; /* timer timeout value in jiffies */
@@ -96,6 +99,68 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
gpiod_set_value(info->echo_pin, 0);
}
+/*
+ * Look up the optional "inactive" pinctrl state. It requires a "default"
+ * state (applied by the driver core before probe) and is rejected without
+ * one. Absent pinctrl, or an absent "inactive" state, is not an error.
+ */
+static int pps_gpio_get_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ struct pinctrl_state *pins_default;
+
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ /*
+ * A DT device without "pinctrl-0" yields -ENODEV, which
+ * is not an error here; propagate anything else.
+ */
+ if (PTR_ERR(data->pinctrl) == -ENODEV) {
+ data->pinctrl = NULL;
+ return 0;
+ }
+ return dev_err_probe(dev, PTR_ERR(data->pinctrl),
+ "failed to get pinctrl\n");
+ }
+
+ /* The "inactive" state is optional. */
+ data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
+ if (IS_ERR(data->pins_inactive)) {
+ data->pins_inactive = NULL;
+ return 0;
+ }
+
+ /* "inactive" requires a "default" state to return from. */
+ pins_default = pinctrl_lookup_state(data->pinctrl, "default");
+ if (IS_ERR(pins_default)) {
+ data->pins_inactive = NULL;
+ return dev_err_probe(dev, PTR_ERR(pins_default),
+ "\"inactive\" pinctrl state requires a \"default\" state\n");
+ }
+
+ return 0;
+}
+
+/*
+ * Restore the "inactive" pinctrl state, handing the pins back to whatever
+ * function uses them while pps-gpio is not driving PPS. This undoes the
+ * "default" state the driver core applied before probe. A no-op for boards
+ * that describe no "inactive" state.
+ */
+static void pps_gpio_release_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ if (!data->pins_inactive)
+ return;
+
+ ret = pinctrl_select_state(data->pinctrl, data->pins_inactive);
+ if (ret)
+ dev_warn(dev, "failed to select inactive pinctrl state: %d\n",
+ ret);
+}
+
static int pps_gpio_setup(struct device *dev)
{
struct pps_gpio_device_data *data = dev_get_drvdata(dev);
@@ -156,15 +221,25 @@ static int pps_gpio_probe(struct platform_device *pdev)
dev_set_drvdata(dev, data);
+ /*
+ * pinctrl setup (optional states) first, so the "inactive" mux can be
+ * restored on any later probe-failure path via err_release_pins.
+ */
+ ret = pps_gpio_get_pins(dev);
+ if (ret)
+ return ret;
+
/* GPIO setup */
ret = pps_gpio_setup(dev);
if (ret)
- return ret;
+ goto err_release_pins;
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0)
- return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ goto err_release_pins;
+ }
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -185,7 +260,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (IS_ERR(data->pps)) {
dev_err(dev, "failed to register IRQ %d as PPS source\n",
data->irq);
- return PTR_ERR(data->pps);
+ ret = PTR_ERR(data->pps);
+ goto err_release_pins;
}
/* register IRQ interrupt handler */
@@ -195,14 +271,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
- data->irq);
+ dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
+ goto err_release_pins;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
data->irq);
return 0;
+
+err_release_pins:
+ /* Restore the inactive mux on probe failure; safe to do last here. */
+ pps_gpio_release_pins(dev);
+ return ret;
}
static void pps_gpio_remove(struct platform_device *pdev)
@@ -216,9 +298,31 @@ static void pps_gpio_remove(struct platform_device *pdev)
timer_delete_sync(&data->echo_timer);
gpiod_set_value(data->echo_pin, 0);
}
+ /* release the pins last, once nothing can drive them */
+ pps_gpio_release_pins(&pdev->dev);
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
+static void pps_gpio_shutdown(struct platform_device *pdev)
+{
+ struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+
+ /*
+ * The kernel keeps running after device_shutdown() (e.g. to load and
+ * start a kexec image), so quiesce the hardware before touching the
+ * mux: free the IRQ and stop the echo timer first, then release the
+ * pins last, so no callback can drive a pin after it is handed back.
+ * The PPS source is left registered; that is a remove-time concern.
+ */
+ free_irq(data->irq, data);
+ /* reset the echo state, if the board has an echo GPIO */
+ if (data->echo_pin) {
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ }
+ pps_gpio_release_pins(&pdev->dev);
+}
+
static const struct of_device_id pps_gpio_dt_ids[] = {
{ .compatible = "pps-gpio", },
{ /* sentinel */ }
@@ -228,6 +332,7 @@ MODULE_DEVICE_TABLE(of, pps_gpio_dt_ids);
static struct platform_driver pps_gpio_driver = {
.probe = pps_gpio_probe,
.remove = pps_gpio_remove,
+ .shutdown = pps_gpio_shutdown,
.driver = {
.name = PPS_GPIO_NAME,
.of_match_table = pps_gpio_dt_ids,
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-22 10:30 ` [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-22 12:46 ` Rodolfo Giometti
2026-09-22 14:55 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 12:46 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Tue, Sep 22, 2026 at 10:30:51AM +0000, Eliav Farber wrote:
The probe-failure path looks right to me now. Only small things left.
> +static void pps_gpio_shutdown(struct platform_device *pdev)
This runs on every board, including the ones that describe no "inactive"
state, where the free_irq() and the echo teardown buy nothing. Harmless
as far as I can see, but the commit message only speaks about the mux --
worth a line there?
> if (IS_ERR(data->pps)) {
> dev_err(dev, "failed to register IRQ %d as PPS source\n",
> data->irq);
This stayed dev_err() while both its neighbours became dev_err_probe().
Nothing is broken, but "matches the rest of the file" was the argument
for patch 1.
I would keep timer_delete_sync() here rather than moving to
timer_shutdown_sync(): the kernel-doc motivates the latter with the
circular-dependency case, which does not apply here -- the only rearm
path is the echo, behind free_irq().
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread* RE: [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-22 12:46 ` Rodolfo Giometti
@ 2026-09-22 14:55 ` Farber, Eliav
2026-09-23 7:50 ` Rodolfo Giometti
0 siblings, 1 reply; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 14:55 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Tue, Sep 22, 2026 at 02:46:00PM +0200, Rodolfo Giometti wrote:
> On Tue, Sep 22, 2026 at 10:30:51AM +0000, Eliav Farber wrote:
> > + if (IS_ERR(data->pps)) {
> > + dev_err(dev, "failed to register IRQ %d as PPS source\n",
> > + data->irq);
>
> This stayed dev_err() while both its neighbours became dev_err_probe().
> Nothing is broken, but "matches the rest of the file" was the argument
> for patch 1.
I noticed this too while doing the changes, but wasn't sure where it
should go, so I would rather ask than guess.
The awkward part is that patch 1's actual fix is propagating the real
error code -- the -EINVAL -> ret change on the gpiod_to_irq() and
request_threaded_irq() paths. The dev_err_probe() form there is a
consequence of that (and of your v4 nit), not the point of the patch.
The pps_register_source() path never had the errno bug: it already did
return PTR_ERR(data->pps), so converting it is purely a logging/style
consistency change, unrelated to what patch 1 sets out to fix. Patch 4
is not an obvious home either: the dev_err()/dev_err_probe() split
predates it (it exists from patch 1 onward), so patch 4 would only be
tidying it up in passing rather than introducing it.
So I see three ways to place it:
1. Fold it into patch 1. Pro: patch 1 is where the file first gains
dev_err_probe(), so all three probe error paths become consistent
in the same commit. Con: it widens patch 1 beyond "propagate the
error code" into a path that never masked the error, muddying an
otherwise focused fix. It would use the return-form
(return dev_err_probe(...)), which patch 4 then rewrites into the
log-only + goto err_release_pins form like the other two.
2. Fold it into patch 4. Pro: patch 4 already rewrites these paths
into the log-only dev_err_probe() + goto form, so the conversion
rides along with churn it is already making. Con: the
dev_err()/dev_err_probe() split predates patch 4 -- it exists from
patch 1 onward -- so patch 4 is not really where the inconsistency
originates.
3. A small separate patch after patch 1, "pps: clients: gpio: use
dev_err_probe() consistently in probe" (or similar). Pro: keeps
each patch single-purpose -- patch 1 stays "propagate the code",
the style unification is its own reviewable change.
Which do you prefer?
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-22 14:55 ` Farber, Eliav
@ 2026-09-23 7:50 ` Rodolfo Giometti
0 siblings, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-23 7:50 UTC (permalink / raw)
To: Farber, Eliav, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Tue, Sep 22, 2026 at 02:55:05PM +0000, Eliav Farber wrote:
> Which do you prefer?
I think option 2 is the best, since patch 4 has to modify that block
anyway, and it's simply an extra line within a section of code that is
being changed regardless.
You just need to mention it in the commit message for patch 4 so it
doesn't look like a superfluous or random change.
In any case, this doesn't block anything.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v6 0/4] pps-gpio: restore pin mux on unbind and shutdown
2026-09-22 10:30 ` [PATCH v5 0/4] " Eliav Farber
` (3 preceding siblings ...)
2026-09-22 10:30 ` [PATCH v5 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-23 18:22 ` Eliav Farber
2026-09-23 18:22 ` [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
` (4 more replies)
4 siblings, 5 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-23 18:22 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and mux the
pins to a different function when the pps-gpio driver is not active.
The driver core already selects the "default" pinctrl state before probe,
so the pins can be muxed for GPIO/PPS use while the driver is bound without
any driver change. Nothing, however, hands the pins back when the driver
is unbound or the system is shut down (for example before kexec), leaving
them stuck in the GPIO mux for the next kernel.
This series lets pps-gpio select an optional "inactive" pinctrl state in
remove() and shutdown(), so a board can describe the alternate mux there
and have it restored. The state is looked up and selected by the driver
itself (devm_pinctrl_get() + pinctrl_lookup_state() + pinctrl_select_
state()), so its meaning is unambiguous and it does not depend on
CONFIG_PM. It is a no-op for boards that do not describe an "inactive"
state.
Patch 1 is a small preparatory fix to propagate the gpiod_to_irq() and
request_threaded_irq() error codes (rather than a hardcoded -EINVAL) on
the probe error paths that patch 4 then converts to gotos. Patch 2 is a
small preparatory fix restoring the echo-GPIO guard in remove() that a
later cleanup dropped, since patch 4 mirrors remove()'s teardown in the
new shutdown() and would otherwise carry the same latent issue. Patch 3
documents the optional "default"/"inactive" pinctrl-names in the binding;
patch 4 implements the driver side.
Tested on an Amazon AL11 K2V6 JRD10 board: binding/unbinding each pps-gpio
device toggles the corresponding pin-mux register between the GPIO
function and the alternate ec_ptp_trigger_in function as expected, and
re-binding restores the GPIO function via the core-applied "default"
state. Reading the mux register with the device left unbound confirms the
"inactive" mux persists. Also tested with a pps-gpio node that describes
no pinctrl at all, where probe, remove and shutdown behave as before.
For the probe-failure path (see below), a forced-defer test confirmed that
a probe failing after pps_gpio_get_pins() releases the pins to "inactive",
the core re-applies "default" before the next attempt, and the mux settles
back at "default" once probe finally succeeds, with no spurious PPS event
or warning across the cycles.
Changes in v6:
- Patch 4: fold the pps_register_source() failure path into the same
dev_err_probe() + goto err_release_pins style as the other two paths
sharing that label, with a commit-message note that it is a logging
change, not a fix (Rodolfo Giometti)
- Patch 3: unchanged. Rob Herring confirmed the two-entry
["default", "inactive"] items list is fine as-is; the "sleep" state
discussed on the list is not something this binding needs
- No other functional changes
Changes in v5:
- Patch 1: use dev_err_probe() on both error paths instead of a bare
dev_err() + return, so the propagated -EPROBE_DEFER is logged at debug
level rather than spamming the console and the code is emitted
symbolically. Drop Bartosz's Reviewed-by as the patch changed
materially (Rodolfo Giometti)
- New patch 2: restore the data->echo_pin guard around the echo-timer
teardown in remove(), dropped by commit fde046a8c490. timer_delete_sync()
on a never-initialised timer trips debug_assert_init() under
CONFIG_DEBUG_OBJECTS_TIMERS. Split out so patch 4's new shutdown() does
not reintroduce it (Rodolfo Giometti)
- Patch 4: resolve the v4 probe-failure open question by taking option "A
+ keep the NULL guard": look the pinctrl states up first in probe(),
before pps_gpio_setup(), and route the setup() failure through
err_release_pins too, so every path the driver can act on restores
"inactive". Guard the echo-timer teardown in the new shutdown() with
data->echo_pin, matching remove() (Rodolfo Giometti)
Changes in v4:
- Patch 1: add Fixes: 161520451dfa and Bartosz Golaszewski's Reviewed-by.
The hardcoded -EINVAL predates 4461d65176b4, so the tag points at the
original driver rather than the descriptor conversion
- Patch (binding): rework per Rob Herring - drop the prose, use an ordered
"items" list ("default" then "inactive") with minItems: 1, fixing the
"['default', 'inactive'] is too long" dt_binding_check error
Changes in v3 (addressing Sashiko's and Rodolfo Giometti's review):
- New preparatory patch to propagate the gpiod_to_irq() and
request_threaded_irq() error codes instead of overwriting them with
-EINVAL
- Do not constrain pinctrl-names to a fixed ["default", "inactive"] tuple
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl described" rather
than a probe failure; keep propagating everything else incl.
-EPROBE_DEFER
- Restore the "inactive" mux on probe failure via a new err_release_pins
label
- Warn if applying the "inactive" state fails rather than ignoring the
pinctrl_select_state() return
Changes in v2 (all addressing Rodolfo Giometti's review):
- Rename the released state from "idle" to "inactive" ("idle" is the
runtime-PM state in pinctrl-state.h)
- Look the state up in the driver instead of pinctrl_pm_select_idle_state(),
removing the CONFIG_PM dependency
- Fix shutdown(): tear down (free_irq/timer) before the mux change,
matching remove()
- Require a "default" state whenever "inactive" is present and reject the
mismatch
Link: https://lore.kernel.org/all/20260916134744.46354-1-farbere@amazon.com/ [v1]
Link: https://lore.kernel.org/all/20260916182641.9768-1-farbere@amazon.com/ [v2]
Link: https://lore.kernel.org/all/20260917075611.47881-1-farbere@amazon.com/ [v3]
Link: https://lore.kernel.org/all/20260919171157.5502-1-farbere@amazon.com/ [v4]
Link: https://lore.kernel.org/all/20260922103051.5257-1-farbere@amazon.com/ [v5]
Eliav Farber (4):
pps: clients: gpio: propagate probe error codes
pps: clients: gpio: only tear down the echo timer when it exists
dt-bindings: pps: pps-gpio: document optional pinctrl states
pps: clients: gpio: release pins to an inactive state on remove and
shutdown
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +-
drivers/pps/clients/pps-gpio.c | 127 ++++++++++++++++--
2 files changed, 129 insertions(+), 12 deletions(-)
base-commit: 93f51579e7df248780214094418f205253383cc5
--
2.47.3
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
@ 2026-09-23 18:22 ` Eliav Farber
2026-09-24 12:58 ` Rodolfo Giometti
2026-09-24 14:46 ` Bartosz Golaszewski
2026-09-23 18:22 ` [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
` (3 subsequent siblings)
4 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-23 18:22 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
On the two probe error paths that map and request the interrupt, probe
overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
-EPROBE_DEFER from the caller. The request_threaded_irq() failure message
also logged the IRQ number but not the errno.
Switch both paths to dev_err_probe() so the actual error code is returned
and logged symbolically, and so a repeated -EPROBE_DEFER during boot is
logged at debug level rather than spamming the console. This also matches
pps_gpio_setup() in the same file, which already uses dev_err_probe().
Fixes: 161520451dfa ("pps: new client driver using GPIO")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v6:
- No change
Changes in v5:
- Use dev_err_probe() on both error paths instead of dev_err() + return,
so a propagated -EPROBE_DEFER is logged at debug level (no console spam
on repeated deferral) and the code is emitted symbolically. This also
matches pps_gpio_setup() in the same file. Drop Bartosz Golaszewski's
Reviewed-by as the patch changed materially
Changes in v4:
- Add Fixes: 161520451dfa ("pps: new client driver using GPIO") and
Bartosz Golaszewski's Reviewed-by. The hardcoded -EINVAL on both error
paths predates 4461d65176b4 (which only switched gpio_to_irq() to
gpiod_to_irq() and left those returns as context), so the tag points at
the original driver rather than the descriptor conversion
Changes in v3:
- New patch, split out of the pinctrl change: while converting the probe
error paths to a goto, Takashi Sakamoto noted that the hardcoded -EINVAL
discards the real gpiod_to_irq()/request_threaded_irq() error, so fix
that separately first
drivers/pps/clients/pps-gpio.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 73ec2c7335e5..ccc2fb470b7e 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -163,10 +163,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0) {
- dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return -EINVAL;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -197,8 +195,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
- return -EINVAL;
+ return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes
2026-09-23 18:22 ` [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-24 12:58 ` Rodolfo Giometti
2026-09-24 14:46 ` Bartosz Golaszewski
1 sibling, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-24 12:58 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:40PM +0000, Eliav Farber wrote:
> On the two probe error paths that map and request the interrupt, probe
> overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
> hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
> -EPROBE_DEFER from the caller. The request_threaded_irq() failure message
> also logged the IRQ number but not the errno.
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes
2026-09-23 18:22 ` [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-09-24 12:58 ` Rodolfo Giometti
@ 2026-09-24 14:46 ` Bartosz Golaszewski
1 sibling, 0 replies; 57+ messages in thread
From: Bartosz Golaszewski @ 2026-09-24 14:46 UTC (permalink / raw)
To: Eliav Farber
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel,
Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
On Wed, 23 Sep 2026 20:22:40 +0200, Eliav Farber <farbere@amazon.com> said:
> On the two probe error paths that map and request the interrupt, probe
> overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
> hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
> -EPROBE_DEFER from the caller. The request_threaded_irq() failure message
> also logged the IRQ number but not the errno.
>
> Switch both paths to dev_err_probe() so the actual error code is returned
> and logged symbolically, and so a repeated -EPROBE_DEFER during boot is
> logged at debug level rather than spamming the console. This also matches
> pps_gpio_setup() in the same file, which already uses dev_err_probe().
>
> Fixes: 161520451dfa ("pps: new client driver using GPIO")
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
2026-09-23 18:22 ` [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-23 18:22 ` Eliav Farber
2026-09-24 12:59 ` Rodolfo Giometti
2026-09-24 14:47 ` Bartosz Golaszewski
2026-09-23 18:22 ` [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
` (2 subsequent siblings)
4 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-23 18:22 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
remove() calls timer_delete_sync() on data->echo_timer unconditionally,
but the timer is only initialised by timer_setup() in probe() when the
board describes an "echo" GPIO. On a board without echo-gpios the timer is
never set up, so remove() operates on a timer_list that was never
initialised.
The guard used to be there: it was dropped by commit fde046a8c490 ("pps:
clients: gpio: Remove redundant condition in ->remove()") on the grounds
that "the timer along with GPIO API are NULL-aware". That is true for the
GPIO API - gpiod_set_value() is a no-op for a NULL descriptor - but not
for the timer: timer_delete_sync() on a timer that was never timer_setup()
initialised trips the debug_assert_init() check and emits a debugobjects
"not initialized" warning under CONFIG_DEBUG_OBJECTS_TIMERS.
Restore the data->echo_pin guard around the echo teardown, mirroring the
condition under which the timer is set up in probe(). gpiod_set_value() is
kept under the same guard as it only makes sense together with the echo
timer.
Fixes: fde046a8c490 ("pps: clients: gpio: Remove redundant condition in ->remove()")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v6:
- No change
Changes in v5:
- New patch. Split out because patch 4 mirrors remove()'s teardown in the
new shutdown(); guarding the echo teardown here first keeps that latent
issue out of both paths (Rodolfo Giometti)
drivers/pps/clients/pps-gpio.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index ccc2fb470b7e..aec534c246af 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -211,9 +211,11 @@ static void pps_gpio_remove(struct platform_device *pdev)
free_irq(data->irq, data);
pps_unregister_source(data->pps);
- timer_delete_sync(&data->echo_timer);
- /* reset echo pin in any case */
- gpiod_set_value(data->echo_pin, 0);
+ /* reset the echo state, if the board has an echo GPIO */
+ if (data->echo_pin) {
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ }
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists
2026-09-23 18:22 ` [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
@ 2026-09-24 12:59 ` Rodolfo Giometti
2026-09-24 14:47 ` Bartosz Golaszewski
1 sibling, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-24 12:59 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:41PM +0000, Eliav Farber wrote:
> remove() calls timer_delete_sync() on data->echo_timer unconditionally,
> but the timer is only initialised by timer_setup() in probe() when the
> board describes an "echo" GPIO. On a board without echo-gpios the timer is
> never set up, so remove() operates on a timer_list that was never
> initialised.
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists
2026-09-23 18:22 ` [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
2026-09-24 12:59 ` Rodolfo Giometti
@ 2026-09-24 14:47 ` Bartosz Golaszewski
1 sibling, 0 replies; 57+ messages in thread
From: Bartosz Golaszewski @ 2026-09-24 14:47 UTC (permalink / raw)
To: Eliav Farber
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel,
Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
On Wed, 23 Sep 2026 20:22:41 +0200, Eliav Farber <farbere@amazon.com> said:
> remove() calls timer_delete_sync() on data->echo_timer unconditionally,
> but the timer is only initialised by timer_setup() in probe() when the
> board describes an "echo" GPIO. On a board without echo-gpios the timer is
> never set up, so remove() operates on a timer_list that was never
> initialised.
>
> The guard used to be there: it was dropped by commit fde046a8c490 ("pps:
> clients: gpio: Remove redundant condition in ->remove()") on the grounds
> that "the timer along with GPIO API are NULL-aware". That is true for the
> GPIO API - gpiod_set_value() is a no-op for a NULL descriptor - but not
> for the timer: timer_delete_sync() on a timer that was never timer_setup()
> initialised trips the debug_assert_init() check and emits a debugobjects
> "not initialized" warning under CONFIG_DEBUG_OBJECTS_TIMERS.
>
> Restore the data->echo_pin guard around the echo teardown, mirroring the
> condition under which the timer is set up in probe(). gpiod_set_value() is
> kept under the same guard as it only makes sense together with the echo
> timer.
>
> Fixes: fde046a8c490 ("pps: clients: gpio: Remove redundant condition in ->remove()")
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
2026-09-23 18:22 ` [PATCH v6 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-09-23 18:22 ` [PATCH v6 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
@ 2026-09-23 18:22 ` Eliav Farber
2026-09-23 18:41 ` Rob Herring
2026-09-24 12:59 ` Rodolfo Giometti
2026-09-23 18:22 ` [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
2026-09-24 12:58 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Rodolfo Giometti
4 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-23 18:22 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
When the PPS input GPIO is routed through a pin controller, a board may
need to mux those pins to a different function while pps-gpio is not
driving PPS (for example after the driver is unbound or across a kexec).
Document the optional "default" and "inactive" pinctrl-names and show
both in the example. The "default" state selects the PPS/GPIO function
and is applied by the driver core before probe; the optional "inactive"
state, when present, describes the mux to restore when the driver is
unbound or the system is shut down. "default" is pinctrl-0, matching the
implicit ordering the pinctrl core already assigns it, and "inactive" is
pinctrl-1; the driver looks each state up by name.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v6:
- No change. Rob Herring confirmed the two-entry ["default", "inactive"]
items list is fine as-is; the "sleep" state discussed on the list is not
something this binding needs
Changes in v4:
- Rework per Rob Herring: do not express the ordering in prose; use an
ordered "items" list ("default" then "inactive") with minItems: 1,
since pinctrl-0 is already implicitly "default" and its position is
fixed. This also fixes the "['default', 'inactive'] is too long"
dt_binding_check error seen on v3. Reword the commit message accordingly
Changes in v3:
- Do not constrain pinctrl-names to a fixed ["default", "inactive"]
tuple. The driver looks the states up by name, so "inactive" may
appear in any position and other states may coexist; only require
(via "contains") that a "default" state exists, and reword the
description accordingly
Changes in v2:
- Rename the released state from "idle" to "inactive"
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
index 383a838744eb..61b5de6724ea 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
@@ -28,6 +28,17 @@ properties:
description: Indicates a falling edge assert, when present. Rising edge if absent.
type: boolean
+ pinctrl-names:
+ description:
+ The "default" state selects the PPS/GPIO function and is applied by the
+ driver core before probe. The optional "inactive" state, when present,
+ is selected when the driver is unbound or the system is shut down,
+ handing the pins back to their alternate function.
+ minItems: 1
+ items:
+ - const: default
+ - const: inactive
+
required:
- compatible
- gpios
@@ -40,8 +51,9 @@ examples:
pps {
compatible = "pps-gpio";
- pinctrl-names = "default";
+ pinctrl-names = "default", "inactive";
pinctrl-0 = <&pinctrl_pps>;
+ pinctrl-1 = <&pinctrl_pps_inactive>;
gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
assert-falling-edge;
echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-23 18:22 ` [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-23 18:41 ` Rob Herring
2026-09-24 12:59 ` Rodolfo Giometti
1 sibling, 0 replies; 57+ messages in thread
From: Rob Herring @ 2026-09-23 18:41 UTC (permalink / raw)
To: Eliav Farber
Cc: Rodolfo Giometti, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:42PM +0000, Eliav Farber wrote:
> When the PPS input GPIO is routed through a pin controller, a board may
> need to mux those pins to a different function while pps-gpio is not
> driving PPS (for example after the driver is unbound or across a kexec).
>
> Document the optional "default" and "inactive" pinctrl-names and show
> both in the example. The "default" state selects the PPS/GPIO function
> and is applied by the driver core before probe; the optional "inactive"
> state, when present, describes the mux to restore when the driver is
> unbound or the system is shut down. "default" is pinctrl-0, matching the
> implicit ordering the pinctrl core already assigns it, and "inactive" is
> pinctrl-1; the driver looks each state up by name.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> Changes in v6:
> - No change. Rob Herring confirmed the two-entry ["default", "inactive"]
> items list is fine as-is; the "sleep" state discussed on the list is not
> something this binding needs
>
> Changes in v4:
> - Rework per Rob Herring: do not express the ordering in prose; use an
> ordered "items" list ("default" then "inactive") with minItems: 1,
> since pinctrl-0 is already implicitly "default" and its position is
> fixed. This also fixes the "['default', 'inactive'] is too long"
> dt_binding_check error seen on v3. Reword the commit message accordingly
>
> Changes in v3:
> - Do not constrain pinctrl-names to a fixed ["default", "inactive"]
> tuple. The driver looks the states up by name, so "inactive" may
> appear in any position and other states may coexist; only require
> (via "contains") that a "default" state exists, and reword the
> description accordingly
>
> Changes in v2:
> - Rename the released state from "idle" to "inactive"
>
> .../devicetree/bindings/pps/pps-gpio.yaml | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
In the future, don't thread new versions to old versions.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-23 18:22 ` [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
2026-09-23 18:41 ` Rob Herring
@ 2026-09-24 12:59 ` Rodolfo Giometti
1 sibling, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-24 12:59 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:42PM +0000, Eliav Farber wrote:
> Document the optional "default" and "inactive" pinctrl-names and show
> both in the example. The "default" state selects the PPS/GPIO function
> and is applied by the driver core before probe; the optional "inactive"
> state, when present, describes the mux to restore when the driver is
> unbound or the system is shut down. "default" is pinctrl-0, matching the
> implicit ordering the pinctrl core already assigns it, and "inactive" is
> pinctrl-1; the driver looks each state up by name.
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
` (2 preceding siblings ...)
2026-09-23 18:22 ` [PATCH v6 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-23 18:22 ` Eliav Farber
2026-09-24 12:59 ` Rodolfo Giometti
2026-10-01 7:53 ` Linus Walleij
2026-09-24 12:58 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Rodolfo Giometti
4 siblings, 2 replies; 57+ messages in thread
From: Eliav Farber @ 2026-09-23 18:22 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and need to
mux it to another function when pps-gpio is not driving PPS. The driver
core applies the "default" pinctrl state before probe, so the pins are
muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
the pins back when the driver is unbound or the system is shut down, so
they stay stuck in the GPIO function for whatever runs next, kexec
included.
Look up an optional "inactive" pinctrl state in probe via
devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
pinctrl_select_state() in remove() and shutdown(). The state is looked up
and selected by the driver itself rather than reusing the runtime-PM
"idle"/"sleep" states, so its meaning is unambiguous and it does not
depend on CONFIG_PM. Boards that do not describe an "inactive" state are
unaffected.
Since "inactive" is only meaningful as the mux to restore after the
core-applied "default" state, reject an "inactive" state that is not
paired with a "default" one rather than releasing pins that were never
put into a defined PPS state.
Look up the pinctrl states first in probe(), before pps_gpio_setup(), and
route every subsequent failure through a common err_release_pins label.
The driver core applies the "default" mux before probe(), so a probe that
fails after this point would otherwise leave the pins stuck in "default";
releasing them to "inactive" on the error path is the symmetrical
counterpart to what the core did on the driver's behalf. A failure in
pps_gpio_get_pins() itself returns directly, as no state was taken yet;
pps_gpio_release_pins() is a no-op when no "inactive" state was found.
Convert the pps_register_source() failure path to dev_err_probe() too,
so all three probe error paths that share err_release_pins log the same
way. This path already returned PTR_ERR(data->pps), so this is a logging
change, not a fix.
The mux must not change while something can still drive the pins. On
shutdown() the requested IRQ and the echo timer would otherwise outlive
the mux change -- device_shutdown() is not the end of the road, the
kernel keeps running to load and start the kexec image -- so a timer
callback or the PPS handler could poke a line that by then belongs to
another function. Tear down in the same order as remove(): free_irq()
first, then the echo timer (only when the board has an echo GPIO, as in
remove()), and the mux change last. shutdown() does not unregister the
PPS source, which is a remove-time concern.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v6:
- Convert the pps_register_source() failure path to dev_err_probe() +
goto err_release_pins as well, so all three probe error paths sharing
err_release_pins log the same way. It already returned PTR_ERR(data->pps),
so this is a logging change, not a fix. Fold in here rather than a
separate patch since this block is rewritten by this patch anyway, with
a commit-message note (Rodolfo Giometti)
Changes in v5:
- Resolve the v4 probe-failure open question by taking option "A + keep
the NULL guard": look the pinctrl states up first in probe(), before
pps_gpio_setup(), and route the pps_gpio_setup() failure through
err_release_pins too, so every path the driver can act on restores
"inactive". pps_gpio_release_pins() keeps its NULL guard, so it is a
no-op when no "inactive" state was found. Verified on the AL11 K2V6
JRD10 with a forced-defer test: the pins are released to "inactive" on
each failed attempt, the core re-applies "default" before the next, and
the mux settles at "default" on the eventual success, with no spurious
PPS event (Rodolfo Giometti)
- Clear data->pins_inactive when rejecting an "inactive" state that has no
"default", so a board deliberately rejected here can never have its pins
released to "inactive"
- Guard the echo-timer teardown in the new shutdown() with data->echo_pin,
matching remove() after the preceding patch (Rodolfo Giometti)
- Convert the gpiod_to_irq()/request_threaded_irq() error paths to the
log-only dev_err_probe() + goto err_release_pins form, following patch 1
Changes in v4:
- No functional change (probe-failure raised as an open question, now
resolved in v5 above)
Changes in v3:
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl", not a probe
failure; keep propagating everything else, e.g. -EPROBE_DEFER
- Restore the "inactive" mux on probe failure via a new err_release_pins
label
- Warn if pinctrl_select_state() fails to apply the "inactive" state
rather than silently ignoring the error
- Trim and de-duplicate the added comments
Changes in v2:
- Rename the released state from "idle" to "inactive"
- Look the state up in the driver with devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state() instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency
- Fix shutdown() to free_irq() and the echo teardown before the mux
change, matching remove()
- Require a "default" state whenever "inactive" is present and reject the
mismatch
drivers/pps/clients/pps-gpio.c | 121 ++++++++++++++++++++++++++++++---
1 file changed, 113 insertions(+), 8 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index aec534c246af..e6d39ca6777d 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/pps_kernel.h>
#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/list.h>
#include <linux/property.h>
#include <linux/timer.h>
@@ -30,6 +31,8 @@ struct pps_gpio_device_data {
struct gpio_desc *gpio_pin; /* GPIO port descriptors */
struct gpio_desc *echo_pin;
struct timer_list echo_timer; /* timer to reset echo active state */
+ struct pinctrl *pinctrl; /* pin control handle */
+ struct pinctrl_state *pins_inactive; /* pins released when unbound */
bool assert_falling_edge;
unsigned int echo_active_ms; /* PPS echo active duration */
unsigned long echo_timeout; /* timer timeout value in jiffies */
@@ -96,6 +99,68 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
gpiod_set_value(info->echo_pin, 0);
}
+/*
+ * Look up the optional "inactive" pinctrl state. It requires a "default"
+ * state (applied by the driver core before probe) and is rejected without
+ * one. Absent pinctrl, or an absent "inactive" state, is not an error.
+ */
+static int pps_gpio_get_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ struct pinctrl_state *pins_default;
+
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ /*
+ * A DT device without "pinctrl-0" yields -ENODEV, which
+ * is not an error here; propagate anything else.
+ */
+ if (PTR_ERR(data->pinctrl) == -ENODEV) {
+ data->pinctrl = NULL;
+ return 0;
+ }
+ return dev_err_probe(dev, PTR_ERR(data->pinctrl),
+ "failed to get pinctrl\n");
+ }
+
+ /* The "inactive" state is optional. */
+ data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
+ if (IS_ERR(data->pins_inactive)) {
+ data->pins_inactive = NULL;
+ return 0;
+ }
+
+ /* "inactive" requires a "default" state to return from. */
+ pins_default = pinctrl_lookup_state(data->pinctrl, "default");
+ if (IS_ERR(pins_default)) {
+ data->pins_inactive = NULL;
+ return dev_err_probe(dev, PTR_ERR(pins_default),
+ "\"inactive\" pinctrl state requires a \"default\" state\n");
+ }
+
+ return 0;
+}
+
+/*
+ * Restore the "inactive" pinctrl state, handing the pins back to whatever
+ * function uses them while pps-gpio is not driving PPS. This undoes the
+ * "default" state the driver core applied before probe. A no-op for boards
+ * that describe no "inactive" state.
+ */
+static void pps_gpio_release_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ if (!data->pins_inactive)
+ return;
+
+ ret = pinctrl_select_state(data->pinctrl, data->pins_inactive);
+ if (ret)
+ dev_warn(dev, "failed to select inactive pinctrl state: %d\n",
+ ret);
+}
+
static int pps_gpio_setup(struct device *dev)
{
struct pps_gpio_device_data *data = dev_get_drvdata(dev);
@@ -156,15 +221,25 @@ static int pps_gpio_probe(struct platform_device *pdev)
dev_set_drvdata(dev, data);
+ /*
+ * pinctrl setup (optional states) first, so the "inactive" mux can be
+ * restored on any later probe-failure path via err_release_pins.
+ */
+ ret = pps_gpio_get_pins(dev);
+ if (ret)
+ return ret;
+
/* GPIO setup */
ret = pps_gpio_setup(dev);
if (ret)
- return ret;
+ goto err_release_pins;
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0)
- return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ goto err_release_pins;
+ }
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -183,9 +258,10 @@ static int pps_gpio_probe(struct platform_device *pdev)
pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
data->pps = pps_register_source(&data->info, pps_default_params);
if (IS_ERR(data->pps)) {
- dev_err(dev, "failed to register IRQ %d as PPS source\n",
- data->irq);
- return PTR_ERR(data->pps);
+ ret = PTR_ERR(data->pps);
+ dev_err_probe(dev, ret, "failed to register IRQ %d as PPS source\n",
+ data->irq);
+ goto err_release_pins;
}
/* register IRQ interrupt handler */
@@ -195,14 +271,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
- data->irq);
+ dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
+ goto err_release_pins;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
data->irq);
return 0;
+
+err_release_pins:
+ /* Restore the inactive mux on probe failure; safe to do last here. */
+ pps_gpio_release_pins(dev);
+ return ret;
}
static void pps_gpio_remove(struct platform_device *pdev)
@@ -216,9 +298,31 @@ static void pps_gpio_remove(struct platform_device *pdev)
timer_delete_sync(&data->echo_timer);
gpiod_set_value(data->echo_pin, 0);
}
+ /* release the pins last, once nothing can drive them */
+ pps_gpio_release_pins(&pdev->dev);
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
+static void pps_gpio_shutdown(struct platform_device *pdev)
+{
+ struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+
+ /*
+ * The kernel keeps running after device_shutdown() (e.g. to load and
+ * start a kexec image), so quiesce the hardware before touching the
+ * mux: free the IRQ and stop the echo timer first, then release the
+ * pins last, so no callback can drive a pin after it is handed back.
+ * The PPS source is left registered; that is a remove-time concern.
+ */
+ free_irq(data->irq, data);
+ /* reset the echo state, if the board has an echo GPIO */
+ if (data->echo_pin) {
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ }
+ pps_gpio_release_pins(&pdev->dev);
+}
+
static const struct of_device_id pps_gpio_dt_ids[] = {
{ .compatible = "pps-gpio", },
{ /* sentinel */ }
@@ -228,6 +332,7 @@ MODULE_DEVICE_TABLE(of, pps_gpio_dt_ids);
static struct platform_driver pps_gpio_driver = {
.probe = pps_gpio_probe,
.remove = pps_gpio_remove,
+ .shutdown = pps_gpio_shutdown,
.driver = {
.name = PPS_GPIO_NAME,
.of_match_table = pps_gpio_dt_ids,
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-23 18:22 ` [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-24 12:59 ` Rodolfo Giometti
2026-10-01 7:53 ` Linus Walleij
1 sibling, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-24 12:59 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:43PM +0000, Eliav Farber wrote:
> Convert the pps_register_source() failure path to dev_err_probe() too,
> so all three probe error paths that share err_release_pins log the same
> way. This path already returned PTR_ERR(data->pps), so this is a logging
> change, not a fix.
That reads right, thanks.
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-23 18:22 ` [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
2026-09-24 12:59 ` Rodolfo Giometti
@ 2026-10-01 7:53 ` Linus Walleij
1 sibling, 0 replies; 57+ messages in thread
From: Linus Walleij @ 2026-10-01 7:53 UTC (permalink / raw)
To: Eliav Farber
Cc: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 8:23 PM Eliav Farber <farbere@amazon.com> wrote:
> Some boards route the PPS input GPIO through a pin controller and need to
> mux it to another function when pps-gpio is not driving PPS. The driver
> core applies the "default" pinctrl state before probe, so the pins are
> muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
> the pins back when the driver is unbound or the system is shut down, so
> they stay stuck in the GPIO function for whatever runs next, kexec
> included.
>
> Look up an optional "inactive" pinctrl state in probe via
> devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
> pinctrl_select_state() in remove() and shutdown(). The state is looked up
> and selected by the driver itself rather than reusing the runtime-PM
> "idle"/"sleep" states, so its meaning is unambiguous and it does not
> depend on CONFIG_PM. Boards that do not describe an "inactive" state are
> unaffected.
>
> Since "inactive" is only meaningful as the mux to restore after the
> core-applied "default" state, reject an "inactive" state that is not
> paired with a "default" one rather than releasing pins that were never
> put into a defined PPS state.
>
> Look up the pinctrl states first in probe(), before pps_gpio_setup(), and
> route every subsequent failure through a common err_release_pins label.
> The driver core applies the "default" mux before probe(), so a probe that
> fails after this point would otherwise leave the pins stuck in "default";
> releasing them to "inactive" on the error path is the symmetrical
> counterpart to what the core did on the driver's behalf. A failure in
> pps_gpio_get_pins() itself returns directly, as no state was taken yet;
> pps_gpio_release_pins() is a no-op when no "inactive" state was found.
>
> Convert the pps_register_source() failure path to dev_err_probe() too,
> so all three probe error paths that share err_release_pins log the same
> way. This path already returned PTR_ERR(data->pps), so this is a logging
> change, not a fix.
>
> The mux must not change while something can still drive the pins. On
> shutdown() the requested IRQ and the echo timer would otherwise outlive
> the mux change -- device_shutdown() is not the end of the road, the
> kernel keeps running to load and start the kexec image -- so a timer
> callback or the PPS handler could poke a line that by then belongs to
> another function. Tear down in the same order as remove(): free_irq()
> first, then the echo timer (only when the board has an echo GPIO, as in
> remove()), and the mux change last. shutdown() does not unregister the
> PPS source, which is a remove-time concern.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
v6 looks reasonable to me!
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v6 0/4] pps-gpio: restore pin mux on unbind and shutdown
2026-09-23 18:22 ` [PATCH v6 0/4] pps-gpio: restore pin mux on unbind " Eliav Farber
` (3 preceding siblings ...)
2026-09-23 18:22 ` [PATCH v6 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-24 12:58 ` Rodolfo Giometti
4 siblings, 0 replies; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-24 12:58 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andrew Morton
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Wed, Sep 23, 2026 at 06:22:39PM +0000, Eliav Farber wrote:
> Changes in v6:
> - Patch 4: fold the pps_register_source() failure path into the same
> dev_err_probe() + goto err_release_pins style as the other two paths
> sharing that label, with a commit-message note that it is a logging
> change, not a fix (Rodolfo Giometti)
That is exactly what I had in mind, thanks.
I applied the series here on v7.3-rc4: it applies cleanly, checkpatch
--strict is quiet on all four patches, and drivers/pps builds with W=1
and CONFIG_DEBUG_OBJECTS_TIMERS=y. I have acked the four patches
individually.
Andrew, could you please pick this up? LinuxPPS has no tree of its
own.
One leftover from the v5 review, not a blocker: the new shutdown()
runs on every board, including the ones that describe no "inactive"
state, where the free_irq() and the echo teardown buy nothing, and the
commit message of patch 4 still only speaks about the mux. If the
series needs a respin for some other reason, a line there would be
worth having -- otherwise it is fine as it stands.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
` (3 preceding siblings ...)
2026-09-19 17:11 ` [PATCH v4 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
@ 2026-09-19 17:11 ` Eliav Farber
2026-09-22 7:40 ` Rodolfo Giometti
2026-09-19 17:11 ` [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
2026-09-19 17:11 ` [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
6 siblings, 1 reply; 57+ messages in thread
From: Eliav Farber @ 2026-09-19 17:11 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel, Bartosz Golaszewski
On the two probe error paths that map and request the interrupt, probe
overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
-EPROBE_DEFER from the caller. The request_threaded_irq() failure message
also logged the IRQ number but not the errno.
Return the actual error code from both paths, and add the errno to the
request_threaded_irq() failure message.
Fixes: 161520451dfa ("pps: new client driver using GPIO")
Signed-off-by: Eliav Farber <farbere@amazon.com>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Changes in v4:
- Add Fixes: 161520451dfa ("pps: new client driver using GPIO") and
Bartosz Golaszewski's Reviewed-by. The hardcoded -EINVAL on both error
paths predates 4461d65176b4 (which only switched gpio_to_irq() to
gpiod_to_irq() and left those returns as context), so the tag points at
the original driver rather than the descriptor conversion
Changes in v3:
- New patch, split out of the pinctrl change: while converting the probe
error paths to a goto, Takashi Sakamoto noted that the hardcoded -EINVAL
discards the real gpiod_to_irq()/request_threaded_irq() error, so fix
that separately first
drivers/pps/clients/pps-gpio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 73ec2c7335e5..038c55c5f7d4 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -165,7 +165,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
ret = gpiod_to_irq(data->gpio_pin);
if (ret < 0) {
dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return -EINVAL;
+ return ret;
}
data->irq = ret;
@@ -197,8 +197,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
- return -EINVAL;
+ dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
+ return ret;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes
2026-09-19 17:11 ` [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-22 7:40 ` Rodolfo Giometti
2026-09-22 8:08 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 7:40 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel,
Bartosz Golaszewski
On Sat, Sep 19, 2026 at 05:11:55PM +0000, Eliav Farber wrote:
> Fixes: 161520451dfa ("pps: new client driver using GPIO")
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
You are right and I was wrong on v3. I went and looked: the hardcoded
-EINVAL on the gpio_to_irq() path is already there in 161520451dfa,
and 4461d65176b4 only turned gpio_to_irq() into gpiod_to_irq() while
leaving those returns alone. The tag points where it should.
> @@ -165,7 +165,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
> ret = gpiod_to_irq(data->gpio_pin);
> if (ret < 0) {
> dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
> - return -EINVAL;
> + return ret;
> }
> data->irq = ret;
Now that the real error survives, -EPROBE_DEFER survives with it -- and
this dev_err() will then shout once per retry, which is precisely what
dev_err_probe() exists to avoid. Wouldn't it be the right thing here?
pps_gpio_setup() in this same file already uses it, and so does
pps_gpio_get_pins() in your patch 3, so it would also leave the file
consistent with itself.
> @@ -197,8 +197,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
> data->info.name, data);
> if (ret) {
> pps_unregister_source(data->pps);
> - dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
> - return -EINVAL;
> + dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
> + return ret;
> }
Same question here, although I agree a deferral is far less likely on
this one.
Either way this is a nit, not an objection: the patch is right as it
stands.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread* RE: [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes
2026-09-22 7:40 ` Rodolfo Giometti
@ 2026-09-22 8:08 ` Farber, Eliav
2026-09-22 8:22 ` Rodolfo Giometti
0 siblings, 1 reply; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 8:08 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
Bartosz Golaszewski
On Mon, Sep 22, 2026 at 09:40:00AM +0200, Rodolfo Giometti wrote:
> > @@ -165,7 +165,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
> > ret = gpiod_to_irq(data->gpio_pin);
> > if (ret < 0) {
> > dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
> > - return -EINVAL;
> > + return ret;
> > }
> > data->irq = ret;
>
> Now that the real error survives, -EPROBE_DEFER survives with it -- and
> this dev_err() will then shout once per retry, which is precisely what
> dev_err_probe() exists to avoid. Wouldn't it be the right thing here?
> pps_gpio_setup() in this same file already uses it, and so does
> pps_gpio_get_pins() in your patch 3, so it would also leave the file
> consistent with itself.
Agreed on silencing the deferral spam. I avoided return dev_err_probe()
because patch 3 turns both returns into goto err_release_pins, and
dev_err_probe()'s idiom is to return the error, not goto.
To get the same suppression without the return, I'd open-code what its
kernel-doc says it replaces, minus the return:
if (ret != -EPROBE_DEFER)
dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
else
dev_dbg(dev, "failed to map GPIO to IRQ: %d\n", ret);
goto err_release_pins;
Would you like me to change it to this?
> > @@ -197,8 +197,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
> > data->info.name, data);
> > if (ret) {
> > pps_unregister_source(data->pps);
> > - dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
> > - return -EINVAL;
> > + dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
> > + return ret;
> > }
>
> Same question here, although I agree a deferral is far less likely on
> this one.
Same treatment here if you want it.
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes
2026-09-22 8:08 ` Farber, Eliav
@ 2026-09-22 8:22 ` Rodolfo Giometti
2026-09-22 10:06 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 8:22 UTC (permalink / raw)
To: Farber, Eliav, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
Bartosz Golaszewski
On Tue, Sep 22, 2026 at 08:08:09AM +0000, Farber, Eliav wrote:
> Agreed on silencing the deferral spam. I avoided return dev_err_probe()
> because patch 3 turns both returns into goto err_release_pins, and
> dev_err_probe()'s idiom is to return the error, not goto.
You don't have to return it. Calling it just for the message and then
jumping is fine, and ret already carries the code the label needs:
if (ret < 0) {
dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
goto err_release_pins;
}
But please don't open-code it: on the -EPROBE_DEFER branch it also
calls device_set_deferred_probe_reason(), so the hand-written version
silently drops the reason from /sys/kernel/debug/devices_deferred.
One more thing: dev_err_probe() already prints the code as %pe, so drop
the ": %d" from both messages -- data->irq of course stays in the second
one.
In this patch it can still be a plain
return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
and patch 3 turns it into the goto form above. Same for the request_irq
one.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes
2026-09-22 8:22 ` Rodolfo Giometti
@ 2026-09-22 10:06 ` Farber, Eliav
0 siblings, 0 replies; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 10:06 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
Bartosz Golaszewski
On Mon, Sep 22, 2026 at 10:22:00AM +0200, Rodolfo Giometti wrote:
> On Tue, Sep 22, 2026 at 08:08:09AM +0000, Farber, Eliav wrote:
> > Agreed on silencing the deferral spam. I avoided return
> > dev_err_probe() because patch 3 turns both returns into goto
> > err_release_pins, and dev_err_probe()'s idiom is to return the error, not goto.
>
> You don't have to return it. Calling it just for the message and then
> jumping is fine, and ret already carries the code the label needs:
>
> if (ret < 0) {
> dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
> goto err_release_pins;
> }
>
> But please don't open-code it: on the -EPROBE_DEFER branch it also
> calls device_set_deferred_probe_reason(), so the hand-written version
> silently drops the reason from /sys/kernel/debug/devices_deferred.
Done in v5: this patch uses return dev_err_probe(), and the pinctrl patch
turns it into the log-only dev_err_probe() + goto form you show above.
Also dropped the ": %d" from both messages (kept data->irq).
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
` (4 preceding siblings ...)
2026-09-19 17:11 ` [PATCH v4 1/3] pps: clients: gpio: propagate probe error codes Eliav Farber
@ 2026-09-19 17:11 ` Eliav Farber
2026-09-22 7:40 ` Rodolfo Giometti
2026-09-19 17:11 ` [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
6 siblings, 1 reply; 57+ messages in thread
From: Eliav Farber @ 2026-09-19 17:11 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
When the PPS input GPIO is routed through a pin controller, a board may
need to mux those pins to a different function while pps-gpio is not
driving PPS (for example after the driver is unbound or across a kexec).
Document the optional "default" and "inactive" pinctrl-names and show
both in the example. The "default" state selects the PPS/GPIO function
and is applied by the driver core before probe; the optional "inactive"
state, when present, describes the mux to restore when the driver is
unbound or the system is shut down. "default" is pinctrl-0, matching the
implicit ordering the pinctrl core already assigns it, and "inactive" is
pinctrl-1; the driver looks each state up by name.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v4:
- Rework per Rob Herring: do not express the ordering in prose; use an
ordered "items" list ("default" then "inactive") with minItems: 1,
since pinctrl-0 is already implicitly "default" and its position is
fixed. This also fixes the "['default', 'inactive'] is too long"
dt_binding_check error seen on v3. Reword the commit message accordingly
Changes in v3:
- Do not constrain pinctrl-names to a fixed ["default", "inactive"]
tuple. The driver looks the states up by name, so "inactive" may
appear in any position and other states may coexist; only require
(via "contains") that a "default" state exists, and reword the
description accordingly
Changes in v2:
- Rename the released state from "idle" to "inactive"
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
index 383a838744eb..61b5de6724ea 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
@@ -28,6 +28,17 @@ properties:
description: Indicates a falling edge assert, when present. Rising edge if absent.
type: boolean
+ pinctrl-names:
+ description:
+ The "default" state selects the PPS/GPIO function and is applied by the
+ driver core before probe. The optional "inactive" state, when present,
+ is selected when the driver is unbound or the system is shut down,
+ handing the pins back to their alternate function.
+ minItems: 1
+ items:
+ - const: default
+ - const: inactive
+
required:
- compatible
- gpios
@@ -40,8 +51,9 @@ examples:
pps {
compatible = "pps-gpio";
- pinctrl-names = "default";
+ pinctrl-names = "default", "inactive";
pinctrl-0 = <&pinctrl_pps>;
+ pinctrl-1 = <&pinctrl_pps_inactive>;
gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
assert-falling-edge;
echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-19 17:11 ` [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-22 7:40 ` Rodolfo Giometti
2026-09-22 10:11 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 7:40 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Sat, Sep 19, 2026 at 05:11:56PM +0000, Eliav Farber wrote:
> + minItems: 1
> + items:
> + - const: default
> + - const: inactive
Just to close what I reported on v3: I ran the binding check on this
one and the "['default', 'inactive'] is too long" failure is gone, so
consider my objection withdrawn. The binding itself is Rob's call.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-09-22 7:40 ` Rodolfo Giometti
@ 2026-09-22 10:11 ` Farber, Eliav
0 siblings, 0 replies; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 10:11 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Mon, Sep 22, 2026 at 09:40:00AM +0200, Rodolfo Giometti wrote:
> On Sat, Sep 19, 2026 at 05:11:56PM +0000, Eliav Farber wrote:
> > + minItems: 1
> > + items:
> > + - const: default
> > + - const: inactive
>
> Just to close what I reported on v3: I ran the binding check on this
> one and the "['default', 'inactive'] is too long" failure is gone, so
> consider my objection withdrawn. The binding itself is Rob's call.
Right, the v4 rework to the ordered "items" list (per Rob) fixed that.
v5 carries the binding unchanged.
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-17 7:56 ` [PATCH v3 0/3] pps-gpio: restore pin mux on unbind " Eliav Farber
` (5 preceding siblings ...)
2026-09-19 17:11 ` [PATCH v4 2/3] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-09-19 17:11 ` Eliav Farber
2026-09-22 7:40 ` Rodolfo Giometti
6 siblings, 1 reply; 57+ messages in thread
From: Eliav Farber @ 2026-09-19 17:11 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and need to
mux it to another function when pps-gpio is not driving PPS. The driver
core applies the "default" pinctrl state before probe, so the pins are
muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
the pins back when the driver is unbound or the system is shut down, so
they stay stuck in the GPIO function for whatever runs next, kexec
included.
Look up an optional "inactive" pinctrl state in probe via
devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
pinctrl_select_state() in remove() and shutdown(). The state is looked up
and selected by the driver itself rather than reusing the runtime-PM
"idle"/"sleep" states, so its meaning is unambiguous and it does not
depend on CONFIG_PM. Boards that do not describe an "inactive" state are
unaffected.
Since "inactive" is only meaningful as the mux to restore after the
core-applied "default" state, reject an "inactive" state that is not
paired with a "default" one rather than releasing pins that were never
put into a defined PPS state.
The mux must not change while something can still drive the pins. On
shutdown() the requested IRQ and the echo timer would otherwise outlive
the mux change -- device_shutdown() is not the end of the road, the
kernel keeps running to load and start the kexec image -- so a timer
callback or the PPS handler could poke a line that by then belongs to
another function. Tear down in the same order as remove(): free_irq() and
timer_delete_sync() first, and the mux change last. shutdown() does not
unregister the PPS source, which is a remove-time concern.
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
Changes in v4:
- No functional change. Raising an open question on the probe-failure
path for maintainer guidance rather than changing it (Sashiko [Medium]
on v3 3/3): a probe that fails before pps_gpio_get_pins() has run
(devm_kzalloc() or pps_gpio_setup()) returns without going through
err_release_pins, so the pins are left in the core-applied "default"
state rather than "inactive". Note the pins are physically muxed to
"default" by the driver core before probe(), and devres release on a
failed probe frees the pinctrl handle without selecting a prior state,
so the "default" mux persists. Options:
A. Move pps_gpio_get_pins() to the top of probe and route the
pps_gpio_setup() failure through err_release_pins too, so every
path the driver can act on restores "inactive". The devm_kzalloc()
failure is inherently before the driver holds any pinctrl handle,
so no option can cover it in the driver.
B. Keep as-is: treat "inactive" as a successful-ownership concern. A
probe that never looked up the state never took the pins, so
leaving the long-standing core-applied "default" is acceptable.
C. As A, but keep the release helper's NULL guard so it is robust
regardless of ordering.
I lean towards B; happy to implement A/C if uniform failure paths are
preferred.
Changes in v3:
- Treat -ENODEV from devm_pinctrl_get() (a DT device without a
"pinctrl-0" property) as "no pinctrl", not a probe failure; keep
propagating everything else, e.g. -EPROBE_DEFER
- Restore the "inactive" mux on probe failure too: route the error
paths after pps_gpio_get_pins() through a new err_release_pins label
so a failed probe does not leave the pins stuck in the "default" state
- Warn if pinctrl_select_state() fails to apply the "inactive" state
rather than silently ignoring the error (pps_gpio_release_pins() now
takes the struct device to log against)
- Trim and de-duplicate the added comments
Changes in v2:
- Rename the released state from "idle" to "inactive"
- Look the state up in the driver with devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state() instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency
- Fix shutdown() to free_irq() and timer_delete_sync() before the mux
change, matching remove(), so no IRQ or timer callback can drive a pin
after it has been handed back
- Require a "default" state whenever "inactive" is present and reject the
mismatch
drivers/pps/clients/pps-gpio.c | 100 ++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 038c55c5f7d4..2ee8fba8520c 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/pps_kernel.h>
#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/list.h>
#include <linux/property.h>
#include <linux/timer.h>
@@ -30,6 +31,8 @@ struct pps_gpio_device_data {
struct gpio_desc *gpio_pin; /* GPIO port descriptors */
struct gpio_desc *echo_pin;
struct timer_list echo_timer; /* timer to reset echo active state */
+ struct pinctrl *pinctrl; /* pin control handle */
+ struct pinctrl_state *pins_inactive; /* pins released when unbound */
bool assert_falling_edge;
unsigned int echo_active_ms; /* PPS echo active duration */
unsigned long echo_timeout; /* timer timeout value in jiffies */
@@ -96,6 +99,66 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
gpiod_set_value(info->echo_pin, 0);
}
+/*
+ * Look up the optional "inactive" pinctrl state. It requires a "default"
+ * state (applied by the driver core before probe) and is rejected without
+ * one. Absent pinctrl, or an absent "inactive" state, is not an error.
+ */
+static int pps_gpio_get_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ struct pinctrl_state *pins_default;
+
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ /*
+ * A DT device without "pinctrl-0" yields -ENODEV, which
+ * is not an error here; propagate anything else.
+ */
+ if (PTR_ERR(data->pinctrl) == -ENODEV) {
+ data->pinctrl = NULL;
+ return 0;
+ }
+ return dev_err_probe(dev, PTR_ERR(data->pinctrl),
+ "failed to get pinctrl\n");
+ }
+
+ /* The "inactive" state is optional. */
+ data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
+ if (IS_ERR(data->pins_inactive)) {
+ data->pins_inactive = NULL;
+ return 0;
+ }
+
+ /* "inactive" requires a "default" state to return from. */
+ pins_default = pinctrl_lookup_state(data->pinctrl, "default");
+ if (IS_ERR(pins_default))
+ return dev_err_probe(dev, PTR_ERR(pins_default),
+ "\"inactive\" pinctrl state requires a \"default\" state\n");
+
+ return 0;
+}
+
+/*
+ * Restore the "inactive" pinctrl state, handing the pins back to whatever
+ * function uses them while pps-gpio is not driving PPS. This undoes the
+ * "default" state the driver core applied before probe. A no-op for boards
+ * that describe no "inactive" state.
+ */
+static void pps_gpio_release_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ if (!data->pins_inactive)
+ return;
+
+ ret = pinctrl_select_state(data->pinctrl, data->pins_inactive);
+ if (ret)
+ dev_warn(dev, "failed to select inactive pinctrl state: %d\n",
+ ret);
+}
+
static int pps_gpio_setup(struct device *dev)
{
struct pps_gpio_device_data *data = dev_get_drvdata(dev);
@@ -161,11 +224,16 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (ret)
return ret;
+ /* pinctrl setup (optional states) */
+ ret = pps_gpio_get_pins(dev);
+ if (ret)
+ return ret;
+
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
if (ret < 0) {
dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return ret;
+ goto err_release_pins;
}
data->irq = ret;
@@ -187,7 +255,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (IS_ERR(data->pps)) {
dev_err(dev, "failed to register IRQ %d as PPS source\n",
data->irq);
- return PTR_ERR(data->pps);
+ ret = PTR_ERR(data->pps);
+ goto err_release_pins;
}
/* register IRQ interrupt handler */
@@ -198,13 +267,18 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (ret) {
pps_unregister_source(data->pps);
dev_err(dev, "failed to acquire IRQ %d: %d\n", data->irq, ret);
- return ret;
+ goto err_release_pins;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
data->irq);
return 0;
+
+err_release_pins:
+ /* Restore the inactive mux on probe failure; safe to do last here. */
+ pps_gpio_release_pins(dev);
+ return ret;
}
static void pps_gpio_remove(struct platform_device *pdev)
@@ -216,9 +290,28 @@ static void pps_gpio_remove(struct platform_device *pdev)
timer_delete_sync(&data->echo_timer);
/* reset echo pin in any case */
gpiod_set_value(data->echo_pin, 0);
+ /* release the pins last, once nothing can drive them */
+ pps_gpio_release_pins(&pdev->dev);
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
+static void pps_gpio_shutdown(struct platform_device *pdev)
+{
+ struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+
+ /*
+ * The kernel keeps running after device_shutdown() (e.g. to load and
+ * start a kexec image), so quiesce the hardware before touching the
+ * mux: free the IRQ and stop the echo timer first, then release the
+ * pins last, so no callback can drive a pin after it is handed back.
+ * The PPS source is left registered; that is a remove-time concern.
+ */
+ free_irq(data->irq, data);
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ pps_gpio_release_pins(&pdev->dev);
+}
+
static const struct of_device_id pps_gpio_dt_ids[] = {
{ .compatible = "pps-gpio", },
{ /* sentinel */ }
@@ -228,6 +321,7 @@ MODULE_DEVICE_TABLE(of, pps_gpio_dt_ids);
static struct platform_driver pps_gpio_driver = {
.probe = pps_gpio_probe,
.remove = pps_gpio_remove,
+ .shutdown = pps_gpio_shutdown,
.driver = {
.name = PPS_GPIO_NAME,
.of_match_table = pps_gpio_dt_ids,
--
2.47.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-19 17:11 ` [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
@ 2026-09-22 7:40 ` Rodolfo Giometti
2026-09-22 10:13 ` Farber, Eliav
0 siblings, 1 reply; 57+ messages in thread
From: Rodolfo Giometti @ 2026-09-22 7:40 UTC (permalink / raw)
To: Eliav Farber, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree, linux-gpio, linux-kernel
On Sat, Sep 19, 2026 at 05:11:57PM +0000, Eliav Farber wrote:
> +static void pps_gpio_shutdown(struct platform_device *pdev)
> +{
> + struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> +
> + /*
> + * The kernel keeps running after device_shutdown() (e.g. to load and
> + * start a kexec image), so quiesce the hardware before touching the
> + * mux: free the IRQ and stop the echo timer first, then release the
> + * pins last, so no callback can drive a pin after it is handed back.
> + * The PPS source is left registered; that is a remove-time concern.
> + */
> + free_irq(data->irq, data);
> + timer_delete_sync(&data->echo_timer);
> + gpiod_set_value(data->echo_pin, 0);
> + pps_gpio_release_pins(&pdev->dev);
> +}
The ordering argument convinced me, and I checked it against remove():
free_irq() really is the first thing remove() does, so releasing the
pins last is safe in both. Good.
The timer_delete_sync() worries me a little, though. timer_setup()
only runs when data->echo_pin is set, so on a board without echo-gpios
this touches a timer_list that was never initialised. remove() has
carried the same line for years, but remove() only happens on an
unbind, which hardly anyone does; .shutdown runs on every reboot and
every kexec, on every pps-gpio board there is. And as far as I can
tell no in-tree DT using pps-gpio describes echo-gpios at all, so that
is the ordinary case rather than the corner one.
Would you mind guarding it with if (data->echo_pin)? The
gpiod_set_value() beside it is already a no-op for a NULL descriptor,
so that one is fine as it stands. Whether you want to give remove()
the same guard while you are there is up to you -- it is pre-existing,
so I would not insist on it in this series.
> +/*
> + * Look up the optional "inactive" pinctrl state. It requires a "default"
> + * state (applied by the driver core before probe) and is rejected without
> + * one. Absent pinctrl, or an absent "inactive" state, is not an error.
> + */
> +static int pps_gpio_get_pins(struct device *dev)
The rest of the patch reads well to me, and the comments are the right
length now. My answer on the probe-failure question is on the cover
letter.
Ciao,
Rodolfo
^ permalink raw reply [flat|nested] 57+ messages in thread* RE: [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-09-22 7:40 ` Rodolfo Giometti
@ 2026-09-22 10:13 ` Farber, Eliav
0 siblings, 0 replies; 57+ messages in thread
From: Farber, Eliav @ 2026-09-22 10:13 UTC (permalink / raw)
To: Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam, Andrew Morton,
Takashi Sakamoto, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
On Mon, Sep 22, 2026 at 09:40:00AM +0200, Rodolfo Giometti wrote:
> On Sat, Sep 19, 2026 at 05:11:57PM +0000, Eliav Farber wrote:
> > +static void pps_gpio_shutdown(struct platform_device *pdev)
> > +{
> > + struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> > +
> > + /*
> > + * The kernel keeps running after device_shutdown() (e.g. to load and
> > + * start a kexec image), so quiesce the hardware before touching the
> > + * mux: free the IRQ and stop the echo timer first, then release the
> > + * pins last, so no callback can drive a pin after it is handed back.
> > + * The PPS source is left registered; that is a remove-time concern.
> > + */
> > + free_irq(data->irq, data);
> > + timer_delete_sync(&data->echo_timer);
> > + gpiod_set_value(data->echo_pin, 0);
> > + pps_gpio_release_pins(&pdev->dev);
> > +}
>
> The timer_delete_sync() worries me a little, though. timer_setup()
> only runs when data->echo_pin is set, so on a board without echo-gpios
> this touches a timer_list that was never initialised.
>
> Would you mind guarding it with if (data->echo_pin)? [...]
> Whether you want to give remove() the same guard while you are there is
> up to you -- it is pre-existing, so I would not insist on it in this
> series.
Done in v5. shutdown() now guards the echo teardown with data->echo_pin,
and I fixed remove() too -- as a separate preparatory patch, since it is
pre-existing (Fixes: fde046a8c490, which had dropped that guard). It also
trips debug_assert_init() under CONFIG_DEBUG_OBJECTS_TIMERS, not just an
uninitialised-timer concern.
Thanks,
Eliav
^ permalink raw reply [flat|nested] 57+ messages in thread