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
