[PATCH v3 0/1] Small fixes to the cyttsp5 touchscreen driver

[PATCH v3 0/1] Small fixes to the cyttsp5 touchscreen driver

[Maximilian Weigand]

From: Maximilian Weigand <mweigand@mweigand.net>

Currently the cyttsp5 driver does not put the device to sleep during
suspend, which can have unwanted side effects if an irq trigger of type
EDGE_FALLING is used, as well as leads to increased power usage.

Correspondingly, sleep and wakeup commands to the touchscreen were
derived from the GPL-2 vendor/android driver by Cypress Semiconductor
(copyright note for Cypress Semiconductor is already in the current
driver).

Tested on the Pine64 PineNote.

Changes in v3:
- dropped patches 1,2,5: already applied in v2
- dropped patch 4 "Input: cyttsp5 - properly initialize the device as a
  pm wakeup device" - functionality is already taken care of in the
  kernel
- dropped patch 3 "dt-bindings: input: cypress,tt21000 - fix interrupt
  type in dts example": it was suggested that the driver should work
  with both interrupt types, falling edge and level low. Once a solution
  is found it will be submitted as a separate patch.
- reworked patch 6 "Input: cyttsp5 - implement proper sleep and wakeup
  procedures" in response to review comments:
	- use the existing completion instead of adding a new one for
	  sleep/wakeup command handling
	- use device_may_wakeup() to determine if the device should be
	  suspended upon entering standby
	- clarified commit message

Changes in v2:
- fix subject lines
- fix 'unused variable' errors reported by the kernel test robot
- clean up commit message of patch 2

Maximilian Weigand (1):
  Input: cyttsp5 - implement proper sleep and wakeup procedures

 drivers/input/touchscreen/cyttsp5.c | 118 ++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)


base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
--
2.39.2

[PATCH v3 1/1] Input: cyttsp5 - implement proper sleep and wakeup procedures

[Maximilian Weigand]

From: Maximilian Weigand <mweigand@mweigand.net>

The touchscreen can be put into a deep sleep state that prevents it from
emitting touch irqs. Put the touchscreen into deep sleep during suspend
if it is not marked as a wakeup source.

This also fixes a problem with the touchscreen getting unresponsive after
system resume when a falling edge trigger is used for the interrupt.
When left on during suspend, the touchscreen would pull the interrupt
line down in response to touch events, leaving the interrupt effectively
disabled after resume.

Signed-off-by: Maximilian Weigand <mweigand@mweigand.net>
Reviewed-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/cyttsp5.c | 118 ++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
index 30102cb80fac..bdb63eeed59c 100644
--- a/drivers/input/touchscreen/cyttsp5.c
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -43,6 +43,7 @@
 #define HID_DESC_REG				0x1
 #define HID_INPUT_REG				0x3
 #define HID_OUTPUT_REG				0x4
+#define HID_COMMAND_REG             0x5

 #define REPORT_ID_TOUCH				0x1
 #define REPORT_ID_BTN				0x3
@@ -68,6 +69,7 @@
 #define HID_APP_OUTPUT_REPORT_ID		0x2F
 #define HID_BL_RESPONSE_REPORT_ID		0x30
 #define HID_BL_OUTPUT_REPORT_ID			0x40
+#define HID_RESPONSE_REPORT_ID      0xF0

 #define HID_OUTPUT_RESPONSE_REPORT_OFFSET	2
 #define HID_OUTPUT_RESPONSE_CMD_OFFSET		4
@@ -78,9 +80,15 @@
 #define HID_SYSINFO_BTN_MASK			GENMASK(7, 0)
 #define HID_SYSINFO_MAX_BTN			8

+#define HID_CMD_SET_POWER           0x8
+
+#define HID_POWER_ON                0x0
+#define HID_POWER_SLEEP             0x1
+
 #define CY_HID_OUTPUT_TIMEOUT_MS		200
 #define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT_MS	3000
 #define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT_MS	4000
+#define CY_HID_SET_POWER_TIMEOUT		500

 /* maximum number of concurrent tracks */
 #define TOUCH_REPORT_SIZE			10
@@ -100,6 +108,14 @@
 #define TOUCH_REPORT_USAGE_PG_MIN		0xFF010063
 #define TOUCH_COL_USAGE_PG			0x000D0022

+#define SET_CMD_LOW(byte, bits) \
+	((byte) = (((byte) & 0xF0) | ((bits) & 0x0F)))
+#define SET_CMD_HIGH(byte, bits)\
+	((byte) = (((byte) & 0x0F) | ((bits) & 0xF0)))
+#define SET_CMD_OPCODE(byte, opcode) SET_CMD_LOW(byte, opcode)
+#define SET_CMD_REPORT_TYPE(byte, type) SET_CMD_HIGH(byte, ((type) << 4))
+#define SET_CMD_REPORT_ID(byte, id) SET_CMD_LOW(byte, id)
+
 /* System Information interface definitions */
 struct cyttsp5_sensing_conf_data_dev {
 	u8 electrodes_x;
@@ -557,6 +573,82 @@ static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
 	return cyttsp5_get_sysinfo_regs(ts);
 }

+static int cyttsp5_enter_sleep(struct cyttsp5 *ts)
+{
+	int rc;
+	u8 cmd[2];
+
+	memset(cmd, 0, sizeof(cmd));
+
+	SET_CMD_REPORT_TYPE(cmd[0], 0);
+	SET_CMD_REPORT_ID(cmd[0], HID_POWER_SLEEP);
+	SET_CMD_OPCODE(cmd[1], HID_CMD_SET_POWER);
+
+	rc = cyttsp5_write(ts, HID_COMMAND_REG, cmd, 2);
+	if (rc) {
+		dev_err(ts->dev, "Failed to write command %d", rc);
+		return rc;
+	}
+
+	rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
+				msecs_to_jiffies(CY_HID_SET_POWER_TIMEOUT));
+	if (rc <= 0) {
+		dev_err(ts->dev, "HID output cmd execution timed out\n");
+		rc = -ETIMEDOUT;
+		return rc;
+	}
+
+	/* validate */
+	if ((ts->response_buf[2] != HID_RESPONSE_REPORT_ID)
+			|| ((ts->response_buf[3] & 0x3) != HID_POWER_SLEEP)
+			|| ((ts->response_buf[4] & 0xF) != HID_CMD_SET_POWER)) {
+		rc = -EINVAL;
+		dev_err(ts->dev, "Validation of the sleep response failed\n");
+		return rc;
+	}
+
+	return 0;
+
+}
+
+static int cyttsp5_wakeup(struct cyttsp5 *ts)
+{
+	int rc;
+	u8 cmd[2];
+
+	memset(cmd, 0, sizeof(cmd));
+
+	SET_CMD_REPORT_TYPE(cmd[0], 0);
+	SET_CMD_REPORT_ID(cmd[0], HID_POWER_ON);
+	SET_CMD_OPCODE(cmd[1], HID_CMD_SET_POWER);
+
+	rc = cyttsp5_write(ts, HID_COMMAND_REG, cmd, 2);
+	if (rc) {
+		dev_err(ts->dev, "Failed to write command %d", rc);
+		return rc;
+	}
+
+	rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
+				msecs_to_jiffies(CY_HID_SET_POWER_TIMEOUT));
+	if (rc <= 0) {
+		dev_err(ts->dev, "HID output cmd execution timed out\n");
+		rc = -ETIMEDOUT;
+		return rc;
+	}
+
+	/* validate */
+	if ((ts->response_buf[2] != HID_RESPONSE_REPORT_ID)
+			|| ((ts->response_buf[3] & 0x3) != HID_POWER_ON)
+			|| ((ts->response_buf[4] & 0xF) != HID_CMD_SET_POWER)) {
+		rc = -EINVAL;
+		dev_err(ts->dev, "Validation of the wakeup response failed\n");
+		return rc;
+	}
+
+	return 0;
+
+}
+
 static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
 {
 	int rc;
@@ -675,6 +767,10 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
 	case HID_BTN_REPORT_ID:
 		cyttsp5_btn_attention(ts->dev);
 		break;
+	case HID_RESPONSE_REPORT_ID:
+		memcpy(ts->response_buf, ts->input_buf, size);
+		complete(&ts->cmd_done);
+		break;
 	default:
 		/* It is not an input but a command response */
 		memcpy(ts->response_buf, ts->input_buf, size);
@@ -886,10 +982,32 @@ static const struct i2c_device_id cyttsp5_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);

+static int __maybe_unused cyttsp5_suspend(struct device *dev)
+{
+	struct cyttsp5 *ts = dev_get_drvdata(dev);
+
+	if (!device_may_wakeup(dev))
+		cyttsp5_enter_sleep(ts);
+	return 0;
+}
+
+static int __maybe_unused cyttsp5_resume(struct device *dev)
+{
+	struct cyttsp5 *ts = dev_get_drvdata(dev);
+
+	if (!device_may_wakeup(dev))
+		cyttsp5_wakeup(ts);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cyttsp5_pm, cyttsp5_suspend, cyttsp5_resume);
+
 static struct i2c_driver cyttsp5_i2c_driver = {
 	.driver = {
 		.name = CYTTSP5_NAME,
 		.of_match_table = cyttsp5_of_match,
+		.pm     = &cyttsp5_pm,
 	},
 	.probe_new = cyttsp5_i2c_probe,
 	.id_table = cyttsp5_i2c_id,
--
2.39.2

Re: [PATCH v3 1/1] Input: cyttsp5 - implement proper sleep and wakeup procedures

[Dmitry Torokhov]

Hi Maximilian,

On Thu, May 04, 2023 at 02:03:16PM +0200, Maximilian Weigand wrote:
> +static int cyttsp5_enter_sleep(struct cyttsp5 *ts)
...
> +static int cyttsp5_wakeup(struct cyttsp5 *ts)

These 2 look like twins, I collapsed them into cyttsp5_power_control()
and applied the patch, thank you.

-- 
Dmitry