Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:

> From: Thierry Reding <treding@xxxxxxxxxx>
>
> Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure.
> This allows a bit of boiler plate to be removed and while at it enables
> support for hierarchical domains, which is useful to support PMC wake
> events on Tegra210 and earlier.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>

The patch didn't apply to my "devel" branch for some reason
so have a look at that, seems gpio-tegra.c has some changes not
in my tree.

>  struct tegra_gpio_soc_config {
> @@ -93,12 +91,12 @@ struct tegra_gpio_soc_config {
>  struct tegra_gpio_info {
>         struct device                           *dev;
>         void __iomem                            *regs;
> -       struct irq_domain                       *irq_domain;
>         struct tegra_gpio_bank                  *bank_info;
>         const struct tegra_gpio_soc_config      *soc;
>         struct gpio_chip                        gc;
>         struct irq_chip                         ic;
>         u32                                     bank_count;
> +       unsigned int                            *irqs;

So this is hierarchical with several IRQs.

>  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>         unsigned int gpio = d->hwirq, port = GPIO_PORT(gpio), lvl_type;
> -       struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> -       struct tegra_gpio_info *tgi = bank->tgi;
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +       struct tegra_gpio_bank *bank;
>         unsigned long flags;
> -       u32 val;
>         int ret;
> +       u32 val;
> +
> +       bank = &tgi->bank_info[GPIO_BANK(d->hwirq)];

So the general idea is to look up the bank from the IRQ offset.

But...

> -       return 0;
> +       if (d->parent_data)
> +               ret = irq_chip_set_type_parent(d, type);
> +
> +       return ret;

I don't quite get this. This makes sense if there is one parent IRQ
per interrupt, but if one of the users of a GPIO in a bank sets the
IRQ type to edge and then another one comes in and set another
of the lines to level and then the function comes here, what type
gets set on the parent? Whichever comes last?

Normally with banked GPIOs collecting several lines in a cascaded
fashion, the GPIO out of the bank toward the GIC is level triggered.

I don't understand how this GPIO controller can be hierarchical,
it looks cascaded by the definition of the document
Documentation/driver-api/gpio/driver.rst

Yours,
Linus Walleij