RE: [PATCH v2 2/2] pps: clients: gpio: release pins to an inactive state on remove and shutdown

[Farber, Eliav]

On 17/09/2026 10:14, Rodolfo Giometti wrote:
>> +static int pps_gpio_get_pins(struct device *dev) {
>> +     data->pinctrl = devm_pinctrl_get(dev);
>> +     if (IS_ERR(data->pinctrl))
>> +             return dev_err_probe(dev, PTR_ERR(data->pinctrl),
>> +                                  "failed to get pinctrl\n");
>
> pinctrl_dt_to_map() returns -ENODEV when the node has no pinctrl-0 and
> create_pinctrl() forwards it. So this fails the probe on every pps-gpio
> board that describes no pinctrl at all, which is most of them; your
> setup has one, so the test does not show it. Shouldn't -ENODEV simply
> mean "no pinctrl here, nothing to do"?

Agreed, that's a real regression for boards without pinctrl. In v3
-ENODEV is treated as "no pinctrl described", not an error, and
everything else (including -EPROBE_DEFER) is still propagated:

        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");
        }

I've also tested a pps-gpio node with no pinctrl at all to confirm probe,
remove and shutdown behave as before.

>> +static void pps_gpio_release_pins(struct pps_gpio_device_data *data)
>> +{
>> +     if (data->pins_inactive)
>> +             pinctrl_select_state(data->pinctrl, data->pins_inactive);
>> +}
>
> The return value is the only sign that the mux was not restored, which
> is what the whole series is for. Why drop it?

You're right. In v3 the failure is logged; the helper now takes the
struct device so it can warn (remove()/shutdown() are void and the probe
error path must keep the original error, so it warns rather than
propagates):

        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);
        }

Thanks,
Eliav