Re: [PATCH v4 3/3] pps: clients: gpio: release pins to an inactive state on remove and shutdown

[Rodolfo Giometti]

On Sat, Sep 19, 2026 at 05:11:57PM +0000, Eliav Farber wrote:
> +static void pps_gpio_shutdown(struct platform_device *pdev)
> +{
> +	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * The kernel keeps running after device_shutdown() (e.g. to load and
> +	 * start a kexec image), so quiesce the hardware before touching the
> +	 * mux: free the IRQ and stop the echo timer first, then release the
> +	 * pins last, so no callback can drive a pin after it is handed back.
> +	 * The PPS source is left registered; that is a remove-time concern.
> +	 */
> +	free_irq(data->irq, data);
> +	timer_delete_sync(&data->echo_timer);
> +	gpiod_set_value(data->echo_pin, 0);
> +	pps_gpio_release_pins(&pdev->dev);
> +}

The ordering argument convinced me, and I checked it against remove():
free_irq() really is the first thing remove() does, so releasing the
pins last is safe in both.  Good.

The timer_delete_sync() worries me a little, though.  timer_setup()
only runs when data->echo_pin is set, so on a board without echo-gpios
this touches a timer_list that was never initialised.  remove() has
carried the same line for years, but remove() only happens on an
unbind, which hardly anyone does; .shutdown runs on every reboot and
every kexec, on every pps-gpio board there is.  And as far as I can
tell no in-tree DT using pps-gpio describes echo-gpios at all, so that
is the ordinary case rather than the corner one.

Would you mind guarding it with if (data->echo_pin)?  The
gpiod_set_value() beside it is already a no-op for a NULL descriptor,
so that one is fine as it stands.  Whether you want to give remove()
the same guard while you are there is up to you -- it is pre-existing,
so I would not insist on it in this series.

> +/*
> + * Look up the optional "inactive" pinctrl state. It requires a "default"
> + * state (applied by the driver core before probe) and is rejected without
> + * one. Absent pinctrl, or an absent "inactive" state, is not an error.
> + */
> +static int pps_gpio_get_pins(struct device *dev)

The rest of the patch reads well to me, and the comments are the right
length now.  My answer on the probe-failure question is on the cover
letter.

Ciao,

Rodolfo