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
>>
>> 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
>> Signed-off-by: Vignesh R
>> Acked-by: Lee Jones
>> ---
>>
>> 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
