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

[Lina Iyer <ilina@codeaurora.org>]

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@chromium.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>
>> ---
>> 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)