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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Prabhakar,

On Thu, Dec 22, 2022 at 12:50 PM Lad, Prabhakar
<prabhakar.csengg@xxxxxxxxx> wrote:
> 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?

Sure, sounds fine.

You can postpone parsing interrupt-names in the driver (until a new
SoC arrives that uses a different number of IRQ or 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