lkml.org 
[lkml]   [2018]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/3] Input: ti_am335x_tsc - Ack pending IRQs at probe and before suspend
From
Date


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@ti.com>
>>
>> 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@ti.com>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>
>> 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

\
 
 \ /
  Last update: 2018-04-17 10:20    [W:0.039 / U:2.152 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site