Re: [PATCH RFC v2 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

On Thu, Oct 03 2019 at 06:17 -0600, Linus Walleij wrote:
> On Fri, Sep 13, 2019 at 11:59 PM Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
>
> > Some GPIOs are marked as wakeup capable and are routed to another
> > interrupt controller that is an always-domain and can detect interrupts
> > even most of the SoC is powered off. The wakeup interrupt controller
> > wakes up the GIC and replays the interrupt at the GIC.
> >
> > Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
> > and ensure the wakeup GPIOs are handled correctly.
> >
> > Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > ----
> > Changes in RFC v2:
> >         - Define irq_domain_qcom_handle_wakeup()
> >         - Rebase on top of GPIO hierarchy support in linux-next
> >         - Set the chained irq handler for summary line
>
> This is looking better every time I look at it, it's really complex
> but alas the problem is hard to solve so it requires complex solutions.
>
> > @@ -1006,6 +1091,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> >         struct gpio_irq_chip *girq;
> >         int ret;
> >         unsigned ngpio = pctrl->soc->ngpios;
> > +       struct device_node *dn;
>
> I usually call the variable "np"
Will change.

> > @@ -1021,17 +1107,40 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> >
> >         pctrl->irq_chip.name = "msmgpio";
> >         pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
> > +       pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
> >         pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
> >         pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
> >         pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> > +       pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
>
> This part and the functions called seem fine!
>
> > +       dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
> > +       if (dn) {
> > +               int i;
> > +               bool skip;
> > +               unsigned int gpio;
> > +
> > +               chip->irq.parent_domain = irq_find_matching_host(dn,
> > +                                                DOMAIN_BUS_WAKEUP);
> > +               of_node_put(dn);
> > +               if (!chip->irq.parent_domain)
> > +                       return -EPROBE_DEFER;
> > +               chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
> > +
> > +               skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
> > +               for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
> > +                       gpio = pctrl->soc->wakeirq_map[i].gpio;
> > +                       set_bit(gpio, pctrl->skip_wake_irqs);
> > +               }
> > +       }
>
> OK I guess this is how we should do it, maybe add a comment to clarify
> that we are checking the parent irqdomain of the chained IRQ to see
> if we need to avoid disabling the irq as it is used for wakeup. (IIUC
> what the code does!)
Okay.

> > +       /*
> > +        * Since we are chained to the GIC using the TLMM summary line
> > +        * and in hierarchy with the wakeup parent interrupt controller,
> > +        * explicitly set the chained summary line. We need to do this because
> > +        * the summary line is not routed to the wakeup parent but directly
> > +        * to the GIC.
> > +        */
> > +       gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
> > +                                    msm_gpio_irq_handler);
>
> I don't think this part is needed, we already have:
>
> girq->parent_handler = msm_gpio_irq_handler;
> girq->num_parents = 1;
> girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
>     GFP_KERNEL);
> if (!girq->parents)
>     return -ENOMEM;
> girq->default_type = IRQ_TYPE_NONE;
> girq->handler = handle_bad_irq;
> girq->parents[0] = pctrl->irq;
>
> This will make the irq chain when calling gpiochip_add_data(), so
> just delete this and see if everything works as before.
I thought it didn't work without this change and I am not sure why it
started working after I did. May be it was a bad set of patches that I
pulled in.

> Other than that it looks fine!
Thanks for your review.

--Lina