Re: [PATCH v2 2/3] Input: ti_am335x_tsc - Ack pending IRQs at probe and before suspend

[Vignesh R]

On Monday 16 April 2018 11:29 PM, Dmitry Torokhov wrote:
> On Sat, Apr 14, 2018 at 03:21:52PM +0530, Vignesh R wrote:
>> From: Grygorii Strashko <grygorii.strashko@xxxxxx>
>> 
>> It is seen that just enabling the TSC module triggers a HW_PEN IRQ
>> without any interaction with touchscreen by user. This results in first
>> suspend/resume sequence to fail as system immediately wakes up from
>> suspend as soon as HW_PEN IRQ is enabled in suspend handler due to the
>> pending IRQ. Therefore clear all IRQs at probe and also in suspend
> 
> Are the interrupts configured as edge?
> 

No, its a level interrupt.

>> callback for sanity.
>> 
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
>> Signed-off-by: Vignesh R <vigneshr@xxxxxx>
>> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>
>> ---
>> 
>> v2: Add Acks from v1.
>> 
>>Â drivers/input/touchscreen/ti_am335x_tsc.c | 2 ++
>>Â include/linux/mfd/ti_am335x_tscadc.hÂÂÂÂÂ | 1 +
>>Â 2 files changed, 3 insertions(+)
>> 
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index 810e05c9c4f5..dcd9db768169 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -439,6 +439,7 @@ static int titsc_probe(struct platform_device *pdev)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "irq wake enable failed.\n");
>>ÂÂÂÂÂÂÂ }
>>Â 
>> +ÂÂÂÂ titsc_writel(ts_dev, REG_IRQSTATUS, IRQENB_MASK);
>>ÂÂÂÂÂÂÂ titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
>>ÂÂÂÂÂÂÂ titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
> 
> Out of curiosity, should this be:
> 
> ÂÂÂÂÂÂÂ titsc_writel(ts_dev, REG_IRQENABLE,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ IRQENB_FIFO0THRES | IRQENB_EOS);
> 
> ?
> 
> Because 2nd titsc_writel() overwrites the first? Or separate writes to
> the same register are distinct?
> 

As per TRM, writing 0 to any bit of REG_IRQENABLE has no effect(IRQs are
cleared by writing to REG_IRQCLR). Therefore, second write does not
overwrite the first. I agree that there is nothing that prevents us from
enabling both IRQs in a single write. That can be a separate cleanup patch.

>>ÂÂÂÂÂÂÂ err = titsc_config_wires(ts_dev);
>> @@ -504,6 +505,7 @@ static int __maybe_unused titsc_suspend(struct device *dev)
>>Â 
>>ÂÂÂÂÂÂÂ tscadc_dev = ti_tscadc_dev_get(to_platform_device(dev));
>>ÂÂÂÂÂÂÂ if (device_may_wakeup(tscadc_dev->dev)) {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ titsc_writel(ts_dev, REG_IRQSTATUS, IRQENB_MASK);
> 
> The comment in titsc_irq() says that we should not be touching
> IRQENB_FIFO0THRES as it is used by another driver, but here we are
> whacking it. Can you elaborate why this is safe?
> 
> You might need to rework the interrupt handling since you have several
> drivers...
> 

I guess you meant IRQENB_FIFO1THRES(FIFO0 is used for TSC and FIFO1 is
ADC). Yes, this driver must not touch FIFO1 related IRQs, I will fix
that up in next version.


>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ idle = titsc_readl(ts_dev, REG_IRQENABLE);
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ titsc_writel(ts_dev, REG_IRQENABLE,
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (idle | IRQENB_HW_PEN));
>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>> index b9a53e013bff..1a6a34f726cc 100644
>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>> @@ -63,6 +63,7 @@
>>Â #define IRQENB_FIFO1OVRRUNÂÂ BIT(6)
>>Â #define IRQENB_FIFO1UNDRFLWÂ BIT(7)
>>Â #define IRQENB_PENUPÂÂÂÂÂÂÂÂ BIT(9)
>> +#define IRQENB_MASKÂÂÂÂÂÂÂÂÂ (0x7FF)
>>Â 
>>Â /* Step Configuration */
>>Â #define STEPCONFIG_MODE_MASK (3 << 0)
>> -- 
>> 2.17.0
>> 
> 
> Thanks.
> 
> -- 
> Dmitry

-- 
Regards
Vignesh