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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

On Sun, Jun 02, 2019 at 03:46:00PM +0200, Linus Walleij wrote:
> On Wed, May 29, 2019 at 4:53 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Hierarchical IRQ domains can be used to stack different IRQ controllers
> > on top of each other. One specific use-case where this can be useful is
> > if a power management controller has top-level controls for wakeup
> > interrupts. In such cases, the power management controller can be a
> > parent to other interrupt controllers and program additional registers
> > when an IRQ has its wake capability enabled or disabled.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v3:
> > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs()
> > - add missing kerneldoc for new parent_domain field
> > - keep IRQ_DOMAIN dependency for clarity
> >
> > Changes in v2:
> > - select IRQ_DOMAIN_HIERARCHY to avoid build failure
> > - move more code into the gpiolib core
> 
> This is looking really good!
> 
> >  config GPIOLIB_IRQCHIP
> >         select IRQ_DOMAIN
> > +       select IRQ_DOMAIN_HIERARCHY
> >         bool
> 
> Hm OK I guess. It would be ugly to ifdef all hierarchy
> code in gpiolib.
> 
> >  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] = IRQ_TYPE_NONE;
> > +
> > +               return irq_create_fwspec_mapping(&spec);
> > +       }
> > +
> >         return irq_create_mapping(chip->irq.domain, offset);
> 
> This is looking really good!
> 
> > +       /*
> > +        * 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;
> 
> Please drop this. The default .to_irq() should be good for everyone.
> Also patch 2/2 now contains a identical copy of the gpiolib
> .to_irq() which I suspect you indended to drop, actually.

It's not actually identical to the gpiolib implementation. There's still
the conversion to the non-linear DT representation for GPIO specifiers
from the linear GPIO number space, which is not taken care of by the
gpiolib variant. That's precisely the point why this patch makes it
possible to let the driver override things.

More generally, if we drop this we restrict access to the built-in
hierarchical support to devices that use 2 cells as the specifier. Most
drivers support that, but there are a few exceptions:

  - gpio-lpc32xx
  - gpio-mt7621
  - gpio-pxa

gpio-pxa seems like it's really just two-cell, but the other two are
definitely different. As discussed before gpio-tegra186 is also
different but could be tweaked into doing two-cell by generating the
GPIO/IRQ map upfront.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]