* [PATCH v2 0/4] Add support for the Cypress cyttsp5
@ 2021-11-03 11:48 Alistair Francis
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-03 11:48 UTC (permalink / raw)
To: linux-arm-kernel, devicetree, linux-input, linux-kernel
Cc: mylene.josserand, linus.walleij, andreas, rydberg,
dmitry.torokhov, robh+dt, alistair23, Alistair Francis
This patch series builds on top of [1] and adds support for the cyttsp5
touchscreen controller for the reMarkable 2.
I first tried to add an I2C HID device. Although the cyttsp5 has some HID
looking aspects it is not HID compatible. Just in trying to probe the device
I found:
- The HID descriptor has extra padding
- The HID descriptor sets the high bytes of the descriptor length
- The HID descriptor has extra unrecognised tags
- The HID reset command doesn't appear to work
I don't think there is a way to use the I2C HID framework with the cyttsp5.
For anyone interested you can see the work here [2]. In that branch though I
can only obtain a HID descriptor, nothing else works without more core
changes.
So instead I rebased the series from [1]. Converted to the new yaml DTS
documentation, added regulator support and fixed a x/y miscalculation bug.
1: https://lwn.net/ml/linux-kernel/20180703094309.18514-1-mylene.josserand@bootlin.com/
2: https://github.com/alistair23/linux/commits/rM2-mainline-cyttsp5-hid
Alistair Francis (2):
ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen
ARM: dts: imx7d: remarkable2: Enable the cyttsp5
Mylène Josserand (2):
Input: Add driver for Cypress Generation 5 touchscreen
Documentation: DT: bindings: input: Add documentation for cyttsp5
.../input/touchscreen/cypress,tt21000.yaml | 92 ++
arch/arm/boot/dts/imx7d-remarkable2.dts | 89 ++
arch/arm/configs/imx_v6_v7_defconfig | 1 +
drivers/input/touchscreen/Kconfig | 14 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/cyttsp5.c | 1135 +++++++++++++++++
6 files changed, 1332 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
create mode 100644 drivers/input/touchscreen/cyttsp5.c
--
2.31.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
@ 2021-11-03 11:48 ` Alistair Francis
2021-11-05 15:01 ` Andreas Kemnade
` (2 more replies)
2021-11-03 11:48 ` [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5 Alistair Francis
` (3 subsequent siblings)
4 siblings, 3 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-03 11:48 UTC (permalink / raw)
To: linux-arm-kernel, devicetree, linux-input, linux-kernel
Cc: mylene.josserand, linus.walleij, andreas, rydberg,
dmitry.torokhov, robh+dt, alistair23, Mylène Josserand,
Maxime Ripard, Alistair Francis
From: Mylène Josserand <mylene.josserand@bootlin.com>
This is the basic driver for the Cypress TrueTouch Gen5 touchscreen
controllers. This driver supports only the I2C bus but it uses regmap
so SPI support could be added later.
The touchscreen can retrieve some defined zone that are handled as
buttons (according to the hardware). That is why it handles
button and multitouch events.
Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
Message-Id: <20180703094309.18514-2-mylene.josserand@bootlin.com>
Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
drivers/input/touchscreen/Kconfig | 14 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/cyttsp5.c | 1135 +++++++++++++++++++++++++++
3 files changed, 1150 insertions(+)
create mode 100644 drivers/input/touchscreen/cyttsp5.c
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index d4e74738c5a8..92c8b15b0025 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -284,6 +284,20 @@ config TOUCHSCREEN_CYTTSP4_SPI
To compile this driver as a module, choose M here: the
module will be called cyttsp4_spi.
+config TOUCHSCREEN_CYTTSP5
+ tristate "Cypress TrueTouch Gen5 Touchscreen Driver"
+ depends on OF && I2C
+ select REGMAP_I2C
+ select CRC_ITU_T
+ help
+ Driver for Parade TrueTouch Standard Product
+ Generation 5 touchscreen controllers.
+ I2C bus interface support only.
+ Say Y here if you have a Cypress Gen5 touchscreen.
+ If unsure, say N.
+ To compile this driver as a module, choose M here: the
+ module will be called cyttsp5.
+
config TOUCHSCREEN_DA9034
tristate "Touchscreen support for Dialog Semiconductor DA9034"
depends on PMIC_DA903X
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7d34100f7f22..d4d6e56e70de 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o
obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_CORE) += cyttsp4_core.o
obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_I2C) += cyttsp4_i2c.o cyttsp_i2c_common.o
obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_SPI) += cyttsp4_spi.o
+obj-$(CONFIG_TOUCHSCREEN_CYTTSP5) += cyttsp5.o
obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o
obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.o
obj-$(CONFIG_TOUCHSCREEN_DYNAPRO) += dynapro.o
diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
new file mode 100644
index 000000000000..77938cb0c4bb
--- /dev/null
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -0,0 +1,1135 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Parade TrueTouch(TM) Standard Product V5 Module.
+ *
+ * Copyright (C) 2015 Parade Technologies
+ * Copyright (C) 2012-2015 Cypress Semiconductor
+ * Copyright (C) 2018 Bootlin
+ *
+ * Authors: Mylène Josserand <mylene.josserand@bootlin.com>
+ * Alistair Francis <alistair@alistair23.me>
+ *
+ */
+
+#include <asm/unaligned.h>
+#include <linux/crc-itu-t.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#define CYTTSP5_NAME "cyttsp5"
+#define CY_I2C_DATA_SIZE (2 * 256)
+#define HID_VERSION 0x0100
+#define CY_MAX_INPUT 512
+#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32
+#define CY_BITS_PER_BTN 1
+#define CY_NUM_BTN_EVENT_ID ((1 << CY_BITS_PER_BTN) - 1)
+
+#define MAX_AREA 255
+#define HID_OUTPUT_BL_SOP 0x1
+#define HID_OUTPUT_BL_EOP 0x17
+#define HID_OUTPUT_BL_LAUNCH_APP 0x3B
+#define HID_OUTPUT_BL_LAUNCH_APP_SIZE 11
+#define HID_OUTPUT_GET_SYSINFO 0x2
+#define HID_OUTPUT_GET_SYSINFO_SIZE 5
+#define HID_OUTPUT_MAX_CMD_SIZE 12
+
+#define HID_DESC_REG 0x1
+#define HID_INPUT_REG 0x3
+#define HID_OUTPUT_REG 0x4
+
+#define REPORT_ID_TOUCH 0x1
+#define REPORT_ID_BTN 0x3
+#define REPORT_SIZE_5 5
+#define REPORT_SIZE_8 8
+#define REPORT_SIZE_16 16
+
+/* Touch reports offsets */
+/* Header offsets */
+#define TOUCH_REPORT_DESC_HDR_CONTACTCOUNT 16
+/* Record offsets */
+#define TOUCH_REPORT_DESC_CONTACTID 8
+#define TOUCH_REPORT_DESC_X 16
+#define TOUCH_REPORT_DESC_Y 32
+#define TOUCH_REPORT_DESC_P 48
+#define TOUCH_REPORT_DESC_MAJ 56
+#define TOUCH_REPORT_DESC_MIN 64
+
+/* HID */
+#define HID_TOUCH_REPORT_ID 0x1
+#define HID_BTN_REPORT_ID 0x3
+#define HID_APP_RESPONSE_REPORT_ID 0x1F
+#define HID_APP_OUTPUT_REPORT_ID 0x2F
+#define HID_BL_RESPONSE_REPORT_ID 0x30
+#define HID_BL_OUTPUT_REPORT_ID 0x40
+
+#define HID_OUTPUT_RESPONSE_REPORT_OFFSET 2
+#define HID_OUTPUT_RESPONSE_CMD_OFFSET 4
+#define HID_OUTPUT_RESPONSE_CMD_MASK 0x7F
+
+#define HID_SYSINFO_SENSING_OFFSET 33
+#define HID_SYSINFO_BTN_OFFSET 48
+#define HID_SYSINFO_BTN_MASK 0xFF
+#define HID_SYSINFO_MAX_BTN 8
+
+/* Timeout in ms */
+#define CY_HID_OUTPUT_TIMEOUT 200
+#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000
+#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000
+
+/* maximum number of concurrent tracks */
+#define TOUCH_REPORT_SIZE 10
+#define TOUCH_INPUT_HEADER_SIZE 7
+#define BTN_REPORT_SIZE 9
+#define BTN_INPUT_HEADER_SIZE 5
+
+#define MAX_CY_TCH_T_IDS 32
+
+/* All usage pages for Touch Report */
+#define TOUCH_REPORT_USAGE_PG_X 0x00010030
+#define TOUCH_REPORT_USAGE_PG_Y 0x00010031
+#define TOUCH_REPORT_USAGE_PG_P 0x000D0030
+#define TOUCH_REPORT_USAGE_PG_CONTACTID 0x000D0051
+#define TOUCH_REPORT_USAGE_PG_CONTACTCOUNT 0x000D0054
+#define TOUCH_REPORT_USAGE_PG_MAJ 0xFF010062
+#define TOUCH_REPORT_USAGE_PG_MIN 0xFF010063
+#define TOUCH_COL_USAGE_PG 0x000D0022
+
+/* helpers */
+#define HI_BYTE(x) ((u8)(((x) >> 8) & 0xFF))
+#define LOW_BYTE(x) ((u8)((x) & 0xFF))
+
+/* System Information interface definitions */
+struct cyttsp5_sensing_conf_data_dev {
+ u8 electrodes_x;
+ u8 electrodes_y;
+ __le16 len_x;
+ __le16 len_y;
+ __le16 res_x;
+ __le16 res_y;
+ __le16 max_z;
+ u8 origin_x;
+ u8 origin_y;
+ u8 btn;
+ u8 scan_mode;
+ u8 max_num_of_tch_per_refresh_cycle;
+} __packed;
+
+struct cyttsp5_sensing_conf_data {
+ u16 res_x;
+ u16 res_y;
+ u16 max_z;
+ u16 len_x;
+ u16 len_y;
+ u8 origin_x;
+ u8 origin_y;
+ u8 max_tch;
+};
+
+enum hid_cmd_state {
+ HID_CMD_DONE,
+ HID_CMD_BUSY,
+};
+
+enum cyttsp5_tch_abs { /* for ordering within the extracted touch data array */
+ CY_TCH_X, /* X */
+ CY_TCH_Y, /* Y */
+ CY_TCH_P, /* P (Z) */
+ CY_TCH_T, /* TOUCH ID */
+ CY_TCH_MAJ, /* TOUCH_MAJOR */
+ CY_TCH_MIN, /* TOUCH_MINOR */
+ CY_TCH_NUM_ABS,
+};
+
+struct cyttsp5_tch_abs_params {
+ size_t ofs; /* abs byte offset */
+ size_t size; /* size in bits */
+ size_t min; /* min value */
+ size_t max; /* max value */
+ size_t bofs; /* bit offset */
+};
+
+struct cyttsp5_touch {
+ int hdr;
+ int abs[CY_TCH_NUM_ABS];
+};
+
+struct cyttsp5_sysinfo {
+ struct cyttsp5_sensing_conf_data sensing_conf_data;
+ int num_btns;
+ struct cyttsp5_tch_abs_params tch_hdr;
+ struct cyttsp5_tch_abs_params tch_abs[CY_TCH_NUM_ABS];
+ u32 key_code[HID_SYSINFO_MAX_BTN];
+ u8 *xy_mode;
+ u8 *xy_data;
+};
+
+struct cyttsp5_hid_desc {
+ __le16 hid_desc_len;
+ u8 packet_id;
+ u8 reserved_byte;
+ __le16 bcd_version;
+ __le16 report_desc_len;
+ __le16 report_desc_register;
+ __le16 input_register;
+ __le16 max_input_len;
+ __le16 output_register;
+ __le16 max_output_len;
+ __le16 command_register;
+ __le16 data_register;
+ __le16 vendor_id;
+ __le16 product_id;
+ __le16 version_id;
+ u8 reserved[4];
+} __packed;
+
+struct cyttsp5 {
+ struct device *dev;
+ struct mutex system_lock;
+ wait_queue_head_t wait_q;
+ struct cyttsp5_sysinfo sysinfo;
+ int hid_cmd_state;
+ struct cyttsp5_hid_desc hid_desc;
+ u8 cmd_buf[CYTTSP5_PREALLOCATED_CMD_BUFFER];
+ u8 input_buf[CY_MAX_INPUT];
+ u8 response_buf[CY_MAX_INPUT];
+ struct gpio_desc *reset_gpio;
+ struct input_dev *input;
+ char phys[NAME_MAX];
+ int num_prv_rec;
+ struct regmap *regmap;
+ struct touchscreen_properties prop;
+ struct regulator *vdd;
+};
+
+/*
+ * For what understood in the datasheet, the register does not
+ * matter. For consistency, used the Input Register address
+ * but it does mean anything to the device. The important data
+ * to send is the I2C address
+ */
+static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
+{
+ int rc;
+ u32 size;
+ u8 temp[2];
+
+ if (!buf)
+ return -EINVAL;
+
+ /* Read the frame to retrieve the size */
+ rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, 2);
+ if (rc < 0)
+ return rc;
+
+ size = get_unaligned_le16(temp);
+ if (!size || size == 2)
+ return 0;
+
+ if (size > max)
+ return -EINVAL;
+
+ /* Get the real value */
+ return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size);
+}
+
+static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
+ size_t size)
+{
+ u8 cmd[HID_OUTPUT_MAX_CMD_SIZE];
+
+ if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE) {
+ return -ENOMEM;
+ }
+
+ /* High bytes of register address needed as first byte of cmd */
+ cmd[0] = HI_BYTE(reg);
+
+ /* Copy the rest of the data */
+ if (data)
+ memcpy(&cmd[1], data, size);
+
+ /* The hardware wants to receive a frame with the address register
+ * contains in the first two bytes. As the regmap_write function
+ * add the register adresse in the frame, we use the LOW_BYTE as
+ * first frame byte for the address register and the first
+ * data byte is the high register + left of the cmd to send
+ */
+ return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1);
+}
+
+static void cyttsp5_final_sync(struct input_dev *input, int max_slots,
+ unsigned long *ids)
+{
+ int t;
+
+ for (t = 0; t < max_slots; t++) {
+ if (test_bit(t, ids))
+ continue;
+ input_mt_slot(input, t);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
+ }
+
+ input_sync(input);
+}
+
+static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots)
+{
+ int t;
+
+ if (ts->num_prv_rec == 0)
+ return;
+
+ for (t = 0; t < max_slots; t++) {
+ input_mt_slot(ts->input, t);
+ input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false);
+ }
+}
+
+static void cyttsp5_mt_lift_all(struct cyttsp5 *ts)
+{
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int max = si->tch_abs[CY_TCH_T].max;
+
+ if (ts->num_prv_rec != 0) {
+ cyttsp5_report_slot_liftoff(ts, max);
+ input_sync(ts->input);
+ ts->num_prv_rec = 0;
+ }
+}
+
+static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
+ int bofs)
+{
+ int nbyte;
+
+ for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
+ *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
+
+ *axis &= max - 1;
+}
+
+static void cyttsp5_get_touch_record(struct cyttsp5 *ts,
+ struct cyttsp5_touch *touch, u8 *xy_data)
+{
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ enum cyttsp5_tch_abs abs;
+
+ for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) {
+ cyttsp5_get_touch_axis(&touch->abs[abs],
+ si->tch_abs[abs].size,
+ si->tch_abs[abs].max,
+ xy_data + si->tch_abs[abs].ofs,
+ si->tch_abs[abs].bofs);
+ }
+}
+
+static void cyttsp5_get_mt_touches(struct cyttsp5 *ts,
+ struct cyttsp5_touch *tch, int num_cur_tch)
+{
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int i, t = 0;
+ DECLARE_BITMAP(ids, MAX_CY_TCH_T_IDS);
+ u8 *tch_addr;
+ int tmp;
+
+ bitmap_zero(ids, MAX_CY_TCH_T_IDS);
+ memset(tch->abs, 0, sizeof(tch->abs));
+
+ for (i = 0; i < num_cur_tch; i++) {
+ tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE);
+ cyttsp5_get_touch_record(ts, tch, tch_addr);
+
+ /* Convert MAJOR/MINOR from mm to resolution */
+ tmp = tch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x;
+ tch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x;
+ tmp = tch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x;
+ tch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x;
+
+ t = tch->abs[CY_TCH_T];
+ input_mt_slot(ts->input, t);
+ input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true);
+ __set_bit(t, ids);
+
+ /* position and pressure fields */
+ input_report_abs(ts->input, ABS_MT_POSITION_X,
+ tch->abs[CY_TCH_X]);
+ input_report_abs(ts->input, ABS_MT_POSITION_Y,
+ tch->abs[CY_TCH_Y]);
+ input_report_abs(ts->input, ABS_MT_PRESSURE,
+ tch->abs[CY_TCH_P]);
+
+ /* Get the extended touch fields */
+ input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR,
+ tch->abs[CY_TCH_MAJ]);
+ input_report_abs(ts->input, ABS_MT_TOUCH_MINOR,
+ tch->abs[CY_TCH_MIN]);
+
+ touchscreen_report_pos(ts->input, &ts->prop,
+ tch->abs[CY_TCH_X], tch->abs[CY_TCH_Y],
+ true);
+ }
+
+ cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids);
+
+ ts->num_prv_rec = num_cur_tch;
+}
+
+/* read xy_data for all current touches */
+static int cyttsp5_xy_worker(struct cyttsp5 *ts)
+{
+ struct device *dev = ts->dev;
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int max_tch = si->sensing_conf_data.max_tch;
+ struct cyttsp5_touch tch;
+ u8 num_cur_tch;
+
+ cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size,
+ si->tch_hdr.max,
+ si->xy_mode + 3 + si->tch_hdr.ofs,
+ si->tch_hdr.bofs);
+
+ num_cur_tch = tch.hdr;
+ if (num_cur_tch > max_tch) {
+ dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch);
+ num_cur_tch = max_tch;
+ }
+
+ if (num_cur_tch == 0 && ts->num_prv_rec == 0)
+ return 0;
+
+ /* extract xy_data for all currently reported touches */
+ if (num_cur_tch)
+ cyttsp5_get_mt_touches(ts, &tch, num_cur_tch);
+ else
+ cyttsp5_mt_lift_all(ts);
+
+ return 0;
+}
+
+static int cyttsp5_mt_attention(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int rc;
+
+ if (si->xy_mode[2] != HID_TOUCH_REPORT_ID)
+ return 0;
+
+ /* core handles handshake */
+ rc = cyttsp5_xy_worker(ts);
+ if (rc < 0)
+ dev_err(dev, "xy_worker error r=%d\n", rc);
+
+ return rc;
+}
+
+static int cyttsp5_setup_input_device(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int max_x, max_y, max_p;
+ int max_x_tmp, max_y_tmp;
+ int rc;
+
+ __set_bit(EV_ABS, ts->input->evbit);
+ __set_bit(EV_REL, ts->input->evbit);
+ __set_bit(EV_KEY, ts->input->evbit);
+
+ max_x_tmp = si->sensing_conf_data.res_x;
+ max_y_tmp = si->sensing_conf_data.res_y;
+ max_x = max_x_tmp - 1;
+ max_y = max_y_tmp - 1;
+ max_p = si->sensing_conf_data.max_z;
+
+ input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0);
+
+ __set_bit(ABS_MT_POSITION_X, ts->input->absbit);
+ __set_bit(ABS_MT_POSITION_Y, ts->input->absbit);
+ __set_bit(ABS_MT_PRESSURE, ts->input->absbit);
+
+ input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0);
+ input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
+ input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0);
+
+ input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
+ input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0);
+
+ rc = input_register_device(ts->input);
+ if (rc < 0)
+ dev_err(dev, "Error, failed register input device r=%d\n", rc);
+
+ return rc;
+}
+
+#ifdef CONFIG_OF
+static int cyttsp5_parse_dt_key_code(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ struct device_node *np;
+ int i;
+
+ np = dev->of_node;
+ if (!np)
+ return -EINVAL;
+
+ if (!si->num_btns)
+ return 0;
+
+ /* Initialize the button to RESERVED */
+ for (i = 0; i < si->num_btns; i++)
+ si->key_code[i] = KEY_RESERVED;
+
+ return of_property_read_u32_array(np, "linux,keycodes",
+ si->key_code, si->num_btns);
+}
+#else
+static int cyttsp5_parse_dt_key_code(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int i;
+
+ if (!si->num_btns)
+ return 0;
+
+ /* Initialize the button to RESERVED */
+ for (i = 0; i < si->num_btns; i++)
+ si->key_code[i] = KEY_RESERVED;
+
+ return 0;
+}
+#endif
+
+static int cyttsp5_btn_attention(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int cur_btn;
+ int cur_btn_state;
+
+ if (si->xy_mode[2] != HID_BTN_REPORT_ID || !si->num_btns)
+ return 0;
+
+ /* extract button press/release touch information */
+ for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) {
+ /* Get current button state */
+ cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN))
+ & CY_NUM_BTN_EVENT_ID;
+
+ input_report_key(ts->input, si->key_code[cur_btn],
+ cur_btn_state);
+ input_sync(ts->input);
+ }
+
+ return 0;
+}
+
+static u16 cyttsp5_compute_crc(u8 *buf, u32 size)
+{
+ u16 remainder = 0xFFFF;
+ u16 xor_mask = 0x0000;
+ u32 index;
+ u32 byte_value;
+ u32 table_index;
+ u32 crc_bit_width = sizeof(u16) * 8;
+
+ /* Divide the message by polynomial, via the table. */
+ for (index = 0; index < size; index++) {
+ byte_value = buf[index];
+ table_index = ((byte_value >> 4) & 0x0F)
+ ^ (remainder >> (crc_bit_width - 4));
+ remainder = crc_itu_t_table[table_index]
+ ^ (remainder << 4);
+ table_index = (byte_value & 0x0F)
+ ^ (remainder >> (crc_bit_width - 4));
+ remainder = crc_itu_t_table[table_index]
+ ^ (remainder << 4);
+ }
+
+ /* Perform the final remainder CRC. */
+ return remainder ^ xor_mask;
+}
+
+static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code)
+{
+ u16 size, crc;
+ u8 status, offset;
+ int command_code;
+
+ size = get_unaligned_le16(&ts->response_buf[0]);
+
+ if (!size)
+ return 0;
+
+ offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET];
+
+ if (offset == HID_BL_RESPONSE_REPORT_ID) {
+ if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) {
+ dev_err(ts->dev, "HID output response, wrong SOP\n");
+ return -EPROTO;
+ }
+
+ if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
+ dev_err(ts->dev, "HID output response, wrong EOP\n");
+ return -EPROTO;
+ }
+
+ crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
+ if (ts->response_buf[size - 3] != LOW_BYTE(crc) ||
+ ts->response_buf[size - 2] != HI_BYTE(crc)) {
+ dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n",
+ crc);
+ return -EPROTO;
+ }
+
+ status = ts->response_buf[5];
+ if (status) {
+ dev_err(ts->dev, "HID output response, ERROR:%d\n",
+ status);
+ return -EPROTO;
+ }
+ }
+
+ if (offset == HID_APP_RESPONSE_REPORT_ID) {
+ command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET]
+ & HID_OUTPUT_RESPONSE_CMD_MASK;
+ if (command_code != code) {
+ dev_err(ts->dev,
+ "HID output response, wrong command_code:%X\n",
+ command_code);
+ return -EPROTO;
+ }
+ }
+
+ return 0;
+}
+
+static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts)
+{
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+ int i;
+ unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET]
+ & HID_SYSINFO_BTN_MASK;
+
+ si->num_btns = 0;
+ for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
+ if (btns & BIT(i))
+ si->num_btns++;
+ }
+}
+
+static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
+{
+ struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data;
+ struct cyttsp5_sensing_conf_data_dev *scd_dev =
+ (struct cyttsp5_sensing_conf_data_dev *)
+ &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+
+ cyttsp5_si_get_btn_data(ts);
+
+ scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle;
+ scd->res_x = get_unaligned_le16(&scd_dev->res_x);
+ scd->res_y = get_unaligned_le16(&scd_dev->res_y);
+ scd->max_z = get_unaligned_le16(&scd_dev->max_z);
+ scd->len_x = get_unaligned_le16(&scd_dev->len_x);
+ scd->len_y = get_unaligned_le16(&scd_dev->len_y);
+
+ si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
+ GFP_KERNEL);
+ if (!si->xy_data)
+ return -ENOMEM;
+
+ si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE,
+ GFP_KERNEL);
+ if (!si->xy_mode)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
+{
+ int rc;
+ u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
+
+ ts->hid_cmd_state = HID_CMD_BUSY;
+
+ /* HI bytes of Output register address */
+ cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
+ cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
+ cmd[2] = HID_APP_OUTPUT_REPORT_ID;
+ cmd[3] = 0x0; /* Reserved */
+ cmd[4] = HID_OUTPUT_GET_SYSINFO;
+
+ rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
+ HID_OUTPUT_GET_SYSINFO_SIZE);
+ if (rc) {
+ dev_err(ts->dev, "Failed to write command %d", rc);
+ goto error;
+ }
+
+ rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
+ msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT));
+ if (!rc) {
+ dev_err(ts->dev, "HID output cmd execution timed out\n");
+ rc = -ETIME;
+ goto error;
+ }
+
+ rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO);
+ if (rc) {
+ dev_err(ts->dev, "Validation of the response failed\n");
+ goto error;
+ }
+
+ return cyttsp5_get_sysinfo_regs(ts);
+
+error:
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_DONE;
+ mutex_unlock(&ts->system_lock);
+ return rc;
+}
+
+static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
+{
+ int rc;
+ u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
+ u16 crc;
+
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_BUSY;
+ mutex_unlock(&ts->system_lock);
+
+ cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+ cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+ cmd[2] = HID_BL_OUTPUT_REPORT_ID;
+ cmd[3] = 0x0; /* Reserved */
+ cmd[4] = HID_OUTPUT_BL_SOP;
+ cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
+ cmd[6] = 0x0; /* Low bytes of data */
+ cmd[7] = 0x0; /* Hi bytes of data */
+ crc = cyttsp5_compute_crc(&cmd[4], 4);
+ cmd[8] = LOW_BYTE(crc);
+ cmd[9] = HI_BYTE(crc);
+ cmd[10] = HID_OUTPUT_BL_EOP;
+
+ rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
+ HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+ if (rc) {
+ dev_err(ts->dev, "Failed to write command %d", rc);
+ goto error;
+ }
+
+ rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
+ msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT));
+ if (!rc) {
+ dev_err(ts->dev, "HID output cmd execution timed out\n");
+ rc = -ETIME;
+ goto error;
+ }
+
+ rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP);
+ if (rc) {
+ dev_err(ts->dev, "Validation of the response failed\n");
+ goto error;
+ }
+
+ return rc;
+
+error:
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_DONE;
+ mutex_unlock(&ts->system_lock);
+
+ return rc;
+}
+
+static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
+ struct cyttsp5_hid_desc *desc)
+{
+ struct device *dev = ts->dev;
+ __le16 hid_desc_register = HID_DESC_REG;
+ int rc;
+ u8 cmd[2];
+
+ /* Read HID descriptor length and version */
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_BUSY;
+ mutex_unlock(&ts->system_lock);
+
+ /* Set HID descriptor register */
+ memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register));
+
+ rc = cyttsp5_write(ts, HID_DESC_REG, NULL, 0);
+ if (rc < 0) {
+ dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc);
+ goto error;
+ }
+
+ rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
+ msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT));
+ if (!rc) {
+ dev_err(ts->dev, "HID get descriptor timed out\n");
+ rc = -ETIME;
+ goto error;
+ }
+
+ memcpy(desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc));
+
+ /* Check HID descriptor length and version */
+ if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) ||
+ le16_to_cpu(desc->bcd_version) != HID_VERSION) {
+ dev_err(dev, "Unsupported HID version\n");
+ return -ENODEV;
+ }
+
+ goto exit;
+
+error:
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_DONE;
+ mutex_unlock(&ts->system_lock);
+exit:
+ return rc;
+}
+
+static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size,
+ int offset)
+{
+ tch_abs->ofs = offset / 8;
+ tch_abs->size = report_size / 8;
+ if (report_size % 8)
+ tch_abs->size += 1;
+ tch_abs->min = 0;
+ tch_abs->max = 1 << report_size;
+ tch_abs->bofs = offset - (tch_abs->ofs << 3);
+
+ return 0;
+}
+
+static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
+{
+ memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE);
+ memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE],
+ BTN_REPORT_SIZE);
+
+ return 0;
+}
+
+static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
+{
+ int max_tch = si->sensing_conf_data.max_tch;
+ int num_cur_tch;
+ int length;
+ struct cyttsp5_tch_abs_params *tch = &si->tch_hdr;
+
+ memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE);
+
+ cyttsp5_get_touch_axis(&num_cur_tch, tch->size,
+ tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs);
+ if (unlikely(num_cur_tch > max_tch))
+ num_cur_tch = max_tch;
+
+ length = num_cur_tch * TOUCH_REPORT_SIZE;
+
+ memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length);
+
+ return 0;
+}
+
+static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
+{
+ struct cyttsp5 *ts = handle;
+ int report_id;
+ int size;
+ int rc;
+
+ rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT);
+ if (rc)
+ return IRQ_HANDLED;
+
+ size = get_unaligned_le16(&ts->input_buf[0]);
+
+ /* check reset */
+ if (size == 0) {
+ memcpy(ts->response_buf, ts->input_buf, 2);
+
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_DONE;
+ mutex_unlock(&ts->system_lock);
+ wake_up(&ts->wait_q);
+ return IRQ_HANDLED;
+ }
+
+ report_id = ts->input_buf[2];
+
+ if (report_id == HID_TOUCH_REPORT_ID) {
+ move_touch_data(ts, &ts->sysinfo);
+ cyttsp5_mt_attention(ts->dev);
+ } else if (report_id == HID_BTN_REPORT_ID) {
+ move_button_data(ts, &ts->sysinfo);
+ cyttsp5_btn_attention(ts->dev);
+ } else {
+ /* It is not an input but a command response */
+ memcpy(ts->response_buf, ts->input_buf, size);
+
+ mutex_lock(&ts->system_lock);
+ ts->hid_cmd_state = HID_CMD_DONE;
+ mutex_unlock(&ts->system_lock);
+ wake_up(&ts->wait_q);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int cyttsp5_deassert_int(struct cyttsp5 *ts)
+{
+ u16 size;
+ u8 buf[2];
+ int rc;
+
+ rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
+ if (rc < 0)
+ return rc;
+
+ size = get_unaligned_le16(&buf[0]);
+ if (size == 2 || size == 0)
+ return 0;
+
+ return -EINVAL;
+}
+
+static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
+{
+ struct cyttsp5_sysinfo *si = &ts->sysinfo;
+
+ fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16,
+ TOUCH_REPORT_DESC_X);
+ fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16,
+ TOUCH_REPORT_DESC_Y);
+ fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8,
+ TOUCH_REPORT_DESC_P);
+ fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5,
+ TOUCH_REPORT_DESC_CONTACTID);
+ fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5,
+ TOUCH_REPORT_DESC_HDR_CONTACTCOUNT);
+ fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8,
+ TOUCH_REPORT_DESC_MAJ);
+ fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8,
+ TOUCH_REPORT_DESC_MIN);
+
+ return 0;
+}
+
+static int cyttsp5_startup(struct cyttsp5 *ts)
+{
+ int rc;
+
+ rc = cyttsp5_deassert_int(ts);
+ if (rc) {
+ dev_err(ts->dev, "Error on deassert int r=%d\n", rc);
+ return -ENODEV;
+ }
+
+ /*
+ * Launch the application as the device starts in bootloader mode
+ * because of a power-on-reset
+ */
+ rc = cyttsp5_hid_output_bl_launch_app(ts);
+ if (rc < 0) {
+ dev_err(ts->dev, "Error on launch app r=%d\n", rc);
+ return rc;
+ }
+
+ rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
+ if (rc < 0) {
+ dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", rc);
+ return rc;
+ }
+
+ rc = cyttsp5_fill_all_touch(ts);
+ if (rc < 0) {
+ dev_err(ts->dev, "Error on report descriptor r=%d\n", rc);
+ return rc;
+ }
+
+ rc = cyttsp5_hid_output_get_sysinfo(ts);
+ if (rc) {
+ dev_err(ts->dev, "Error on getting sysinfo r=%d\n", rc);
+ return rc;
+ }
+
+ return rc;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cyttsp5_of_match[] = {
+ { .compatible = "cypress,tt21000", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, cyttsp5_of_match);
+#endif
+
+static const struct i2c_device_id cyttsp5_i2c_id[] = {
+ { CYTTSP5_NAME, 0, },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);
+
+static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name)
+{
+ struct cyttsp5 *ts;
+ struct cyttsp5_sysinfo *si;
+ int rc = 0, i;
+
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ /* Initialize device info */
+ ts->regmap = regmap;
+ ts->dev = dev;
+ si = &ts->sysinfo;
+ dev_set_drvdata(dev, ts);
+
+ /* Initialize mutexes and spinlocks */
+ mutex_init(&ts->system_lock);
+
+ /* Initialize wait queue */
+ init_waitqueue_head(&ts->wait_q);
+
+ /* Power up the device */
+ ts->vdd = regulator_get(dev, "vdd");
+ if (IS_ERR(ts->vdd)) {
+ rc = PTR_ERR(ts->vdd);
+ dev_set_drvdata(dev, NULL);
+ kfree(ts);
+ return rc;
+ }
+
+ rc = regulator_enable(ts->vdd);
+ if (rc) {
+ regulator_put(ts->vdd);
+ dev_set_drvdata(dev, NULL);
+ kfree(ts);
+ return rc;
+ }
+
+ /* Reset the gpio to be in a reset state */
+ ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(ts->reset_gpio)) {
+ rc = PTR_ERR(ts->reset_gpio);
+ dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
+ return rc;
+ }
+ gpiod_set_value(ts->reset_gpio, 1);
+
+ /* Need a delay to have device up */
+ msleep(20);
+
+ rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ name, ts);
+ if (rc) {
+ dev_err(dev, "unable to request IRQ\n");
+ return rc;
+ }
+
+ rc = cyttsp5_startup(ts);
+ if (rc) {
+ dev_err(ts->dev, "Fail initial startup r=%d\n", rc);
+ return rc;
+ }
+
+ rc = cyttsp5_parse_dt_key_code(dev);
+ if (rc < 0) {
+ dev_err(ts->dev, "Error while parsing dts %d\n", rc);
+ return rc;
+ }
+
+ ts->input = devm_input_allocate_device(dev);
+ if (!ts->input) {
+ dev_err(dev, "Error, failed to allocate input device\n");
+ return -ENODEV;
+ }
+
+ ts->input->name = "cyttsp5";
+ scnprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev));
+ ts->input->phys = ts->phys;
+ ts->input->dev.parent = ts->dev;
+ input_set_drvdata(ts->input, ts);
+
+ touchscreen_parse_properties(ts->input, true, &ts->prop);
+
+ __set_bit(EV_KEY, ts->input->evbit);
+ __set_bit(ABS_X, ts->input->absbit);
+ __set_bit(ABS_Y, ts->input->absbit);
+ __set_bit(BTN_TOUCH, ts->input->keybit);
+ for (i = 0; i < si->num_btns; i++)
+ __set_bit(si->key_code[i], ts->input->keybit);
+
+ return cyttsp5_setup_input_device(dev);
+}
+
+static int cyttsp5_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct regmap *regmap;
+ static const struct regmap_config config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ };
+
+ regmap = devm_regmap_init_i2c(client, &config);
+ if (IS_ERR(regmap)) {
+ dev_err(&client->dev, "regmap allocation failed: %ld\n",
+ PTR_ERR(regmap));
+ return PTR_ERR(regmap);
+ }
+
+ return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
+}
+
+static int cyttsp5_remove(struct device *dev)
+{
+ struct cyttsp5 *ts = dev_get_drvdata(dev);
+
+ input_unregister_device(ts->input);
+
+ return 0;
+}
+
+static int cyttsp5_i2c_remove(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+
+ return cyttsp5_remove(dev);
+}
+
+static struct i2c_driver cyttsp5_i2c_driver = {
+ .driver = {
+ .name = CYTTSP5_NAME,
+ .of_match_table = of_match_ptr(cyttsp5_of_match),
+ },
+ .probe = cyttsp5_i2c_probe,
+ .remove = cyttsp5_i2c_remove,
+ .id_table = cyttsp5_i2c_id,
+};
+
+module_i2c_driver(cyttsp5_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product");
+MODULE_AUTHOR("Mylène Josserand <mylene.josserand@bootlin.com>");
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
@ 2021-11-03 11:48 ` Alistair Francis
2021-11-03 23:07 ` Rob Herring
2021-11-05 14:21 ` Andreas Kemnade
2021-11-03 11:48 ` [PATCH v2 3/4] ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen Alistair Francis
` (2 subsequent siblings)
4 siblings, 2 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-03 11:48 UTC (permalink / raw)
To: linux-arm-kernel, devicetree, linux-input, linux-kernel
Cc: mylene.josserand, linus.walleij, andreas, rydberg,
dmitry.torokhov, robh+dt, alistair23, Alistair Francis
From: Mylène Josserand <mylene.josserand@free-electrons.com>
Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings
documentation. It can use I2C or SPI bus.
This touchscreen can handle some defined zone that are designed and
sent as button. To be able to customize the keycode sent, the
"linux,code" property in a "button" sub-node can be used.
Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
Message-Id: <20170529144538.29187-3-mylene.josserand@free-electrons.com>
Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
.../input/touchscreen/cypress,tt21000.yaml | 92 +++++++++++++++++++
1 file changed, 92 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
new file mode 100644
index 000000000000..ff7eca412440
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/cypress,cyttsp5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cypress TT2100 touchscreen controller
+
+description: The Cypress TT2100 series (also known as "CYTTSP5" after
+ the marketing name Cypress TrueTouch Standard Product series 5).
+
+maintainers:
+ - Alistair Francis <alistair@alistair23.me>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ const: cypress,tt21000
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vdd-supply:
+ description: Regulator for voltage.
+
+ reset-gpios:
+ maxItems: 1
+
+ linux,code:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: EV_ABS specific event code generated by the axis.
+
+patternProperties:
+ "^button-[0-9]+$":
+ type: object
+ properties:
+ linux,code:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Keycode to emit
+
+ required:
+ - linux,code
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - vdd-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/input/linux-event-codes.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@24 {
+ compatible = "cypress,tt2100";
+ reg = <0x24>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&tp_reset_ds203>;
+ interrupt-parent = <&pio>;
+ interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
+ vdd-supply = <®_touch>;
+
+ button@0 {
+ linux,code = <KEY_HOMEPAGE>;
+ };
+
+ button@1 {
+ linux,code = <KEY_MENU>;
+ };
+
+ button@2 {
+ linux,code = <KEY_BACK>;
+ };
+ };
+ };
+...
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/4] ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
2021-11-03 11:48 ` [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5 Alistair Francis
@ 2021-11-03 11:48 ` Alistair Francis
2021-11-03 11:48 ` [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5 Alistair Francis
2021-11-05 14:52 ` [PATCH v2 0/4] Add support for the Cypress cyttsp5 Andreas Kemnade
4 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-03 11:48 UTC (permalink / raw)
To: linux-arm-kernel, devicetree, linux-input, linux-kernel
Cc: mylene.josserand, linus.walleij, andreas, rydberg,
dmitry.torokhov, robh+dt, alistair23, Alistair Francis
Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
arch/arm/configs/imx_v6_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index b2db0d6509bf..969c970aace3 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -172,6 +172,7 @@ CONFIG_TOUCHSCREEN_ADS7846=y
CONFIG_TOUCHSCREEN_AD7879=y
CONFIG_TOUCHSCREEN_AD7879_I2C=y
CONFIG_TOUCHSCREEN_ATMEL_MXT=y
+CONFIG_TOUCHSCREEN_CYTTSP5=y
CONFIG_TOUCHSCREEN_DA9052=y
CONFIG_TOUCHSCREEN_EGALAX=y
CONFIG_TOUCHSCREEN_GOODIX=y
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
` (2 preceding siblings ...)
2021-11-03 11:48 ` [PATCH v2 3/4] ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen Alistair Francis
@ 2021-11-03 11:48 ` Alistair Francis
2021-11-06 2:25 ` Dmitry Torokhov
2021-11-05 14:52 ` [PATCH v2 0/4] Add support for the Cypress cyttsp5 Andreas Kemnade
4 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2021-11-03 11:48 UTC (permalink / raw)
To: linux-arm-kernel, devicetree, linux-input, linux-kernel
Cc: mylene.josserand, linus.walleij, andreas, rydberg,
dmitry.torokhov, robh+dt, alistair23, Alistair Francis
Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
| 89 +++++++++++++++++++++++++
1 file changed, 89 insertions(+)
--git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 89cbf13097a4..cb5c63963a3c 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -8,6 +8,7 @@
/dts-v1/;
#include "imx7d.dtsi"
+#include <dt-bindings/input/linux-event-codes.h>
/ {
model = "reMarkable 2.0";
@@ -34,6 +35,18 @@ reg_brcm: regulator-brcm {
startup-delay-us = <150>;
};
+ reg_touch: regulator-touch {
+ compatible = "regulator-fixed";
+ regulator-name = "VDD_3V3_TOUCH";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&pinctrl_touch_reg>;
+ pinctrl-1 = <&pinctrl_touch_reg>;
+ gpio = <&gpio1 11 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
wifi_pwrseq: wifi_pwrseq {
compatible = "mmc-pwrseq-simple";
pinctrl-names = "default";
@@ -51,6 +64,59 @@ &clks {
assigned-clock-rates = <0>, <32768>;
};
+&i2c3 {
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c3>;
+ status = "okay";
+
+ tsc@24 {
+ compatible = "cypress,tt21000";
+ reg = <0x24>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_touch>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&gpio1 13 0>;
+ vdd-supply = <®_touch>;
+ touchscreen-size-x = <880>;
+ touchscreen-size-y = <1280>;
+
+ button@0 {
+ linux,code = <KEY_HOMEPAGE>;
+ };
+
+ button@1 {
+ linux,code = <KEY_MENU>;
+ };
+
+ button@2 {
+ linux,code = <KEY_BACK>;
+ };
+
+ button@3 {
+ linux,code = <KEY_SEARCH>;
+ };
+
+ button@4 {
+ linux,code = <KEY_VOLUMEDOWN>;
+ };
+
+ button@5 {
+ linux,code = <KEY_VOLUMEUP>;
+ };
+
+ button@6 {
+ linux,code = <KEY_CAMERA>;
+ };
+
+ button@7 {
+ linux,code = <KEY_POWER>;
+ };
+ };
+};
+
&snvs_pwrkey {
status = "okay";
};
@@ -125,6 +191,29 @@ MX7D_PAD_SAI1_TX_BCLK__GPIO6_IO13 0x14
>;
};
+ pinctrl_touch: touchgrp {
+ fsl,pins = <
+ /* CYTTSP interrupt */
+ MX7D_PAD_GPIO1_IO14__GPIO1_IO14 0x54
+ /* CYTTSP reset */
+ MX7D_PAD_GPIO1_IO13__GPIO1_IO13 0x04
+ >;
+ };
+
+ pinctrl_i2c3: i2c3grp {
+ fsl,pins = <
+ MX7D_PAD_I2C3_SDA__I2C3_SDA 0x4000007f
+ MX7D_PAD_I2C3_SCL__I2C3_SCL 0x4000007f
+ >;
+ };
+
+ pinctrl_touch_reg: touchreggrp {
+ fsl,pins = <
+ /* TOUCH_PWR_EN */
+ MX7D_PAD_GPIO1_IO11__GPIO1_IO11 0x14
+ >;
+ };
+
pinctrl_uart1: uart1grp {
fsl,pins = <
MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX 0x79
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-03 11:48 ` [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5 Alistair Francis
@ 2021-11-03 23:07 ` Rob Herring
2021-11-05 14:21 ` Andreas Kemnade
1 sibling, 0 replies; 22+ messages in thread
From: Rob Herring @ 2021-11-03 23:07 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, linux-kernel, linus.walleij, rydberg,
dmitry.torokhov, linux-input, alistair23, robh+dt, devicetree,
mylene.josserand, andreas
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]
On Wed, 03 Nov 2021 21:48:28 +1000, Alistair Francis wrote:
> From: Mylène Josserand <mylene.josserand@free-electrons.com>
>
> Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings
> documentation. It can use I2C or SPI bus.
> This touchscreen can handle some defined zone that are designed and
> sent as button. To be able to customize the keycode sent, the
> "linux,code" property in a "button" sub-node can be used.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> Message-Id: <20170529144538.29187-3-mylene.josserand@free-electrons.com>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
> .../input/touchscreen/cypress,tt21000.yaml | 92 +++++++++++++++++++
> 1 file changed, 92 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml: $id: relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/input/touchscreen/cypress,tt21000.yaml#
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:37.26-39.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:41.26-43.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@1: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:45.26-47.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@2: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dt.yaml:0:0: /example-0/i2c/touchscreen@24: failed to match any schema with compatible: ['cypress,tt2100']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1550218
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-03 11:48 ` [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5 Alistair Francis
2021-11-03 23:07 ` Rob Herring
@ 2021-11-05 14:21 ` Andreas Kemnade
2021-11-10 12:59 ` Alistair Francis
1 sibling, 1 reply; 22+ messages in thread
From: Andreas Kemnade @ 2021-11-05 14:21 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, devicetree, linux-input, linux-kernel,
mylene.josserand, linus.walleij, rydberg, dmitry.torokhov,
robh+dt, alistair23
Hi,
I finally found time to test this.
On Wed, 3 Nov 2021 21:48:28 +1000
Alistair Francis <alistair@alistair23.me> wrote:
[...]
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + touchscreen@24 {
> + compatible = "cypress,tt2100";
> + reg = <0x24>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&tp_reset_ds203>;
> + interrupt-parent = <&pio>;
> + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
hmm, in the code is IRQ_TRIGGER_FALLING but here is LEVEL_LOW, hmm what
it is really?
> + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
hmm, if the reset gpio at the chip is active low (I guess it is) that
would indicate an inverter between SoC and gpio. So a nonstandard setup
as an example, probably not a good idea.
Regards,
Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/4] Add support for the Cypress cyttsp5
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
` (3 preceding siblings ...)
2021-11-03 11:48 ` [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5 Alistair Francis
@ 2021-11-05 14:52 ` Andreas Kemnade
2021-11-25 9:44 ` Alistair Francis
4 siblings, 1 reply; 22+ messages in thread
From: Andreas Kemnade @ 2021-11-05 14:52 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, devicetree, linux-input, linux-kernel,
mylene.josserand, linus.walleij, rydberg, dmitry.torokhov,
robh+dt, alistair23
Hallo Alistair,
my Xorg (debian buster installation) does not like that:
[ 3112.462] (**) cyttsp5: Applying InputClass "evdev touchscreen catchall"
[ 3112.462] (**) cyttsp5: Applying InputClass "libinput touchscreen catchall"
[ 3112.462] (II) Using input driver 'libinput' for 'cyttsp5'
[ 3112.463] (**) cyttsp5: always reports core events
[ 3112.463] (**) Option "Device" "/dev/input/event1"
[ 3112.463] (**) Option "_source" "server/udev"
[ 3112.496] (II) event1 - cyttsp5: is tagged by udev as: Touchscreen
[ 3112.498] (EE) event1 - cyttsp5: kernel bug: device has min == max on ABS_X
[ 3112.498] (II) event1 - cyttsp5: was rejected
[ 3112.498] (II) event1 - not using input device '/dev/input/event1'.
[ 3112.499] (EE) libinput: cyttsp5: Failed to create a device for /dev/input/event1
# evtest /dev/input/event1
Input driver version is 1.0.1
Input device ID: bus 0x0 vendor 0x0 product 0x0 version 0x0
Input device name: "cyttsp5"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 2 (EV_REL)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 0
Min 0
Max 0 <-- upsets xorg
Event code 1 (ABS_Y)
Value 0
Min 0
Max 0
Event code 47 (ABS_MT_SLOT)
Value 0
Min 0
Max 31
Event code 48 (ABS_MT_TOUCH_MAJOR)
Value 0
Min 0
Max 255
Event code 49 (ABS_MT_TOUCH_MINOR)
Value 0
Min 0
Max 255
Event code 53 (ABS_MT_POSITION_X)
Value 0
Min 0
Max 1447
Event code 54 (ABS_MT_POSITION_Y)
Value 0
Min 0
Max 1071
Event code 57 (ABS_MT_TRACKING_ID)
Value 0
Min 0
Max 65535
Event code 58 (ABS_MT_PRESSURE)
Value 0
Min 0
Max 255
I specified touchscreen-size-x and touchscreen-size-y in my devicetree.
No idea why it is like this yet.
Regards,
Andreas
On Wed, 3 Nov 2021 21:48:26 +1000
Alistair Francis <alistair@alistair23.me> wrote:
> This patch series builds on top of [1] and adds support for the cyttsp5
> touchscreen controller for the reMarkable 2.
>
> I first tried to add an I2C HID device. Although the cyttsp5 has some HID
> looking aspects it is not HID compatible. Just in trying to probe the device
> I found:
> - The HID descriptor has extra padding
> - The HID descriptor sets the high bytes of the descriptor length
> - The HID descriptor has extra unrecognised tags
> - The HID reset command doesn't appear to work
>
> I don't think there is a way to use the I2C HID framework with the cyttsp5.
> For anyone interested you can see the work here [2]. In that branch though I
> can only obtain a HID descriptor, nothing else works without more core
> changes.
>
> So instead I rebased the series from [1]. Converted to the new yaml DTS
> documentation, added regulator support and fixed a x/y miscalculation bug.
>
> 1: https://lwn.net/ml/linux-kernel/20180703094309.18514-1-mylene.josserand@bootlin.com/
> 2: https://github.com/alistair23/linux/commits/rM2-mainline-cyttsp5-hid
>
> Alistair Francis (2):
> ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen
> ARM: dts: imx7d: remarkable2: Enable the cyttsp5
>
> Mylène Josserand (2):
> Input: Add driver for Cypress Generation 5 touchscreen
> Documentation: DT: bindings: input: Add documentation for cyttsp5
>
> .../input/touchscreen/cypress,tt21000.yaml | 92 ++
> arch/arm/boot/dts/imx7d-remarkable2.dts | 89 ++
> arch/arm/configs/imx_v6_v7_defconfig | 1 +
> drivers/input/touchscreen/Kconfig | 14 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/cyttsp5.c | 1135 +++++++++++++++++
> 6 files changed, 1332 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
> create mode 100644 drivers/input/touchscreen/cyttsp5.c
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
@ 2021-11-05 15:01 ` Andreas Kemnade
2021-11-06 6:47 ` Dmitry Torokhov
2021-12-23 13:47 ` Andy Shevchenko
2 siblings, 0 replies; 22+ messages in thread
From: Andreas Kemnade @ 2021-11-05 15:01 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, devicetree, linux-input, linux-kernel,
mylene.josserand, linus.walleij, rydberg, dmitry.torokhov,
robh+dt, alistair23, Mylène Josserand, Maxime Ripard
On Wed, 3 Nov 2021 21:48:27 +1000
Alistair Francis <alistair@alistair23.me> wrote:
[...]
> +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name)
> +{
> + struct cyttsp5 *ts;
> + struct cyttsp5_sysinfo *si;
> + int rc = 0, i;
> +
> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + /* Initialize device info */
> + ts->regmap = regmap;
> + ts->dev = dev;
> + si = &ts->sysinfo;
> + dev_set_drvdata(dev, ts);
> +
> + /* Initialize mutexes and spinlocks */
> + mutex_init(&ts->system_lock);
> +
> + /* Initialize wait queue */
> + init_waitqueue_head(&ts->wait_q);
> +
> + /* Power up the device */
> + ts->vdd = regulator_get(dev, "vdd");
> + if (IS_ERR(ts->vdd)) {
> + rc = PTR_ERR(ts->vdd);
> + dev_set_drvdata(dev, NULL);
> + kfree(ts);
> + return rc;
> + }
> +
> + rc = regulator_enable(ts->vdd);
> + if (rc) {
> + regulator_put(ts->vdd);
> + dev_set_drvdata(dev, NULL);
> + kfree(ts);
> + return rc;
> + }
> +
> + /* Reset the gpio to be in a reset state */
> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
so we deassert reset for
> + if (IS_ERR(ts->reset_gpio)) {
> + rc = PTR_ERR(ts->reset_gpio);
> + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> + return rc;
> + }
almost no time
> + gpiod_set_value(ts->reset_gpio, 1);
and then assert it, keeping the chip in reset
> +
> + /* Need a delay to have device up */
> + msleep(20);
> +
and why it should wake up?
I reversed the logic here to test. I have
reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
in my tests.
> + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + name, ts);
falling or level low (according to the example in the
dt schema)?
Regards,
Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5
2021-11-03 11:48 ` [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5 Alistair Francis
@ 2021-11-06 2:25 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2021-11-06 2:25 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, devicetree, linux-input, linux-kernel,
mylene.josserand, linus.walleij, andreas, rydberg, robh+dt,
alistair23
On Wed, Nov 03, 2021 at 09:48:30PM +1000, Alistair Francis wrote:
> + tsc@24 {
> + compatible = "cypress,tt21000";
> + reg = <0x24>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_touch>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
> + reset-gpios = <&gpio1 13 0>;
Reset lines are almost universally active low. Are you sure it is active
high in your case?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
2021-11-05 15:01 ` Andreas Kemnade
@ 2021-11-06 6:47 ` Dmitry Torokhov
2021-12-01 12:27 ` Alistair Francis
2021-12-23 13:47 ` Andy Shevchenko
2 siblings, 1 reply; 22+ messages in thread
From: Dmitry Torokhov @ 2021-11-06 6:47 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm-kernel, devicetree, linux-input, linux-kernel,
mylene.josserand, linus.walleij, andreas, rydberg, robh+dt,
alistair23, Mylène Josserand, Maxime Ripard
Hi Alistair,
On Wed, Nov 03, 2021 at 09:48:27PM +1000, Alistair Francis wrote:
> From: Mylène Josserand <mylene.josserand@bootlin.com>
>
> This is the basic driver for the Cypress TrueTouch Gen5 touchscreen
> controllers. This driver supports only the I2C bus but it uses regmap
> so SPI support could be added later.
> The touchscreen can retrieve some defined zone that are handled as
> buttons (according to the hardware). That is why it handles
> button and multitouch events.
>
> Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> Message-Id: <20180703094309.18514-2-mylene.josserand@bootlin.com>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
> drivers/input/touchscreen/Kconfig | 14 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/cyttsp5.c | 1135 +++++++++++++++++++++++++++
> 3 files changed, 1150 insertions(+)
> create mode 100644 drivers/input/touchscreen/cyttsp5.c
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index d4e74738c5a8..92c8b15b0025 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -284,6 +284,20 @@ config TOUCHSCREEN_CYTTSP4_SPI
> To compile this driver as a module, choose M here: the
> module will be called cyttsp4_spi.
>
> +config TOUCHSCREEN_CYTTSP5
> + tristate "Cypress TrueTouch Gen5 Touchscreen Driver"
> + depends on OF && I2C
> + select REGMAP_I2C
> + select CRC_ITU_T
> + help
> + Driver for Parade TrueTouch Standard Product
> + Generation 5 touchscreen controllers.
> + I2C bus interface support only.
> + Say Y here if you have a Cypress Gen5 touchscreen.
> + If unsure, say N.
> + To compile this driver as a module, choose M here: the
> + module will be called cyttsp5.
> +
> config TOUCHSCREEN_DA9034
> tristate "Touchscreen support for Dialog Semiconductor DA9034"
> depends on PMIC_DA903X
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 7d34100f7f22..d4d6e56e70de 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o
> obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_CORE) += cyttsp4_core.o
> obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_I2C) += cyttsp4_i2c.o cyttsp_i2c_common.o
> obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_SPI) += cyttsp4_spi.o
> +obj-$(CONFIG_TOUCHSCREEN_CYTTSP5) += cyttsp5.o
> obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o
> obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.o
> obj-$(CONFIG_TOUCHSCREEN_DYNAPRO) += dynapro.o
> diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
> new file mode 100644
> index 000000000000..77938cb0c4bb
> --- /dev/null
> +++ b/drivers/input/touchscreen/cyttsp5.c
> @@ -0,0 +1,1135 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Parade TrueTouch(TM) Standard Product V5 Module.
> + *
> + * Copyright (C) 2015 Parade Technologies
> + * Copyright (C) 2012-2015 Cypress Semiconductor
> + * Copyright (C) 2018 Bootlin
> + *
> + * Authors: Mylène Josserand <mylene.josserand@bootlin.com>
> + * Alistair Francis <alistair@alistair23.me>
> + *
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/crc-itu-t.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#define CYTTSP5_NAME "cyttsp5"
> +#define CY_I2C_DATA_SIZE (2 * 256)
> +#define HID_VERSION 0x0100
> +#define CY_MAX_INPUT 512
> +#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32
> +#define CY_BITS_PER_BTN 1
> +#define CY_NUM_BTN_EVENT_ID ((1 << CY_BITS_PER_BTN) - 1)
> +
> +#define MAX_AREA 255
> +#define HID_OUTPUT_BL_SOP 0x1
> +#define HID_OUTPUT_BL_EOP 0x17
> +#define HID_OUTPUT_BL_LAUNCH_APP 0x3B
> +#define HID_OUTPUT_BL_LAUNCH_APP_SIZE 11
> +#define HID_OUTPUT_GET_SYSINFO 0x2
> +#define HID_OUTPUT_GET_SYSINFO_SIZE 5
> +#define HID_OUTPUT_MAX_CMD_SIZE 12
> +
> +#define HID_DESC_REG 0x1
> +#define HID_INPUT_REG 0x3
> +#define HID_OUTPUT_REG 0x4
> +
> +#define REPORT_ID_TOUCH 0x1
> +#define REPORT_ID_BTN 0x3
> +#define REPORT_SIZE_5 5
> +#define REPORT_SIZE_8 8
> +#define REPORT_SIZE_16 16
> +
> +/* Touch reports offsets */
> +/* Header offsets */
> +#define TOUCH_REPORT_DESC_HDR_CONTACTCOUNT 16
> +/* Record offsets */
> +#define TOUCH_REPORT_DESC_CONTACTID 8
> +#define TOUCH_REPORT_DESC_X 16
> +#define TOUCH_REPORT_DESC_Y 32
> +#define TOUCH_REPORT_DESC_P 48
> +#define TOUCH_REPORT_DESC_MAJ 56
> +#define TOUCH_REPORT_DESC_MIN 64
> +
> +/* HID */
> +#define HID_TOUCH_REPORT_ID 0x1
> +#define HID_BTN_REPORT_ID 0x3
> +#define HID_APP_RESPONSE_REPORT_ID 0x1F
> +#define HID_APP_OUTPUT_REPORT_ID 0x2F
> +#define HID_BL_RESPONSE_REPORT_ID 0x30
> +#define HID_BL_OUTPUT_REPORT_ID 0x40
> +
> +#define HID_OUTPUT_RESPONSE_REPORT_OFFSET 2
> +#define HID_OUTPUT_RESPONSE_CMD_OFFSET 4
> +#define HID_OUTPUT_RESPONSE_CMD_MASK 0x7F
> +
> +#define HID_SYSINFO_SENSING_OFFSET 33
> +#define HID_SYSINFO_BTN_OFFSET 48
> +#define HID_SYSINFO_BTN_MASK 0xFF
> +#define HID_SYSINFO_MAX_BTN 8
> +
> +/* Timeout in ms */
> +#define CY_HID_OUTPUT_TIMEOUT 200
> +#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000
> +#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000
> +
> +/* maximum number of concurrent tracks */
> +#define TOUCH_REPORT_SIZE 10
> +#define TOUCH_INPUT_HEADER_SIZE 7
> +#define BTN_REPORT_SIZE 9
> +#define BTN_INPUT_HEADER_SIZE 5
> +
> +#define MAX_CY_TCH_T_IDS 32
> +
> +/* All usage pages for Touch Report */
> +#define TOUCH_REPORT_USAGE_PG_X 0x00010030
> +#define TOUCH_REPORT_USAGE_PG_Y 0x00010031
> +#define TOUCH_REPORT_USAGE_PG_P 0x000D0030
> +#define TOUCH_REPORT_USAGE_PG_CONTACTID 0x000D0051
> +#define TOUCH_REPORT_USAGE_PG_CONTACTCOUNT 0x000D0054
> +#define TOUCH_REPORT_USAGE_PG_MAJ 0xFF010062
> +#define TOUCH_REPORT_USAGE_PG_MIN 0xFF010063
> +#define TOUCH_COL_USAGE_PG 0x000D0022
> +
> +/* helpers */
> +#define HI_BYTE(x) ((u8)(((x) >> 8) & 0xFF))
> +#define LOW_BYTE(x) ((u8)((x) & 0xFF))
> +
> +/* System Information interface definitions */
> +struct cyttsp5_sensing_conf_data_dev {
> + u8 electrodes_x;
> + u8 electrodes_y;
> + __le16 len_x;
> + __le16 len_y;
> + __le16 res_x;
> + __le16 res_y;
> + __le16 max_z;
> + u8 origin_x;
> + u8 origin_y;
> + u8 btn;
> + u8 scan_mode;
> + u8 max_num_of_tch_per_refresh_cycle;
> +} __packed;
> +
> +struct cyttsp5_sensing_conf_data {
> + u16 res_x;
> + u16 res_y;
> + u16 max_z;
> + u16 len_x;
> + u16 len_y;
> + u8 origin_x;
> + u8 origin_y;
> + u8 max_tch;
> +};
> +
> +enum hid_cmd_state {
> + HID_CMD_DONE,
> + HID_CMD_BUSY,
> +};
> +
> +enum cyttsp5_tch_abs { /* for ordering within the extracted touch data array */
> + CY_TCH_X, /* X */
> + CY_TCH_Y, /* Y */
> + CY_TCH_P, /* P (Z) */
> + CY_TCH_T, /* TOUCH ID */
> + CY_TCH_MAJ, /* TOUCH_MAJOR */
> + CY_TCH_MIN, /* TOUCH_MINOR */
> + CY_TCH_NUM_ABS,
> +};
> +
> +struct cyttsp5_tch_abs_params {
> + size_t ofs; /* abs byte offset */
> + size_t size; /* size in bits */
> + size_t min; /* min value */
> + size_t max; /* max value */
> + size_t bofs; /* bit offset */
> +};
> +
> +struct cyttsp5_touch {
> + int hdr;
> + int abs[CY_TCH_NUM_ABS];
> +};
> +
> +struct cyttsp5_sysinfo {
> + struct cyttsp5_sensing_conf_data sensing_conf_data;
> + int num_btns;
> + struct cyttsp5_tch_abs_params tch_hdr;
> + struct cyttsp5_tch_abs_params tch_abs[CY_TCH_NUM_ABS];
> + u32 key_code[HID_SYSINFO_MAX_BTN];
> + u8 *xy_mode;
> + u8 *xy_data;
> +};
> +
> +struct cyttsp5_hid_desc {
> + __le16 hid_desc_len;
> + u8 packet_id;
> + u8 reserved_byte;
> + __le16 bcd_version;
> + __le16 report_desc_len;
> + __le16 report_desc_register;
> + __le16 input_register;
> + __le16 max_input_len;
> + __le16 output_register;
> + __le16 max_output_len;
> + __le16 command_register;
> + __le16 data_register;
> + __le16 vendor_id;
> + __le16 product_id;
> + __le16 version_id;
> + u8 reserved[4];
> +} __packed;
> +
> +struct cyttsp5 {
> + struct device *dev;
> + struct mutex system_lock;
> + wait_queue_head_t wait_q;
Looking through the driver I think what you need is completion.
> + struct cyttsp5_sysinfo sysinfo;
> + int hid_cmd_state;
> + struct cyttsp5_hid_desc hid_desc;
> + u8 cmd_buf[CYTTSP5_PREALLOCATED_CMD_BUFFER];
> + u8 input_buf[CY_MAX_INPUT];
> + u8 response_buf[CY_MAX_INPUT];
> + struct gpio_desc *reset_gpio;
> + struct input_dev *input;
> + char phys[NAME_MAX];
> + int num_prv_rec;
> + struct regmap *regmap;
> + struct touchscreen_properties prop;
> + struct regulator *vdd;
> +};
> +
> +/*
> + * For what understood in the datasheet, the register does not
> + * matter. For consistency, used the Input Register address
> + * but it does mean anything to the device. The important data
> + * to send is the I2C address
> + */
> +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
> +{
> + int rc;
Please call this "error". Also elsewhere, variables that hold either
error code or success (0) should be called "error".
> + u32 size;
> + u8 temp[2];
> +
> + if (!buf)
> + return -EINVAL;
This is an internal function. How can it be called with NULL buffer?
> +
> + /* Read the frame to retrieve the size */
> + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, 2);
sizeof(temp)
> + if (rc < 0)
regmap_bulk_read() retirns 0 on success, so the check should be
if (error)
> + return rc;
> +
> + size = get_unaligned_le16(temp);
> + if (!size || size == 2)
> + return 0;
> +
> + if (size > max)
> + return -EINVAL;
> +
> + /* Get the real value */
> + return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size);
> +}
> +
> +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
> + size_t size)
> +{
> + u8 cmd[HID_OUTPUT_MAX_CMD_SIZE];
> +
> + if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE) {
> + return -ENOMEM;
Maybe -E2BIG or -EINVAL.
> + }
Unnecessary curly braces.
> +
> + /* High bytes of register address needed as first byte of cmd */
> + cmd[0] = HI_BYTE(reg);
> +
> + /* Copy the rest of the data */
> + if (data)
> + memcpy(&cmd[1], data, size);
> +
> + /* The hardware wants to receive a frame with the address register
> + * contains in the first two bytes. As the regmap_write function
> + * add the register adresse in the frame, we use the LOW_BYTE as
> + * first frame byte for the address register and the first
> + * data byte is the high register + left of the cmd to send
> + */
> + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1);
> +}
> +
> +static void cyttsp5_final_sync(struct input_dev *input, int max_slots,
> + unsigned long *ids)
> +{
> + int t;
> +
> + for (t = 0; t < max_slots; t++) {
> + if (test_bit(t, ids))
> + continue;
> + input_mt_slot(input, t);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> + }
> +
> + input_sync(input);
> +}
> +
> +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots)
> +{
> + int t;
> +
> + if (ts->num_prv_rec == 0)
> + return;
> +
> + for (t = 0; t < max_slots; t++) {
> + input_mt_slot(ts->input, t);
> + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false);
> + }
> +}
> +
> +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts)
> +{
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int max = si->tch_abs[CY_TCH_T].max;
> +
> + if (ts->num_prv_rec != 0) {
> + cyttsp5_report_slot_liftoff(ts, max);
> + input_sync(ts->input);
> + ts->num_prv_rec = 0;
> + }
> +}
> +
> +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
> + int bofs)
> +{
> + int nbyte;
> +
> + for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
> + *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
> +
> + *axis &= max - 1;
Can it be that max is not power of 2?
> +}
> +
> +static void cyttsp5_get_touch_record(struct cyttsp5 *ts,
> + struct cyttsp5_touch *touch, u8 *xy_data)
> +{
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + enum cyttsp5_tch_abs abs;
> +
> + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) {
> + cyttsp5_get_touch_axis(&touch->abs[abs],
> + si->tch_abs[abs].size,
> + si->tch_abs[abs].max,
> + xy_data + si->tch_abs[abs].ofs,
> + si->tch_abs[abs].bofs);
> + }
Unnecessary curly braces.
> +}
> +
> +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts,
> + struct cyttsp5_touch *tch, int num_cur_tch)
> +{
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int i, t = 0;
> + DECLARE_BITMAP(ids, MAX_CY_TCH_T_IDS);
> + u8 *tch_addr;
> + int tmp;
> +
> + bitmap_zero(ids, MAX_CY_TCH_T_IDS);
> + memset(tch->abs, 0, sizeof(tch->abs));
> +
> + for (i = 0; i < num_cur_tch; i++) {
> + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE);
> + cyttsp5_get_touch_record(ts, tch, tch_addr);
> +
> + /* Convert MAJOR/MINOR from mm to resolution */
> + tmp = tch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x;
> + tch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x;
> + tmp = tch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x;
> + tch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x;
> +
> + t = tch->abs[CY_TCH_T];
> + input_mt_slot(ts->input, t);
> + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true);
> + __set_bit(t, ids);
> +
> + /* position and pressure fields */
> + input_report_abs(ts->input, ABS_MT_POSITION_X,
> + tch->abs[CY_TCH_X]);
> + input_report_abs(ts->input, ABS_MT_POSITION_Y,
> + tch->abs[CY_TCH_Y]);
> + input_report_abs(ts->input, ABS_MT_PRESSURE,
> + tch->abs[CY_TCH_P]);
> +
> + /* Get the extended touch fields */
> + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR,
> + tch->abs[CY_TCH_MAJ]);
> + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR,
> + tch->abs[CY_TCH_MIN]);
> +
> + touchscreen_report_pos(ts->input, &ts->prop,
> + tch->abs[CY_TCH_X], tch->abs[CY_TCH_Y],
> + true);
I have no idea why you first report position manually (without applying
transformation) and then finally use touchscreen_report_pos() to report
is again, now properly.
> + }
> +
> + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids);
I think if you use INPUT_MT_DROP_UNUSED when initializing slots and call
input_mt_sync_frame() then cyttsp5_final_sync() will not be needed.
> +
> + ts->num_prv_rec = num_cur_tch;
> +}
> +
> +/* read xy_data for all current touches */
> +static int cyttsp5_xy_worker(struct cyttsp5 *ts)
> +{
> + struct device *dev = ts->dev;
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int max_tch = si->sensing_conf_data.max_tch;
> + struct cyttsp5_touch tch;
> + u8 num_cur_tch;
> +
> + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size,
I am struggling to understand why this member is called "hdr" and why it
is a part of cyttsp5_touch.
> + si->tch_hdr.max,
> + si->xy_mode + 3 + si->tch_hdr.ofs,
> + si->tch_hdr.bofs);
> +
> + num_cur_tch = tch.hdr;
> + if (num_cur_tch > max_tch) {
> + dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch);
> + num_cur_tch = max_tch;
> + }
> +
> + if (num_cur_tch == 0 && ts->num_prv_rec == 0)
> + return 0;
> +
> + /* extract xy_data for all currently reported touches */
> + if (num_cur_tch)
> + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch);
> + else
> + cyttsp5_mt_lift_all(ts);
Not needed with INPUT_MT_DROP_UNUSED.
> +
> + return 0;
> +}
> +
> +static int cyttsp5_mt_attention(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int rc;
> +
> + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID)
> + return 0;
It is only ever called with this condition, why do we need to check
this again?
> +
> + /* core handles handshake */
> + rc = cyttsp5_xy_worker(ts);
> + if (rc < 0)
> + dev_err(dev, "xy_worker error r=%d\n", rc);
> +
> + return rc;
> +}
> +
> +static int cyttsp5_setup_input_device(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int max_x, max_y, max_p;
> + int max_x_tmp, max_y_tmp;
> + int rc;
> +
> + __set_bit(EV_ABS, ts->input->evbit);
input_set_abs_params() does this I believe.
> + __set_bit(EV_REL, ts->input->evbit);
I do not believe the driver emits any EV_REL events.
> + __set_bit(EV_KEY, ts->input->evbit);
I think I saw it done elsewhere already.
> +
> + max_x_tmp = si->sensing_conf_data.res_x;
> + max_y_tmp = si->sensing_conf_data.res_y;
> + max_x = max_x_tmp - 1;
> + max_y = max_y_tmp - 1;
> + max_p = si->sensing_conf_data.max_z;
> +
> + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0);
Error handling. You also want to use INPUT_MT_POINTER and potentially
INPUT_MT_DROP_UNUSED.
> +
> + __set_bit(ABS_MT_POSITION_X, ts->input->absbit);
> + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit);
> + __set_bit(ABS_MT_PRESSURE, ts->input->absbit);
These 3 are not needed.
> +
> + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0);
> + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
> + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0);
> +
> + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0);
> +
> + rc = input_register_device(ts->input);
> + if (rc < 0)
> + dev_err(dev, "Error, failed register input device r=%d\n", rc);
> +
> + return rc;
> +}
> +
> +#ifdef CONFIG_OF
> +static int cyttsp5_parse_dt_key_code(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + struct device_node *np;
> + int i;
> +
> + np = dev->of_node;
> + if (!np)
> + return -EINVAL;
> +
> + if (!si->num_btns)
> + return 0;
> +
> + /* Initialize the button to RESERVED */
> + for (i = 0; i < si->num_btns; i++)
> + si->key_code[i] = KEY_RESERVED;
> +
> + return of_property_read_u32_array(np, "linux,keycodes",
> + si->key_code, si->num_btns);
I would use device_property_read_u32_array() instead and did not make it
limited to OF.
> +}
> +#else
> +static int cyttsp5_parse_dt_key_code(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int i;
> +
> + if (!si->num_btns)
> + return 0;
> +
> + /* Initialize the button to RESERVED */
> + for (i = 0; i < si->num_btns; i++)
> + si->key_code[i] = KEY_RESERVED;
> +
> + return 0;
> +}
> +#endif
> +
> +static int cyttsp5_btn_attention(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int cur_btn;
> + int cur_btn_state;
> +
> + if (si->xy_mode[2] != HID_BTN_REPORT_ID || !si->num_btns)
> + return 0;
> +
> + /* extract button press/release touch information */
> + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) {
> + /* Get current button state */
> + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN))
> + & CY_NUM_BTN_EVENT_ID;
> +
> + input_report_key(ts->input, si->key_code[cur_btn],
> + cur_btn_state);
> + input_sync(ts->input);
> + }
> +
> + return 0;
> +}
> +
> +static u16 cyttsp5_compute_crc(u8 *buf, u32 size)
> +{
> + u16 remainder = 0xFFFF;
> + u16 xor_mask = 0x0000;
> + u32 index;
> + u32 byte_value;
> + u32 table_index;
> + u32 crc_bit_width = sizeof(u16) * 8;
> +
> + /* Divide the message by polynomial, via the table. */
> + for (index = 0; index < size; index++) {
> + byte_value = buf[index];
> + table_index = ((byte_value >> 4) & 0x0F)
> + ^ (remainder >> (crc_bit_width - 4));
> + remainder = crc_itu_t_table[table_index]
> + ^ (remainder << 4);
> + table_index = (byte_value & 0x0F)
> + ^ (remainder >> (crc_bit_width - 4));
> + remainder = crc_itu_t_table[table_index]
> + ^ (remainder << 4);
> + }
> +
> + /* Perform the final remainder CRC. */
> + return remainder ^ xor_mask;
Do we have matching implementation in lib/crc* by any chance?
> +}
> +
> +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code)
> +{
> + u16 size, crc;
> + u8 status, offset;
> + int command_code;
> +
> + size = get_unaligned_le16(&ts->response_buf[0]);
> +
> + if (!size)
> + return 0;
> +
> + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET];
This is definitely not an "offset".
> +
> + if (offset == HID_BL_RESPONSE_REPORT_ID) {
switch (report_id) {
> + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) {
> + dev_err(ts->dev, "HID output response, wrong SOP\n");
> + return -EPROTO;
> + }
> +
> + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
> + dev_err(ts->dev, "HID output response, wrong EOP\n");
> + return -EPROTO;
> + }
> +
> + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
> + if (ts->response_buf[size - 3] != LOW_BYTE(crc) ||
> + ts->response_buf[size - 2] != HI_BYTE(crc)) {
> + dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n",
> + crc);
> + return -EPROTO;
> + }
> +
> + status = ts->response_buf[5];
> + if (status) {
> + dev_err(ts->dev, "HID output response, ERROR:%d\n",
> + status);
> + return -EPROTO;
> + }
> + }
> +
> + if (offset == HID_APP_RESPONSE_REPORT_ID) {
> + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET]
> + & HID_OUTPUT_RESPONSE_CMD_MASK;
> + if (command_code != code) {
> + dev_err(ts->dev,
> + "HID output response, wrong command_code:%X\n",
> + command_code);
> + return -EPROTO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts)
> +{
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int i;
> + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET]
> + & HID_SYSINFO_BTN_MASK;
> +
> + si->num_btns = 0;
> + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
> + if (btns & BIT(i))
> + si->num_btns++;
> + }
hweight*()?
> +}
> +
> +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
> +{
> + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data;
> + struct cyttsp5_sensing_conf_data_dev *scd_dev =
> + (struct cyttsp5_sensing_conf_data_dev *)
> + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> +
> + cyttsp5_si_get_btn_data(ts);
> +
> + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle;
> + scd->res_x = get_unaligned_le16(&scd_dev->res_x);
> + scd->res_y = get_unaligned_le16(&scd_dev->res_y);
> + scd->max_z = get_unaligned_le16(&scd_dev->max_z);
> + scd->len_x = get_unaligned_le16(&scd_dev->len_x);
> + scd->len_y = get_unaligned_le16(&scd_dev->len_y);
> +
> + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
> + GFP_KERNEL);
What it the absolute maximum for tracked contacts? I wonder if it makes
sense to allocate this buffer separately.
> + if (!si->xy_data)
> + return -ENOMEM;
> +
> + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE,
> + GFP_KERNEL);
> + if (!si->xy_mode)
> + return -ENOMEM;
Why do we need to allocate 7 bytes separately?
> +
> + return 0;
> +}
> +
> +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
> +{
> + int rc;
> + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
> +
> + ts->hid_cmd_state = HID_CMD_BUSY;
> +
> + /* HI bytes of Output register address */
> + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> + cmd[2] = HID_APP_OUTPUT_REPORT_ID;
> + cmd[3] = 0x0; /* Reserved */
> + cmd[4] = HID_OUTPUT_GET_SYSINFO;
> +
> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> + HID_OUTPUT_GET_SYSINFO_SIZE);
> + if (rc) {
> + dev_err(ts->dev, "Failed to write command %d", rc);
> + goto error;
> + }
> +
> + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
> + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT));
> + if (!rc) {
> + dev_err(ts->dev, "HID output cmd execution timed out\n");
> + rc = -ETIME;
> + goto error;
> + }
> +
> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO);
> + if (rc) {
> + dev_err(ts->dev, "Validation of the response failed\n");
> + goto error;
> + }
> +
> + return cyttsp5_get_sysinfo_regs(ts);
> +
> +error:
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_DONE;
> + mutex_unlock(&ts->system_lock);
> + return rc;
> +}
> +
> +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
> +{
> + int rc;
> + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
> + u16 crc;
> +
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_BUSY;
> + mutex_unlock(&ts->system_lock);
> +
> + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> + cmd[2] = HID_BL_OUTPUT_REPORT_ID;
> + cmd[3] = 0x0; /* Reserved */
> + cmd[4] = HID_OUTPUT_BL_SOP;
> + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
> + cmd[6] = 0x0; /* Low bytes of data */
> + cmd[7] = 0x0; /* Hi bytes of data */
> + crc = cyttsp5_compute_crc(&cmd[4], 4);
> + cmd[8] = LOW_BYTE(crc);
> + cmd[9] = HI_BYTE(crc);
> + cmd[10] = HID_OUTPUT_BL_EOP;
> +
> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> + HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> + if (rc) {
> + dev_err(ts->dev, "Failed to write command %d", rc);
> + goto error;
> + }
> +
> + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
> + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT));
> + if (!rc) {
> + dev_err(ts->dev, "HID output cmd execution timed out\n");
> + rc = -ETIME;
> + goto error;
> + }
> +
> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP);
> + if (rc) {
> + dev_err(ts->dev, "Validation of the response failed\n");
> + goto error;
> + }
> +
> + return rc;
> +
> +error:
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_DONE;
> + mutex_unlock(&ts->system_lock);
> +
> + return rc;
> +}
> +
> +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
> + struct cyttsp5_hid_desc *desc)
> +{
> + struct device *dev = ts->dev;
> + __le16 hid_desc_register = HID_DESC_REG;
> + int rc;
> + u8 cmd[2];
> +
> + /* Read HID descriptor length and version */
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_BUSY;
> + mutex_unlock(&ts->system_lock);
> +
> + /* Set HID descriptor register */
> + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register));
> +
> + rc = cyttsp5_write(ts, HID_DESC_REG, NULL, 0);
> + if (rc < 0) {
> + dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc);
> + goto error;
> + }
> +
> + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
> + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT));
> + if (!rc) {
> + dev_err(ts->dev, "HID get descriptor timed out\n");
> + rc = -ETIME;
> + goto error;
> + }
> +
> + memcpy(desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc));
> +
> + /* Check HID descriptor length and version */
> + if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) ||
> + le16_to_cpu(desc->bcd_version) != HID_VERSION) {
> + dev_err(dev, "Unsupported HID version\n");
> + return -ENODEV;
> + }
> +
> + goto exit;
> +
> +error:
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_DONE;
> + mutex_unlock(&ts->system_lock);
> +exit:
> + return rc;
> +}
> +
> +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size,
> + int offset)
> +{
> + tch_abs->ofs = offset / 8;
> + tch_abs->size = report_size / 8;
> + if (report_size % 8)
> + tch_abs->size += 1;
> + tch_abs->min = 0;
> + tch_abs->max = 1 << report_size;
> + tch_abs->bofs = offset - (tch_abs->ofs << 3);
> +
> + return 0;
> +}
> +
> +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
> +{
> + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE);
> + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE],
> + BTN_REPORT_SIZE);
> +
> + return 0;
> +}
> +
> +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
> +{
> + int max_tch = si->sensing_conf_data.max_tch;
> + int num_cur_tch;
> + int length;
> + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr;
> +
> + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE);
> +
> + cyttsp5_get_touch_axis(&num_cur_tch, tch->size,
> + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs);
> + if (unlikely(num_cur_tch > max_tch))
> + num_cur_tch = max_tch;
> +
> + length = num_cur_tch * TOUCH_REPORT_SIZE;
> +
> + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length);
Why do we need to do all these moves? Can we parse the date directly
from the input buffer?
> +
> + return 0;
> +}
> +
> +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
> +{
> + struct cyttsp5 *ts = handle;
> + int report_id;
> + int size;
> + int rc;
> +
> + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT);
> + if (rc)
> + return IRQ_HANDLED;
> +
> + size = get_unaligned_le16(&ts->input_buf[0]);
> +
> + /* check reset */
> + if (size == 0) {
> + memcpy(ts->response_buf, ts->input_buf, 2);
> +
> + mutex_lock(&ts->system_lock);
Why is this lock needed exactly?
> + ts->hid_cmd_state = HID_CMD_DONE;
> + mutex_unlock(&ts->system_lock);
> + wake_up(&ts->wait_q);
> + return IRQ_HANDLED;
> + }
> +
> + report_id = ts->input_buf[2];
I'd probably do:
if (size == 0) {
/* reset */
report_id = HID_EMPTY_RESPONSE_ID; /* = 0 */
size = 2;
} else {
report_id = ts->input_buf[2];
}
> +
> + if (report_id == HID_TOUCH_REPORT_ID) {
switch (report_id) {
> + move_touch_data(ts, &ts->sysinfo);
> + cyttsp5_mt_attention(ts->dev);
> + } else if (report_id == HID_BTN_REPORT_ID) {
> + move_button_data(ts, &ts->sysinfo);
> + cyttsp5_btn_attention(ts->dev);
> + } else {
> + /* It is not an input but a command response */
> + memcpy(ts->response_buf, ts->input_buf, size);
> +
> + mutex_lock(&ts->system_lock);
> + ts->hid_cmd_state = HID_CMD_DONE;
> + mutex_unlock(&ts->system_lock);
> + wake_up(&ts->wait_q);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int cyttsp5_deassert_int(struct cyttsp5 *ts)
> +{
> + u16 size;
> + u8 buf[2];
> + int rc;
> +
> + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
> + if (rc < 0)
> + return rc;
> +
> + size = get_unaligned_le16(&buf[0]);
> + if (size == 2 || size == 0)
> + return 0;
If you were to use level interrupts this probably would not be needed.
> +
> + return -EINVAL;
> +}
> +
> +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
> +{
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> +
> + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16,
> + TOUCH_REPORT_DESC_X);
> + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16,
> + TOUCH_REPORT_DESC_Y);
> + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8,
> + TOUCH_REPORT_DESC_P);
> + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5,
> + TOUCH_REPORT_DESC_CONTACTID);
> + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5,
> + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT);
> + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8,
> + TOUCH_REPORT_DESC_MAJ);
> + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8,
> + TOUCH_REPORT_DESC_MIN);
> +
> + return 0;
> +}
> +
> +static int cyttsp5_startup(struct cyttsp5 *ts)
> +{
> + int rc;
> +
> + rc = cyttsp5_deassert_int(ts);
> + if (rc) {
> + dev_err(ts->dev, "Error on deassert int r=%d\n", rc);
> + return -ENODEV;
> + }
> +
> + /*
> + * Launch the application as the device starts in bootloader mode
> + * because of a power-on-reset
> + */
> + rc = cyttsp5_hid_output_bl_launch_app(ts);
> + if (rc < 0) {
> + dev_err(ts->dev, "Error on launch app r=%d\n", rc);
> + return rc;
> + }
> +
> + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
> + if (rc < 0) {
> + dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", rc);
> + return rc;
> + }
> +
> + rc = cyttsp5_fill_all_touch(ts);
> + if (rc < 0) {
> + dev_err(ts->dev, "Error on report descriptor r=%d\n", rc);
> + return rc;
> + }
> +
> + rc = cyttsp5_hid_output_get_sysinfo(ts);
> + if (rc) {
> + dev_err(ts->dev, "Error on getting sysinfo r=%d\n", rc);
> + return rc;
> + }
> +
> + return rc;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cyttsp5_of_match[] = {
> + { .compatible = "cypress,tt21000", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, cyttsp5_of_match);
> +#endif
> +
> +static const struct i2c_device_id cyttsp5_i2c_id[] = {
> + { CYTTSP5_NAME, 0, },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);
> +
> +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name)
> +{
> + struct cyttsp5 *ts;
> + struct cyttsp5_sysinfo *si;
> + int rc = 0, i;
> +
> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + /* Initialize device info */
> + ts->regmap = regmap;
> + ts->dev = dev;
> + si = &ts->sysinfo;
> + dev_set_drvdata(dev, ts);
> +
> + /* Initialize mutexes and spinlocks */
> + mutex_init(&ts->system_lock);
> +
> + /* Initialize wait queue */
> + init_waitqueue_head(&ts->wait_q);
> +
> + /* Power up the device */
> + ts->vdd = regulator_get(dev, "vdd");
Do not mix managed an unmanaged resources. You are leaking this
regulator.
> + if (IS_ERR(ts->vdd)) {
> + rc = PTR_ERR(ts->vdd);
> + dev_set_drvdata(dev, NULL);
> + kfree(ts);
> + return rc;
> + }
> +
> + rc = regulator_enable(ts->vdd);
> + if (rc) {
> + regulator_put(ts->vdd);
> + dev_set_drvdata(dev, NULL);
> + kfree(ts);
> + return rc;
> + }
> +
> + /* Reset the gpio to be in a reset state */
> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ts->reset_gpio)) {
> + rc = PTR_ERR(ts->reset_gpio);
> + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> + return rc;
> + }
> + gpiod_set_value(ts->reset_gpio, 1);
> +
> + /* Need a delay to have device up */
> + msleep(20);
> +
> + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
Please do not override platform setup with hardcoded triggers. Also, it
is strongly recommended to use level interrupts for these peripherals.
This is also likely unsafe if controller is not completely shut off and
is capable of generating interrupts given input device is not yet
allocated.
> + name, ts);
> + if (rc) {
> + dev_err(dev, "unable to request IRQ\n");
> + return rc;
> + }
> +
> + rc = cyttsp5_startup(ts);
> + if (rc) {
> + dev_err(ts->dev, "Fail initial startup r=%d\n", rc);
> + return rc;
> + }
> +
> + rc = cyttsp5_parse_dt_key_code(dev);
> + if (rc < 0) {
> + dev_err(ts->dev, "Error while parsing dts %d\n", rc);
> + return rc;
> + }
> +
> + ts->input = devm_input_allocate_device(dev);
> + if (!ts->input) {
> + dev_err(dev, "Error, failed to allocate input device\n");
> + return -ENODEV;
> + }
> +
> + ts->input->name = "cyttsp5";
> + scnprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev));
> + ts->input->phys = ts->phys;
> + ts->input->dev.parent = ts->dev;
No need to do this, devm_input_allocate_device() sets up parent.
> + input_set_drvdata(ts->input, ts);
> +
> + touchscreen_parse_properties(ts->input, true, &ts->prop);
> +
> + __set_bit(EV_KEY, ts->input->evbit);
> + __set_bit(ABS_X, ts->input->absbit);
> + __set_bit(ABS_Y, ts->input->absbit);
> + __set_bit(BTN_TOUCH, ts->input->keybit);
These 3 should not be needed.
> + for (i = 0; i < si->num_btns; i++)
> + __set_bit(si->key_code[i], ts->input->keybit);
> +
> + return cyttsp5_setup_input_device(dev);
> +}
> +
> +static int cyttsp5_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct regmap *regmap;
> + static const struct regmap_config config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + };
> +
> + regmap = devm_regmap_init_i2c(client, &config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "regmap allocation failed: %ld\n",
> + PTR_ERR(regmap));
> + return PTR_ERR(regmap);
> + }
> +
> + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
> +}
> +
> +static int cyttsp5_remove(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> +
> + input_unregister_device(ts->input);
This is not needed as input device is devm managed. This entire function
can be dropped.
> +
> + return 0;
> +}
> +
> +static int cyttsp5_i2c_remove(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> +
> + return cyttsp5_remove(dev);
> +}
> +
> +static struct i2c_driver cyttsp5_i2c_driver = {
> + .driver = {
> + .name = CYTTSP5_NAME,
> + .of_match_table = of_match_ptr(cyttsp5_of_match),
> + },
> + .probe = cyttsp5_i2c_probe,
> + .remove = cyttsp5_i2c_remove,
> + .id_table = cyttsp5_i2c_id,
> +};
> +
> +module_i2c_driver(cyttsp5_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product");
> +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@bootlin.com>");
> --
> 2.31.1
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-05 14:21 ` Andreas Kemnade
@ 2021-11-10 12:59 ` Alistair Francis
2021-11-10 17:36 ` Andreas Kemnade
0 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2021-11-10 12:59 UTC (permalink / raw)
To: Andreas Kemnade
Cc: Alistair Francis, linux-arm-kernel, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Henrik Rydberg, Dmitry Torokhov, Rob Herring
On Sat, Nov 6, 2021 at 12:22 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi,
>
> I finally found time to test this.
>
> On Wed, 3 Nov 2021 21:48:28 +1000
> Alistair Francis <alistair@alistair23.me> wrote:
>
> [...]
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + touchscreen@24 {
> > + compatible = "cypress,tt2100";
> > + reg = <0x24>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&tp_reset_ds203>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
> hmm, in the code is IRQ_TRIGGER_FALLING but here is LEVEL_LOW, hmm what
> it is really?
The reMarkable uses IRQ_TYPE_EDGE_FALLING, but this example isn't
based on that. It' based on the original documentation from the patch
series.
>
> > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
>
> hmm, if the reset gpio at the chip is active low (I guess it is) that
> would indicate an inverter between SoC and gpio. So a nonstandard setup
> as an example, probably not a good idea.
It seems to be common for the cypress,tt2100, as the original
documentation and the rM2 both do this. Does the Kobo not do this?
Alistair
>
> Regards,
> Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-10 12:59 ` Alistair Francis
@ 2021-11-10 17:36 ` Andreas Kemnade
2021-11-11 15:16 ` Linus Walleij
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Kemnade @ 2021-11-10 17:36 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, linux-arm-kernel, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Henrik Rydberg, Dmitry Torokhov, Rob Herring
On Wed, 10 Nov 2021 22:59:50 +1000
Alistair Francis <alistair23@gmail.com> wrote:
[...]
> >
> > > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
> >
> > hmm, if the reset gpio at the chip is active low (I guess it is) that
> > would indicate an inverter between SoC and gpio. So a nonstandard setup
> > as an example, probably not a good idea.
>
> It seems to be common for the cypress,tt2100, as the original
> documentation and the rM2 both do this. Does the Kobo not do this?
>
You have a kind of double inversion here, so things are automagically fixed.
IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
and in the driver
/* Reset the gpio to be in a reset state */
ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(ts->reset_gpio)) {
rc = PTR_ERR(ts->reset_gpio);
dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
return rc;
}
gpiod_set_value(ts->reset_gpio, 0);
That is the way how other active-low reset lines are handled.
Regards,
Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-10 17:36 ` Andreas Kemnade
@ 2021-11-11 15:16 ` Linus Walleij
2021-11-18 12:51 ` Alistair Francis
0 siblings, 1 reply; 22+ messages in thread
From: Linus Walleij @ 2021-11-11 15:16 UTC (permalink / raw)
To: Andreas Kemnade
Cc: Alistair Francis, Alistair Francis, linux-arm-kernel, devicetree,
linux-input, Linux Kernel Mailing List, Mylene Josserand,
Henrik Rydberg, Dmitry Torokhov, Rob Herring
On Wed, Nov 10, 2021 at 6:37 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> Alistair Francis <alistair23@gmail.com> wrote:
> You have a kind of double inversion here, so things are automagically fixed.
> IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
> and in the driver
>
> /* Reset the gpio to be in a reset state */
> ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(ts->reset_gpio)) {
> rc = PTR_ERR(ts->reset_gpio);
> dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> return rc;
> }
> gpiod_set_value(ts->reset_gpio, 0);
>
> That is the way how other active-low reset lines are handled.
Correct.
This is a source of confusion, I contemplated just changing the name
of GPIOD_OUT_HIGH to GPIOD_OUT_ASSERTED etc to indicate
what is going on.
gpiod_set_value(ts->reset_gpio, 0) should similarly be interpreted
as "de-assert this line" no matter the polarity.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5
2021-11-11 15:16 ` Linus Walleij
@ 2021-11-18 12:51 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-18 12:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Andreas Kemnade, Alistair Francis, linux-arm-kernel, devicetree,
linux-input, Linux Kernel Mailing List, Mylene Josserand,
Henrik Rydberg, Dmitry Torokhov, Rob Herring
On Fri, Nov 12, 2021 at 1:16 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Nov 10, 2021 at 6:37 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > Alistair Francis <alistair23@gmail.com> wrote:
>
> > You have a kind of double inversion here, so things are automagically fixed.
> > IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
> > and in the driver
> >
> > /* Reset the gpio to be in a reset state */
> > ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > if (IS_ERR(ts->reset_gpio)) {
> > rc = PTR_ERR(ts->reset_gpio);
> > dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > return rc;
> > }
> > gpiod_set_value(ts->reset_gpio, 0);
> >
> > That is the way how other active-low reset lines are handled.
>
> Correct.
>
> This is a source of confusion, I contemplated just changing the name
> of GPIOD_OUT_HIGH to GPIOD_OUT_ASSERTED etc to indicate
> what is going on.
>
> gpiod_set_value(ts->reset_gpio, 0) should similarly be interpreted
> as "de-assert this line" no matter the polarity.
Thanks! I have fixed this
Alistair
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/4] Add support for the Cypress cyttsp5
2021-11-05 14:52 ` [PATCH v2 0/4] Add support for the Cypress cyttsp5 Andreas Kemnade
@ 2021-11-25 9:44 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2021-11-25 9:44 UTC (permalink / raw)
To: Andreas Kemnade
Cc: Alistair Francis, linux-arm-kernel, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Henrik Rydberg, Dmitry Torokhov, Rob Herring
On Sat, Nov 6, 2021 at 12:52 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hallo Alistair,
>
> my Xorg (debian buster installation) does not like that:
> [ 3112.462] (**) cyttsp5: Applying InputClass "evdev touchscreen catchall"
> [ 3112.462] (**) cyttsp5: Applying InputClass "libinput touchscreen catchall"
> [ 3112.462] (II) Using input driver 'libinput' for 'cyttsp5'
> [ 3112.463] (**) cyttsp5: always reports core events
> [ 3112.463] (**) Option "Device" "/dev/input/event1"
> [ 3112.463] (**) Option "_source" "server/udev"
> [ 3112.496] (II) event1 - cyttsp5: is tagged by udev as: Touchscreen
> [ 3112.498] (EE) event1 - cyttsp5: kernel bug: device has min == max on ABS_X
> [ 3112.498] (II) event1 - cyttsp5: was rejected
> [ 3112.498] (II) event1 - not using input device '/dev/input/event1'.
> [ 3112.499] (EE) libinput: cyttsp5: Failed to create a device for /dev/input/event1
>
> # evtest /dev/input/event1
> Input driver version is 1.0.1
> Input device ID: bus 0x0 vendor 0x0 product 0x0 version 0x0
> Input device name: "cyttsp5"
> Supported events:
> Event type 0 (EV_SYN)
> Event type 1 (EV_KEY)
> Event code 330 (BTN_TOUCH)
> Event type 2 (EV_REL)
> Event type 3 (EV_ABS)
> Event code 0 (ABS_X)
> Value 0
> Min 0
> Max 0 <-- upsets xorg
> Event code 1 (ABS_Y)
> Value 0
> Min 0
> Max 0
> Event code 47 (ABS_MT_SLOT)
> Value 0
> Min 0
> Max 31
> Event code 48 (ABS_MT_TOUCH_MAJOR)
> Value 0
> Min 0
> Max 255
> Event code 49 (ABS_MT_TOUCH_MINOR)
> Value 0
> Min 0
> Max 255
> Event code 53 (ABS_MT_POSITION_X)
> Value 0
> Min 0
> Max 1447
> Event code 54 (ABS_MT_POSITION_Y)
> Value 0
> Min 0
> Max 1071
> Event code 57 (ABS_MT_TRACKING_ID)
> Value 0
> Min 0
> Max 65535
> Event code 58 (ABS_MT_PRESSURE)
> Value 0
> Min 0
> Max 255
>
> I specified touchscreen-size-x and touchscreen-size-y in my devicetree.
> No idea why it is like this yet.
Thanks! I see the same problem on my side as well. I'm not using X so
I don't see a crash and didn't notice the error.
I'll work on fixing this.
Alistair
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-11-06 6:47 ` Dmitry Torokhov
@ 2021-12-01 12:27 ` Alistair Francis
2021-12-13 5:45 ` Dmitry Torokhov
0 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2021-12-01 12:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alistair Francis, linux-arm-kernel, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Andreas Kemnade, Henrik Rydberg, Rob Herring,
Mylène Josserand, Maxime Ripard
On Sat, Nov 6, 2021 at 4:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Alistair,
>
> On Wed, Nov 03, 2021 at 09:48:27PM +1000, Alistair Francis wrote:
> > From: Mylčne Josserand <mylene.josserand@bootlin.com>
> >
...
> > +
> > +/*
> > + * For what understood in the datasheet, the register does not
> > + * matter. For consistency, used the Input Register address
> > + * but it does mean anything to the device. The important data
> > + * to send is the I2C address
> > + */
> > +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
> > +{
> > + int rc;
>
> Please call this "error". Also elsewhere, variables that hold either
> error code or success (0) should be called "error".
>
> > + u32 size;
> > + u8 temp[2];
> > +
> > + if (!buf)
> > + return -EINVAL;
>
> This is an internal function. How can it be called with NULL buffer?
I have removed the check
>
> > +
> > + /* Read the frame to retrieve the size */
> > + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, 2);
>
> sizeof(temp)
>
> > + if (rc < 0)
>
> regmap_bulk_read() retirns 0 on success, so the check should be
>
> if (error)
>
> > + return rc;
> > +
> > + size = get_unaligned_le16(temp);
> > + if (!size || size == 2)
> > + return 0;
> > +
> > + if (size > max)
> > + return -EINVAL;
> > +
> > + /* Get the real value */
> > + return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size);
> > +}
> > +
> > +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
> > + size_t size)
> > +{
> > + u8 cmd[HID_OUTPUT_MAX_CMD_SIZE];
> > +
> > + if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE) {
> > + return -ENOMEM;
>
> Maybe -E2BIG or -EINVAL.
>
> > + }
>
> Unnecessary curly braces.
>
> > +
> > + /* High bytes of register address needed as first byte of cmd */
> > + cmd[0] = HI_BYTE(reg);
> > +
> > + /* Copy the rest of the data */
> > + if (data)
> > + memcpy(&cmd[1], data, size);
> > +
> > + /* The hardware wants to receive a frame with the address register
> > + * contains in the first two bytes. As the regmap_write function
> > + * add the register adresse in the frame, we use the LOW_BYTE as
> > + * first frame byte for the address register and the first
> > + * data byte is the high register + left of the cmd to send
> > + */
> > + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1);
> > +}
> > +
> > +static void cyttsp5_final_sync(struct input_dev *input, int max_slots,
> > + unsigned long *ids)
> > +{
> > + int t;
> > +
> > + for (t = 0; t < max_slots; t++) {
> > + if (test_bit(t, ids))
> > + continue;
> > + input_mt_slot(input, t);
> > + input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> > + }
> > +
> > + input_sync(input);
> > +}
> > +
> > +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots)
> > +{
> > + int t;
> > +
> > + if (ts->num_prv_rec == 0)
> > + return;
> > +
> > + for (t = 0; t < max_slots; t++) {
> > + input_mt_slot(ts->input, t);
> > + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false);
> > + }
> > +}
> > +
> > +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts)
> > +{
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int max = si->tch_abs[CY_TCH_T].max;
> > +
> > + if (ts->num_prv_rec != 0) {
> > + cyttsp5_report_slot_liftoff(ts, max);
> > + input_sync(ts->input);
> > + ts->num_prv_rec = 0;
> > + }
> > +}
> > +
> > +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
> > + int bofs)
> > +{
> > + int nbyte;
> > +
> > + for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
> > + *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
> > +
> > + *axis &= max - 1;
>
> Can it be that max is not power of 2?
Not that I have seen.
>
> > +}
> > +
> > +static void cyttsp5_get_touch_record(struct cyttsp5 *ts,
> > + struct cyttsp5_touch *touch, u8 *xy_data)
> > +{
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + enum cyttsp5_tch_abs abs;
> > +
> > + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) {
> > + cyttsp5_get_touch_axis(&touch->abs[abs],
> > + si->tch_abs[abs].size,
> > + si->tch_abs[abs].max,
> > + xy_data + si->tch_abs[abs].ofs,
> > + si->tch_abs[abs].bofs);
> > + }
>
> Unnecessary curly braces.
>
> > +}
> > +
> > +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts,
> > + struct cyttsp5_touch *tch, int num_cur_tch)
> > +{
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int i, t = 0;
> > + DECLARE_BITMAP(ids, MAX_CY_TCH_T_IDS);
> > + u8 *tch_addr;
> > + int tmp;
> > +
> > + bitmap_zero(ids, MAX_CY_TCH_T_IDS);
> > + memset(tch->abs, 0, sizeof(tch->abs));
> > +
> > + for (i = 0; i < num_cur_tch; i++) {
> > + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE);
> > + cyttsp5_get_touch_record(ts, tch, tch_addr);
> > +
> > + /* Convert MAJOR/MINOR from mm to resolution */
> > + tmp = tch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x;
> > + tch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x;
> > + tmp = tch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x;
> > + tch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x;
> > +
> > + t = tch->abs[CY_TCH_T];
> > + input_mt_slot(ts->input, t);
> > + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true);
> > + __set_bit(t, ids);
> > +
> > + /* position and pressure fields */
> > + input_report_abs(ts->input, ABS_MT_POSITION_X,
> > + tch->abs[CY_TCH_X]);
> > + input_report_abs(ts->input, ABS_MT_POSITION_Y,
> > + tch->abs[CY_TCH_Y]);
> > + input_report_abs(ts->input, ABS_MT_PRESSURE,
> > + tch->abs[CY_TCH_P]);
> > +
> > + /* Get the extended touch fields */
> > + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR,
> > + tch->abs[CY_TCH_MAJ]);
> > + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR,
> > + tch->abs[CY_TCH_MIN]);
> > +
> > + touchscreen_report_pos(ts->input, &ts->prop,
> > + tch->abs[CY_TCH_X], tch->abs[CY_TCH_Y],
> > + true);
>
> I have no idea why you first report position manually (without applying
> transformation) and then finally use touchscreen_report_pos() to report
> is again, now properly.
Fixed!
>
> > + }
> > +
> > + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids);
>
> I think if you use INPUT_MT_DROP_UNUSED when initializing slots and call
> input_mt_sync_frame() then cyttsp5_final_sync() will not be needed.
Yep, I have removed cyttsp5_final_sync()
>
> > +
> > + ts->num_prv_rec = num_cur_tch;
> > +}
> > +
> > +/* read xy_data for all current touches */
> > +static int cyttsp5_xy_worker(struct cyttsp5 *ts)
> > +{
> > + struct device *dev = ts->dev;
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int max_tch = si->sensing_conf_data.max_tch;
> > + struct cyttsp5_touch tch;
> > + u8 num_cur_tch;
> > +
> > + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size,
>
> I am struggling to understand why this member is called "hdr" and why it
> is a part of cyttsp5_touch.
Good point, removed.
>
> > + si->tch_hdr.max,
> > + si->xy_mode + 3 + si->tch_hdr.ofs,
> > + si->tch_hdr.bofs);
> > +
> > + num_cur_tch = tch.hdr;
> > + if (num_cur_tch > max_tch) {
> > + dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch);
> > + num_cur_tch = max_tch;
> > + }
> > +
> > + if (num_cur_tch == 0 && ts->num_prv_rec == 0)
> > + return 0;
> > +
> > + /* extract xy_data for all currently reported touches */
> > + if (num_cur_tch)
> > + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch);
> > + else
> > + cyttsp5_mt_lift_all(ts);
>
> Not needed with INPUT_MT_DROP_UNUSED.
I tried INPUT_MT_DROP_UNUSED and I still need this function
>
> > +
> > + return 0;
> > +}
> > +
> > +static int cyttsp5_mt_attention(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int rc;
> > +
> > + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID)
> > + return 0;
>
> It is only ever called with this condition, why do we need to check
> this again?
Removed.
>
> > +
> > + /* core handles handshake */
> > + rc = cyttsp5_xy_worker(ts);
> > + if (rc < 0)
> > + dev_err(dev, "xy_worker error r=%d\n", rc);
> > +
> > + return rc;
> > +}
> > +
> > +static int cyttsp5_setup_input_device(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int max_x, max_y, max_p;
> > + int max_x_tmp, max_y_tmp;
> > + int rc;
> > +
> > + __set_bit(EV_ABS, ts->input->evbit);
>
> input_set_abs_params() does this I believe.
Removed
>
> > + __set_bit(EV_REL, ts->input->evbit);
>
> I do not believe the driver emits any EV_REL events.
If I delete this my userspace program doesn't start.
>
> > + __set_bit(EV_KEY, ts->input->evbit);
>
> I think I saw it done elsewhere already.
Yep, removed.
>
> > +
> > + max_x_tmp = si->sensing_conf_data.res_x;
> > + max_y_tmp = si->sensing_conf_data.res_y;
> > + max_x = max_x_tmp - 1;
> > + max_y = max_y_tmp - 1;
> > + max_p = si->sensing_conf_data.max_z;
> > +
> > + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0);
>
> Error handling. You also want to use INPUT_MT_POINTER and potentially
> INPUT_MT_DROP_UNUSED.
Done!
>
> > +
> > + __set_bit(ABS_MT_POSITION_X, ts->input->absbit);
> > + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit);
> > + __set_bit(ABS_MT_PRESSURE, ts->input->absbit);
>
> These 3 are not needed.
>
> > +
> > + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0);
> > + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
> > + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0);
> > +
> > + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> > + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0);
> > +
> > + rc = input_register_device(ts->input);
> > + if (rc < 0)
> > + dev_err(dev, "Error, failed register input device r=%d\n", rc);
> > +
> > + return rc;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static int cyttsp5_parse_dt_key_code(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + struct device_node *np;
> > + int i;
> > +
> > + np = dev->of_node;
> > + if (!np)
> > + return -EINVAL;
> > +
> > + if (!si->num_btns)
> > + return 0;
> > +
> > + /* Initialize the button to RESERVED */
> > + for (i = 0; i < si->num_btns; i++)
> > + si->key_code[i] = KEY_RESERVED;
> > +
> > + return of_property_read_u32_array(np, "linux,keycodes",
> > + si->key_code, si->num_btns);
>
> I would use device_property_read_u32_array() instead and did not make it
> limited to OF.
>
> > +}
> > +#else
> > +static int cyttsp5_parse_dt_key_code(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int i;
> > +
> > + if (!si->num_btns)
> > + return 0;
> > +
> > + /* Initialize the button to RESERVED */
> > + for (i = 0; i < si->num_btns; i++)
> > + si->key_code[i] = KEY_RESERVED;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int cyttsp5_btn_attention(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int cur_btn;
> > + int cur_btn_state;
> > +
> > + if (si->xy_mode[2] != HID_BTN_REPORT_ID || !si->num_btns)
> > + return 0;
> > +
> > + /* extract button press/release touch information */
> > + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) {
> > + /* Get current button state */
> > + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN))
> > + & CY_NUM_BTN_EVENT_ID;
> > +
> > + input_report_key(ts->input, si->key_code[cur_btn],
> > + cur_btn_state);
> > + input_sync(ts->input);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static u16 cyttsp5_compute_crc(u8 *buf, u32 size)
> > +{
> > + u16 remainder = 0xFFFF;
> > + u16 xor_mask = 0x0000;
> > + u32 index;
> > + u32 byte_value;
> > + u32 table_index;
> > + u32 crc_bit_width = sizeof(u16) * 8;
> > +
> > + /* Divide the message by polynomial, via the table. */
> > + for (index = 0; index < size; index++) {
> > + byte_value = buf[index];
> > + table_index = ((byte_value >> 4) & 0x0F)
> > + ^ (remainder >> (crc_bit_width - 4));
> > + remainder = crc_itu_t_table[table_index]
> > + ^ (remainder << 4);
> > + table_index = (byte_value & 0x0F)
> > + ^ (remainder >> (crc_bit_width - 4));
> > + remainder = crc_itu_t_table[table_index]
> > + ^ (remainder << 4);
> > + }
> > +
> > + /* Perform the final remainder CRC. */
> > + return remainder ^ xor_mask;
>
> Do we have matching implementation in lib/crc* by any chance?
Yep, it seems that we do.
>
> > +}
> > +
> > +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code)
> > +{
> > + u16 size, crc;
> > + u8 status, offset;
> > + int command_code;
> > +
> > + size = get_unaligned_le16(&ts->response_buf[0]);
> > +
> > + if (!size)
> > + return 0;
> > +
> > + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET];
>
> This is definitely not an "offset".
>
> > +
> > + if (offset == HID_BL_RESPONSE_REPORT_ID) {
>
> switch (report_id) {
>
> > + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) {
> > + dev_err(ts->dev, "HID output response, wrong SOP\n");
> > + return -EPROTO;
> > + }
> > +
> > + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
> > + dev_err(ts->dev, "HID output response, wrong EOP\n");
> > + return -EPROTO;
> > + }
> > +
> > + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
> > + if (ts->response_buf[size - 3] != LOW_BYTE(crc) ||
> > + ts->response_buf[size - 2] != HI_BYTE(crc)) {
> > + dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n",
> > + crc);
> > + return -EPROTO;
> > + }
> > +
> > + status = ts->response_buf[5];
> > + if (status) {
> > + dev_err(ts->dev, "HID output response, ERROR:%d\n",
> > + status);
> > + return -EPROTO;
> > + }
> > + }
> > +
> > + if (offset == HID_APP_RESPONSE_REPORT_ID) {
> > + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET]
> > + & HID_OUTPUT_RESPONSE_CMD_MASK;
> > + if (command_code != code) {
> > + dev_err(ts->dev,
> > + "HID output response, wrong command_code:%X\n",
> > + command_code);
> > + return -EPROTO;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts)
> > +{
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > + int i;
> > + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET]
> > + & HID_SYSINFO_BTN_MASK;
> > +
> > + si->num_btns = 0;
> > + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
> > + if (btns & BIT(i))
> > + si->num_btns++;
> > + }
>
> hweight*()?
Fixed!
>
> > +}
> > +
> > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
> > +{
> > + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data;
> > + struct cyttsp5_sensing_conf_data_dev *scd_dev =
> > + (struct cyttsp5_sensing_conf_data_dev *)
> > + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +
> > + cyttsp5_si_get_btn_data(ts);
> > +
> > + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle;
> > + scd->res_x = get_unaligned_le16(&scd_dev->res_x);
> > + scd->res_y = get_unaligned_le16(&scd_dev->res_y);
> > + scd->max_z = get_unaligned_le16(&scd_dev->max_z);
> > + scd->len_x = get_unaligned_le16(&scd_dev->len_x);
> > + scd->len_y = get_unaligned_le16(&scd_dev->len_y);
> > +
> > + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
> > + GFP_KERNEL);
>
> What it the absolute maximum for tracked contacts? I wonder if it makes
> sense to allocate this buffer separately.
I have removed this as well and just use the input_buf.
>
> > + if (!si->xy_data)
> > + return -ENOMEM;
> > +
> > + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE,
> > + GFP_KERNEL);
> > + if (!si->xy_mode)
> > + return -ENOMEM;
>
> Why do we need to allocate 7 bytes separately?
I have removed this.
>
> > +
> > + return 0;
> > +}
> > +
...
> > +
> > +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
> > +{
> > + int max_tch = si->sensing_conf_data.max_tch;
> > + int num_cur_tch;
> > + int length;
> > + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr;
> > +
> > + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE);
> > +
> > + cyttsp5_get_touch_axis(&num_cur_tch, tch->size,
> > + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs);
> > + if (unlikely(num_cur_tch > max_tch))
> > + num_cur_tch = max_tch;
> > +
> > + length = num_cur_tch * TOUCH_REPORT_SIZE;
> > +
> > + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length);
>
> Why do we need to do all these moves? Can we parse the date directly
> from the input buffer?
We don't need the moves. I have removed them.
>
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
> > +{
> > + struct cyttsp5 *ts = handle;
> > + int report_id;
> > + int size;
> > + int rc;
> > +
> > + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT);
> > + if (rc)
> > + return IRQ_HANDLED;
> > +
> > + size = get_unaligned_le16(&ts->input_buf[0]);
> > +
> > + /* check reset */
> > + if (size == 0) {
> > + memcpy(ts->response_buf, ts->input_buf, 2);
> > +
> > + mutex_lock(&ts->system_lock);
>
> Why is this lock needed exactly?
I don't think it is, I have removed it.
>
> > + ts->hid_cmd_state = HID_CMD_DONE;
> > + mutex_unlock(&ts->system_lock);
> > + wake_up(&ts->wait_q);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + report_id = ts->input_buf[2];
>
> I'd probably do:
>
> if (size == 0) {
> /* reset */
> report_id = HID_EMPTY_RESPONSE_ID; /* = 0 */
> size = 2;
> } else {
> report_id = ts->input_buf[2];
> }
>
> > +
> > + if (report_id == HID_TOUCH_REPORT_ID) {
>
> switch (report_id) {
Good idea, fixed.
>
> > + move_touch_data(ts, &ts->sysinfo);
> > + cyttsp5_mt_attention(ts->dev);
> > + } else if (report_id == HID_BTN_REPORT_ID) {
> > + move_button_data(ts, &ts->sysinfo);
> > + cyttsp5_btn_attention(ts->dev);
> > + } else {
> > + /* It is not an input but a command response */
> > + memcpy(ts->response_buf, ts->input_buf, size);
> > +
> > + mutex_lock(&ts->system_lock);
> > + ts->hid_cmd_state = HID_CMD_DONE;
> > + mutex_unlock(&ts->system_lock);
> > + wake_up(&ts->wait_q);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int cyttsp5_deassert_int(struct cyttsp5 *ts)
> > +{
> > + u16 size;
> > + u8 buf[2];
> > + int rc;
> > +
> > + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
> > + if (rc < 0)
> > + return rc;
> > +
> > + size = get_unaligned_le16(&buf[0]);
> > + if (size == 2 || size == 0)
> > + return 0;
>
> If you were to use level interrupts this probably would not be needed.
I have tried with/without this and I still can't get level interrupts working.
>
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
> > +{
> > + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +
> > + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16,
> > + TOUCH_REPORT_DESC_X);
> > + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16,
> > + TOUCH_REPORT_DESC_Y);
> > + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8,
> > + TOUCH_REPORT_DESC_P);
> > + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5,
> > + TOUCH_REPORT_DESC_CONTACTID);
> > + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5,
> > + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT);
> > + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8,
> > + TOUCH_REPORT_DESC_MAJ);
> > + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8,
> > + TOUCH_REPORT_DESC_MIN);
> > +
> > + return 0;
> > +}
> > +
> > +static int cyttsp5_startup(struct cyttsp5 *ts)
> > +{
> > + int rc;
> > +
> > + rc = cyttsp5_deassert_int(ts);
> > + if (rc) {
> > + dev_err(ts->dev, "Error on deassert int r=%d\n", rc);
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > + * Launch the application as the device starts in bootloader mode
> > + * because of a power-on-reset
> > + */
> > + rc = cyttsp5_hid_output_bl_launch_app(ts);
> > + if (rc < 0) {
> > + dev_err(ts->dev, "Error on launch app r=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
> > + if (rc < 0) {
> > + dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + rc = cyttsp5_fill_all_touch(ts);
> > + if (rc < 0) {
> > + dev_err(ts->dev, "Error on report descriptor r=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + rc = cyttsp5_hid_output_get_sysinfo(ts);
> > + if (rc) {
> > + dev_err(ts->dev, "Error on getting sysinfo r=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cyttsp5_of_match[] = {
> > + { .compatible = "cypress,tt21000", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, cyttsp5_of_match);
> > +#endif
> > +
> > +static const struct i2c_device_id cyttsp5_i2c_id[] = {
> > + { CYTTSP5_NAME, 0, },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);
> > +
> > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> > + const char *name)
> > +{
> > + struct cyttsp5 *ts;
> > + struct cyttsp5_sysinfo *si;
> > + int rc = 0, i;
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + /* Initialize device info */
> > + ts->regmap = regmap;
> > + ts->dev = dev;
> > + si = &ts->sysinfo;
> > + dev_set_drvdata(dev, ts);
> > +
> > + /* Initialize mutexes and spinlocks */
> > + mutex_init(&ts->system_lock);
> > +
> > + /* Initialize wait queue */
> > + init_waitqueue_head(&ts->wait_q);
> > +
> > + /* Power up the device */
> > + ts->vdd = regulator_get(dev, "vdd");
>
> Do not mix managed an unmanaged resources. You are leaking this
> regulator.
I'm not clear what I should do differently here.
>
> > + if (IS_ERR(ts->vdd)) {
> > + rc = PTR_ERR(ts->vdd);
> > + dev_set_drvdata(dev, NULL);
> > + kfree(ts);
> > + return rc;
> > + }
> > +
> > + rc = regulator_enable(ts->vdd);
> > + if (rc) {
> > + regulator_put(ts->vdd);
> > + dev_set_drvdata(dev, NULL);
> > + kfree(ts);
> > + return rc;
> > + }
> > +
> > + /* Reset the gpio to be in a reset state */
> > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(ts->reset_gpio)) {
> > + rc = PTR_ERR(ts->reset_gpio);
> > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > + return rc;
> > + }
> > + gpiod_set_value(ts->reset_gpio, 1);
> > +
> > + /* Need a delay to have device up */
> > + msleep(20);
> > +
> > + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
> > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>
> Please do not override platform setup with hardcoded triggers. Also, it
> is strongly recommended to use level interrupts for these peripherals.
>
> This is also likely unsafe if controller is not completely shut off and
> is capable of generating interrupts given input device is not yet
> allocated.
I have dropped the `IRQF_TRIGGER_FALLING |`
I have tried to use level interrupts, but I can't get the device
working with them.
>
> > + name, ts);
> > + if (rc) {
> > + dev_err(dev, "unable to request IRQ\n");
> > + return rc;
> > + }
> > +
> > + rc = cyttsp5_startup(ts);
> > + if (rc) {
> > + dev_err(ts->dev, "Fail initial startup r=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + rc = cyttsp5_parse_dt_key_code(dev);
> > + if (rc < 0) {
> > + dev_err(ts->dev, "Error while parsing dts %d\n", rc);
> > + return rc;
> > + }
> > +
> > + ts->input = devm_input_allocate_device(dev);
> > + if (!ts->input) {
> > + dev_err(dev, "Error, failed to allocate input device\n");
> > + return -ENODEV;
> > + }
> > +
> > + ts->input->name = "cyttsp5";
> > + scnprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev));
> > + ts->input->phys = ts->phys;
> > + ts->input->dev.parent = ts->dev;
>
> No need to do this, devm_input_allocate_device() sets up parent.
>
> > + input_set_drvdata(ts->input, ts);
> > +
> > + touchscreen_parse_properties(ts->input, true, &ts->prop);
> > +
> > + __set_bit(EV_KEY, ts->input->evbit);
> > + __set_bit(ABS_X, ts->input->absbit);
> > + __set_bit(ABS_Y, ts->input->absbit);
> > + __set_bit(BTN_TOUCH, ts->input->keybit);
>
> These 3 should not be needed.
>
> > + for (i = 0; i < si->num_btns; i++)
> > + __set_bit(si->key_code[i], ts->input->keybit);
> > +
> > + return cyttsp5_setup_input_device(dev);
> > +}
> > +
> > +static int cyttsp5_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct regmap *regmap;
> > + static const struct regmap_config config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + };
> > +
> > + regmap = devm_regmap_init_i2c(client, &config);
> > + if (IS_ERR(regmap)) {
> > + dev_err(&client->dev, "regmap allocation failed: %ld\n",
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
> > +}
> > +
> > +static int cyttsp5_remove(struct device *dev)
> > +{
> > + struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +
> > + input_unregister_device(ts->input);
>
> This is not needed as input device is devm managed. This entire function
> can be dropped.
Removed
>
> > +
> > + return 0;
> > +}
> > +
> > +static int cyttsp5_i2c_remove(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > +
> > + return cyttsp5_remove(dev);
> > +}
> > +
> > +static struct i2c_driver cyttsp5_i2c_driver = {
> > + .driver = {
> > + .name = CYTTSP5_NAME,
> > + .of_match_table = of_match_ptr(cyttsp5_of_match),
> > + },
> > + .probe = cyttsp5_i2c_probe,
> > + .remove = cyttsp5_i2c_remove,
> > + .id_table = cyttsp5_i2c_id,
> > +};
> > +
> > +module_i2c_driver(cyttsp5_i2c_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product");
> > +MODULE_AUTHOR("Mylčne Josserand <mylene.josserand@bootlin.com>");
> > --
> > 2.31.1
> >
>
> Thanks.
Thanks for the in depth review.
I have addressed all of the issues that I didn't comment on.
Alistair
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-12-01 12:27 ` Alistair Francis
@ 2021-12-13 5:45 ` Dmitry Torokhov
2021-12-13 18:49 ` Andreas Kemnade
2021-12-18 22:18 ` Andreas Kemnade
0 siblings, 2 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2021-12-13 5:45 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, linux-arm-kernel, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Andreas Kemnade, Henrik Rydberg, Rob Herring,
Mylène Josserand, Maxime Ripard
On Wed, Dec 01, 2021 at 10:27:46PM +1000, Alistair Francis wrote:
> On Sat, Nov 6, 2021 at 4:47 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Wed, Nov 03, 2021 at 09:48:27PM +1000, Alistair Francis wrote:
> > > From: Mylčne Josserand <mylene.josserand@bootlin.com>
> > >
...
> > > + si->tch_hdr.max,
> > > + si->xy_mode + 3 + si->tch_hdr.ofs,
> > > + si->tch_hdr.bofs);
> > > +
> > > + num_cur_tch = tch.hdr;
> > > + if (num_cur_tch > max_tch) {
> > > + dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch);
> > > + num_cur_tch = max_tch;
> > > + }
> > > +
> > > + if (num_cur_tch == 0 && ts->num_prv_rec == 0)
> > > + return 0;
> > > +
> > > + /* extract xy_data for all currently reported touches */
> > > + if (num_cur_tch)
> > > + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch);
> > > + else
> > > + cyttsp5_mt_lift_all(ts);
> >
> > Not needed with INPUT_MT_DROP_UNUSED.
>
> I tried INPUT_MT_DROP_UNUSED and I still need this function
I am curious why that is. Probably call to input_mt_sync_frame() was in
a wrong place?
...
> > > +
> > > +static int cyttsp5_deassert_int(struct cyttsp5 *ts)
> > > +{
> > > + u16 size;
> > > + u8 buf[2];
> > > + int rc;
> > > +
> > > + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
> > > + if (rc < 0)
> > > + return rc;
> > > +
> > > + size = get_unaligned_le16(&buf[0]);
> > > + if (size == 2 || size == 0)
> > > + return 0;
> >
> > If you were to use level interrupts this probably would not be needed.
>
> I have tried with/without this and I still can't get level interrupts working.
Does the platform support level interrupts?
...
> > > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> > > + const char *name)
> > > +{
> > > + struct cyttsp5 *ts;
> > > + struct cyttsp5_sysinfo *si;
> > > + int rc = 0, i;
> > > +
> > > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > > + if (!ts)
> > > + return -ENOMEM;
> > > +
> > > + /* Initialize device info */
> > > + ts->regmap = regmap;
> > > + ts->dev = dev;
> > > + si = &ts->sysinfo;
> > > + dev_set_drvdata(dev, ts);
> > > +
> > > + /* Initialize mutexes and spinlocks */
> > > + mutex_init(&ts->system_lock);
> > > +
> > > + /* Initialize wait queue */
> > > + init_waitqueue_head(&ts->wait_q);
> > > +
> > > + /* Power up the device */
> > > + ts->vdd = regulator_get(dev, "vdd");
> >
> > Do not mix managed an unmanaged resources. You are leaking this
> > regulator.
>
> I'm not clear what I should do differently here.
devm_regulator_get().
>
> >
> > > + if (IS_ERR(ts->vdd)) {
> > > + rc = PTR_ERR(ts->vdd);
> > > + dev_set_drvdata(dev, NULL);
> > > + kfree(ts);
You can't call kfree() for memory allocated with devm_kzalloc().
> > > + return rc;
> > > + }
> > > +
> > > + rc = regulator_enable(ts->vdd);
> > > + if (rc) {
> > > + regulator_put(ts->vdd);
> > > + dev_set_drvdata(dev, NULL);
> > > + kfree(ts);
> > > + return rc;
> > > + }
> > > +
> > > + /* Reset the gpio to be in a reset state */
> > > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > + if (IS_ERR(ts->reset_gpio)) {
> > > + rc = PTR_ERR(ts->reset_gpio);
> > > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > > + return rc;
> > > + }
> > > + gpiod_set_value(ts->reset_gpio, 1);
> > > +
> > > + /* Need a delay to have device up */
> > > + msleep(20);
> > > +
> > > + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
> > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >
> > Please do not override platform setup with hardcoded triggers. Also, it
> > is strongly recommended to use level interrupts for these peripherals.
> >
> > This is also likely unsafe if controller is not completely shut off and
> > is capable of generating interrupts given input device is not yet
> > allocated.
>
> I have dropped the `IRQF_TRIGGER_FALLING |`
>
> I have tried to use level interrupts, but I can't get the device
> working with them.
That is weird, does the interrupt controller support level interrupts?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-12-13 5:45 ` Dmitry Torokhov
@ 2021-12-13 18:49 ` Andreas Kemnade
2021-12-18 22:18 ` Andreas Kemnade
1 sibling, 0 replies; 22+ messages in thread
From: Andreas Kemnade @ 2021-12-13 18:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alistair Francis, Alistair Francis, linux-arm-kernel, devicetree,
linux-input, Linux Kernel Mailing List, Mylene Josserand,
Linus Walleij, Henrik Rydberg, Rob Herring,
Mylène Josserand, Maxime Ripard
Hi,
On Sun, 12 Dec 2021 21:45:48 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
> > > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > >
> > > Please do not override platform setup with hardcoded triggers. Also, it
> > > is strongly recommended to use level interrupts for these peripherals.
> > >
> > > This is also likely unsafe if controller is not completely shut off and
> > > is capable of generating interrupts given input device is not yet
> > > allocated.
> >
> > I have dropped the `IRQF_TRIGGER_FALLING |`
> >
> > I have tried to use level interrupts, but I can't get the device
> > working with them.
>
> That is weird, does the interrupt controller support level interrupts?
>
hmm, it should, at least on the imx6 I am using them for some other
devices, I guess, on the imx7 they should work, too.
I hope I find time to give that a try the next days.
Regards,
Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-12-13 5:45 ` Dmitry Torokhov
2021-12-13 18:49 ` Andreas Kemnade
@ 2021-12-18 22:18 ` Andreas Kemnade
2021-12-21 23:28 ` Alistair Francis
1 sibling, 1 reply; 22+ messages in thread
From: Andreas Kemnade @ 2021-12-18 22:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alistair Francis, Alistair Francis, linux-arm-kernel, devicetree,
linux-input, Linux Kernel Mailing List, Mylene Josserand,
Linus Walleij, Henrik Rydberg, Rob Herring,
Mylène Josserand, Maxime Ripard
Hi,
On Sun, 12 Dec 2021 21:45:48 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
[...]
> > > This is also likely unsafe if controller is not completely shut off and
> > > is capable of generating interrupts given input device is not yet
> > > allocated.
> >
> > I have dropped the `IRQF_TRIGGER_FALLING |`
> >
> > I have tried to use level interrupts, but I can't get the device
> > working with them.
>
> That is weird, does the interrupt controller support level interrupts?
>
I have set level interrupts via the devicetree,
things seem to work on Kobo Clara HD:
/proc/interrupts
203: 37402 gpio-mxc 6 Level tt21000
touchscreen@24 {
compatible = "cypress,tt21000";
reg = <0x24>;
pinctrl-names = "default","sleep";
pinctrl-0 = <&pinctrl_cyttsp5_gpio>;
pinctrl-1 = <&pinctrl_cyttsp5_gpio_sleep>;
interrupt-parent = <&gpio5>;
interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
};
Regards,
Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-12-18 22:18 ` Andreas Kemnade
@ 2021-12-21 23:28 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2021-12-21 23:28 UTC (permalink / raw)
To: Andreas Kemnade
Cc: Dmitry Torokhov, Alistair Francis, linux-arm-kernel, devicetree,
linux-input, Linux Kernel Mailing List, Mylene Josserand,
Linus Walleij, Henrik Rydberg, Rob Herring,
Mylène Josserand, Maxime Ripard
On Sun, Dec 19, 2021 at 8:18 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi,
>
> On Sun, 12 Dec 2021 21:45:48 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> [...]
> > > > This is also likely unsafe if controller is not completely shut off and
> > > > is capable of generating interrupts given input device is not yet
> > > > allocated.
> > >
> > > I have dropped the `IRQF_TRIGGER_FALLING |`
> > >
> > > I have tried to use level interrupts, but I can't get the device
> > > working with them.
> >
> > That is weird, does the interrupt controller support level interrupts?
> >
> I have set level interrupts via the devicetree,
> things seem to work on Kobo Clara HD:
> /proc/interrupts
> 203: 37402 gpio-mxc 6 Level tt21000
>
> touchscreen@24 {
> compatible = "cypress,tt21000";
> reg = <0x24>;
> pinctrl-names = "default","sleep";
> pinctrl-0 = <&pinctrl_cyttsp5_gpio>;
> pinctrl-1 = <&pinctrl_cyttsp5_gpio_sleep>;
> interrupt-parent = <&gpio5>;
> interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> };
I have tried using IRQ_TYPE_LEVEL_LOW as well, but the device doesn't
start correctly and the commands time out.
It works fine with IRQ_TYPE_EDGE_FALLING though.
Alistair
>
> Regards,
> Andreas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
2021-11-05 15:01 ` Andreas Kemnade
2021-11-06 6:47 ` Dmitry Torokhov
@ 2021-12-23 13:47 ` Andy Shevchenko
2 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2021-12-23 13:47 UTC (permalink / raw)
To: Alistair Francis
Cc: linux-arm Mailing List, devicetree, linux-input,
Linux Kernel Mailing List, Mylene Josserand, Linus Walleij,
Andreas Kemnade, Henrik Rydberg, Dmitry Torokhov, Rob Herring,
Alistair Francis, Mylène Josserand, Maxime Ripard
On Wed, Nov 3, 2021 at 1:49 PM Alistair Francis <alistair@alistair23.me> wrote:
...
> Message-Id: <20180703094309.18514-2-mylene.josserand@bootlin.com>
What is this?
...
> +config TOUCHSCREEN_CYTTSP5
> + tristate "Cypress TrueTouch Gen5 Touchscreen Driver"
> + depends on OF && I2C
Is it really OF dependent?
> + select REGMAP_I2C
> + select CRC_ITU_T
> + help
> + Driver for Parade TrueTouch Standard Product
> + Generation 5 touchscreen controllers.
> + I2C bus interface support only.
> + Say Y here if you have a Cypress Gen5 touchscreen.
> + If unsure, say N.
> + To compile this driver as a module, choose M here: the
> + module will be called cyttsp5.
Utilize lines better, i.e. increase density of the words on them.
...
> +/*
> + * Parade TrueTouch(TM) Standard Product V5 Module.
> + *
> + * Copyright (C) 2015 Parade Technologies
> + * Copyright (C) 2012-2015 Cypress Semiconductor
> + * Copyright (C) 2018 Bootlin
> + *
> + * Authors: Mylène Josserand <mylene.josserand@bootlin.com>
> + * Alistair Francis <alistair@alistair23.me>
> + *
Redundant.
> + */
...
> +#include <asm/unaligned.h>
Can you move it after linux/* ones?
> +#include <linux/crc-itu-t.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio.h>
This is wrong. New code shouldn't include this header.
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
...
> +#define CY_BITS_PER_BTN 1
> +#define CY_NUM_BTN_EVENT_ID ((1 << CY_BITS_PER_BTN) - 1)
Seems like a mask depending on the amount of bits.
Perhaps GENMASK(1, 0)?
...
> +#define HID_OUTPUT_RESPONSE_CMD_MASK 0x7F
GENMASK()
...
> +#define HID_SYSINFO_BTN_MASK 0xFF
Ditto.
...
> +/* Timeout in ms */
Drop this comment and use _MS suffix in the definitions.
> +#define CY_HID_OUTPUT_TIMEOUT 200
> +#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000
> +#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000
...
> +/* helpers */
> +#define HI_BYTE(x) ((u8)(((x) >> 8) & 0xFF))
> +#define LOW_BYTE(x) ((u8)((x) & 0xFF))
Why are these needed? Just use them directly.
...
> +enum cyttsp5_tch_abs { /* for ordering within the extracted touch data array */
> + CY_TCH_X, /* X */
> + CY_TCH_Y, /* Y */
> + CY_TCH_P, /* P (Z) */
> + CY_TCH_T, /* TOUCH ID */
> + CY_TCH_MAJ, /* TOUCH_MAJOR */
> + CY_TCH_MIN, /* TOUCH_MINOR */
> + CY_TCH_NUM_ABS,
If it is a terminator, drop the comma.
> +};
...
> +/*
> + * For what understood in the datasheet, the register does not
is understood
> + * matter. For consistency, used the Input Register address
use
> + * but it does mean anything to the device. The important data
> + * to send is the I2C address
> + */
> + /* The hardware wants to receive a frame with the address register
Be consistent with multi-line comments. Here you need to add a leading
empty line.
> + * contains in the first two bytes. As the regmap_write function
contained
> + * add the register adresse in the frame, we use the LOW_BYTE as
> + * first frame byte for the address register and the first
> + * data byte is the high register + left of the cmd to send
> + */
...
> + for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
> + *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
+=
...
> +#ifdef CONFIG_OF
Why? You may easily combine the two.
> +static int cyttsp5_parse_dt_key_code(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + struct device_node *np;
> + int i;
> + np = dev->of_node;
> + if (!np)
> + return -EINVAL;
Redundant. The of_property_*() will do it anyway.
> + if (!si->num_btns)
> + return 0;
> +
> + /* Initialize the button to RESERVED */
> + for (i = 0; i < si->num_btns; i++)
> + si->key_code[i] = KEY_RESERVED;
memset32() ?
> + return of_property_read_u32_array(np, "linux,keycodes",
> + si->key_code, si->num_btns);
> +}
> +#else
> +static int cyttsp5_parse_dt_key_code(struct device *dev)
> +{
> + struct cyttsp5 *ts = dev_get_drvdata(dev);
> + struct cyttsp5_sysinfo *si = &ts->sysinfo;
> + int i;
> +
> + if (!si->num_btns)
> + return 0;
> +
> + /* Initialize the button to RESERVED */
> + for (i = 0; i < si->num_btns; i++)
> + si->key_code[i] = KEY_RESERVED;
> +
> + return 0;
> +}
Combine with the above and get rid of ugly ifdeffery.
> +#endif
...
> + size = get_unaligned_le16(&ts->response_buf[0]);
> +
Redundant blank line.
> + if (!size)
> + return 0;
...
> + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
> + if (ts->response_buf[size - 3] != LOW_BYTE(crc) ||
> + ts->response_buf[size - 2] != HI_BYTE(crc)) {
What's wrong with a direct comparison with _le16 value?
> + dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n",
> + crc);
> + return -EPROTO;
> + }
...
> + si->num_btns = 0;
> + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
> + if (btns & BIT(i))
> + si->num_btns++;
> + }
NIH for_each_set_bit(), but do one more step to see that this is
actually a Hamming weight. We have functions. Hence, make it one line
with one simple call.
...
> + struct cyttsp5_sensing_conf_data_dev *scd_dev =
> + (struct cyttsp5_sensing_conf_data_dev *)
Why casting?
> + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
...
> + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
> + GFP_KERNEL);
Use _kcalloc().
> + if (!si->xy_data)
> + return -ENOMEM;
...
> + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
Use put_unaligned_le16() instead.
...
> + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
> + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT));
> + if (!rc) {
> + dev_err(ts->dev, "HID output cmd execution timed out\n");
> + rc = -ETIME;
What is this supposed to mean?
> + goto error;
> + }
...
> + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
put_unaligned_le16()
...
> + cmd[6] = 0x0; /* Low bytes of data */
> + cmd[7] = 0x0; /* Hi bytes of data */
Ditto.
...
> + crc = cyttsp5_compute_crc(&cmd[4], 4);
> + cmd[8] = LOW_BYTE(crc);
> + cmd[9] = HI_BYTE(crc);
Ditto.
...
> + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE),
> + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT));
> + if (!rc) {
> + dev_err(ts->dev, "HID output cmd execution timed out\n");
> + rc = -ETIME;
???
> + goto error;
> + }
...
> + memcpy(desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc));
sizeof(*desc) ?
...
> + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
sizeof(buf)
> + if (rc < 0)
> + return rc;
...
> +#ifdef CONFIG_OF
Drop this. It usually brings more pain than helps.
> +static const struct of_device_id cyttsp5_of_match[] = {
> + { .compatible = "cypress,tt21000", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, cyttsp5_of_match);
> +#endif
> +
> +static const struct i2c_device_id cyttsp5_i2c_id[] = {
> + { CYTTSP5_NAME, 0, },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);
Move both closer to their users (below).
...
> + dev_set_drvdata(dev, NULL);
No need to repeat what device core does for everybody for the last 10+ years.
...
> + dev_set_drvdata(dev, NULL);
Ditto.
...
> + /* Reset the gpio to be in a reset state */
> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ts->reset_gpio)) {
> + rc = PTR_ERR(ts->reset_gpio);
> + dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> + return rc;
> + }
No minimum reset timeout?
> + gpiod_set_value(ts->reset_gpio, 1);
Or what does this all mean? Shouldn't you simply use _OUT_HIGH above?
> + /* Need a delay to have device up */
> + msleep(20);
...
> +static struct i2c_driver cyttsp5_i2c_driver = {
> + .driver = {
> + .name = CYTTSP5_NAME,
> + .of_match_table = of_match_ptr(cyttsp5_of_match),
Drop of_match_ptr() as well.
> + },
> + .probe = cyttsp5_i2c_probe,
> + .remove = cyttsp5_i2c_remove,
> + .id_table = cyttsp5_i2c_id,
> +};
> +
Redundant blank line.
> +module_i2c_driver(cyttsp5_i2c_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-12-23 13:49 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 11:48 [PATCH v2 0/4] Add support for the Cypress cyttsp5 Alistair Francis
2021-11-03 11:48 ` [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen Alistair Francis
2021-11-05 15:01 ` Andreas Kemnade
2021-11-06 6:47 ` Dmitry Torokhov
2021-12-01 12:27 ` Alistair Francis
2021-12-13 5:45 ` Dmitry Torokhov
2021-12-13 18:49 ` Andreas Kemnade
2021-12-18 22:18 ` Andreas Kemnade
2021-12-21 23:28 ` Alistair Francis
2021-12-23 13:47 ` Andy Shevchenko
2021-11-03 11:48 ` [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5 Alistair Francis
2021-11-03 23:07 ` Rob Herring
2021-11-05 14:21 ` Andreas Kemnade
2021-11-10 12:59 ` Alistair Francis
2021-11-10 17:36 ` Andreas Kemnade
2021-11-11 15:16 ` Linus Walleij
2021-11-18 12:51 ` Alistair Francis
2021-11-03 11:48 ` [PATCH v2 3/4] ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen Alistair Francis
2021-11-03 11:48 ` [PATCH v2 4/4] ARM: dts: imx7d: remarkable2: Enable the cyttsp5 Alistair Francis
2021-11-06 2:25 ` Dmitry Torokhov
2021-11-05 14:52 ` [PATCH v2 0/4] Add support for the Cypress cyttsp5 Andreas Kemnade
2021-11-25 9:44 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).