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

[Farber, Eliav]

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