Re: [PATCH v1 4/4] gpiolib: Reuse device's fwnode to create IRQ domain

[Linus Walleij]

On Wed, Mar 3, 2021 at 10:35 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Wed, Mar 03, 2021 at 10:22:02AM +0100, Linus Walleij wrote:

> > But this:
> >
> > > @@ -1504,15 +1497,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
> > >                         return ret;
> > >         } else {
> > >                 /* Some drivers provide custom irqdomain ops */
> > > -               if (gc->irq.domain_ops)
> > > -                       ops = gc->irq.domain_ops;
> > > -
> > > -               if (!ops)
> > > -                       ops = &gpiochip_domain_ops;
> > > -               gc->irq.domain = irq_domain_add_simple(np,
> > > -                       gc->ngpio,
> > > -                       gc->irq.first,
> > > -                       ops, gc);
> > > +               ops = gc->irq.domain_ops ?: &gpiochip_domain_ops;
> > > +               if (gc->irq.first)
> > > +                       gc->irq.domain = irq_domain_create_legacy(fwnode, gc->ngpio,
> > > +                                                                 gc->irq.first, 0,
> > > +                                                                 ops, gc);
> > > +               else
> > > +                       gc->irq.domain = irq_domain_create_linear(fwnode, gc->ngpio,
> > > +                                                                 ops, gc);
> >
> > This looks like a refactoring and reimplementation of irq_domain_add_simple()?
>
> If you named it as irq_domain_create_simple(), then yes, but the problem is
> that we don't have irq_domain_create_simple() API right now.
>
> > Why, and should it rather be a separate patch?
>
> Nope.

OK I looked closer at irq_domain_add_simple(), and what it does different
is to call irq_alloc_descs() for all lines if using sparse IRQs and then
associate them. irq_domain_create_linear|legacy() does not allocate IRQ
descriptors because it assumes something like DT or ACPI will do that
on-demand when drivers request IRQs.

This may be dangerous because some old platforms do not resolve IRQs
at runtime and you will get NULL pointer exceptions.

We then need to make sure all callers do what is done in e.g.
drivers/gpio/gpio-omap.c in the #ifdef CONFIG_ARCH_OMAP1 clause:
they need to be augmented to call irq_alloc_descs() explicitly,
and I don't think all of them do it as nicely for us as OMAP1.

I might be overly cautious though, however that is why this code
uses irq_domain_add_simple(), came in commit
commit 2854d167cc545d0642277bf8b77f972a91146fc6

Yours,
Linus Walleij