.../devicetree/bindings/pps/pps-gpio.yaml | 14 ++- drivers/pps/clients/pps-gpio.c | 102 +++++++++++++++++- 2 files changed, 111 insertions(+), 5 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 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
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
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.3remove() 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.3When 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.3On 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
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
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
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
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.3On 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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.