Re: [PATCH v3 7/8] soc: renesas: Add support for Renesas RZ/N1 GPIO Interrupt Multiplexer

Herve Codina
2025-09-19T13:15:03Z

Hi Wolfram,

On Fri, 19 Sep 2025 11:41:39 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Hi Herve,
>
> > +#define IRQMUX_MAX_IRQS 8
> > +
> > +static int irqmux_setup(struct device *dev, struct device_node *np, u32 __iomem *regs)
>
> The whole driver would benefit from a 'rzn1_irqmux' instead of 'irqmux'
> prefix, I'd say.

Agree, I will used the 'rzn1_irqmux' prefix.

>
> > + for_each_of_imap_item(&imap_parser, &imap_item) {
> > + /*
> > + * The child #address-cells is 0 (already checked). The first
> > + * value in imap item is the src hwirq.
> > + *
> > + * imap items matches 1:1 the interrupt lines that could
> > + * be configured by registers (same order, same number).
> > + * Configure the related register with the src hwirq retrieved
> > + * from the interrupt-map.
> > + */
>
> I haven't looked into the above for_each_of_imap_item-helper. But
> wouldn't it be possibleto retrieve the GIC_SPI number as well and use
> the correct register based on that? That would remove the need of an
> already sorted interrupt-map.

Hum, this give the knowledge of the GIC interrupt number in the driver itself.

Not sure that the mapping between the output interrupt line number N (handled
by register index N) and the GIC interrupt number X should be hardcoded in
the driver.

In my v1 series iteration, I used the 'interrupts' property to provide this
missing information:
interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, /* line 0 (reg index 0) route to GIC 103 */
<GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>, /* line 1 (reg index 1) route to GIC 104 */
<GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>, /* line 2 (reg index 2) route to GIC 105 */
...

Base on the interrupts table and the interrupt-map, I deduce the reg index:
- From interrupt-map, got a GIC interrupt number
- From interrupts table and the GIC interrupt number, got the line/reg index.

Rob asked to use only interrupt-map and use directly the interrupt-map index as
the hardware index:
https://lore.kernel.org/lkml/20250801111753.382f52ac@bootlin.com/

>
> > + if (index > IRQMUX_MAX_IRQS) {
> > + of_node_put(imap_item.parent_args.np);
> > + dev_err(dev, "too much items in interrupt-map\n");
> > + return -EINVAL;
>
> -E2BIG? With such a unique errno, we could even drop the dev_err.

Yes sure.

>
> > + }
> > +
> > + writel(imap_item.child_imap[0], regs + index);
> > + index++;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int irqmux_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > + u32 __iomem *regs;
> > + int nr_irqs;
> > + int ret;
> > +
> > + regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(regs))
> > + return PTR_ERR(regs);
> > +
> > + nr_irqs = of_irq_count(np);
> > + if (nr_irqs < 0)
> > + return nr_irqs;
> > +
> > + if (nr_irqs > IRQMUX_MAX_IRQS) {
> > + dev_err(dev, "too many output interrupts\n");
> > + return -ENOENT;
>
> -E2BIG? Wait, isn't this the same check twice?

This is not the same check but this one should not be there.

Indeed of_irq_count() counts the number of items available in the
'interrupts' property. This is not used anymore.

I missed to remove it from v1 to v2 updates (and also from v2 to v3).

The of_irq_count() call and related checks will be remove in the next
iteration.

Best regards,
Hervé