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

[Geert Uytterhoeven]

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?

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

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

    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