Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Jul 3, 2019 at 11:22 AM Brian Masney <masneyb@xxxxxxxxxxxxx> wrote:
> On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> >  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;
> >
> > -     return irq_create_mapping(chip->irq.domain, offset);
> > +     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);
> > +     }
>
> spmi-gpio's to_irq() needs to add one to the offset:
>
>         static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>         {
>                 struct pmic_gpio_state *state = gpiochip_get_data(chip);
>                 struct irq_fwspec fwspec;
>
>                 fwspec.fwnode = state->fwnode;
>                 fwspec.param_count = 2;
>                 fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET;
>                 /*
>                  * Set the type to a safe value temporarily. This will be overwritten
>                  * later with the proper value by irq_set_type.
>                  */
>                 fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>
>                 return irq_create_fwspec_mapping(&fwspec);
>         }
>
> ssbi-gpio will have the same problem as well.
>
> What do you think about adding a new field to the struct gpio_irq_chip
> inside the CONFIG_IRQ_DOMAIN_HIERARCHY ifdef called something like
> to_irq_offset? (I'm bad at naming things.)

I think to cover Lina's need and following the direction set out
by Thierry's desire to have callback so we can control the
parent-to-child translation with code, it might be best to have
an optional callback for translating fwspec.param[0].

Thierry seems to need exactly this for the Tegra driver to,
I think that is why it has custom ops today.

> Also, instead of hardcoding IRQ_TYPE_NONE, what do you think about using
> the default_type field that's available?

I want to get rid of the .default_type in the long run, it is
nominally only for board files where the .set_type() isn't
ever getting called. For anything modern, the .set_type()
callback is always called before any irq is used.

Yours,
Linus Walleij