Re: [PATCH v2 5/8] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Wed, Jan 30 2019 at 15:45 -0700, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-01-24 12:22:02)
> > To allow GPIOs to wakeup the system from suspend or deep idle, the
> > wakeup capable GPIOs are setup in hierarchy with interrupts from the
> > wakeup-parent irqchip.
> >
> > In older SoC's, the TLMM will handover detection to the parent irqchip
> > and in newer SoC's, the parent irqchip may also be active as well as the
> > TLMM and therefore the GPIOs need to be masked at TLMM to avoid
> > duplicate interrupts. To enable both these configurations to exist,
> > allow the parent irqchip to dictate the TLMM irqchip's behavior when
> > masking/unmasking the interrupt.
> >
> > Co-developed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> >
> > ---
> > Changes in v2:
> >         - Fix bug when unmaksing PDC interrupt
>
> What was the bug? 
The problem was an incorrect merge (possibly manual), causing the PDC to
be not used at all. This is what I had -

static void msm_gpio_irq_mask(struct irq_data *d)                                  
{                                                                                  
        struct gpio_chip *gc = irq_data_get_irq_chip_data(d);                      
        struct msm_pinctrl *pctrl = gpiochip_get_data(gc);                         
        const struct msm_pingroup *g;                                              
        unsigned long flags;                                                       
        u32 val;                                                                   
                                                                                   
        g = &pctrl->soc->groups[d->hwirq];                                         
                                                                                   
        if (d->parent_data)                                                        
                irq_chip_unmask_parent(d);                                         
                                                                                   
        /* Monitored by parent wakeup controller? Keep masked */                   
        if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))                         
                return;                                                            
                                                                                   

The above unmask_parent() call in an irq_mask() callback was the bug.

> Is that why the mask callback in this gpio chip no
> longer calls the parent irq chip? We should keep calling the parent
> irqchip from what I can tell. Otherwise, we may never mask the irq at
> the PDC and only mask it at the GPIO level, which may not even care
> about it if it's being monitored by the PDC.
>
> This causes me to get a bunch of interrupts on my touchscreen when I
> touch it once vs. only a handful (like 4) when I fix it with the below
> patch:
Hmm... I did not see an issue in my local testing :(
Thanks for the fix.

> Can you fold it in?
Yes, I will fold it in and send a rev out with the fix.

Thanks,
Lina

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index dd72ec8fb8db..9b45219893bd 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -682,6 +682,9 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> 	clear_bit(d->hwirq, pctrl->enabled_irqs);
>
> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (d->parent_data)
> +		irq_chip_mask_parent(d);
> }
>
> static void msm_gpio_irq_unmask(struct irq_data *d)