RE: [PATCH 4/8] pinctrl: renesas: rzt2h: add GPIO IRQ chip to handle interrupts

[Cosmin-Gabriel Tanislav]

> From: Linus Walleij <linusw@xxxxxxxxxx>
> Sent: Thursday, December 4, 2025 1:34 AM
> 
> Hi Cosmin,
> 
> thanks for your patch!
> 

Hi Linus, thank you for your feedback.

> On Fri, Nov 21, 2025 at 12:27 PM Cosmin Tanislav
> <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> wrote:
> 
> > The Renesas RZ/T2H (R9A09G077) and Renesas RZ/N2H (R9A09G087) SoCs have
> > IRQ-capable pins handled by the ICU, which forwards them to the GIC.
> >
> > The ICU supports 16 IRQ lines, the pins map to these lines arbitrarily,
> > and the mapping is not configurable.
> >
> > Add a GPIO IRQ chip that can be used to configure these pins as IRQ
> > lines.
> >
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>
> 
> Mention in the commit that this is achieved with a
> hierarchical IRQ domain please. (I really like how this
> was done!) Also mention that wakeup capability is
> also implemented as part of the patch.
> 

Ack, I will reword the commit message and include your suggestions.

> You probably need a:
> 
> select IRQ_DOMAIN_HIERARCHY
> 
> In the rzt2h Kconfig entry?
> 

Yes, along with a select GPIOLIB_IRQCHIP.

I will add both in the next version.

> > +static int rzt2h_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct rzt2h_pinctrl *pctrl = container_of(gc, struct rzt2h_pinctrl, gpio_chip);
> > +       int ret;
> > +
> > +       ret = irq_chip_set_wake_parent(d, on);
> > +       if (ret)
> > +               return ret;
> > +
> 
> Add a comment here:
> 
> /*
>  * If any of the IRQs are in use, then put the entire pin controller
>  * on the device wakeup path.
>  */
> 

Ack.

> > +       if (on)
> > +               atomic_inc(&pctrl->wakeup_path);
> > +       else
> > +               atomic_dec(&pctrl->wakeup_path);
> 
> BTW this is an elegant piece of code I think a lot of other drivers
> need...
> 
> > +static int rzt2h_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> > +                                           unsigned int child,
> > +                                           unsigned int child_type,
> > +                                           unsigned int *parent,
> > +                                           unsigned int *parent_type)
> > +{
> > +       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;
> > +
> > +       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))
> > +               return -EBUSY;
> > +
> > +       rzt2h_pinctrl_set_pfc_mode(pctrl, port, pin, PFC_FUNC_INTERRUPT);
> > +
> > +       *parent = parent_irq;
> > +       *parent_type = child_type;
> > +
> > +       return 0;
> > +}
> 
> Complex, but easy to follow, understand and debug.
> Good job here!
> 

Thank you!

> Yours,
> Linus Walleij