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

[Lad, Prabhakar]

Hi Geert,

On Wed, Dec 21, 2022 at 12:18 PM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> 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
>
Ok.

> > > +                     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".
>
Maybe I will read all the interrupts based on index only for all the
SoCs and we still add interrupt-names in dt bindings with the
dt_binding check we can make sure all the interrupts for each SoC
exist in the DT and the driver still reads them based on index. Does
that sound good?

Cheers,
Prabhakar