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

[Cosmin-Gabriel Tanislav]

> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Tuesday, September 8, 2026 12:25 PM
> 
> Hi Cosmin,
> 
> Thanks for your patch!
> 
> 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.

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.

> >
> > Cc: stable@xxxxxxxxxx
> > Fixes: 829dde3369a9 ("pinctrl: renesas: rzt2h: Add GPIO IRQ chip to handle interrupts")
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>
> 
> > --- 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?

> > +       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.

>     pctrl->saved_pm[irq_idx] = rzt2h_pinctrl_set_pfc_mode(pctrl, port, pin,
>                                                      PFC_FUNC_INTERRUPT);
> 
> >
> >         *parent = parent_irq;
> 
> 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