[PATCH v2 0/3] ti_am335x_tsc: Fix suspend/resume

[PATCH v2 0/3] ti_am335x_tsc: Fix suspend/resume

[Vignesh R]

This patch series fixes couple of issues wrt suspend/resume with TI AM335x
TSC driver. Disable and clear any pending IRQs before suspend, and
handle case where TSC wakeup would fail, if there were touch events
during suspend.

v2:
Rebase onto latest linux-next.
v1:https://lkml.org/lkml/2016/5/16/150

Grygorii Strashko (2):
  Input: ti_am335x_tsc - Ack pending IRQs at probe and before suspend
  Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

Vignesh R (1):
  Input: ti_am335x_tsc - Mark IRQ as wakeup capable

 drivers/input/touchscreen/ti_am335x_tsc.c | 14 ++++++++++++++
 include/linux/mfd/ti_am335x_tscadc.h      |  1 +
 2 files changed, 15 insertions(+)

-- 
2.17.0

[PATCH v2 0/3] ti_am335x_tsc: Fix suspend/resume

[Vignesh R]

This patch series fixes couple of issues wrt suspend/resume with TI AM335x
TSC driver. Disable and clear any pending IRQs before suspend, and
handle case where TSC wakeup would fail, if there were touch events
during suspend.

v2:
Rebase onto latest linux-next.
v1:https://lkml.org/lkml/2016/5/16/150

Grygorii Strashko (2):
  Input: ti_am335x_tsc - Ack pending IRQs at probe and before suspend
  Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

Vignesh R (1):
  Input: ti_am335x_tsc - Mark IRQ as wakeup capable

 drivers/input/touchscreen/ti_am335x_tsc.c | 14 ++++++++++++++
 include/linux/mfd/ti_am335x_tscadc.h      |  1 +
 2 files changed, 15 insertions(+)

-- 
2.17.0

[PATCH v2 1/3] Input: ti_am335x_tsc - Mark IRQ as wakeup capable

[Vignesh R]

On AM335x, ti_am335x_tsc can wake up the system from suspend, mark the
IRQ as wakeup capable, so that device irq is not disabled during system
suspend.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

v2: No changes

 drivers/input/touchscreen/ti_am335x_tsc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index f1043ae71dcc..810e05c9c4f5 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/sort.h>
+#include <linux/pm_wakeirq.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -432,6 +433,12 @@ static int titsc_probe(struct platform_device *pdev)
 		goto err_free_mem;
 	}
 
+	if (device_may_wakeup(tscadc_dev->dev)) {
+		err = dev_pm_set_wake_irq(tscadc_dev->dev, ts_dev->irq);
+		if (err)
+			dev_err(&pdev->dev, "irq wake enable failed.\n");
+	}
+
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
 	err = titsc_config_wires(ts_dev);
@@ -462,6 +469,7 @@ static int titsc_probe(struct platform_device *pdev)
 	return 0;
 
 err_free_irq:
+	dev_pm_clear_wake_irq(tscadc_dev->dev);
 	free_irq(ts_dev->irq, ts_dev);
 err_free_mem:
 	input_free_device(input_dev);
@@ -474,6 +482,7 @@ static int titsc_remove(struct platform_device *pdev)
 	struct titsc *ts_dev = platform_get_drvdata(pdev);
 	u32 steps;
 
+	dev_pm_clear_wake_irq(ts_dev->mfd_tscadc->dev);
 	free_irq(ts_dev->irq, ts_dev);
 
 	/* total steps followed by the enable mask */
-- 
2.17.0

[PATCH v2 1/3] Input: ti_am335x_tsc - Mark IRQ as wakeup capable

[Vignesh R]

On AM335x, ti_am335x_tsc can wake up the system from suspend, mark the
IRQ as wakeup capable, so that device irq is not disabled during system
suspend.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

v2: No changes

 drivers/input/touchscreen/ti_am335x_tsc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index f1043ae71dcc..810e05c9c4f5 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/sort.h>
+#include <linux/pm_wakeirq.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -432,6 +433,12 @@ static int titsc_probe(struct platform_device *pdev)
 		goto err_free_mem;
 	}
 
+	if (device_may_wakeup(tscadc_dev->dev)) {
+		err = dev_pm_set_wake_irq(tscadc_dev->dev, ts_dev->irq);
+		if (err)
+			dev_err(&pdev->dev, "irq wake enable failed.\n");
+	}
+
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
 	err = titsc_config_wires(ts_dev);
@@ -462,6 +469,7 @@ static int titsc_probe(struct platform_device *pdev)
 	return 0;
 
 err_free_irq:
+	dev_pm_clear_wake_irq(tscadc_dev->dev);
 	free_irq(ts_dev->irq, ts_dev);
 err_free_mem:
 	input_free_device(input_dev);
@@ -474,6 +482,7 @@ static int titsc_remove(struct platform_device *pdev)
 	struct titsc *ts_dev = platform_get_drvdata(pdev);
 	u32 steps;
 
+	dev_pm_clear_wake_irq(ts_dev->mfd_tscadc->dev);
 	free_irq(ts_dev->irq, ts_dev);
 
 	/* total steps followed by the enable mask */
-- 
2.17.0

Re: [PATCH v2 1/3] Input: ti_am335x_tsc - Mark IRQ as wakeup capable

[Dmitry Torokhov]

On Sat, Apr 14, 2018 at 03:21:51PM +0530, Vignesh R wrote:
> On AM335x, ti_am335x_tsc can wake up the system from suspend, mark the
> IRQ as wakeup capable, so that device irq is not disabled during system
> suspend.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> v2: No changes
> 
>  drivers/input/touchscreen/ti_am335x_tsc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index f1043ae71dcc..810e05c9c4f5 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -27,6 +27,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/sort.h>
> +#include <linux/pm_wakeirq.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  
> @@ -432,6 +433,12 @@ static int titsc_probe(struct platform_device *pdev)
>  		goto err_free_mem;
>  	}
>  
> +	if (device_may_wakeup(tscadc_dev->dev)) {
> +		err = dev_pm_set_wake_irq(tscadc_dev->dev, ts_dev->irq);

Hmm, most of the drivers simply use enable_irq_wake()/disable_irq_wake()
in suspend/resume paths and use dev_pm_set_wake_irq() only for dedicated
and distinct wake interrupts. Why do we not follow the same pattern
here?

Thanks.

> +		if (err)
> +			dev_err(&pdev->dev, "irq wake enable failed.\n");
> +	}
> +
>  	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
>  	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
>  	err = titsc_config_wires(ts_dev);
> @@ -462,6 +469,7 @@ static int titsc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_free_irq:
> +	dev_pm_clear_wake_irq(tscadc_dev->dev);
>  	free_irq(ts_dev->irq, ts_dev);
>  err_free_mem:
>  	input_free_device(input_dev);
> @@ -474,6 +482,7 @@ static int titsc_remove(struct platform_device *pdev)
>  	struct titsc *ts_dev = platform_get_drvdata(pdev);
>  	u32 steps;
>  
> +	dev_pm_clear_wake_irq(ts_dev->mfd_tscadc->dev);
>  	free_irq(ts_dev->irq, ts_dev);
>  
>  	/* total steps followed by the enable mask */
> -- 
> 2.17.0
> 

-- 
Dmitry

Re: [PATCH v2 1/3] Input: ti_am335x_tsc - Mark IRQ as wakeup capable

[Vignesh R]

Hi,

On Monday 16 April 2018 11:15 PM, Dmitry Torokhov wrote:
> On Sat, Apr 14, 2018 at 03:21:51PM +0530, Vignesh R wrote:
>> On AM335x, ti_am335x_tsc can wake up the system from suspend, mark the
>> IRQ as wakeup capable, so that device irq is not disabled during system
>> suspend.
>> 
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>> 
>> v2: No changes
>> 
>>  drivers/input/touchscreen/ti_am335x_tsc.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index f1043ae71dcc..810e05c9c4f5 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>>  #include <linux/sort.h>
>> +#include <linux/pm_wakeirq.h>
>>  
>>  #include <linux/mfd/ti_am335x_tscadc.h>
>>  
>> @@ -432,6 +433,12 @@ static int titsc_probe(struct platform_device *pdev)
>>                goto err_free_mem;
>>        }
>>  
>> +     if (device_may_wakeup(tscadc_dev->dev)) {
>> +             err = dev_pm_set_wake_irq(tscadc_dev->dev, ts_dev->irq);
> 
> Hmm, most of the drivers simply use enable_irq_wake()/disable_irq_wake()
> in suspend/resume paths 

dev_pm_*_wake_irq() function are alternative to above

[1]:
For most drivers, we should be able to drop the following
boilerplate code from runtime_suspend and runtime_resume
functions:

	...
	device_init_wakeup(dev, true);
	...
	if (device_may_wakeup(dev))
		enable_irq_wake(irq);
	...
	if (device_may_wakeup(dev))
		disable_irq_wake(irq);
	...
	device_init_wakeup(dev, false);
	...

We can replace it with just the following init and exit
time code:

	...
	device_init_wakeup(dev, true);
	dev_pm_set_wake_irq(dev, irq);
	...
	dev_pm_clear_wake_irq(dev);
	device_init_wakeup(dev, false);

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/power/wakeirq.c?id=4990d4fe327b9d9a7a3be7103a82699406fdde69

I see device_may_wakeup() check is not required. Will drop that in next version.

> and use dev_pm_set_wake_irq() only for dedicated
> and distinct wake interrupts. Why do we not follow the same pattern
> here?

Thats dev_pm_*_dedicated_wake_irq()


Thanks for the review!


-- 
Regards
Vignesh

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

[Vignesh R]

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
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);
 	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);
 		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

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

[Vignesh R]

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
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);
 	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);
 		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

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

[Dmitry Torokhov]

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?

> 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?

>  	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...

>  		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

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@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

[PATCH v2 3/3] Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

[Vignesh R]

From: Grygorii Strashko <grygorii.strashko@ti.com>

Prevent system suspend while user has finger on touch screen,
because TSC is wakeup source and suspending device while in use will
result in failure to disable the module.
This patch uses pm_stay_awake() and pm_relax() APIs to prevent and
resume system suspend as required.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Vignesh R <vigneshr@ti.com>
---

v2: No changes.

 drivers/input/touchscreen/ti_am335x_tsc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index dcd9db768169..43b22e071842 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -275,6 +275,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	if (status & IRQENB_HW_PEN) {
 		ts_dev->pen_down = true;
 		irqclr |= IRQENB_HW_PEN;
+		pm_stay_awake(ts_dev->mfd_tscadc->dev);
 	}
 
 	if (status & IRQENB_PENUP) {
@@ -284,6 +285,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 			input_report_key(input_dev, BTN_TOUCH, 0);
 			input_report_abs(input_dev, ABS_PRESSURE, 0);
 			input_sync(input_dev);
+			pm_relax(ts_dev->mfd_tscadc->dev);
 		} else {
 			ts_dev->pen_down = true;
 		}
@@ -524,6 +526,7 @@ static int __maybe_unused titsc_resume(struct device *dev)
 		titsc_writel(ts_dev, REG_IRQWAKEUP,
 				0x00);
 		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
+		pm_relax(ts_dev->mfd_tscadc->dev);
 	}
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
-- 
2.17.0

[PATCH v2 3/3] Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

[Vignesh R]

From: Grygorii Strashko <grygorii.strashko@ti.com>

Prevent system suspend while user has finger on touch screen,
because TSC is wakeup source and suspending device while in use will
result in failure to disable the module.
This patch uses pm_stay_awake() and pm_relax() APIs to prevent and
resume system suspend as required.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Vignesh R <vigneshr@ti.com>
---

v2: No changes.

 drivers/input/touchscreen/ti_am335x_tsc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index dcd9db768169..43b22e071842 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -275,6 +275,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	if (status & IRQENB_HW_PEN) {
 		ts_dev->pen_down = true;
 		irqclr |= IRQENB_HW_PEN;
+		pm_stay_awake(ts_dev->mfd_tscadc->dev);
 	}
 
 	if (status & IRQENB_PENUP) {
@@ -284,6 +285,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 			input_report_key(input_dev, BTN_TOUCH, 0);
 			input_report_abs(input_dev, ABS_PRESSURE, 0);
 			input_sync(input_dev);
+			pm_relax(ts_dev->mfd_tscadc->dev);
 		} else {
 			ts_dev->pen_down = true;
 		}
@@ -524,6 +526,7 @@ static int __maybe_unused titsc_resume(struct device *dev)
 		titsc_writel(ts_dev, REG_IRQWAKEUP,
 				0x00);
 		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
+		pm_relax(ts_dev->mfd_tscadc->dev);
 	}
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
-- 
2.17.0

Re: [PATCH v2 3/3] Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

[Dmitry Torokhov]

On Sat, Apr 14, 2018 at 03:21:53PM +0530, Vignesh R wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Prevent system suspend while user has finger on touch screen,
> because TSC is wakeup source and suspending device while in use will
> result in failure to disable the module.
> This patch uses pm_stay_awake() and pm_relax() APIs to prevent and
> resume system suspend as required.

This looks like common behavior for all touchscreens and many other
input devices, but other systems seem to cope without having to add
pm_stay_awake() and pm_relax(). I wonder why your system requires it and
whether we can generalize this somehow.

> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> v2: No changes.
> 
>  drivers/input/touchscreen/ti_am335x_tsc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index dcd9db768169..43b22e071842 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -275,6 +275,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	if (status & IRQENB_HW_PEN) {
>  		ts_dev->pen_down = true;
>  		irqclr |= IRQENB_HW_PEN;
> +		pm_stay_awake(ts_dev->mfd_tscadc->dev);
>  	}
>  
>  	if (status & IRQENB_PENUP) {
> @@ -284,6 +285,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  			input_report_key(input_dev, BTN_TOUCH, 0);
>  			input_report_abs(input_dev, ABS_PRESSURE, 0);
>  			input_sync(input_dev);
> +			pm_relax(ts_dev->mfd_tscadc->dev);
>  		} else {
>  			ts_dev->pen_down = true;
>  		}
> @@ -524,6 +526,7 @@ static int __maybe_unused titsc_resume(struct device *dev)
>  		titsc_writel(ts_dev, REG_IRQWAKEUP,
>  				0x00);
>  		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
> +		pm_relax(ts_dev->mfd_tscadc->dev);
>  	}
>  	titsc_step_config(ts_dev);
>  	titsc_writel(ts_dev, REG_FIFO0THR,
> -- 
> 2.17.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/3] Input: ti_am335x_tsc - Prevent system suspend when TSC is in use

[Vignesh R]

On Monday 16 April 2018 11:31 PM, Dmitry Torokhov wrote:
> On Sat, Apr 14, 2018 at 03:21:53PM +0530, Vignesh R wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>> 
>> Prevent system suspend while user has finger on touch screen,
>> because TSC is wakeup source and suspending device while in use will
>> result in failure to disable the module.
>> This patch uses pm_stay_awake() and pm_relax() APIs to prevent and
>> resume system suspend as required.
> 
> This looks like common behavior for all touchscreens and many other
> input devices, but other systems seem to cope without having to add
> pm_stay_awake() and pm_relax(). I wonder why your system requires it and
> whether we can generalize this somehow.
> 

Not sure if other touch drivers with IP level wakeup, have been tested
in the same way as above scenario. But, if user has finger on
touchscreen, its better not to enter suspend until user stops
interacting with wakeup source.
I see pm_stay_awake()/pm_relax() pair as the only way to stop system
suspend when wakeup source is active. Also, one other touchscreen
driver(zforce_ts.c) and gpio_keys.c use pm_stay_awake()/pm_relax() pair.


Regards
Vignesh
>> 
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>> 
>> v2: No changes.
>> 
>>  drivers/input/touchscreen/ti_am335x_tsc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index dcd9db768169..43b22e071842 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -275,6 +275,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>>        if (status & IRQENB_HW_PEN) {
>>                ts_dev->pen_down = true;
>>                irqclr |= IRQENB_HW_PEN;
>> +             pm_stay_awake(ts_dev->mfd_tscadc->dev);
>>        }
>>  
>>        if (status & IRQENB_PENUP) {
>> @@ -284,6 +285,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>>                        input_report_key(input_dev, BTN_TOUCH, 0);
>>                        input_report_abs(input_dev, ABS_PRESSURE, 0);
>>                        input_sync(input_dev);
>> +                     pm_relax(ts_dev->mfd_tscadc->dev);
>>                } else {
>>                        ts_dev->pen_down = true;
>>                }
>> @@ -524,6 +526,7 @@ static int __maybe_unused titsc_resume(struct device *dev)
>>                titsc_writel(ts_dev, REG_IRQWAKEUP,
>>                                0x00);
>>                titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
>> +             pm_relax(ts_dev->mfd_tscadc->dev);
>>        }
>>        titsc_step_config(ts_dev);
>>        titsc_writel(ts_dev, REG_FIFO0THR,
>> -- 
>> 2.17.0
>> 
> 
> Thanks.
> 
> -- 
> Dmitry