Re: [PATCH 2/7] pinctrl: renesas: rzt2h: restore correct pin mode on IRQ free

[Geert Uytterhoeven]

Hi Cosmin,

On Tue, 8 Sept 2026 at 13:33, Cosmin-Gabriel Tanislav
<cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> wrote:
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > On Mon, 17 Aug 2026 at 20:58, Cosmin Tanislav
> > <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> wrote:
> > > rzt2h_gpio_irq_domain_free() calls rzt2h_pinctrl_set_gpio_en() with
> > > false leaving the pin in interrupt function instead of returning it to
> > > GPIO mode.
> >
> > OK....
> >
> > > Pass true to rzt2h_pinctrl_set_gpio_en() to take the pin out of
> > > interrupt function after we're done using it as an IRQ.
> > >
> > > rzt2h_pinctrl_set_pfc_mode() switches the pin to Hi-Z, losing the
> > > previous PM value.
> > >
> > > Save the PM value before switching to Hi-Z, and restore it after the
> > > IRQ is freed.
> >
> > Why is it a problem if the pin is in Hi-Z after the interrupt has been freed?
> > Isn't that the right state for an unused pin?
>
> It wouldn't be an issue if it was actually unused once the IRQ is freed,
> but that's not the case, because you can call gpiod_to_irq() on a GPIO,
> and then request_irq() on the IRQ returned by it. If you then call
> free_irq(), you only free the IRQ, not the GPIO itself. After that the
> user of these APIs should still be able to continue using the GPIO.

OK.

> If the pin is left in Hi-Z after freeing the IRQ, rzt2h_gpio_get() will
> see that the pin is not in peripheral mode anymore, it will then see
> that neither PM_INPUT nor PM_OUTPUT are set, and will return -EINVAL.

That is fixed by "[PATCH 4/7] pinctrl: renesas: rzt2h: fix reading pin
value in IRQ function" anyway, right?

> > > --- a/drivers/pinctrl/renesas/pinctrl-rzt2h.c
> > > +++ b/drivers/pinctrl/renesas/pinctrl-rzt2h.c
> >
> > > @@ -170,6 +171,25 @@ static int rzt2h_validate_pin(struct rzt2h_pinctrl *pctrl, unsigned int offset)
> > >         return (pincfg & BIT(pin)) ? 0 : -EINVAL;
> > >  }
> > >
> > > +static u8 rzt2h_pin_read_pm(struct rzt2h_pinctrl *pctrl, u8 port, u8 pin)
> > > +{
> > > +       u16 reg = rzt2h_pinctrl_readw(pctrl, port, PM(port));
> > > +
> > > +       return field_get(PM_PIN_MASK(pin), reg);
> > > +}
> > > +
> > > +static void rzt2h_pin_write_pm(struct rzt2h_pinctrl *pctrl, u8 port, u8 pin, u8 pm)
> > > +{
> > > +       u16 reg;
> > > +
> > > +       guard(raw_spinlock_irqsave)(&pctrl->lock);
> > > +
> > > +       reg = rzt2h_pinctrl_readw(pctrl, port, PM(port));
> > > +       reg &= ~PM_PIN_MASK(pin);
> > > +       reg |= (u16)pm << (pin * 2);
> >
> > General remark: the need for this cast is one reason why function
> > parameters should use "unsigned int" instead of u8.  The other reason is
> > that the compiler may add code to mask the parameters to 8 bits,
> > depending on the CPU architecture.
>
> Should I switch the pm parameter to unsigned int for V2 then?

Yes please.

> > > +       rzt2h_pinctrl_writew(pctrl, port, reg, PM(port));
> > > +}
> > > +
> > >  static void rzt2h_pinctrl_set_gpio_en(struct rzt2h_pinctrl *pctrl,
> > >                                       u8 port, u8 pin, bool en)
> > >  {
> > > @@ -1023,16 +1043,23 @@ static int rzt2h_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> > >         struct rzt2h_pinctrl *pctrl = gpiochip_get_data(gc);
> > >         u8 port = RZT2H_PIN_ID_TO_PORT(child);
> > >         u8 pin = RZT2H_PIN_ID_TO_PIN(child);
> > > -       u8 parent_irq;
> > > +       u8 parent_irq, irq_idx;
> > >
> > >         parent_irq = rzt2h_gpio_irq_map[child];
> > >         if (parent_irq < RZT2H_INTERRUPTS_START)
> > >                 return -EINVAL;
> > >
> > > -       if (test_and_set_bit(parent_irq - RZT2H_INTERRUPTS_START,
> > > -                            pctrl->used_irqs))
> > > +       irq_idx = parent_irq - RZT2H_INTERRUPTS_START;
> > > +       if (test_and_set_bit(irq_idx, pctrl->used_irqs))
> > >                 return -EBUSY;
> > >
> > > +       /*
> > > +        * rzt2h_pinctrl_set_pfc_mode() sets PM to Hi-Z before switching to the
> > > +        * interrupt function, losing the previous PM value.
> > > +        * Save it so it can be restored when the IRQ is freed.
> > > +        */
> > > +       pctrl->saved_pm[irq_idx] = rzt2h_pin_read_pm(pctrl, port, pin);
> > > +
> > >         rzt2h_pinctrl_set_pfc_mode(pctrl, port, pin, PFC_FUNC_INTERRUPT);
> >
> > If rzt2h_pinctrl_set_pfc_mode() would return the old PM value, you
> > could drop rzt2h_pin_read_pm(), and write:
>
> The main function of rzt2h_pinctrl_set_pfc_mode() is to change the PFC
> mode. Returning the old PM value as a side effect would mean the
> function does two unrelated things.
>
> The extra register read this avoids only happens on IRQ request/release,
> not a hot path, so I'd rather keep the two concerns separate if that's
> okay.

Fair enough.

>
> >     pctrl->saved_pm[irq_idx] = rzt2h_pinctrl_set_pfc_mode(pctrl, port, pin,
> >                                                      PFC_FUNC_INTERRUPT);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds