Re: [PATCH 1/2] RFC: gpio: Add support for hierarchical IRQ domains

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Mon, Jun 3, 2019 at 6:12 PM Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 02/06/2019 21:54, Linus Walleij wrote:

> > +     /* DT will deal with mapping each IRQ as we go along */
> > +     if (is_of_node(gpiochip->irq.fwnode))
> > +             return;
> > +
> > +     /*
> > +      * This is for legacy and boardfile "irqchip" fwnodes: allocate
> > +      * irqs upfront instead of dynamically since we don't have the
> > +      * dynamic type of allocation that hardware description languages
> > +      * provide.
> > +      */
> > +     if (is_fwnode_irqchip(gpiochip->irq.fwnode)) {
>
> How about ACPI-based systems? Do they even exist in this scheme? This is
> a genuine question, as I have no idea...

I assume we can just add another if() clause for them?

I have no ACPI system using gpiochip in my mental or practical
world, but I assume they are not very special wrt this since they
invented fwnode and drove it all the way into the core.

I don't know if I should add upfront code for them since I can't
test it (that I know).

> > +                     fwspec.fwnode = gpiochip->irq.fwnode;
> > +                     /* This is the hwirq for the GPIO line side of things */
> > +                     fwspec.param[0] = map->hwirq;
> > +                     /* Just pick something */
> > +                     fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> > +                     fwspec.param_count = 2;
> > +                     ret = __irq_domain_alloc_irqs(gpiochip->irq.domain,
> > +                                                   /* just pick something */
> > +                                                   -1,
> > +                                                   1,
> > +                                                   NUMA_NO_NODE,
> > +                                                   &fwspec,
>
> It feels a bit odd that we're building a fwspec for something that
> already has all the required information (the alloc function has the
> gpio_irq_chip as host_data). I have the feeling that we could just have
> a single __irq_domain_alloc_irqs() call with irq.parent_n_irq_maps as
> the nr_irqs, and let the alloc function sort it out...

Oh you're probably right, since the domain is implicitly linear
anyways. I suppose this is part of the answer to Thierry's question
actually: since I do this one irq at the time the domain will look
like such as well.

> > +     /* Some drivers provide custom irqdomain ops */
> >       if (gpiochip->irq.domain_ops)
> >               ops = gpiochip->irq.domain_ops;
>
> Is that already a requirement? Or an educated guess?

Yeah look at drivers/gpio/gpio-tegra186.c:

irq = &gpio->gpio.irq;
(...)
irq->domain_ops = &tegra186_gpio_irq_domain_ops;

what I just discovered is that this driver does not call
irq_domain_add* or irq_domain_create* not does it ask gpiolib
to do it so I have no idea how that thing is even working, so I
asked Thierry to explain it.

Right now my assumption is that is uses a NULL initalized irqdomain
and assign some random translation functions to it and hope
for the best or something, I might be missing something here.

Yours,
Linus Walleij