Re: [PATCH v2 4/9] irqchip: irq-renesas-rzg2l: Add support for RZ/G2UL SoC

[Geert Uytterhoeven]

On Wed, Dec 21, 2022 at 11:20 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> On Wed, 21 Dec 2022 00:02:37 +0000,
> Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> > RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> > which it has additional registers.
> >
> > This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> > and now that we have interrupt-names property the driver code parses the
> > interrupts based on names and for backward compatibility we fallback to
> > parse interrupts based on index.
> >
> > For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> > too and in future when the interrupt handler will be registered for
> > BUS_ERR_INT we will have to implement a new callback.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

> > +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> > +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> > +                                              struct device_node *np)
> > +{
> > +     struct property *pp;
> >       unsigned int i;
> >       int ret;
> >
> > +     /*
> > +      * first check if interrupt-names property exists if so parse them by name
> > +      * or else parse them by index for backward compatibility.
> > +      */
> > +     pp = of_find_property(np, "interrupt-names", NULL);
> > +     if (pp) {
> > +             char *irq_name;
> > +
> > +             /* parse IRQ0-7 */
> > +             for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> > +                     irq_name = kasprintf(GFP_KERNEL, "irq%d", i);

%u

> > +                     if (!irq_name)
> > +                             return -ENOMEM;
> > +
> > +                     ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);
>
> Am I the only one that find it rather odd to construct a name from an
> index, only to get another index back?

The issue is that there are two number ranges ("irq%u" and "tint%u"),
stored in a single interrupts property.

An alternative solution would be to get rid of the "interrupt-names",
and use two separate prefixed interrupts properties instead, like is
common for e.g. gpios: "irq-interrupts" and "tint-interrupts".

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