Hi Tony,

Attached my initial attempt to port tsc200x-core to the runtime pm
framework (based on your work for the atmel_mxt driver). I've tested
that it suspends and resumes at the right time (when the files are being
opened), and the touchscreen still works on Maemo Leste.

I'll send the patch later to the linux-omap and the input list if it
makes sense.

The driver still blocks idle... I have a few thoughts, wonder what your
take is. One thing I'd like to assert: am I supposed to read
"cm_idlest1_core" to read the blockers, not "cm_idlest_per", see [1].

It looks like the "tsc200x_stop_scan" methods are called upon pm
suspend, but that does not seem to be enough.

I have a few thoughts:

1. The driver should disable interrupts with enable/disable_irq in
resume/suspend, use IRQF_NO_AUTOEN and not call irq_set_irq_wake

2. Same with the regulator (seems to make less sense, hard powering off
the touch screen)

Is there something else that comes to mind?

Additionally, the driver could/should call some runtime pm upon probe,
like you did in the atmel_mxt driver, perhaps with some autosuspend
profile as well, so that it can suspend when screen is not in use for a
while, but the device is not locked.

Cheers,
Merlijn

PS: I'm keeping track of blocking drivers here:
https://github.com/maemo-leste/bugtracker/issues/545#issuecomment-980458784


[1]

root@(none):/root# sleep 5 ; ./idle.sh
ST_SDRC,ST_OMAPCTRL
OFF:235,RET:67
root@(none):/root# modprobe tsc2005
root@(none):/root# sleep 5 ; ./idle.sh
ST_SDRC,ST_OMAPCTRL,ST_MCSPI1
OFF:235,RET:67
root@(none):/root# sleep 5 ; ./idle.sh
ST_SDRC,ST_OMAPCTRL,ST_MCSPI1
OFF:235,RET:67
root@(none):/root# rmmod tsc2005
root@(none):/root# sleep 5 ; ./idle.sh
ST_SDRC,ST_OMAPCTRL
OFF:329,RET:94
From d43f08d6b99d142a28a3dd97481ce3adec0e7638 Mon Sep 17 00:00:00 2001
From: Merlijn Wajer <merlijn@???>
Date: Sat, 27 Nov 2021 13:31:33 +0100
Subject: [PATCH] wip: Input: tsc200x-core: use runtime PM instead of custom
functions

---
drivers/input/touchscreen/tsc200x-core.c | 110 ++++++++++++++---------
1 file changed, 69 insertions(+), 41 deletions(-)

diff --git a/drivers/input/touchscreen/tsc200x-core.c b/drivers/input/touchscreen/tsc200x-core.c
index b8d720d52013..ae124c356938 100644
--- a/drivers/input/touchscreen/tsc200x-core.c
+++ b/drivers/input/touchscreen/tsc200x-core.c
@@ -18,6 +18,7 @@
#include <linux/delay.h>
#include <linux/pm.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/regmap.h>
#include <linux/gpio/consumer.h>
@@ -97,9 +98,6 @@ struct tsc200x {

     unsigned int        x_plate_ohm;

-    bool            opened;
-    bool            suspended;
-
     bool            pen_down;

     struct regulator    *vio;
@@ -226,9 +224,20 @@ static void tsc200x_reset(struct tsc200x *ts)
     }
 }

-/* must be called with ts->mutex held */
-static void __tsc200x_disable(struct tsc200x *ts)
+static int __maybe_unused tsc200x_runtime_suspend(struct device *dev)
 {
+    struct tsc200x *ts = dev_get_drvdata(dev);
+    struct input_dev *input_dev = ts->idev;
+    int mutex_ac;
+
+    if (!input_dev) {
+        return 0;
+    }
+
+    /* Try to acquire it, if it's already acquired, that's fine too, we just
+     * won't unlock it */
+    mutex_ac = mutex_trylock(&ts->mutex);
+
     tsc200x_stop_scan(ts);

     disable_irq(ts->irq);
@@ -237,11 +246,27 @@ static void __tsc200x_disable(struct tsc200x *ts)
     cancel_delayed_work_sync(&ts->esd_work);

     enable_irq(ts->irq);
+
+    if (mutex_ac)
+        mutex_unlock(&ts->mutex);
+
+    return 0;
 }

-/* must be called with ts->mutex held */
-static void __tsc200x_enable(struct tsc200x *ts)
+static int __maybe_unused tsc200x_runtime_resume(struct device *dev)
 {
+    struct tsc200x *ts = dev_get_drvdata(dev);
+    struct input_dev *input_dev = ts->idev;
+    int mutex_ac;
+
+    if (!input_dev) {
+        return 0;
+    }
+
+    /* Try to acquire it, if it's already acquired, that's fine too, we just
+     * won't unlock it */
+    mutex_ac = mutex_trylock(&ts->mutex);
+
     tsc200x_start_scan(ts);

     if (ts->esd_timeout && ts->reset_gpio) {
@@ -250,6 +275,11 @@ static void __tsc200x_enable(struct tsc200x *ts)
                 round_jiffies_relative(
                     msecs_to_jiffies(ts->esd_timeout)));
     }
+
+    if (mutex_ac)
+        mutex_unlock(&ts->mutex);
+
+    return 0;
 }

 static ssize_t tsc200x_selftest_show(struct device *dev,
@@ -263,12 +293,13 @@ static ssize_t tsc200x_selftest_show(struct device *dev,
     bool success = true;
     int error;

-    mutex_lock(&ts->mutex);

     /*
      * Test TSC200X communications via temp high register.
      */
-    __tsc200x_disable(ts);
+    mutex_lock(&ts->mutex);
+
+    tsc200x_runtime_suspend(dev);

     error = regmap_read(ts->regmap, TSC200X_REG_TEMP_HIGH, &temp_high_orig);
     if (error) {
@@ -322,7 +353,7 @@ static ssize_t tsc200x_selftest_show(struct device *dev,
     }

 out:
-    __tsc200x_enable(ts);
+    tsc200x_runtime_resume(dev);
     mutex_unlock(&ts->mutex);

     return sprintf(buf, "%d\n", success);
@@ -403,22 +434,21 @@ static void tsc200x_esd_work(struct work_struct *work)
 reschedule:
     /* re-arm the watchdog */
     schedule_delayed_work(&ts->esd_work,
-                  round_jiffies_relative(
+                  round_jiffies_relative(
                     msecs_to_jiffies(ts->esd_timeout)));
 }

 static int tsc200x_open(struct input_dev *input)
 {
     struct tsc200x *ts = input_get_drvdata(input);
+    int error;

-    mutex_lock(&ts->mutex);
+    error = pm_runtime_get_sync(ts->dev);

-    if (!ts->suspended)
-        __tsc200x_enable(ts);
-
-    ts->opened = true;
-
-    mutex_unlock(&ts->mutex);
+    if (error < 0) {
+        pm_runtime_put_noidle(ts->dev);
+        return error;
+    }

     return 0;
 }
@@ -427,14 +457,7 @@ static void tsc200x_close(struct input_dev *input)
 {
     struct tsc200x *ts = input_get_drvdata(input);

-    mutex_lock(&ts->mutex);
-
-    if (!ts->suspended)
-        __tsc200x_disable(ts);
-
-    ts->opened = false;
-
-    mutex_unlock(&ts->mutex);
+    pm_runtime_put_sync(ts->dev);
 }

int tsc200x_probe(struct device *dev, int irq, const struct input_id *tsc_id,
@@ -535,6 +558,8 @@ int tsc200x_probe(struct device *dev, int irq, const struct input_id *tsc_id,

     touchscreen_parse_properties(input_dev, false, NULL);

+    pm_runtime_enable(ts->dev);
+
     /* Ensure the touchscreen is off */
     tsc200x_stop_scan(ts);

@@ -572,6 +597,7 @@ int tsc200x_probe(struct device *dev, int irq, const struct input_id *tsc_id,
 err_remove_sysfs:
     sysfs_remove_group(&dev->kobj, &tsc200x_attr_group);
 disable_regulator:
+    pm_runtime_disable(ts->dev);
     regulator_disable(ts->vio);
     return error;
 }
@@ -583,6 +609,7 @@ int tsc200x_remove(struct device *dev)

     sysfs_remove_group(&dev->kobj, &tsc200x_attr_group);

+    pm_runtime_disable(ts->dev);
     regulator_disable(ts->vio);

     return 0;
@@ -592,15 +619,14 @@ EXPORT_SYMBOL_GPL(tsc200x_remove);
 static int __maybe_unused tsc200x_suspend(struct device *dev)
 {
     struct tsc200x *ts = dev_get_drvdata(dev);
+    struct input_dev *input_dev = ts->idev;

-    mutex_lock(&ts->mutex);
+    if (!input_dev)
+        return 0;

-    if (!ts->suspended && ts->opened)
-        __tsc200x_disable(ts);
-
-    ts->suspended = true;
-
-    mutex_unlock(&ts->mutex);
+    if (input_device_enabled(input_dev)) {
+        tsc200x_runtime_suspend(dev);
+    }

     return 0;
 }
@@ -608,20 +634,22 @@ static int __maybe_unused tsc200x_suspend(struct device *dev)
 static int __maybe_unused tsc200x_resume(struct device *dev)
 {
     struct tsc200x *ts = dev_get_drvdata(dev);
+    struct input_dev *input_dev = ts->idev;

-    mutex_lock(&ts->mutex);
+    if (!input_dev)
+        return 0;

-    if (ts->suspended && ts->opened)
-        __tsc200x_enable(ts);
-
-    ts->suspended = false;
-
-    mutex_unlock(&ts->mutex);
+    if (input_device_enabled(input_dev)) {
+        tsc200x_runtime_resume(dev);
+    }

     return 0;
 }

-SIMPLE_DEV_PM_OPS(tsc200x_pm_ops, tsc200x_suspend, tsc200x_resume);
+const struct dev_pm_ops tsc200x_pm_ops = {
+    SET_SYSTEM_SLEEP_PM_OPS(tsc200x_suspend, tsc200x_resume)
+    SET_RUNTIME_PM_OPS(tsc200x_runtime_suspend, tsc200x_runtime_resume, NULL)
+};
 EXPORT_SYMBOL_GPL(tsc200x_pm_ops);

MODULE_AUTHOR("Lauri Leukkunen <lauri.leukkunen@???>");
--
2.32.0