.../devicetree/bindings/pps/pps-gpio.yaml | 14 +- drivers/pps/clients/pps-gpio.c | 123 ++++++++++++++++-- 2 files changed, 127 insertions(+), 10 deletions(-)
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
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
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
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.3On 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>
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>
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.3On 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>
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>
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.3On 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>
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.
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.3On 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
© 2016 - 2026 Red Hat, Inc.