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

[Herve Codina <herve.codina@bootlin.com>]

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é