* [PATCH v7 1/4] pps: clients: gpio: propagate probe error codes
2026-11-04 17:29 [PATCH v7 0/4] pps-gpio: restore pin mux on unbind and shutdown Eliav Farber
@ 2026-11-04 17:29 ` Eliav Farber
2026-11-04 17:29 ` [PATCH v7 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Eliav Farber @ 2026-11-04 17:29 UTC (permalink / raw)
To: Andrew Morton, Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel, Bartosz Golaszewski
On the two probe error paths that map and request the interrupt, probe
overwrote the error from gpiod_to_irq() and request_threaded_irq() with a
hardcoded -EINVAL, hiding meaningful codes such as -EBUSY, -ENOMEM or
-EPROBE_DEFER from the caller. The request_threaded_irq() failure message
also logged the IRQ number but not the errno.
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>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Changes in v7:
- Add Acked-by (Rodolfo Giometti) and Reviewed-by (Bartosz Golaszewski).
No other change
Changes in v6:
- No change
Changes in v5:
- Use dev_err_probe() on both error paths instead of dev_err() + return,
so a propagated -EPROBE_DEFER is logged at debug level (no console spam
on repeated deferral) and the code is emitted symbolically. This also
matches pps_gpio_setup() in the same file. Drop Bartosz Golaszewski's
Reviewed-by as the patch changed materially
Changes in v4:
- Add Fixes: 161520451dfa ("pps: new client driver using GPIO") and
Bartosz Golaszewski's Reviewed-by. The hardcoded -EINVAL on both error
paths predates 4461d65176b4 (which only switched gpio_to_irq() to
gpiod_to_irq() and left those returns as context), so the tag points at
the original driver rather than the descriptor conversion
Changes in v3:
- New patch, split out of the pinctrl change: while converting the probe
error paths to a goto, Takashi Sakamoto noted that the hardcoded -EINVAL
discards the real gpiod_to_irq()/request_threaded_irq() error, so fix
that separately first
drivers/pps/clients/pps-gpio.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 73ec2c7335e5..ccc2fb470b7e 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -163,10 +163,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0) {
- dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
- return -EINVAL;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -197,8 +195,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
- return -EINVAL;
+ return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
--
2.47.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v7 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states
2026-11-04 17:29 [PATCH v7 0/4] pps-gpio: restore pin mux on unbind and shutdown Eliav Farber
2026-11-04 17:29 ` [PATCH v7 1/4] pps: clients: gpio: propagate probe error codes Eliav Farber
2026-11-04 17:29 ` [PATCH v7 2/4] pps: clients: gpio: only tear down the echo timer when it exists Eliav Farber
@ 2026-11-04 17:29 ` Eliav Farber
2026-11-04 17:29 ` [PATCH v7 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown Eliav Farber
3 siblings, 0 replies; 5+ messages in thread
From: Eliav Farber @ 2026-11-04 17:29 UTC (permalink / raw)
To: Andrew Morton, Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
When the PPS input GPIO is routed through a pin controller, a board may
need to mux those pins to a different function while pps-gpio is not
driving PPS (for example after the driver is unbound or across a kexec).
Document the optional "default" and "inactive" pinctrl-names and show
both in the example. The "default" state selects the PPS/GPIO function
and is applied by the driver core before probe; the optional "inactive"
state, when present, describes the mux to restore when the driver is
unbound or the system is shut down. "default" is pinctrl-0, matching the
implicit ordering the pinctrl core already assigns it, and "inactive" is
pinctrl-1; the driver looks each state up by name.
Signed-off-by: Eliav Farber <farbere@amazon.com>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v7:
- Add Acked-by (Rodolfo Giometti) and Reviewed-by (Rob Herring). No other
change
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 v5:
- No change
Changes in v4:
- Rework per Rob Herring: do not express the ordering in prose; use an
ordered "items" list ("default" then "inactive") with minItems: 1,
since pinctrl-0 is already implicitly "default" and its position is
fixed. This also fixes the "['default', 'inactive'] is too long"
dt_binding_check error seen on v3. Reword the commit message accordingly
Changes in v3:
- Do not constrain pinctrl-names to a fixed ["default", "inactive"]
tuple. The driver looks the states up by name, so "inactive" may
appear in any position and other states may coexist; only require
(via "contains") that a "default" state exists, and reword the
description accordingly
Changes in v2:
- Rename the released state from "idle" to "inactive"
.../devicetree/bindings/pps/pps-gpio.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.yaml b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
index 383a838744eb..61b5de6724ea 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.yaml
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.yaml
@@ -28,6 +28,17 @@ properties:
description: Indicates a falling edge assert, when present. Rising edge if absent.
type: boolean
+ pinctrl-names:
+ description:
+ The "default" state selects the PPS/GPIO function and is applied by the
+ driver core before probe. The optional "inactive" state, when present,
+ is selected when the driver is unbound or the system is shut down,
+ handing the pins back to their alternate function.
+ minItems: 1
+ items:
+ - const: default
+ - const: inactive
+
required:
- compatible
- gpios
@@ -40,8 +51,9 @@ examples:
pps {
compatible = "pps-gpio";
- pinctrl-names = "default";
+ pinctrl-names = "default", "inactive";
pinctrl-0 = <&pinctrl_pps>;
+ pinctrl-1 = <&pinctrl_pps_inactive>;
gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
assert-falling-edge;
echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
--
2.47.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v7 4/4] pps: clients: gpio: release pins to an inactive state on remove and shutdown
2026-11-04 17:29 [PATCH v7 0/4] pps-gpio: restore pin mux on unbind and shutdown Eliav Farber
` (2 preceding siblings ...)
2026-11-04 17:29 ` [PATCH v7 3/4] dt-bindings: pps: pps-gpio: document optional pinctrl states Eliav Farber
@ 2026-11-04 17:29 ` Eliav Farber
3 siblings, 0 replies; 5+ messages in thread
From: Eliav Farber @ 2026-11-04 17:29 UTC (permalink / raw)
To: Andrew Morton, Rodolfo Giometti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Linus Walleij, Bartosz Golaszewski, Fabio Estevam,
Takashi Sakamoto, Eliav Farber, devicetree, linux-gpio,
linux-kernel
Some boards route the PPS input GPIO through a pin controller and need to
mux it to another function when pps-gpio is not driving PPS. The driver
core applies the "default" pinctrl state before probe, so the pins are
muxed for GPIO/PPS use while the driver is bound. Nothing, however, hands
the pins back when the driver is unbound or the system is shut down, so
they stay stuck in the GPIO function for whatever runs next, kexec
included.
Look up an optional "inactive" pinctrl state in probe via
devm_pinctrl_get() and pinctrl_lookup_state(), and select it with
pinctrl_select_state() in remove() and shutdown(). The state is looked up
and selected by the driver itself rather than reusing the runtime-PM
"idle"/"sleep" states, so its meaning is unambiguous and it does not
depend on CONFIG_PM. Boards that do not describe an "inactive" state are
unaffected.
Since "inactive" is only meaningful as the mux to restore after the
core-applied "default" state, reject an "inactive" state that is not
paired with a "default" one rather than releasing pins that were never
put into a defined PPS state.
Look up the pinctrl states first in probe(), before pps_gpio_setup(), and
route every subsequent failure through a common err_release_pins label.
The driver core applies the "default" mux before probe(), so a probe that
fails after this point would otherwise leave the pins stuck in "default";
releasing them to "inactive" on the error path is the symmetrical
counterpart to what the core did on the driver's behalf. A failure in
pps_gpio_get_pins() itself returns directly, as no state was taken yet;
pps_gpio_release_pins() is a no-op when no "inactive" state was found.
Convert the pps_register_source() failure path to dev_err_probe() too,
so all three probe error paths that share err_release_pins log the same
way. This path already returned PTR_ERR(data->pps), so this is a logging
change, not a fix.
The mux must not change while something can still drive the pins. On
shutdown() the requested IRQ and the echo timer would otherwise outlive
the mux change -- device_shutdown() is not the end of the road, the
kernel keeps running to load and start the kexec image -- so a timer
callback or the PPS handler could poke a line that by then belongs to
another function. Tear down in the same order as remove(): free_irq()
first, then the echo timer (only when the board has an echo GPIO, as in
remove()), and the mux change last. shutdown() does not unregister the
PPS source, which is a remove-time concern.
shutdown() is registered unconditionally, so it also runs on boards that
describe no "inactive" state. There the mux change is a no-op, and freeing
the IRQ and stopping the echo timer buy nothing, but they are harmless at
that point and keep a single teardown sequence that does not depend on the
pinctrl description.
Signed-off-by: Eliav Farber <farbere@amazon.com>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
---
Changes in v7:
- Commit message only: note that shutdown() also runs on boards with no
"inactive" state, where the IRQ/echo teardown is harmless but not
needed (Rodolfo Giometti). No code change; tags kept
- Add Acked-by (Rodolfo Giometti) and Reviewed-by (Linus Walleij)
Changes in v6:
- Convert the pps_register_source() failure path to dev_err_probe() +
goto err_release_pins as well, so all three probe error paths sharing
err_release_pins log the same way. It already returned PTR_ERR(data->pps),
so this is a logging change, not a fix. Fold in here rather than a
separate patch since this block is rewritten by this patch anyway, with
a commit-message note (Rodolfo Giometti)
Changes in v5:
- Resolve the v4 probe-failure open question by taking option "A + keep
the NULL guard": look the pinctrl states up first in probe(), before
pps_gpio_setup(), and route the pps_gpio_setup() failure through
err_release_pins too, so every path the driver can act on restores
"inactive". pps_gpio_release_pins() keeps its NULL guard, so it is a
no-op when no "inactive" state was found. Verified on the AL11 K2V6
JRD10 with a forced-defer test: the pins are released to "inactive" on
each failed attempt, the core re-applies "default" before the next, and
the mux settles at "default" on the eventual success, with no spurious
PPS event (Rodolfo Giometti)
- Clear data->pins_inactive when rejecting an "inactive" state that has no
"default", so a board deliberately rejected here can never have its pins
released to "inactive"
- Guard the echo-timer teardown in the new shutdown() with data->echo_pin,
matching remove() after the preceding patch (Rodolfo Giometti)
- Convert the gpiod_to_irq()/request_threaded_irq() error paths to the
log-only dev_err_probe() + goto err_release_pins form, following patch 1
Changes in v4:
- No functional change (probe-failure raised as an open question, now
resolved in v5 above)
Changes in v3:
- Treat -ENODEV from devm_pinctrl_get() as "no pinctrl", not a probe
failure; keep propagating everything else, e.g. -EPROBE_DEFER
- Restore the "inactive" mux on probe failure via a new err_release_pins
label
- Warn if pinctrl_select_state() fails to apply the "inactive" state
rather than silently ignoring the error
- Trim and de-duplicate the added comments
Changes in v2:
- Rename the released state from "idle" to "inactive"
- Look the state up in the driver with devm_pinctrl_get() +
pinctrl_lookup_state() + pinctrl_select_state() instead of
pinctrl_pm_select_idle_state(), removing the CONFIG_PM dependency
- Fix shutdown() to free_irq() and the echo teardown before the mux
change, matching remove()
- Require a "default" state whenever "inactive" is present and reject the
mismatch
drivers/pps/clients/pps-gpio.c | 121 ++++++++++++++++++++++++++++++---
1 file changed, 113 insertions(+), 8 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index aec534c246af..e6d39ca6777d 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/pps_kernel.h>
#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/list.h>
#include <linux/property.h>
#include <linux/timer.h>
@@ -30,6 +31,8 @@ struct pps_gpio_device_data {
struct gpio_desc *gpio_pin; /* GPIO port descriptors */
struct gpio_desc *echo_pin;
struct timer_list echo_timer; /* timer to reset echo active state */
+ struct pinctrl *pinctrl; /* pin control handle */
+ struct pinctrl_state *pins_inactive; /* pins released when unbound */
bool assert_falling_edge;
unsigned int echo_active_ms; /* PPS echo active duration */
unsigned long echo_timeout; /* timer timeout value in jiffies */
@@ -96,6 +99,68 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
gpiod_set_value(info->echo_pin, 0);
}
+/*
+ * Look up the optional "inactive" pinctrl state. It requires a "default"
+ * state (applied by the driver core before probe) and is rejected without
+ * one. Absent pinctrl, or an absent "inactive" state, is not an error.
+ */
+static int pps_gpio_get_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ struct pinctrl_state *pins_default;
+
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ /*
+ * A DT device without "pinctrl-0" yields -ENODEV, which
+ * is not an error here; propagate anything else.
+ */
+ if (PTR_ERR(data->pinctrl) == -ENODEV) {
+ data->pinctrl = NULL;
+ return 0;
+ }
+ return dev_err_probe(dev, PTR_ERR(data->pinctrl),
+ "failed to get pinctrl\n");
+ }
+
+ /* The "inactive" state is optional. */
+ data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
+ if (IS_ERR(data->pins_inactive)) {
+ data->pins_inactive = NULL;
+ return 0;
+ }
+
+ /* "inactive" requires a "default" state to return from. */
+ pins_default = pinctrl_lookup_state(data->pinctrl, "default");
+ if (IS_ERR(pins_default)) {
+ data->pins_inactive = NULL;
+ return dev_err_probe(dev, PTR_ERR(pins_default),
+ "\"inactive\" pinctrl state requires a \"default\" state\n");
+ }
+
+ return 0;
+}
+
+/*
+ * Restore the "inactive" pinctrl state, handing the pins back to whatever
+ * function uses them while pps-gpio is not driving PPS. This undoes the
+ * "default" state the driver core applied before probe. A no-op for boards
+ * that describe no "inactive" state.
+ */
+static void pps_gpio_release_pins(struct device *dev)
+{
+ struct pps_gpio_device_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ if (!data->pins_inactive)
+ return;
+
+ ret = pinctrl_select_state(data->pinctrl, data->pins_inactive);
+ if (ret)
+ dev_warn(dev, "failed to select inactive pinctrl state: %d\n",
+ ret);
+}
+
static int pps_gpio_setup(struct device *dev)
{
struct pps_gpio_device_data *data = dev_get_drvdata(dev);
@@ -156,15 +221,25 @@ static int pps_gpio_probe(struct platform_device *pdev)
dev_set_drvdata(dev, data);
+ /*
+ * pinctrl setup (optional states) first, so the "inactive" mux can be
+ * restored on any later probe-failure path via err_release_pins.
+ */
+ ret = pps_gpio_get_pins(dev);
+ if (ret)
+ return ret;
+
/* GPIO setup */
ret = pps_gpio_setup(dev);
if (ret)
- return ret;
+ goto err_release_pins;
/* IRQ setup */
ret = gpiod_to_irq(data->gpio_pin);
- if (ret < 0)
- return dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to map GPIO to IRQ\n");
+ goto err_release_pins;
+ }
data->irq = ret;
/* initialize PPS specific parts of the bookkeeping data structure. */
@@ -183,9 +258,10 @@ static int pps_gpio_probe(struct platform_device *pdev)
pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
data->pps = pps_register_source(&data->info, pps_default_params);
if (IS_ERR(data->pps)) {
- dev_err(dev, "failed to register IRQ %d as PPS source\n",
- data->irq);
- return PTR_ERR(data->pps);
+ ret = PTR_ERR(data->pps);
+ dev_err_probe(dev, ret, "failed to register IRQ %d as PPS source\n",
+ data->irq);
+ goto err_release_pins;
}
/* register IRQ interrupt handler */
@@ -195,14 +271,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
- return dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
- data->irq);
+ dev_err_probe(dev, ret, "failed to acquire IRQ %d\n",
+ data->irq);
+ goto err_release_pins;
}
dev_dbg(&data->pps->dev, "Registered IRQ %d as PPS source\n",
data->irq);
return 0;
+
+err_release_pins:
+ /* Restore the inactive mux on probe failure; safe to do last here. */
+ pps_gpio_release_pins(dev);
+ return ret;
}
static void pps_gpio_remove(struct platform_device *pdev)
@@ -216,9 +298,31 @@ static void pps_gpio_remove(struct platform_device *pdev)
timer_delete_sync(&data->echo_timer);
gpiod_set_value(data->echo_pin, 0);
}
+ /* release the pins last, once nothing can drive them */
+ pps_gpio_release_pins(&pdev->dev);
dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
}
+static void pps_gpio_shutdown(struct platform_device *pdev)
+{
+ struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+
+ /*
+ * The kernel keeps running after device_shutdown() (e.g. to load and
+ * start a kexec image), so quiesce the hardware before touching the
+ * mux: free the IRQ and stop the echo timer first, then release the
+ * pins last, so no callback can drive a pin after it is handed back.
+ * The PPS source is left registered; that is a remove-time concern.
+ */
+ free_irq(data->irq, data);
+ /* reset the echo state, if the board has an echo GPIO */
+ if (data->echo_pin) {
+ timer_delete_sync(&data->echo_timer);
+ gpiod_set_value(data->echo_pin, 0);
+ }
+ pps_gpio_release_pins(&pdev->dev);
+}
+
static const struct of_device_id pps_gpio_dt_ids[] = {
{ .compatible = "pps-gpio", },
{ /* sentinel */ }
@@ -228,6 +332,7 @@ MODULE_DEVICE_TABLE(of, pps_gpio_dt_ids);
static struct platform_driver pps_gpio_driver = {
.probe = pps_gpio_probe,
.remove = pps_gpio_remove,
+ .shutdown = pps_gpio_shutdown,
.driver = {
.name = PPS_GPIO_NAME,
.of_match_table = pps_gpio_dt_ids,
--
2.47.3
^ permalink raw reply related [flat|nested] 5+ messages in thread