On Fri, Dec 14, 2018 at 02:41:01PM +0100, Linus Walleij wrote:
> Hi Thierry!
>
> thanks a lot for the patch! This is really in the right direction
> of how I want things to go with hierarchical IRQs.
>
> Some comments:
>
> On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
>
> > config GPIOLIB_IRQCHIP
> > - select IRQ_DOMAIN
> > + select IRQ_DOMAIN_HIERARCHY
>
> I understand that IRQ_DOMAIN_HIERARCHY implies
> IRQ_DOMAIN but I kind of like if we anyway select both
> (unless Kconfig dislikes it).
>
> Otherwise it looks like we're just using hierarchies.
Okay, I can add that.
>
> > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> > {
> > + struct irq_domain *domain = chip->irq.domain;
> > +
> > if (!gpiochip_irqchip_irq_valid(chip, offset))
> > return -ENXIO;
> >
> > + if (irq_domain_is_hierarchy(domain)) {
> > + struct irq_fwspec spec;
> > +
> > + spec.fwnode = domain->fwnode;
> > + spec.param_count = 2;
> > + spec.param[0] = offset;
> > + spec.param[1] = 0;
> > +
> > + return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec);
> > + }
> > +
> > return irq_create_mapping(chip->irq.domain, offset);
> > }
>
> This is really nice.
>
> > - gpiochip->to_irq = gpiochip_to_irq;
> > + /*
> > + * Allow GPIO chips to override the ->to_irq() if they really need to.
> > + * This should only be very rarely needed, the majority should be fine
> > + * with gpiochip_to_irq().
> > + */
> > + if (!gpiochip->to_irq)
> > + gpiochip->to_irq = gpiochip_to_irq;
>
> And I see this is what your driver does, which leaves the default
> domain hierarchy callback path unused.
Actually this is only temporary until the patch that uses a sparse
number space (with the valid masks). After the last patch in the series
the need to override this goes away, so I could follow-up with a patch
to revert this. Or alternatively we could squash the two Tegra patches.
I think keeping them separate is slightly nicer.
Of course, I would even prefer to not move to the sparse number space,
but you seemed to feel very strongly about it last time we discussed
this.
> It's better if you indirect the pointer like we do with some other
> irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq
> is defined, we save that function pointer and call that at the
> end of the gpiochip_to_irq() pointer, then we override it
> with our own.
>
> Since the Tegra186 clearly .to_irq() clearly is mostly the
> same plus some extra, this should work fine and give
> lesser lines of code in that driver.
That sounds an awful lot like a midlayer. I'm not a big fan of that at
all. There are various reasons for this, but others have described it in
much better detail than I could. See here for example:
https://lwn.net/Articles/336262/
In this particular case one potential problem is that gpiochip_to_irq()
might not always do the right things for everyone, and that it turn may
incentivize people to work around it creatively. For example, one driver
may want to perform some operation before gpiochip_to_irq() is called,
while another driver may have to perform an operation after the call. If
you move towards a midlayer there's no way to satisfy both, unless you
go and add pre- and post-operations of some sort.
I'd like to propose that we instead provide gpiochip_to_irq() as a
helper that drivers can call into. In order to do that, we just need to
make sure that drivers have access to it. Then they can override the
->to_irq() like this:
static int foo_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
{
/* do something before */
...
err = gpiochip_to_irq(chip, offset);
if (err < 0)
return err;
/* do something after */
...
}
That way we get all the benefits of sharing the code and at the same
time we don't impose any artificial constraints on drivers. Typically in
the library pattern drivers that don't need anything extra would simply
set gpiochip_to_irq() as their implementation, so the default assignment
in the gpiolib core wouldn't be necessary, but that's just a minor
implementation detail.
What do you think?
Thierry
Attachment:
signature.asc
Description: PGP signature
