[PATCH v3 1/2] dt-bindings: input: touchscreen: ilitek_ts_i2c: Add bindings

[PATCH v3 1/2] dt-bindings: input: touchscreen: ilitek_ts_i2c: Add bindings

[Joe Hung]

Add binding documentation for ILITEK touch devices.

Signed-off-by: Joe Hung <joe_hung@ilitek.com>
---
Changes in v3:
  - Add include header in examples, and pass the dt binding check

Changes in v2:
  - Convert to DT schema format
  - Using interrupts instead of irq-gpios

 .../input/touchscreen/ilitek_ts_i2c.yaml      | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
new file mode 100644
index 000000000000..5da9020ed272
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/ilitek_ts_i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ilitek I2C Touchscreen Controller
+
+maintainers:
+  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - ilitek,ili2520
+      - ilitek,ili2510
+  reg:
+    const: 0x41
+  interrupts:
+    maxItems: 1
+  reset-gpios:
+    maxItems: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        touchscreen@41 {
+                compatible = "ilitek,ili2520";
+                reg = <0x41>;
+                interrupt-parent = <&gpio1>;
+                interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+                reset-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>;
+        };
+    };
--
2.25.1

[PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Joe Hung]

Add support for ILITEK Lego series of touch devices.
Lego series includes ILITEK 213X/2316/232X/252X.
Designed in compatible way for ILITEK 23XX/251X ICs.

Tested and passed with evaluation board with ILI2520 IC.

Signed-off-by: Joe Hung <joe_hung@ilitek.com>
---
Changes in v3:
  - None

Changes in v2:
  - Remove irq-gpio and related flow

 drivers/input/touchscreen/Kconfig         |   12 +
 drivers/input/touchscreen/Makefile        |    1 +
 drivers/input/touchscreen/ilitek_ts_i2c.c | 1010 +++++++++++++++++++++
 3 files changed, 1023 insertions(+)
 create mode 100644 drivers/input/touchscreen/ilitek_ts_i2c.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index f012fe746df0..9dea7fd0d94f 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
 	  To compile this driver as a module, choose M here: the
 	  module will be called zinitix.

+config TOUCHSCREEN_ILITEK
+	tristate "Ilitek I2C 23XX/25XX/Lego Series Touch ICs"
+	depends on I2C
+	help
+	  Say Y here if you have touchscreen with ILITEK touch IC,
+	  it supports 23XX/25XX/other Lego series.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ilitek_ts_i2c.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 6233541e9173..6749bdc42f41 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
 obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
 obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
 obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
+obj-$(CONFIG_TOUCHSCREEN_ILITEK)        += ilitek_ts_i2c.o
diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
new file mode 100644
index 000000000000..72cb7c82170c
--- /dev/null
+++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
@@ -0,0 +1,1010 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ILITEK Touch IC driver for 23XX, 25XX and Lego series
+ *
+ * Copyright (C) 2011 ILI Technology Corporation.
+ * Copyright (C) 2020 Luca Hsu <luca_hsu@ilitek.com>
+ * Copyright (C) 2021 Joe Hung <joe_hung@ilitek.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/cdev.h>
+#include <linux/uaccess.h>
+#include <linux/version.h>
+#include <linux/rtc.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/proc_fs.h>
+#include <linux/fs.h>
+#include <linux/pm_runtime.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/acpi.h>
+
+#define ILITEK_TS_NAME					"ilitek_ts"
+#define BL_V1_8						(0x10800)
+#define BL_V1_7						(0x10700)
+#define BL_V1_6						(0x10600)
+
+#define ILITEK_TP_CMD_GET_TP_RES			(0x20)
+#define ILITEK_TP_CMD_GET_SCRN_RES			(0x21)
+#define ILITEK_TP_CMD_GET_KEY_INFO			(0x22)
+#define ILITEK_TP_CMD_GET_FW_VER			(0x40)
+#define ILITEK_TP_CMD_GET_PRL_VER			(0x42)
+#define ILITEK_TP_CMD_GET_MCU_VER			(0x61)
+#define ILITEK_TP_CMD_GET_IC_MODE			(0xC0)
+
+#define REPORT_ADDRESS_COUNT				(61)
+#define ILITEK_SUPPORT_MAX_POINT			(40)
+#define ILITEK_SUPPORT_MAX_KEY_CNT			(50)
+
+#define CEIL(n, d)	(((n) % (d)) ? ((n) / (d) + 1) : ((n) / (d)))
+
+struct ilitek_protocol_info {
+	s32 ver;
+	u8 ver_major;
+	u8 ver_mid;
+	u8 ver_minor;
+};
+
+struct ilitek_key_info {
+	s32 id;
+	s32 x;
+	s32 y;
+	s32 status;
+};
+
+struct ilitek_touch_info {
+	u16 id;
+	u16 x;
+	u16 y;
+	u16 p;
+	u16 w;
+	u16 h;
+	u16 status;
+};
+
+struct ilitek_ts_data {
+	struct i2c_client		*client;
+	struct gpio_desc		*reset_gpio;
+	struct input_dev		*input_dev;
+
+	struct mutex			ilitek_mutex;
+	spinlock_t			irq_lock;
+	bool				irq_status;
+	int (*process_and_report)(struct ilitek_ts_data *ilitek_data);
+
+	struct PROTOCOL_MAP		*ptl_cb_func;
+	struct ilitek_protocol_info	ptl;
+
+
+	struct ilitek_key_info		keyinfo[ILITEK_SUPPORT_MAX_KEY_CNT];
+	struct ilitek_touch_info	tpinfo[ILITEK_SUPPORT_MAX_POINT];
+
+	bool				is_touched;
+	bool				touch_key_hold_press;
+
+	bool				ilitek_repeat_start;
+
+	char				product_id[30];
+	u8				mcu_ver[2];
+	u8				ic_mode;
+	u8				firmware_ver[8];
+
+	s32				reset_time;
+	s32				tp_max_x;
+	s32				tp_max_y;
+	s32				screen_max_x;
+	s32				screen_max_y;
+	s32				screen_min_x;
+	s32				screen_min_y;
+	s32				max_tp;
+	s32				x_ch;
+	s32				y_ch;
+	s32				slave_count;
+	s32				format;
+	s32				keycount;
+	s32				key_xlen;
+	s32				key_ylen;
+};
+
+enum ilitek_cmds_v6 {
+	GET_PTL_VER = 0,
+	GET_FW_VER,
+	GET_SCRN_RES,
+	GET_TP_RES,
+	GET_IC_MODE,
+	GET_MCU_VER,
+	GET_KEY_INFO,
+
+	MAX_CMD_CNT	/* ALWAYS keep at the end */
+};
+
+typedef int protocol_func(struct ilitek_ts_data *ilitek_data,
+			  u16 cmd, uint8_t *inbuf, uint8_t *outbuf);
+
+struct PROTOCOL_MAP {
+	u16 cmd;
+	const char *name;
+	protocol_func *func;
+};
+
+/* ILITEK I2C R/W APIs */
+static int ilitek_i2c_transfer(struct i2c_client *client,
+			       struct i2c_msg *msgs, int cnt)
+{
+	int ret = 0, retry = 4;
+
+	while (retry--) {
+		ret = i2c_transfer(client->adapter, msgs, cnt);
+		if (ret >= 0)
+			break;
+		mdelay(20);
+	}
+
+	return ret;
+}
+
+static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ilitek_data,
+				     u8 *cmd, int write_len, int delay,
+				     u8 *data, int read_len)
+{
+	int ret = 0;
+	struct i2c_client *client = ilitek_data->client;
+	struct i2c_msg msgs[2] = {
+		{.addr = client->addr, .flags = 0,
+		 .len = write_len, .buf = cmd,},
+		{.addr = client->addr, .flags = I2C_M_RD,
+		 .len = read_len, .buf = data,}
+	};
+
+	if (ilitek_data->ilitek_repeat_start && delay == 0 &&
+		write_len > 0 && read_len > 0) {
+		ret = ilitek_i2c_transfer(client, msgs, 2);
+		if (ret < 0)
+			return ret;
+	} else {
+		if (write_len > 0) {
+			ret = ilitek_i2c_transfer(client, msgs, 1);
+			if (ret < 0)
+				return ret;
+		}
+		if (delay > 0)
+			mdelay(delay);
+		if (read_len > 0) {
+			ret = ilitek_i2c_transfer(client, msgs + 1, 1);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void ilitek_irq_enable(struct ilitek_ts_data *ilitek_data)
+{
+	unsigned long irqflag = 0;
+
+	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
+	if (!ilitek_data->irq_status) {
+		enable_irq(ilitek_data->client->irq);
+		ilitek_data->irq_status = true;
+	}
+	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
+}
+
+static void ilitek_irq_disable(struct ilitek_ts_data *ilitek_data)
+{
+	unsigned long irqflag = 0;
+
+	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
+	if (ilitek_data->irq_status) {
+		disable_irq_nosync(ilitek_data->client->irq);
+		ilitek_data->irq_status = false;
+	}
+	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
+}
+
+/* ILITEK ISR APIs */
+static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
+			    int x, int y)
+{
+	int j = 0;
+	int x_min;
+	int x_max;
+	int y_min;
+	int y_max;
+
+	for (j = 0; j < ilitek_data->keycount; j++) {
+		x_min = ilitek_data->keyinfo[j].x;
+		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
+		y_min = ilitek_data->keyinfo[j].y;
+		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
+
+		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
+			input_report_key(ilitek_data->input_dev,
+					 ilitek_data->keyinfo[j].id, 1);
+
+			ilitek_data->keyinfo[j].status = 1;
+			ilitek_data->touch_key_hold_press = true;
+			ilitek_data->is_touched = true;
+			break;
+		}
+	}
+}
+
+static int ilitek_key_release(struct ilitek_ts_data *ilitek_data,
+			      int x, int y, bool check_point)
+{
+	int j = 0;
+	int x_min;
+	int x_max;
+	int y_min;
+	int y_max;
+
+	for (j = 0; j < ilitek_data->keycount; j++) {
+		x_min = ilitek_data->keyinfo[j].x;
+		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
+		y_min = ilitek_data->keyinfo[j].y;
+		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
+
+		if (ilitek_data->keyinfo[j].status == 1) {
+			if (!check_point ||
+			    (x < x_min || x > x_max ||
+			     y < y_min || y > y_max)) {
+				input_report_key(ilitek_data->input_dev,
+						 ilitek_data->keyinfo[j].id, 0);
+				ilitek_data->keyinfo[j].status = 0;
+				ilitek_data->touch_key_hold_press = false;
+			}
+		}
+	}
+	return 0;
+}
+
+static void ilitek_touch_down(struct ilitek_ts_data *ilitek_data,
+			      int id, int x, int y, int p, int h, int w)
+{
+	struct input_dev *input = ilitek_data->input_dev;
+
+	input_report_key(input, BTN_TOUCH, 1);
+
+	input_mt_slot(input, id);
+	input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
+
+	input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+	input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+
+	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, h);
+	input_event(input, EV_ABS, ABS_MT_WIDTH_MAJOR, w);
+
+	ilitek_data->tpinfo[id].status = 1;
+	ilitek_data->tpinfo[id].id = id;
+	ilitek_data->tpinfo[id].x = x;
+	ilitek_data->tpinfo[id].y = y;
+	ilitek_data->tpinfo[id].p = p;
+	ilitek_data->tpinfo[id].h = h;
+	ilitek_data->tpinfo[id].w = w;
+	ilitek_data->is_touched = true;
+}
+
+static void ilitek_touch_release(struct ilitek_ts_data *ilitek_data, int id)
+{
+	struct input_dev *input = ilitek_data->input_dev;
+
+	if (ilitek_data->tpinfo[id].status == 1) {
+		input_mt_slot(input, id);
+		input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
+		ilitek_data->tpinfo[id].status = 0;
+	}
+}
+
+static void ilitek_touch_release_all(struct ilitek_ts_data *ilitek_data)
+{
+	struct input_dev *input = ilitek_data->input_dev;
+	int i = 0;
+
+	for (i = 0; i < ilitek_data->max_tp; i++)
+		ilitek_touch_release(ilitek_data, i);
+	input_report_key(input, BTN_TOUCH, 0);
+	input_sync(input);
+
+	ilitek_data->is_touched = false;
+}
+
+static int ilitek_process_and_report_v6(struct ilitek_ts_data *ilitek_data)
+{
+	int ret = 0;
+	u8 buf[512] = {0};
+	int packet_len = 5;
+	int packet_max_point = 10;
+	int report_max_point = 6;
+	int release_point = 0;
+	int i = 0, count = 0;
+	struct input_dev *input = ilitek_data->input_dev;
+	struct device *dev = &ilitek_data->client->dev;
+	u16 x, y, p = 10, w = 10, h = 10, status, id;
+
+	ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0, buf, 64);
+	if (ret < 0) {
+		dev_err(dev, "get touch info failed, err:%d\n", ret);
+		goto err_release_touch_and_key;
+	}
+
+	report_max_point = buf[REPORT_ADDRESS_COUNT];
+	if (report_max_point > ilitek_data->max_tp) {
+		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
+			report_max_point, ilitek_data->max_tp);
+		return -EINVAL;
+	}
+
+	count = CEIL(report_max_point, packet_max_point);
+	for (i = 1; i < count; i++) {
+		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
+			buf + i * 64, 64);
+		if (ret < 0) {
+			dev_err(dev, "get touch info. err, count=%d\n", count);
+			goto err_release_touch_and_key;
+		}
+	}
+
+	for (i = 0; i < report_max_point; i++) {
+		status = buf[i * packet_len + 1] & 0x40;
+		id = buf[i * packet_len + 1] & 0x3F;
+
+		if (!status) {
+			release_point++;
+			ilitek_touch_release(ilitek_data, id);
+			continue;
+		}
+
+		x = (buf[i * packet_len + 3] << 8) + buf[i * packet_len + 2];
+		y = (buf[i * packet_len + 5] << 8) + buf[i * packet_len + 4];
+
+		if (!ilitek_data->is_touched)
+			ilitek_key_down(ilitek_data, x, y);
+
+		if (!ilitek_data->touch_key_hold_press) {
+			if (x > ilitek_data->screen_max_x ||
+			    y > ilitek_data->screen_max_y ||
+			    x < ilitek_data->screen_min_x ||
+			    y < ilitek_data->screen_min_y)
+				continue;
+
+			ilitek_touch_down(ilitek_data, id, x, y, p, h, w);
+		} else {
+			ilitek_key_release(ilitek_data, x, y, 1);
+		}
+	}
+
+	if (release_point == report_max_point) {
+		ilitek_touch_release_all(ilitek_data);
+		ilitek_key_release(ilitek_data, 0, 0, 0);
+		ilitek_data->is_touched = false;
+	}
+	input_sync(input);
+	return 0;
+
+err_release_touch_and_key:
+	ilitek_touch_release_all(ilitek_data);
+	ilitek_key_release(ilitek_data, 0, 0, 0);
+	ilitek_data->is_touched = false;
+	input_sync(input);
+	return ret;
+}
+
+
+/* APIs of cmds for ILITEK Touch IC */
+static int api_protocol_set_cmd(struct ilitek_ts_data *ilitek_data,
+				u16 idx, u8 *inbuf, u8 *outbuf)
+{
+	u16 cmd;
+	int ret = 0;
+
+	if (!ilitek_data || !ilitek_data->ptl_cb_func)
+		return -EINVAL;
+
+	if (idx >= MAX_CMD_CNT)
+		return -EINVAL;
+
+	cmd = ilitek_data->ptl_cb_func[idx].cmd;
+	ret = ilitek_data->ptl_cb_func[idx].func(ilitek_data, cmd,
+						 inbuf, outbuf);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int api_protocol_get_ptl_ver(struct ilitek_ts_data *ilitek_data,
+				    u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = cmd;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 3);
+	if (ret < 0)
+		return ret;
+
+	ilitek_data->ptl.ver = (outbuf[0] << 16) + (outbuf[1] << 8);
+	ilitek_data->ptl.ver_major = outbuf[0];
+	ilitek_data->ptl.ver_mid = outbuf[1];
+	ilitek_data->ptl.ver_minor = outbuf[2];
+
+	return ret;
+}
+
+static int api_protocol_get_mcu_ver(struct ilitek_ts_data *ilitek_data,
+				    u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_MCU_VER;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 32);
+	if (ret < 0)
+		return ret;
+
+	memcpy(ilitek_data->mcu_ver, outbuf, 2);
+	memset(ilitek_data->product_id, 0, sizeof(ilitek_data->product_id));
+	memcpy(ilitek_data->product_id, outbuf+6, 26);
+
+	return ret;
+}
+
+static int api_protocol_get_fw_ver(struct ilitek_ts_data *ilitek_data,
+				   u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_FW_VER;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
+	if (ret < 0)
+		return ret;
+
+	memcpy(ilitek_data->firmware_ver, outbuf, 8);
+
+	return ret;
+}
+
+static int api_protocol_get_scrn_res(struct ilitek_ts_data *ilitek_data,
+				     u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_SCRN_RES;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
+	if (ret < 0)
+		return ret;
+
+	ilitek_data->screen_min_x = (outbuf[1] << 8) + outbuf[0];
+	ilitek_data->screen_min_y = (outbuf[3] << 8) + outbuf[2];
+	ilitek_data->screen_max_x = (outbuf[5] << 8) + outbuf[4];
+	ilitek_data->screen_max_y = (outbuf[7] << 8) + outbuf[6];
+
+	return ret;
+}
+
+static int api_protocol_get_key_info_v6(struct ilitek_ts_data *ilitek_data,
+					u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0, i = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_KEY_INFO;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 10,
+					outbuf, ilitek_data->keycount * 5 + 5);
+	if (ret < 0)
+		return ret;
+
+	ilitek_data->key_xlen = (outbuf[2] << 8) + outbuf[1];
+	ilitek_data->key_ylen = (outbuf[4] << 8) + outbuf[3];
+
+	for (i = 0; i < ilitek_data->keycount; i++) {
+		ilitek_data->keyinfo[i].id = outbuf[i * 5 + 5];
+		ilitek_data->keyinfo[i].x = (outbuf[i * 5 + 7] << 8) +
+					    outbuf[i * 5 + 6];
+		ilitek_data->keyinfo[i].y = (outbuf[i * 5 + 9] << 8) +
+					    outbuf[i * 5 + 8];
+		ilitek_data->keyinfo[i].status = 0;
+	}
+
+	return ret;
+}
+
+static int api_protocol_get_tp_res_v6(struct ilitek_ts_data *ilitek_data,
+				      u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_TP_RES;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 15);
+	if (ret < 0)
+		return ret;
+
+	ilitek_data->tp_max_x = (outbuf[1] << 8) + outbuf[0];
+	ilitek_data->tp_max_y = (outbuf[3] << 8) + outbuf[2];
+	ilitek_data->x_ch = (outbuf[5] << 8) + outbuf[4];
+	ilitek_data->y_ch = (outbuf[7] << 8) + outbuf[6];
+	ilitek_data->max_tp = outbuf[8];
+	ilitek_data->keycount = outbuf[9];
+	ilitek_data->slave_count = outbuf[10];
+	ilitek_data->format = outbuf[12];
+
+	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT) {
+		dev_err(&ilitek_data->client->dev, "Invalid KEY count:%d from FW\n",
+			ilitek_data->keycount);
+		return -EINVAL;
+	}
+
+	if (ilitek_data->keycount > 0) {
+		ret = api_protocol_set_cmd(ilitek_data, GET_KEY_INFO,
+					   NULL, outbuf);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (ilitek_data->max_tp > ILITEK_SUPPORT_MAX_POINT) {
+		dev_err(&ilitek_data->client->dev, "Invalid MAX_TP:%d from FW\n",
+			ilitek_data->max_tp);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int api_protocol_get_ic_mode(struct ilitek_ts_data *ilitek_data,
+				    u16 cmd, u8 *inbuf, u8 *outbuf)
+{
+	int ret = 0;
+	u8 buf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	buf[0] = ILITEK_TP_CMD_GET_IC_MODE;
+	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 2);
+	if (ret < 0)
+		return ret;
+
+	ilitek_data->ic_mode = outbuf[0];
+	return ret;
+}
+
+struct PROTOCOL_MAP ptl_func_map_v3[] = {
+	{ /* V3 cmds */},
+};
+
+struct PROTOCOL_MAP ptl_func_map_v6[] = {
+	[GET_PTL_VER]	= {ILITEK_TP_CMD_GET_PRL_VER, "GET_PTL_VER",
+			   api_protocol_get_ptl_ver},
+	[GET_FW_VER]	= {ILITEK_TP_CMD_GET_FW_VER, "GET_FW_VER",
+			   api_protocol_get_fw_ver},
+	[GET_SCRN_RES]	= {ILITEK_TP_CMD_GET_SCRN_RES, "GET_SCRN_RES",
+			   api_protocol_get_scrn_res},
+	[GET_TP_RES]	= {ILITEK_TP_CMD_GET_TP_RES, "GET_TP_RES",
+			   api_protocol_get_tp_res_v6},
+	[GET_IC_MODE]	= {ILITEK_TP_CMD_GET_IC_MODE, "GET_IC_MODE",
+			   api_protocol_get_ic_mode},
+	[GET_MCU_VER]	= {ILITEK_TP_CMD_GET_MCU_VER, "GET_MOD_NAME",
+			   api_protocol_get_mcu_ver},
+	[GET_KEY_INFO]	= {ILITEK_TP_CMD_GET_KEY_INFO, "GET_KEY_INFO",
+			   api_protocol_get_key_info_v6},
+};
+
+/* Probe APIs */
+static int ilitek_request_gpio(struct device *dev,
+			       struct ilitek_ts_data *ilitek_data)
+{
+	int ret = 0;
+
+	if (!dev || !ilitek_data)
+		return -EINVAL;
+
+	ilitek_data->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ilitek_data->reset_gpio)) {
+		ret = PTR_ERR(ilitek_data->reset_gpio);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ilitek_reset(struct ilitek_ts_data *ilitek_data,
+			 int delay, bool boot)
+{
+	bool init_stats = ilitek_data->irq_status;
+
+	if (!boot)
+		ilitek_irq_disable(ilitek_data);
+
+	if (ilitek_data->reset_gpio) {
+		gpiod_set_value(ilitek_data->reset_gpio, 1);
+		mdelay(10);
+
+		gpiod_set_value(ilitek_data->reset_gpio, 0);
+		mdelay(10);
+
+		gpiod_set_value(ilitek_data->reset_gpio, 1);
+		mdelay(delay);
+	}
+
+	if (!boot && init_stats)
+		ilitek_irq_enable(ilitek_data);
+}
+
+static int ilitek_protocol_init(struct ilitek_ts_data *ilitek_data)
+{
+	int ret = 0;
+	u8 outbuf[64] = {0};
+
+	if (!ilitek_data)
+		return -EINVAL;
+
+	ilitek_data->ptl_cb_func = ptl_func_map_v6;
+
+	ret = api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf);
+	if (ret < 0)
+		return ret;
+
+	if (ilitek_data->ptl.ver_major == 0x3 ||
+	    ilitek_data->ptl.ver == BL_V1_6 ||
+	    ilitek_data->ptl.ver == BL_V1_7) {
+		ilitek_data->process_and_report = NULL;
+		ilitek_data->ptl_cb_func = NULL;
+		ilitek_data->reset_time = 200;
+	} else {
+		ilitek_data->process_and_report = ilitek_process_and_report_v6;
+		ilitek_data->ptl_cb_func = ptl_func_map_v6;
+		ilitek_data->reset_time = 600;
+	}
+
+	return 0;
+}
+
+static int ilitek_read_tp_info(struct ilitek_ts_data *ilitek_data, bool boot)
+{
+	u8 outbuf[256] = {0};
+
+	if (api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf) < 0)
+		return -EFAULT;
+	if (api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf) < 0)
+		return -EFAULT;
+	if (api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf) < 0)
+		return -EFAULT;
+	if (boot && api_protocol_set_cmd(ilitek_data, GET_SCRN_RES, NULL,
+					 outbuf) < 0)
+		return -EFAULT;
+	if (api_protocol_set_cmd(ilitek_data, GET_TP_RES, NULL, outbuf) < 0)
+		return -EFAULT;
+	if (api_protocol_set_cmd(ilitek_data, GET_IC_MODE, NULL, outbuf) < 0)
+		return -EFAULT;
+
+	return 0;
+}
+
+static int ilitek_input_dev_init(struct device *dev,
+				 struct ilitek_ts_data *ilitek_data)
+{
+	int ret = 0, i = 0;
+	struct input_dev *input = NULL;
+
+	if (!dev || !ilitek_data)
+		return -EINVAL;
+
+	input  = input_allocate_device();
+	if (!input)
+		return -ENOMEM;
+
+	input->name = ILITEK_TS_NAME;
+	input->id.bustype = BUS_I2C;
+
+	__set_bit(INPUT_PROP_DIRECT, input->propbit);
+	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	input_set_abs_params(input, ABS_MT_POSITION_X,
+			     ilitek_data->screen_min_x,
+			     ilitek_data->screen_max_x, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_Y,
+			     ilitek_data->screen_min_y,
+			     ilitek_data->screen_max_y, 0, 0);
+	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+	input_set_abs_params(input, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
+
+	input_mt_init_slots(input, ilitek_data->max_tp, INPUT_MT_DIRECT);
+
+	for (i = 0; i < ilitek_data->keycount; i++)
+		set_bit(ilitek_data->keyinfo[i].id & KEY_MAX, input->keybit);
+
+	ret = input_register_device(input);
+	if (ret)
+		dev_err(dev, "failed to register input device, err:%d\n", ret);
+
+	ilitek_data->input_dev = input;
+	return ret;
+}
+
+static irqreturn_t ilitek_i2c_isr(int irq, void *dev_id)
+{
+	struct ilitek_ts_data *ilitek_data = dev_id;
+	int ret = 0;
+
+	if (!ilitek_data->process_and_report) {
+		dev_err(&ilitek_data->client->dev, "report touch cb is NULL\n");
+		return IRQ_NONE;
+	}
+
+	ilitek_irq_disable(ilitek_data);
+	mutex_lock(&ilitek_data->ilitek_mutex);
+	ret = ilitek_data->process_and_report(ilitek_data);
+	mutex_unlock(&ilitek_data->ilitek_mutex);
+	ilitek_irq_enable(ilitek_data);
+
+	if (ret < 0) {
+		dev_err(&ilitek_data->client->dev, "process and report point failed, err:%d\n",
+			ret);
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ilitek_request_irq(struct device *dev,
+			      struct ilitek_ts_data *ilitek_data)
+{
+	int ret = 0;
+
+	spin_lock_init(&ilitek_data->irq_lock);
+	ilitek_data->irq_status = true;
+
+	ret = request_threaded_irq(ilitek_data->client->irq, NULL,
+				   ilitek_i2c_isr,
+				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				   "ilitek_touch_irq", ilitek_data);
+	if (ret)
+		dev_err(dev, "request threaded irq failed, err:%d\n", ret);
+
+	return ret;
+}
+
+static ssize_t firmware_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
+	int ret = 0;
+	u8 outbuf[64] = {0};
+
+	ilitek_irq_disable(ilitek_data);
+	mutex_lock(&ilitek_data->ilitek_mutex);
+	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);
+	mutex_unlock(&ilitek_data->ilitek_mutex);
+	ilitek_irq_enable(ilitek_data);
+
+	if (ret < 0) {
+		dev_err(dev, "api set cmd failed, err:%d\n", ret);
+		return ret;
+	}
+
+	return scnprintf(buf, PAGE_SIZE,
+			 "ILITEK FW ver: %d.%d.%d.%d.%d.%d.%d.%d\n",
+			 ilitek_data->firmware_ver[0],
+			 ilitek_data->firmware_ver[1],
+			 ilitek_data->firmware_ver[2],
+			 ilitek_data->firmware_ver[3],
+			 ilitek_data->firmware_ver[4],
+			 ilitek_data->firmware_ver[5],
+			 ilitek_data->firmware_ver[6],
+			 ilitek_data->firmware_ver[7]);
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+static ssize_t product_id_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
+	int ret = 0;
+	u8 outbuf[64] = {0};
+
+	ilitek_irq_disable(ilitek_data);
+	mutex_lock(&ilitek_data->ilitek_mutex);
+	ret = api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf);
+	mutex_unlock(&ilitek_data->ilitek_mutex);
+	ilitek_irq_enable(ilitek_data);
+
+	if (ret < 0) {
+		dev_err(dev, "api set cmd failed, err:%d\n", ret);
+		return ret;
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "MCU: %s\n", ilitek_data->product_id);
+}
+static DEVICE_ATTR_RO(product_id);
+
+static struct attribute *ilitek_sysfs_attrs[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_product_id.attr,
+	NULL
+};
+
+static struct attribute_group ilitek_attrs_group[] = {
+	{.attrs = ilitek_sysfs_attrs},
+};
+
+static int ilitek_create_sysfsnode(struct device *dev)
+{
+	int ret = 0;
+
+	ret = sysfs_create_group(&dev->kobj, ilitek_attrs_group);
+	if (ret)
+		dev_err(dev, "sysfs create group failed, err:%d\n", ret);
+
+	return ret;
+}
+
+static int ilitek_ts_i2c_probe(struct i2c_client *client,
+			       const struct i2c_device_id *id)
+{
+	struct ilitek_ts_data *ilitek_data = NULL;
+	struct device *dev = &client->dev;
+	int ret = 0;
+
+	ilitek_data = kzalloc(sizeof(*ilitek_data), GFP_KERNEL);
+	if (!ilitek_data)
+		return -ENOMEM;
+
+	ilitek_data->client = client;
+	i2c_set_clientdata(client, ilitek_data);
+
+	mutex_init(&ilitek_data->ilitek_mutex);
+
+	ret = ilitek_request_gpio(dev, ilitek_data);
+	if (ret < 0) {
+		dev_err(dev, "request gpio failed, err:%d", ret);
+		goto err_put_gpiod;
+	}
+
+	ilitek_reset(ilitek_data, 1000, true);
+	ret = ilitek_protocol_init(ilitek_data);
+	if (ret < 0) {
+		dev_err(dev, "protocol init failed, err:%d", ret);
+		goto err_put_gpiod;
+	}
+
+	ret = ilitek_read_tp_info(ilitek_data, true);
+	if (ret < 0) {
+		dev_err(dev, "read tp info failed, err:%d", ret);
+		goto err_put_gpiod;
+	}
+
+	ret = ilitek_input_dev_init(dev, ilitek_data);
+	if (ret < 0) {
+		dev_err(dev, "input dev init failed, err:%d", ret);
+		goto err_free_input_dev;
+	}
+
+	ret = ilitek_request_irq(dev, ilitek_data);
+	if (ret < 0) {
+		dev_err(dev, "request irq failed, err:%d", ret);
+		goto err_free_input_dev;
+	}
+
+	ret = ilitek_create_sysfsnode(dev);
+	if (ret < 0) {
+		dev_err(dev, "create sysfs node failed, err:%d", ret);
+		goto err_free_irq;
+	}
+
+	return 0;
+
+err_free_irq:
+	free_irq(ilitek_data->client->irq, ilitek_data);
+
+	input_unregister_device(ilitek_data->input_dev);
+err_free_input_dev:
+	if (ilitek_data->input_dev)
+		input_free_device(ilitek_data->input_dev);
+err_put_gpiod:
+	if (!IS_ERR(ilitek_data->reset_gpio))
+		gpiod_put(ilitek_data->reset_gpio);
+	kfree(ilitek_data);
+	return ret;
+}
+
+static int ilitek_ts_i2c_remove(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
+
+	sysfs_remove_group(&dev->kobj, ilitek_attrs_group);
+
+	free_irq(ilitek_data->client->irq, ilitek_data);
+
+	input_unregister_device(ilitek_data->input_dev);
+
+	if (!IS_ERR(ilitek_data->reset_gpio))
+		gpiod_put(ilitek_data->reset_gpio);
+
+	kfree(ilitek_data);
+
+	return 0;
+}
+
+static const struct i2c_device_id ilitek_ts_i2c_id[] = {
+	{ILITEK_TS_NAME, 0},
+	{ /* not omitted */ },
+};
+MODULE_DEVICE_TABLE(i2c, ilitek_ts_i2c_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ilitekts_acpi_id[] = {
+	{ "ILTK0001", 0 },
+	{ /* not omitted */ },
+};
+MODULE_DEVICE_TABLE(acpi, ilitekts_acpi_id);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id ilitek_ts_i2c_match[] = {
+	{.compatible = "ilitek,ili2520",},
+	{.compatible = "ilitek,ili2510",},
+	{ /* not omitted */ },
+};
+MODULE_DEVICE_TABLE(of, ilitek_ts_i2c_match);
+#endif
+
+static struct i2c_driver ilitek_ts_i2c_driver = {
+	.driver = {
+		.name = ILITEK_TS_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(ilitek_ts_i2c_match),
+		.acpi_match_table = ACPI_PTR(ilitekts_acpi_id),
+	},
+	.probe = ilitek_ts_i2c_probe,
+	.remove = ilitek_ts_i2c_remove,
+	.id_table = ilitek_ts_i2c_id,
+};
+
+module_i2c_driver(ilitek_ts_i2c_driver);
+
+MODULE_AUTHOR("ILITEK");
+MODULE_DESCRIPTION("ILITEK I2C Touchscreen Driver");
+MODULE_LICENSE("GPL");
--
2.25.1

Re: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Dmitry Torokhov]

Hi Joe,

On Thu, Feb 18, 2021 at 02:44:35PM +0800, Joe Hung wrote:
> Add support for ILITEK Lego series of touch devices.
> Lego series includes ILITEK 213X/2316/232X/252X.
> Designed in compatible way for ILITEK 23XX/251X ICs.
> 
> Tested and passed with evaluation board with ILI2520 IC.

Thank you for your submission.

> 
> Signed-off-by: Joe Hung <joe_hung@ilitek.com>
> ---
> Changes in v3:
>   - None
> 
> Changes in v2:
>   - Remove irq-gpio and related flow
> 
>  drivers/input/touchscreen/Kconfig         |   12 +
>  drivers/input/touchscreen/Makefile        |    1 +
>  drivers/input/touchscreen/ilitek_ts_i2c.c | 1010 +++++++++++++++++++++
>  3 files changed, 1023 insertions(+)
>  create mode 100644 drivers/input/touchscreen/ilitek_ts_i2c.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index f012fe746df0..9dea7fd0d94f 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zinitix.
> 
> +config TOUCHSCREEN_ILITEK
> +	tristate "Ilitek I2C 23XX/25XX/Lego Series Touch ICs"
> +	depends on I2C
> +	help
> +	  Say Y here if you have touchscreen with ILITEK touch IC,
> +	  it supports 23XX/25XX/other Lego series.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ilitek_ts_i2c.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 6233541e9173..6749bdc42f41 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
>  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_ILITEK)        += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> new file mode 100644
> index 000000000000..72cb7c82170c
> --- /dev/null
> +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> @@ -0,0 +1,1010 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ILITEK Touch IC driver for 23XX, 25XX and Lego series
> + *
> + * Copyright (C) 2011 ILI Technology Corporation.
> + * Copyright (C) 2020 Luca Hsu <luca_hsu@ilitek.com>
> + * Copyright (C) 2021 Joe Hung <joe_hung@ilitek.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>

Are you sure you need this include?

> +#include <linux/sched.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/cdev.h>

You do not create character devices in this driver.

> +#include <linux/uaccess.h>
> +#include <linux/version.h>

Why do you need this include?

> +#include <linux/rtc.h>

I do not think this driver uses rtc.

> +#include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/proc_fs.h>
> +#include <linux/fs.h>
> +#include <linux/pm_runtime.h>

I do not think your driver uses runtime PM.

> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/acpi.h>

Please double-check that you include only facilities that the driver
actually uses.

> +
> +#define ILITEK_TS_NAME					"ilitek_ts"
> +#define BL_V1_8						(0x10800)
> +#define BL_V1_7						(0x10700)
> +#define BL_V1_6						(0x10600)
> +
> +#define ILITEK_TP_CMD_GET_TP_RES			(0x20)
> +#define ILITEK_TP_CMD_GET_SCRN_RES			(0x21)
> +#define ILITEK_TP_CMD_GET_KEY_INFO			(0x22)
> +#define ILITEK_TP_CMD_GET_FW_VER			(0x40)
> +#define ILITEK_TP_CMD_GET_PRL_VER			(0x42)
> +#define ILITEK_TP_CMD_GET_MCU_VER			(0x61)
> +#define ILITEK_TP_CMD_GET_IC_MODE			(0xC0)
> +
> +#define REPORT_ADDRESS_COUNT				(61)
> +#define ILITEK_SUPPORT_MAX_POINT			(40)
> +#define ILITEK_SUPPORT_MAX_KEY_CNT			(50)

No need to use parenthesis for simple scalar values.

> +
> +#define CEIL(n, d)	(((n) % (d)) ? ((n) / (d) + 1) : ((n) / (d)))

I think you want standard DIV_ROUND_UP().

> +
> +struct ilitek_protocol_info {

Let's use ili23xx instead of ilitek prefix throughout the file. Or
whatever the first generation of the product supported by this driver.

> +	s32 ver;
> +	u8 ver_major;
> +	u8 ver_mid;
> +	u8 ver_minor;
> +};
> +
> +struct ilitek_key_info {
> +	s32 id;
> +	s32 x;
> +	s32 y;
> +	s32 status;
> +};
> +
> +struct ilitek_touch_info {
> +	u16 id;
> +	u16 x;
> +	u16 y;
> +	u16 p;
> +	u16 w;
> +	u16 h;
> +	u16 status;
> +};
> +
> +struct ilitek_ts_data {
> +	struct i2c_client		*client;
> +	struct gpio_desc		*reset_gpio;
> +	struct input_dev		*input_dev;
> +
> +	struct mutex			ilitek_mutex;
> +	spinlock_t			irq_lock;
> +	bool				irq_status;
> +	int (*process_and_report)(struct ilitek_ts_data *ilitek_data);
> +
> +	struct PROTOCOL_MAP		*ptl_cb_func;
> +	struct ilitek_protocol_info	ptl;
> +
> +
> +	struct ilitek_key_info		keyinfo[ILITEK_SUPPORT_MAX_KEY_CNT];
> +	struct ilitek_touch_info	tpinfo[ILITEK_SUPPORT_MAX_POINT];
> +
> +	bool				is_touched;
> +	bool				touch_key_hold_press;
> +
> +	bool				ilitek_repeat_start;
> +
> +	char				product_id[30];
> +	u8				mcu_ver[2];
> +	u8				ic_mode;
> +	u8				firmware_ver[8];
> +
> +	s32				reset_time;
> +	s32				tp_max_x;
> +	s32				tp_max_y;
> +	s32				screen_max_x;
> +	s32				screen_max_y;
> +	s32				screen_min_x;
> +	s32				screen_min_y;
> +	s32				max_tp;
> +	s32				x_ch;
> +	s32				y_ch;
> +	s32				slave_count;

What is the importance of this?

> +	s32				format;
> +	s32				keycount;
> +	s32				key_xlen;
> +	s32				key_ylen;
> +};
> +
> +enum ilitek_cmds_v6 {
> +	GET_PTL_VER = 0,
> +	GET_FW_VER,
> +	GET_SCRN_RES,
> +	GET_TP_RES,
> +	GET_IC_MODE,
> +	GET_MCU_VER,
> +	GET_KEY_INFO,
> +
> +	MAX_CMD_CNT	/* ALWAYS keep at the end */
> +};
> +
> +typedef int protocol_func(struct ilitek_ts_data *ilitek_data,
> +			  u16 cmd, uint8_t *inbuf, uint8_t *outbuf);

In kernel please use u8 instead of uint8_t, etc.

> +
> +struct PROTOCOL_MAP {
> +	u16 cmd;
> +	const char *name;
> +	protocol_func *func;
> +};
> +
> +/* ILITEK I2C R/W APIs */
> +static int ilitek_i2c_transfer(struct i2c_client *client,
> +			       struct i2c_msg *msgs, int cnt)
> +{
> +	int ret = 0, retry = 4;
> +
> +	while (retry--) {
> +		ret = i2c_transfer(client->adapter, msgs, cnt);
> +		if (ret >= 0)
> +			break;
> +		mdelay(20);
> +	}

Why does it this device need retries?

> +
> +	return ret;
> +}
> +
> +static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ilitek_data,
> +				     u8 *cmd, int write_len, int delay,
> +				     u8 *data, int read_len)
> +{
> +	int ret = 0;
> +	struct i2c_client *client = ilitek_data->client;
> +	struct i2c_msg msgs[2] = {
> +		{.addr = client->addr, .flags = 0,
> +		 .len = write_len, .buf = cmd,},
> +		{.addr = client->addr, .flags = I2C_M_RD,
> +		 .len = read_len, .buf = data,}
> +	};
> +
> +	if (ilitek_data->ilitek_repeat_start && delay == 0 &&

Where is this being set? As far as I understand, repeated start is
property of the i2c controller, not peripheral device.

Also, please see if you can switch to using regmap, as it should take
care of selecting right mode, depending on controller's capabilities.

> +		write_len > 0 && read_len > 0) {
> +		ret = ilitek_i2c_transfer(client, msgs, 2);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		if (write_len > 0) {
> +			ret = ilitek_i2c_transfer(client, msgs, 1);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		if (delay > 0)
> +			mdelay(delay);
> +		if (read_len > 0) {
> +			ret = ilitek_i2c_transfer(client, msgs + 1, 1);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ilitek_irq_enable(struct ilitek_ts_data *ilitek_data)
> +{
> +	unsigned long irqflag = 0;
> +
> +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> +	if (!ilitek_data->irq_status) {
> +		enable_irq(ilitek_data->client->irq);
> +		ilitek_data->irq_status = true;
> +	}
> +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);

The driver should know at any time whether interrupts are disabled or
not, so I'd prefer if we did not have this wrapper, and the one below.

> +}
> +
> +static void ilitek_irq_disable(struct ilitek_ts_data *ilitek_data)
> +{
> +	unsigned long irqflag = 0;
> +
> +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> +	if (ilitek_data->irq_status) {
> +		disable_irq_nosync(ilitek_data->client->irq);
> +		ilitek_data->irq_status = false;
> +	}
> +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
> +}
> +
> +/* ILITEK ISR APIs */
> +static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
> +			    int x, int y)
> +{
> +	int j = 0;
> +	int x_min;
> +	int x_max;
> +	int y_min;
> +	int y_max;
> +
> +	for (j = 0; j < ilitek_data->keycount; j++) {
> +		x_min = ilitek_data->keyinfo[j].x;
> +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> +		y_min = ilitek_data->keyinfo[j].y;
> +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> +
> +		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
> +			input_report_key(ilitek_data->input_dev,
> +					 ilitek_data->keyinfo[j].id, 1);
> +
> +			ilitek_data->keyinfo[j].status = 1;
> +			ilitek_data->touch_key_hold_press = true;
> +			ilitek_data->is_touched = true;
> +			break;
> +		}
> +	}
> +}

This sounds like handling of "soft keys" which is better left for
userspace. OTOH the Linux keycode seems to be coming from controller
firmware. This is really weird.

> +
> +static int ilitek_key_release(struct ilitek_ts_data *ilitek_data,
> +			      int x, int y, bool check_point)
> +{
> +	int j = 0;
> +	int x_min;
> +	int x_max;
> +	int y_min;
> +	int y_max;
> +
> +	for (j = 0; j < ilitek_data->keycount; j++) {
> +		x_min = ilitek_data->keyinfo[j].x;
> +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> +		y_min = ilitek_data->keyinfo[j].y;
> +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> +
> +		if (ilitek_data->keyinfo[j].status == 1) {
> +			if (!check_point ||
> +			    (x < x_min || x > x_max ||
> +			     y < y_min || y > y_max)) {
> +				input_report_key(ilitek_data->input_dev,
> +						 ilitek_data->keyinfo[j].id, 0);
> +				ilitek_data->keyinfo[j].status = 0;
> +				ilitek_data->touch_key_hold_press = false;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void ilitek_touch_down(struct ilitek_ts_data *ilitek_data,
> +			      int id, int x, int y, int p, int h, int w)
> +{
> +	struct input_dev *input = ilitek_data->input_dev;
> +
> +	input_report_key(input, BTN_TOUCH, 1);
> +
> +	input_mt_slot(input, id);
> +	input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
> +
> +	input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> +	input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> +
> +	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, h);
> +	input_event(input, EV_ABS, ABS_MT_WIDTH_MAJOR, w);
> +
> +	ilitek_data->tpinfo[id].status = 1;
> +	ilitek_data->tpinfo[id].id = id;
> +	ilitek_data->tpinfo[id].x = x;
> +	ilitek_data->tpinfo[id].y = y;
> +	ilitek_data->tpinfo[id].p = p;
> +	ilitek_data->tpinfo[id].h = h;
> +	ilitek_data->tpinfo[id].w = w;
> +	ilitek_data->is_touched = true;
> +}
> +
> +static void ilitek_touch_release(struct ilitek_ts_data *ilitek_data, int id)
> +{
> +	struct input_dev *input = ilitek_data->input_dev;
> +
> +	if (ilitek_data->tpinfo[id].status == 1) {
> +		input_mt_slot(input, id);
> +		input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> +		ilitek_data->tpinfo[id].status = 0;
> +	}
> +}
> +
> +static void ilitek_touch_release_all(struct ilitek_ts_data *ilitek_data)
> +{
> +	struct input_dev *input = ilitek_data->input_dev;
> +	int i = 0;
> +
> +	for (i = 0; i < ilitek_data->max_tp; i++)
> +		ilitek_touch_release(ilitek_data, i);
> +	input_report_key(input, BTN_TOUCH, 0);
> +	input_sync(input);
> +
> +	ilitek_data->is_touched = false;
> +}
> +
> +static int ilitek_process_and_report_v6(struct ilitek_ts_data *ilitek_data)
> +{
> +	int ret = 0;
> +	u8 buf[512] = {0};
> +	int packet_len = 5;
> +	int packet_max_point = 10;
> +	int report_max_point = 6;
> +	int release_point = 0;
> +	int i = 0, count = 0;
> +	struct input_dev *input = ilitek_data->input_dev;
> +	struct device *dev = &ilitek_data->client->dev;
> +	u16 x, y, p = 10, w = 10, h = 10, status, id;
> +
> +	ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0, buf, 64);
> +	if (ret < 0) {
> +		dev_err(dev, "get touch info failed, err:%d\n", ret);
> +		goto err_release_touch_and_key;
> +	}
> +
> +	report_max_point = buf[REPORT_ADDRESS_COUNT];
> +	if (report_max_point > ilitek_data->max_tp) {
> +		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
> +			report_max_point, ilitek_data->max_tp);
> +		return -EINVAL;
> +	}
> +
> +	count = CEIL(report_max_point, packet_max_point);
> +	for (i = 1; i < count; i++) {
> +		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
> +			buf + i * 64, 64);
> +		if (ret < 0) {
> +			dev_err(dev, "get touch info. err, count=%d\n", count);
> +			goto err_release_touch_and_key;
> +		}
> +	}

This means you have 1 + count transactions on i2c bus. Have you
considered reading max number of bytes in one transaction, and then soft
out how much of received data is valid?

> +
> +	for (i = 0; i < report_max_point; i++) {
> +		status = buf[i * packet_len + 1] & 0x40;
> +		id = buf[i * packet_len + 1] & 0x3F;
> +
> +		if (!status) {
> +			release_point++;
> +			ilitek_touch_release(ilitek_data, id);
> +			continue;
> +		}
> +
> +		x = (buf[i * packet_len + 3] << 8) + buf[i * packet_len + 2];
> +		y = (buf[i * packet_len + 5] << 8) + buf[i * packet_len + 4];

		x = le16_to_cpup(....);

Please adjust here and elsewhere. Also, if data may be unaligned, please
use get_unaligned_le16() instead.

> +
> +		if (!ilitek_data->is_touched)
> +			ilitek_key_down(ilitek_data, x, y);
> +
> +		if (!ilitek_data->touch_key_hold_press) {
> +			if (x > ilitek_data->screen_max_x ||
> +			    y > ilitek_data->screen_max_y ||
> +			    x < ilitek_data->screen_min_x ||
> +			    y < ilitek_data->screen_min_y)
> +				continue;
> +
> +			ilitek_touch_down(ilitek_data, id, x, y, p, h, w);
> +		} else {
> +			ilitek_key_release(ilitek_data, x, y, 1);
> +		}
> +	}
> +
> +	if (release_point == report_max_point) {
> +		ilitek_touch_release_all(ilitek_data);
> +		ilitek_key_release(ilitek_data, 0, 0, 0);
> +		ilitek_data->is_touched = false;
> +	}
> +	input_sync(input);
> +	return 0;
> +
> +err_release_touch_and_key:
> +	ilitek_touch_release_all(ilitek_data);
> +	ilitek_key_release(ilitek_data, 0, 0, 0);
> +	ilitek_data->is_touched = false;
> +	input_sync(input);
> +	return ret;
> +}
> +
> +
> +/* APIs of cmds for ILITEK Touch IC */
> +static int api_protocol_set_cmd(struct ilitek_ts_data *ilitek_data,
> +				u16 idx, u8 *inbuf, u8 *outbuf)
> +{
> +	u16 cmd;
> +	int ret = 0;
> +
> +	if (!ilitek_data || !ilitek_data->ptl_cb_func)
> +		return -EINVAL;
> +
> +	if (idx >= MAX_CMD_CNT)
> +		return -EINVAL;
> +
> +	cmd = ilitek_data->ptl_cb_func[idx].cmd;
> +	ret = ilitek_data->ptl_cb_func[idx].func(ilitek_data, cmd,
> +						 inbuf, outbuf);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int api_protocol_get_ptl_ver(struct ilitek_ts_data *ilitek_data,
> +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = cmd;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 3);
> +	if (ret < 0)
> +		return ret;
> +
> +	ilitek_data->ptl.ver = (outbuf[0] << 16) + (outbuf[1] << 8);
> +	ilitek_data->ptl.ver_major = outbuf[0];
> +	ilitek_data->ptl.ver_mid = outbuf[1];
> +	ilitek_data->ptl.ver_minor = outbuf[2];
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_mcu_ver(struct ilitek_ts_data *ilitek_data,
> +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_MCU_VER;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 32);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(ilitek_data->mcu_ver, outbuf, 2);
> +	memset(ilitek_data->product_id, 0, sizeof(ilitek_data->product_id));
> +	memcpy(ilitek_data->product_id, outbuf+6, 26);
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_fw_ver(struct ilitek_ts_data *ilitek_data,
> +				   u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_FW_VER;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(ilitek_data->firmware_ver, outbuf, 8);
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_scrn_res(struct ilitek_ts_data *ilitek_data,
> +				     u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_SCRN_RES;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> +	if (ret < 0)
> +		return ret;
> +
> +	ilitek_data->screen_min_x = (outbuf[1] << 8) + outbuf[0];
> +	ilitek_data->screen_min_y = (outbuf[3] << 8) + outbuf[2];
> +	ilitek_data->screen_max_x = (outbuf[5] << 8) + outbuf[4];
> +	ilitek_data->screen_max_y = (outbuf[7] << 8) + outbuf[6];
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_key_info_v6(struct ilitek_ts_data *ilitek_data,
> +					u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0, i = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_KEY_INFO;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 10,
> +					outbuf, ilitek_data->keycount * 5 + 5);
> +	if (ret < 0)
> +		return ret;
> +
> +	ilitek_data->key_xlen = (outbuf[2] << 8) + outbuf[1];
> +	ilitek_data->key_ylen = (outbuf[4] << 8) + outbuf[3];
> +
> +	for (i = 0; i < ilitek_data->keycount; i++) {
> +		ilitek_data->keyinfo[i].id = outbuf[i * 5 + 5];
> +		ilitek_data->keyinfo[i].x = (outbuf[i * 5 + 7] << 8) +
> +					    outbuf[i * 5 + 6];
> +		ilitek_data->keyinfo[i].y = (outbuf[i * 5 + 9] << 8) +
> +					    outbuf[i * 5 + 8];
> +		ilitek_data->keyinfo[i].status = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_tp_res_v6(struct ilitek_ts_data *ilitek_data,
> +				      u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_TP_RES;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 15);
> +	if (ret < 0)
> +		return ret;
> +
> +	ilitek_data->tp_max_x = (outbuf[1] << 8) + outbuf[0];
> +	ilitek_data->tp_max_y = (outbuf[3] << 8) + outbuf[2];
> +	ilitek_data->x_ch = (outbuf[5] << 8) + outbuf[4];
> +	ilitek_data->y_ch = (outbuf[7] << 8) + outbuf[6];
> +	ilitek_data->max_tp = outbuf[8];
> +	ilitek_data->keycount = outbuf[9];
> +	ilitek_data->slave_count = outbuf[10];
> +	ilitek_data->format = outbuf[12];
> +
> +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT) {
> +		dev_err(&ilitek_data->client->dev, "Invalid KEY count:%d from FW\n",
> +			ilitek_data->keycount);
> +		return -EINVAL;
> +	}
> +
> +	if (ilitek_data->keycount > 0) {
> +		ret = api_protocol_set_cmd(ilitek_data, GET_KEY_INFO,
> +					   NULL, outbuf);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (ilitek_data->max_tp > ILITEK_SUPPORT_MAX_POINT) {
> +		dev_err(&ilitek_data->client->dev, "Invalid MAX_TP:%d from FW\n",
> +			ilitek_data->max_tp);
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int api_protocol_get_ic_mode(struct ilitek_ts_data *ilitek_data,
> +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> +{
> +	int ret = 0;
> +	u8 buf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;
> +
> +	buf[0] = ILITEK_TP_CMD_GET_IC_MODE;
> +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ilitek_data->ic_mode = outbuf[0];
> +	return ret;
> +}
> +
> +struct PROTOCOL_MAP ptl_func_map_v3[] = {
> +	{ /* V3 cmds */},
> +};
> +
> +struct PROTOCOL_MAP ptl_func_map_v6[] = {
> +	[GET_PTL_VER]	= {ILITEK_TP_CMD_GET_PRL_VER, "GET_PTL_VER",
> +			   api_protocol_get_ptl_ver},
> +	[GET_FW_VER]	= {ILITEK_TP_CMD_GET_FW_VER, "GET_FW_VER",
> +			   api_protocol_get_fw_ver},
> +	[GET_SCRN_RES]	= {ILITEK_TP_CMD_GET_SCRN_RES, "GET_SCRN_RES",
> +			   api_protocol_get_scrn_res},
> +	[GET_TP_RES]	= {ILITEK_TP_CMD_GET_TP_RES, "GET_TP_RES",
> +			   api_protocol_get_tp_res_v6},
> +	[GET_IC_MODE]	= {ILITEK_TP_CMD_GET_IC_MODE, "GET_IC_MODE",
> +			   api_protocol_get_ic_mode},
> +	[GET_MCU_VER]	= {ILITEK_TP_CMD_GET_MCU_VER, "GET_MOD_NAME",
> +			   api_protocol_get_mcu_ver},
> +	[GET_KEY_INFO]	= {ILITEK_TP_CMD_GET_KEY_INFO, "GET_KEY_INFO",
> +			   api_protocol_get_key_info_v6},
> +};
> +
> +/* Probe APIs */
> +static int ilitek_request_gpio(struct device *dev,
> +			       struct ilitek_ts_data *ilitek_data)
> +{
> +	int ret = 0;
> +
> +	if (!dev || !ilitek_data)
> +		return -EINVAL;
> +
> +	ilitek_data->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ilitek_data->reset_gpio)) {
> +		ret = PTR_ERR(ilitek_data->reset_gpio);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ilitek_reset(struct ilitek_ts_data *ilitek_data,
> +			 int delay, bool boot)
> +{
> +	bool init_stats = ilitek_data->irq_status;
> +
> +	if (!boot)
> +		ilitek_irq_disable(ilitek_data);
> +
> +	if (ilitek_data->reset_gpio) {
> +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> +		mdelay(10);
> +
> +		gpiod_set_value(ilitek_data->reset_gpio, 0);
> +		mdelay(10);
> +
> +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> +		mdelay(delay);

I do not believe you mean to leave reset line activei here. Remember,
gpiod is logical API, with 1 representing line being active, and 0 -
inactive.  gpiolib internally takes into account line polarity, so for
active-low GPIOs gpiod_set_value(gpio, 1) will drive the line low.

> +	}
> +
> +	if (!boot && init_stats)
> +		ilitek_irq_enable(ilitek_data);
> +}
> +
> +static int ilitek_protocol_init(struct ilitek_ts_data *ilitek_data)
> +{
> +	int ret = 0;
> +	u8 outbuf[64] = {0};
> +
> +	if (!ilitek_data)
> +		return -EINVAL;

How can it be NULL?

> +
> +	ilitek_data->ptl_cb_func = ptl_func_map_v6;
> +
> +	ret = api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ilitek_data->ptl.ver_major == 0x3 ||
> +	    ilitek_data->ptl.ver == BL_V1_6 ||
> +	    ilitek_data->ptl.ver == BL_V1_7) {
> +		ilitek_data->process_and_report = NULL;
> +		ilitek_data->ptl_cb_func = NULL;
> +		ilitek_data->reset_time = 200;
> +	} else {
> +		ilitek_data->process_and_report = ilitek_process_and_report_v6;
> +		ilitek_data->ptl_cb_func = ptl_func_map_v6;
> +		ilitek_data->reset_time = 600;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ilitek_read_tp_info(struct ilitek_ts_data *ilitek_data, bool boot)
> +{
> +	u8 outbuf[256] = {0};
> +
> +	if (api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf) < 0)
> +		return -EFAULT;
> +	if (api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf) < 0)
> +		return -EFAULT;
> +	if (api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf) < 0)
> +		return -EFAULT;
> +	if (boot && api_protocol_set_cmd(ilitek_data, GET_SCRN_RES, NULL,
> +					 outbuf) < 0)
> +		return -EFAULT;
> +	if (api_protocol_set_cmd(ilitek_data, GET_TP_RES, NULL, outbuf) < 0)
> +		return -EFAULT;
> +	if (api_protocol_set_cmd(ilitek_data, GET_IC_MODE, NULL, outbuf) < 0)
> +		return -EFAULT;

EFAULT is weird error code to be used here, maybe EIO or EINVAL?

> +
> +	return 0;
> +}
> +
> +static int ilitek_input_dev_init(struct device *dev,
> +				 struct ilitek_ts_data *ilitek_data)
> +{
> +	int ret = 0, i = 0;
> +	struct input_dev *input = NULL;
> +
> +	if (!dev || !ilitek_data)
> +		return -EINVAL;

How can either these 2 be NULL?

> +
> +	input  = input_allocate_device();
> +	if (!input)
> +		return -ENOMEM;
> +
> +	input->name = ILITEK_TS_NAME;
> +	input->id.bustype = BUS_I2C;
> +
> +	__set_bit(INPUT_PROP_DIRECT, input->propbit);
> +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	input_set_abs_params(input, ABS_MT_POSITION_X,
> +			     ilitek_data->screen_min_x,
> +			     ilitek_data->screen_max_x, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
> +			     ilitek_data->screen_min_y,
> +			     ilitek_data->screen_max_y, 0, 0);
> +	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +	input_set_abs_params(input, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);

Please support common touchscreen properties by using
touchscreen_parse_properties() and then touchscreen_set_mt_pos() and/or
touchscreen_report_pos() when reporting contacts.

> +
> +	input_mt_init_slots(input, ilitek_data->max_tp, INPUT_MT_DIRECT);

This needs error handling.

> +
> +	for (i = 0; i < ilitek_data->keycount; i++)
> +		set_bit(ilitek_data->keyinfo[i].id & KEY_MAX, input->keybit);

As I mentioned, soft keys are better handled in userspace.

> +
> +	ret = input_register_device(input);
> +	if (ret)
> +		dev_err(dev, "failed to register input device, err:%d\n", ret);
> +
> +	ilitek_data->input_dev = input;
> +	return ret;
> +}
> +
> +static irqreturn_t ilitek_i2c_isr(int irq, void *dev_id)
> +{
> +	struct ilitek_ts_data *ilitek_data = dev_id;
> +	int ret = 0;
> +
> +	if (!ilitek_data->process_and_report) {
> +		dev_err(&ilitek_data->client->dev, "report touch cb is NULL\n");

How can this happen?

> +		return IRQ_NONE;
> +	}
> +
> +	ilitek_irq_disable(ilitek_data);

You are in the interrupt handler, why do you need to disable interrupts?
Current one will not be acknowledged until you return from ISR.

> +	mutex_lock(&ilitek_data->ilitek_mutex);

What does this mutex protect?

> +	ret = ilitek_data->process_and_report(ilitek_data);
> +	mutex_unlock(&ilitek_data->ilitek_mutex);
> +	ilitek_irq_enable(ilitek_data);
> +
> +	if (ret < 0) {
> +		dev_err(&ilitek_data->client->dev, "process and report point failed, err:%d\n",
> +			ret);
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ilitek_request_irq(struct device *dev,
> +			      struct ilitek_ts_data *ilitek_data)
> +{
> +	int ret = 0;
> +
> +	spin_lock_init(&ilitek_data->irq_lock);
> +	ilitek_data->irq_status = true;
> +
> +	ret = request_threaded_irq(ilitek_data->client->irq, NULL,
> +				   ilitek_i2c_isr,
> +				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

Please reply on platform (ACPI, DT, etc) to set up proper trigger for
the interrupt line. Additionally, you should prefer having level
interrupt, rather than edge, or you risk missing interrupts.

> +				   "ilitek_touch_irq", ilitek_data);
> +	if (ret)
> +		dev_err(dev, "request threaded irq failed, err:%d\n", ret);

I am not sure what is the value of this wrapper.

> +
> +	return ret;
> +}
> +
> +static ssize_t firmware_version_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> +	int ret = 0;
> +	u8 outbuf[64] = {0};
> +
> +	ilitek_irq_disable(ilitek_data);
> +	mutex_lock(&ilitek_data->ilitek_mutex);
> +	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);

Do you need to query this all the time? Is it possible to collect this
data at boot or after firmware flash and store for subsequent use?

> +	mutex_unlock(&ilitek_data->ilitek_mutex);
> +	ilitek_irq_enable(ilitek_data);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> +		return ret;
> +	}
> +
> +	return scnprintf(buf, PAGE_SIZE,
> +			 "ILITEK FW ver: %d.%d.%d.%d.%d.%d.%d.%d\n",
> +			 ilitek_data->firmware_ver[0],
> +			 ilitek_data->firmware_ver[1],
> +			 ilitek_data->firmware_ver[2],
> +			 ilitek_data->firmware_ver[3],
> +			 ilitek_data->firmware_ver[4],
> +			 ilitek_data->firmware_ver[5],
> +			 ilitek_data->firmware_ver[6],
> +			 ilitek_data->firmware_ver[7]);
> +}
> +static DEVICE_ATTR_RO(firmware_version);
> +
> +static ssize_t product_id_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> +	int ret = 0;
> +	u8 outbuf[64] = {0};
> +
> +	ilitek_irq_disable(ilitek_data);
> +	mutex_lock(&ilitek_data->ilitek_mutex);
> +	ret = api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf);
> +	mutex_unlock(&ilitek_data->ilitek_mutex);
> +	ilitek_irq_enable(ilitek_data);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> +		return ret;
> +	}
> +
> +	return scnprintf(buf, PAGE_SIZE, "MCU: %s\n", ilitek_data->product_id);
> +}
> +static DEVICE_ATTR_RO(product_id);
> +
> +static struct attribute *ilitek_sysfs_attrs[] = {
> +	&dev_attr_firmware_version.attr,
> +	&dev_attr_product_id.attr,
> +	NULL
> +};
> +
> +static struct attribute_group ilitek_attrs_group[] = {
> +	{.attrs = ilitek_sysfs_attrs},
> +};
> +
> +static int ilitek_create_sysfsnode(struct device *dev)
> +{
> +	int ret = 0;
> +
> +	ret = sysfs_create_group(&dev->kobj, ilitek_attrs_group);
> +	if (ret)
> +		dev_err(dev, "sysfs create group failed, err:%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int ilitek_ts_i2c_probe(struct i2c_client *client,
> +			       const struct i2c_device_id *id)
> +{
> +	struct ilitek_ts_data *ilitek_data = NULL;
> +	struct device *dev = &client->dev;
> +	int ret = 0;
> +
> +	ilitek_data = kzalloc(sizeof(*ilitek_data), GFP_KERNEL);
> +	if (!ilitek_data)
> +		return -ENOMEM;

Please use devm APIs to make sure resources are freed automatically
after errors. It will also allow you to get rid of
ilitek_ts_i2c_remove().

> +
> +	ilitek_data->client = client;
> +	i2c_set_clientdata(client, ilitek_data);
> +
> +	mutex_init(&ilitek_data->ilitek_mutex);
> +
> +	ret = ilitek_request_gpio(dev, ilitek_data);
> +	if (ret < 0) {
> +		dev_err(dev, "request gpio failed, err:%d", ret);
> +		goto err_put_gpiod;
> +	}
> +
> +	ilitek_reset(ilitek_data, 1000, true);
> +	ret = ilitek_protocol_init(ilitek_data);
> +	if (ret < 0) {
> +		dev_err(dev, "protocol init failed, err:%d", ret);
> +		goto err_put_gpiod;
> +	}
> +
> +	ret = ilitek_read_tp_info(ilitek_data, true);
> +	if (ret < 0) {
> +		dev_err(dev, "read tp info failed, err:%d", ret);
> +		goto err_put_gpiod;
> +	}
> +
> +	ret = ilitek_input_dev_init(dev, ilitek_data);
> +	if (ret < 0) {
> +		dev_err(dev, "input dev init failed, err:%d", ret);
> +		goto err_free_input_dev;
> +	}
> +
> +	ret = ilitek_request_irq(dev, ilitek_data);
> +	if (ret < 0) {
> +		dev_err(dev, "request irq failed, err:%d", ret);
> +		goto err_free_input_dev;
> +	}
> +
> +	ret = ilitek_create_sysfsnode(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "create sysfs node failed, err:%d", ret);
> +		goto err_free_irq;
> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(ilitek_data->client->irq, ilitek_data);
> +
> +	input_unregister_device(ilitek_data->input_dev);
> +err_free_input_dev:
> +	if (ilitek_data->input_dev)
> +		input_free_device(ilitek_data->input_dev);
> +err_put_gpiod:
> +	if (!IS_ERR(ilitek_data->reset_gpio))
> +		gpiod_put(ilitek_data->reset_gpio);
> +	kfree(ilitek_data);
> +	return ret;
> +}
> +
> +static int ilitek_ts_i2c_remove(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> +
> +	sysfs_remove_group(&dev->kobj, ilitek_attrs_group);
> +
> +	free_irq(ilitek_data->client->irq, ilitek_data);
> +
> +	input_unregister_device(ilitek_data->input_dev);
> +
> +	if (!IS_ERR(ilitek_data->reset_gpio))
> +		gpiod_put(ilitek_data->reset_gpio);
> +
> +	kfree(ilitek_data);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ilitek_ts_i2c_id[] = {
> +	{ILITEK_TS_NAME, 0},
> +	{ /* not omitted */ },

What does "not omitted" mean in this context?

> +};
> +MODULE_DEVICE_TABLE(i2c, ilitek_ts_i2c_id);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ilitekts_acpi_id[] = {
> +	{ "ILTK0001", 0 },
> +	{ /* not omitted */ },
> +};
> +MODULE_DEVICE_TABLE(acpi, ilitekts_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ilitek_ts_i2c_match[] = {
> +	{.compatible = "ilitek,ili2520",},
> +	{.compatible = "ilitek,ili2510",},
> +	{ /* not omitted */ },
> +};
> +MODULE_DEVICE_TABLE(of, ilitek_ts_i2c_match);
> +#endif
> +
> +static struct i2c_driver ilitek_ts_i2c_driver = {
> +	.driver = {
> +		.name = ILITEK_TS_NAME,
> +		.owner = THIS_MODULE,

No need to set owner.

> +		.of_match_table = of_match_ptr(ilitek_ts_i2c_match),
> +		.acpi_match_table = ACPI_PTR(ilitekts_acpi_id),

You need power management (suspend/resume) handling too.

> +	},
> +	.probe = ilitek_ts_i2c_probe,
> +	.remove = ilitek_ts_i2c_remove,
> +	.id_table = ilitek_ts_i2c_id,
> +};
> +
> +module_i2c_driver(ilitek_ts_i2c_driver);
> +
> +MODULE_AUTHOR("ILITEK");
> +MODULE_DESCRIPTION("ILITEK I2C Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
> 
> 

Thanks.

-- 
Dmitry

RE: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Joe Hung (洪銘陽)]

Hi Dmitry,

Many thanks for your kind review, most of comments sound great to me.
There some questions with "[Question]" prefix, response as below.

Looking forward to further discussion.

On Sat, Feb 20, 2021 at 21:51:58PM -0800, Dmitry Torokhov wrote:
> Hi Joe,
> 
> On Thu, Feb 18, 2021 at 02:44:35PM +0800, Joe Hung wrote:
> > Add support for ILITEK Lego series of touch devices.
> > Lego series includes ILITEK 213X/2316/232X/252X.
> > Designed in compatible way for ILITEK 23XX/251X ICs.
> > 
> > Tested and passed with evaluation board with ILI2520 IC.
> 
> Thank you for your submission.
> 
> > 
> > Signed-off-by: Joe Hung <joe_hung@ilitek.com>
> > ---
> > Changes in v3:
> >   - None
> > 
> > Changes in v2:
> >   - Remove irq-gpio and related flow
> > 
> >  drivers/input/touchscreen/Kconfig         |   12 +
> >  drivers/input/touchscreen/Makefile        |    1 +
> >  drivers/input/touchscreen/ilitek_ts_i2c.c | 1010 +++++++++++++++++++++
> >  3 files changed, 1023 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/ilitek_ts_i2c.c
> > 
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index f012fe746df0..9dea7fd0d94f 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called zinitix.
> > 
> > +config TOUCHSCREEN_ILITEK
> > +	tristate "Ilitek I2C 23XX/25XX/Lego Series Touch ICs"
> > +	depends on I2C
> > +	help
> > +	  Say Y here if you have touchscreen with ILITEK touch IC,
> > +	  it supports 23XX/25XX/other Lego series.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called ilitek_ts_i2c.
> > +
> >  endif
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 6233541e9173..6749bdc42f41 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
> >  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
> >  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> > +obj-$(CONFIG_TOUCHSCREEN_ILITEK)        += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> > new file mode 100644
> > index 000000000000..72cb7c82170c
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> > @@ -0,0 +1,1010 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ILITEK Touch IC driver for 23XX, 25XX and Lego series
> > + *
> > + * Copyright (C) 2011 ILI Technology Corporation.
> > + * Copyright (C) 2020 Luca Hsu <luca_hsu@ilitek.com>
> > + * Copyright (C) 2021 Joe Hung <joe_hung@ilitek.com>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/i2c.h>
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/delay.h>
> > +#include <linux/kthread.h>
> 
> Are you sure you need this include?

Agreed; will remove it in v4.

> > +#include <linux/sched.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/cdev.h>
> 
> You do not create character devices in this driver.

Agreed; will remove it in v4.

> > +#include <linux/uaccess.h>
> > +#include <linux/version.h>
> 
> Why do you need this include?

Agreed; will remove it in v4.

> > +#include <linux/rtc.h>
> 
> I do not think this driver uses rtc.

Agreed; will remove it in v4.

> > +#include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/fs.h>
> > +#include <linux/pm_runtime.h>
> 
> I do not think your driver uses runtime PM.
> 

Agreed; will remove it in v4.

> > +#include <linux/ctype.h>
> > +#include <linux/errno.h>
> > +#include <linux/acpi.h>
> 
> Please double-check that you include only facilities that the driver
> actually uses.

Agreed; will do.

> 
> > +
> > +#define ILITEK_TS_NAME					"ilitek_ts"
> > +#define BL_V1_8						(0x10800)
> > +#define BL_V1_7						(0x10700)
> > +#define BL_V1_6						(0x10600)
> > +
> > +#define ILITEK_TP_CMD_GET_TP_RES			(0x20)
> > +#define ILITEK_TP_CMD_GET_SCRN_RES			(0x21)
> > +#define ILITEK_TP_CMD_GET_KEY_INFO			(0x22)
> > +#define ILITEK_TP_CMD_GET_FW_VER			(0x40)
> > +#define ILITEK_TP_CMD_GET_PRL_VER			(0x42)
> > +#define ILITEK_TP_CMD_GET_MCU_VER			(0x61)
> > +#define ILITEK_TP_CMD_GET_IC_MODE			(0xC0)
> > +
> > +#define REPORT_ADDRESS_COUNT				(61)
> > +#define ILITEK_SUPPORT_MAX_POINT			(40)
> > +#define ILITEK_SUPPORT_MAX_KEY_CNT			(50)
> 
> No need to use parenthesis for simple scalar values.
> 

Agreed; will modified here in v4.

> > +
> > +#define CEIL(n, d)	(((n) % (d)) ? ((n) / (d) + 1) : ((n) / (d)))
> 
> I think you want standard DIV_ROUND_UP().

Agreed; will use DIV_ROUND_UP directly in v4.

> 
> > +
> > +struct ilitek_protocol_info {
> 
> Let's use ili23xx instead of ilitek prefix throughout the file. Or
> whatever the first generation of the product supported by this driver.
> 

This driver is planned to support 213X/23XX/25XX.

[Question]
Was wondering why ilitek prefix is not feasible ?
Is it fine that 23XX prefix that still support 25XX ICs ?

> > +	s32 ver;
> > +	u8 ver_major;
> > +	u8 ver_mid;
> > +	u8 ver_minor;
> > +};
> > +
> > +struct ilitek_key_info {
> > +	s32 id;
> > +	s32 x;
> > +	s32 y;
> > +	s32 status;
> > +};
> > +
> > +struct ilitek_touch_info {
> > +	u16 id;
> > +	u16 x;
> > +	u16 y;
> > +	u16 p;
> > +	u16 w;
> > +	u16 h;
> > +	u16 status;
> > +};
> > +
> > +struct ilitek_ts_data {
> > +	struct i2c_client		*client;
> > +	struct gpio_desc		*reset_gpio;
> > +	struct input_dev		*input_dev;
> > +
> > +	struct mutex			ilitek_mutex;
> > +	spinlock_t			irq_lock;
> > +	bool				irq_status;
> > +	int (*process_and_report)(struct ilitek_ts_data *ilitek_data);
> > +
> > +	struct PROTOCOL_MAP		*ptl_cb_func;
> > +	struct ilitek_protocol_info	ptl;
> > +
> > +
> > +	struct ilitek_key_info		keyinfo[ILITEK_SUPPORT_MAX_KEY_CNT];
> > +	struct ilitek_touch_info	tpinfo[ILITEK_SUPPORT_MAX_POINT];
> > +
> > +	bool				is_touched;
> > +	bool				touch_key_hold_press;
> > +
> > +	bool				ilitek_repeat_start;
> > +
> > +	char				product_id[30];
> > +	u8				mcu_ver[2];
> > +	u8				ic_mode;
> > +	u8				firmware_ver[8];
> > +
> > +	s32				reset_time;
> > +	s32				tp_max_x;
> > +	s32				tp_max_y;
> > +	s32				screen_max_x;
> > +	s32				screen_max_y;
> > +	s32				screen_min_x;
> > +	s32				screen_min_y;
> > +	s32				max_tp;
> > +	s32				x_ch;
> > +	s32				y_ch;
> > +	s32				slave_count;
> 
> What is the importance of this?

slave_count is different among different ILITEK touch ICs.
It would be used during FW upgrade.
We needs FW upgrade touch IC's slave also.

> > +	s32				format;
> > +	s32				keycount;
> > +	s32				key_xlen;
> > +	s32				key_ylen;
> > +};
> > +
> > +enum ilitek_cmds_v6 {
> > +	GET_PTL_VER = 0,
> > +	GET_FW_VER,
> > +	GET_SCRN_RES,
> > +	GET_TP_RES,
> > +	GET_IC_MODE,
> > +	GET_MCU_VER,
> > +	GET_KEY_INFO,
> > +
> > +	MAX_CMD_CNT	/* ALWAYS keep at the end */
> > +};
> > +
> > +typedef int protocol_func(struct ilitek_ts_data *ilitek_data,
> > +			  u16 cmd, uint8_t *inbuf, uint8_t *outbuf);
> 
> In kernel please use u8 instead of uint8_t, etc.

Agreed; should correct it in v4.

> 
> > +
> > +struct PROTOCOL_MAP {
> > +	u16 cmd;
> > +	const char *name;
> > +	protocol_func *func;
> > +};
> > +
> > +/* ILITEK I2C R/W APIs */
> > +static int ilitek_i2c_transfer(struct i2c_client *client,
> > +			       struct i2c_msg *msgs, int cnt)
> > +{
> > +	int ret = 0, retry = 4;
> > +
> > +	while (retry--) {
> > +		ret = i2c_transfer(client->adapter, msgs, cnt);
> > +		if (ret >= 0)
> > +			break;
> > +		mdelay(20);
> > +	}
> 
> Why does it this device need retries?
> 

I2C transfer may failed cause FW not ready for some reason.
It should be fine for basic probe/ISR flow, and risky for FW update.

[Question]
Should it be removed, and patched with FW update patch ?

> > +
> > +	return ret;
> > +}
> > +
> > +static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ilitek_data,
> > +				     u8 *cmd, int write_len, int delay,
> > +				     u8 *data, int read_len)
> > +{
> > +	int ret = 0;
> > +	struct i2c_client *client = ilitek_data->client;
> > +	struct i2c_msg msgs[2] = {
> > +		{.addr = client->addr, .flags = 0,
> > +		 .len = write_len, .buf = cmd,},
> > +		{.addr = client->addr, .flags = I2C_M_RD,
> > +		 .len = read_len, .buf = data,}
> > +	};
> > +
> > +	if (ilitek_data->ilitek_repeat_start && delay == 0 &&
> 
> Where is this being set? As far as I understand, repeated start is
> property of the i2c controller, not peripheral device.
> 
> Also, please see if you can switch to using regmap, as it should take
> care of selecting right mode, depending on controller's capabilities.
> 

Some ilitek touch ICs may not support "I2C repeat start".
And agreed; it should be removed here in v4.

> > +		write_len > 0 && read_len > 0) {
> > +		ret = ilitek_i2c_transfer(client, msgs, 2);
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		if (write_len > 0) {
> > +			ret = ilitek_i2c_transfer(client, msgs, 1);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +		if (delay > 0)
> > +			mdelay(delay);
> > +		if (read_len > 0) {
> > +			ret = ilitek_i2c_transfer(client, msgs + 1, 1);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void ilitek_irq_enable(struct ilitek_ts_data *ilitek_data)
> > +{
> > +	unsigned long irqflag = 0;
> > +
> > +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> > +	if (!ilitek_data->irq_status) {
> > +		enable_irq(ilitek_data->client->irq);
> > +		ilitek_data->irq_status = true;
> > +	}
> > +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
> 
> The driver should know at any time whether interrupts are disabled or
> not, so I'd prefer if we did not have this wrapper, and the one below.
> 

Agreed; will remove in v4.

> > +}
> > +
> > +static void ilitek_irq_disable(struct ilitek_ts_data *ilitek_data)
> > +{
> > +	unsigned long irqflag = 0;
> > +
> > +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> > +	if (ilitek_data->irq_status) {
> > +		disable_irq_nosync(ilitek_data->client->irq);
> > +		ilitek_data->irq_status = false;
> > +	}
> > +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
> > +}
> > +
> > +/* ILITEK ISR APIs */
> > +static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
> > +			    int x, int y)
> > +{
> > +	int j = 0;
> > +	int x_min;
> > +	int x_max;
> > +	int y_min;
> > +	int y_max;
> > +
> > +	for (j = 0; j < ilitek_data->keycount; j++) {
> > +		x_min = ilitek_data->keyinfo[j].x;
> > +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> > +		y_min = ilitek_data->keyinfo[j].y;
> > +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> > +
> > +		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
> > +			input_report_key(ilitek_data->input_dev,
> > +					 ilitek_data->keyinfo[j].id, 1);
> > +
> > +			ilitek_data->keyinfo[j].status = 1;
> > +			ilitek_data->touch_key_hold_press = true;
> > +			ilitek_data->is_touched = true;
> > +			break;
> > +		}
> > +	}
> > +}
> 
> This sounds like handling of "soft keys" which is better left for
> userspace. OTOH the Linux keycode seems to be coming from controller
> firmware. This is really weird.

It handles keys which key ID and region are get from touch device firmware.
Then check region of touch points to trigger key event.

[Question]
Is it not feasible for a touchscreen driver do key handling?

> > +
> > +static int ilitek_key_release(struct ilitek_ts_data *ilitek_data,
> > +			      int x, int y, bool check_point)
> > +{
> > +	int j = 0;
> > +	int x_min;
> > +	int x_max;
> > +	int y_min;
> > +	int y_max;
> > +
> > +	for (j = 0; j < ilitek_data->keycount; j++) {
> > +		x_min = ilitek_data->keyinfo[j].x;
> > +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> > +		y_min = ilitek_data->keyinfo[j].y;
> > +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> > +
> > +		if (ilitek_data->keyinfo[j].status == 1) {
> > +			if (!check_point ||
> > +			    (x < x_min || x > x_max ||
> > +			     y < y_min || y > y_max)) {
> > +				input_report_key(ilitek_data->input_dev,
> > +						 ilitek_data->keyinfo[j].id, 0);
> > +				ilitek_data->keyinfo[j].status = 0;
> > +				ilitek_data->touch_key_hold_press = false;
> > +			}
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void ilitek_touch_down(struct ilitek_ts_data *ilitek_data,
> > +			      int id, int x, int y, int p, int h, int w)
> > +{
> > +	struct input_dev *input = ilitek_data->input_dev;
> > +
> > +	input_report_key(input, BTN_TOUCH, 1);
> > +
> > +	input_mt_slot(input, id);
> > +	input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
> > +
> > +	input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> > +	input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> > +
> > +	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, h);
> > +	input_event(input, EV_ABS, ABS_MT_WIDTH_MAJOR, w);
> > +
> > +	ilitek_data->tpinfo[id].status = 1;
> > +	ilitek_data->tpinfo[id].id = id;
> > +	ilitek_data->tpinfo[id].x = x;
> > +	ilitek_data->tpinfo[id].y = y;
> > +	ilitek_data->tpinfo[id].p = p;
> > +	ilitek_data->tpinfo[id].h = h;
> > +	ilitek_data->tpinfo[id].w = w;
> > +	ilitek_data->is_touched = true;
> > +}
> > +
> > +static void ilitek_touch_release(struct ilitek_ts_data *ilitek_data, int id)
> > +{
> > +	struct input_dev *input = ilitek_data->input_dev;
> > +
> > +	if (ilitek_data->tpinfo[id].status == 1) {
> > +		input_mt_slot(input, id);
> > +		input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> > +		ilitek_data->tpinfo[id].status = 0;
> > +	}
> > +}
> > +
> > +static void ilitek_touch_release_all(struct ilitek_ts_data *ilitek_data)
> > +{
> > +	struct input_dev *input = ilitek_data->input_dev;
> > +	int i = 0;
> > +
> > +	for (i = 0; i < ilitek_data->max_tp; i++)
> > +		ilitek_touch_release(ilitek_data, i);
> > +	input_report_key(input, BTN_TOUCH, 0);
> > +	input_sync(input);
> > +
> > +	ilitek_data->is_touched = false;
> > +}
> > +
> > +static int ilitek_process_and_report_v6(struct ilitek_ts_data *ilitek_data)
> > +{
> > +	int ret = 0;
> > +	u8 buf[512] = {0};
> > +	int packet_len = 5;
> > +	int packet_max_point = 10;
> > +	int report_max_point = 6;
> > +	int release_point = 0;
> > +	int i = 0, count = 0;
> > +	struct input_dev *input = ilitek_data->input_dev;
> > +	struct device *dev = &ilitek_data->client->dev;
> > +	u16 x, y, p = 10, w = 10, h = 10, status, id;
> > +
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0, buf, 64);
> > +	if (ret < 0) {
> > +		dev_err(dev, "get touch info failed, err:%d\n", ret);
> > +		goto err_release_touch_and_key;
> > +	}
> > +
> > +	report_max_point = buf[REPORT_ADDRESS_COUNT];
> > +	if (report_max_point > ilitek_data->max_tp) {
> > +		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
> > +			report_max_point, ilitek_data->max_tp);
> > +		return -EINVAL;
> > +	}
> > +
> > +	count = CEIL(report_max_point, packet_max_point);
> > +	for (i = 1; i < count; i++) {
> > +		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
> > +			buf + i * 64, 64);
> > +		if (ret < 0) {
> > +			dev_err(dev, "get touch info. err, count=%d\n", count);
> > +			goto err_release_touch_and_key;
> > +		}
> > +	}
> 
> This means you have 1 + count transactions on i2c bus. Have you
> considered reading max number of bytes in one transaction, and then soft
> out how much of received data is valid?
> 

Due to limitation on FW of touch device, data byte for each I2C transaction
is limited. It would lead to I2C communication issue when large count of bytes
are request.

Also, for only a few touch points,
It should be less efficient to query max number of bytes.

> > +
> > +	for (i = 0; i < report_max_point; i++) {
> > +		status = buf[i * packet_len + 1] & 0x40;
> > +		id = buf[i * packet_len + 1] & 0x3F;
> > +
> > +		if (!status) {
> > +			release_point++;
> > +			ilitek_touch_release(ilitek_data, id);
> > +			continue;
> > +		}
> > +
> > +		x = (buf[i * packet_len + 3] << 8) + buf[i * packet_len + 2];
> > +		y = (buf[i * packet_len + 5] << 8) + buf[i * packet_len + 4];
> 
> 		x = le16_to_cpup(....);
> 
> Please adjust here and elsewhere. Also, if data may be unaligned, please
> use get_unaligned_le16() instead.
> 

Agreed; will check and update elsewhere in v4.

> > +
> > +		if (!ilitek_data->is_touched)
> > +			ilitek_key_down(ilitek_data, x, y);
> > +
> > +		if (!ilitek_data->touch_key_hold_press) {
> > +			if (x > ilitek_data->screen_max_x ||
> > +			    y > ilitek_data->screen_max_y ||
> > +			    x < ilitek_data->screen_min_x ||
> > +			    y < ilitek_data->screen_min_y)
> > +				continue;
> > +
> > +			ilitek_touch_down(ilitek_data, id, x, y, p, h, w);
> > +		} else {
> > +			ilitek_key_release(ilitek_data, x, y, 1);
> > +		}
> > +	}
> > +
> > +	if (release_point == report_max_point) {
> > +		ilitek_touch_release_all(ilitek_data);
> > +		ilitek_key_release(ilitek_data, 0, 0, 0);
> > +		ilitek_data->is_touched = false;
> > +	}
> > +	input_sync(input);
> > +	return 0;
> > +
> > +err_release_touch_and_key:
> > +	ilitek_touch_release_all(ilitek_data);
> > +	ilitek_key_release(ilitek_data, 0, 0, 0);
> > +	ilitek_data->is_touched = false;
> > +	input_sync(input);
> > +	return ret;
> > +}
> > +
> > +
> > +/* APIs of cmds for ILITEK Touch IC */
> > +static int api_protocol_set_cmd(struct ilitek_ts_data *ilitek_data,
> > +				u16 idx, u8 *inbuf, u8 *outbuf)
> > +{
> > +	u16 cmd;
> > +	int ret = 0;
> > +
> > +	if (!ilitek_data || !ilitek_data->ptl_cb_func)
> > +		return -EINVAL;
> > +
> > +	if (idx >= MAX_CMD_CNT)
> > +		return -EINVAL;
> > +
> > +	cmd = ilitek_data->ptl_cb_func[idx].cmd;
> > +	ret = ilitek_data->ptl_cb_func[idx].func(ilitek_data, cmd,
> > +						 inbuf, outbuf);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int api_protocol_get_ptl_ver(struct ilitek_ts_data *ilitek_data,
> > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = cmd;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 3);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ilitek_data->ptl.ver = (outbuf[0] << 16) + (outbuf[1] << 8);
> > +	ilitek_data->ptl.ver_major = outbuf[0];
> > +	ilitek_data->ptl.ver_mid = outbuf[1];
> > +	ilitek_data->ptl.ver_minor = outbuf[2];
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_mcu_ver(struct ilitek_ts_data *ilitek_data,
> > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_MCU_VER;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 32);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	memcpy(ilitek_data->mcu_ver, outbuf, 2);
> > +	memset(ilitek_data->product_id, 0, sizeof(ilitek_data->product_id));
> > +	memcpy(ilitek_data->product_id, outbuf+6, 26);
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_fw_ver(struct ilitek_ts_data *ilitek_data,
> > +				   u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_FW_VER;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	memcpy(ilitek_data->firmware_ver, outbuf, 8);
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_scrn_res(struct ilitek_ts_data *ilitek_data,
> > +				     u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_SCRN_RES;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ilitek_data->screen_min_x = (outbuf[1] << 8) + outbuf[0];
> > +	ilitek_data->screen_min_y = (outbuf[3] << 8) + outbuf[2];
> > +	ilitek_data->screen_max_x = (outbuf[5] << 8) + outbuf[4];
> > +	ilitek_data->screen_max_y = (outbuf[7] << 8) + outbuf[6];
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_key_info_v6(struct ilitek_ts_data *ilitek_data,
> > +					u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0, i = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_KEY_INFO;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 10,
> > +					outbuf, ilitek_data->keycount * 5 + 5);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ilitek_data->key_xlen = (outbuf[2] << 8) + outbuf[1];
> > +	ilitek_data->key_ylen = (outbuf[4] << 8) + outbuf[3];
> > +
> > +	for (i = 0; i < ilitek_data->keycount; i++) {
> > +		ilitek_data->keyinfo[i].id = outbuf[i * 5 + 5];
> > +		ilitek_data->keyinfo[i].x = (outbuf[i * 5 + 7] << 8) +
> > +					    outbuf[i * 5 + 6];
> > +		ilitek_data->keyinfo[i].y = (outbuf[i * 5 + 9] << 8) +
> > +					    outbuf[i * 5 + 8];
> > +		ilitek_data->keyinfo[i].status = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_tp_res_v6(struct ilitek_ts_data *ilitek_data,
> > +				      u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_TP_RES;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 15);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ilitek_data->tp_max_x = (outbuf[1] << 8) + outbuf[0];
> > +	ilitek_data->tp_max_y = (outbuf[3] << 8) + outbuf[2];
> > +	ilitek_data->x_ch = (outbuf[5] << 8) + outbuf[4];
> > +	ilitek_data->y_ch = (outbuf[7] << 8) + outbuf[6];
> > +	ilitek_data->max_tp = outbuf[8];
> > +	ilitek_data->keycount = outbuf[9];
> > +	ilitek_data->slave_count = outbuf[10];
> > +	ilitek_data->format = outbuf[12];
> > +
> > +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT) {
> > +		dev_err(&ilitek_data->client->dev, "Invalid KEY count:%d from FW\n",
> > +			ilitek_data->keycount);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ilitek_data->keycount > 0) {
> > +		ret = api_protocol_set_cmd(ilitek_data, GET_KEY_INFO,
> > +					   NULL, outbuf);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	if (ilitek_data->max_tp > ILITEK_SUPPORT_MAX_POINT) {
> > +		dev_err(&ilitek_data->client->dev, "Invalid MAX_TP:%d from FW\n",
> > +			ilitek_data->max_tp);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int api_protocol_get_ic_mode(struct ilitek_ts_data *ilitek_data,
> > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > +{
> > +	int ret = 0;
> > +	u8 buf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> > +
> > +	buf[0] = ILITEK_TP_CMD_GET_IC_MODE;
> > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 2);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ilitek_data->ic_mode = outbuf[0];
> > +	return ret;
> > +}
> > +
> > +struct PROTOCOL_MAP ptl_func_map_v3[] = {
> > +	{ /* V3 cmds */},
> > +};
> > +
> > +struct PROTOCOL_MAP ptl_func_map_v6[] = {
> > +	[GET_PTL_VER]	= {ILITEK_TP_CMD_GET_PRL_VER, "GET_PTL_VER",
> > +			   api_protocol_get_ptl_ver},
> > +	[GET_FW_VER]	= {ILITEK_TP_CMD_GET_FW_VER, "GET_FW_VER",
> > +			   api_protocol_get_fw_ver},
> > +	[GET_SCRN_RES]	= {ILITEK_TP_CMD_GET_SCRN_RES, "GET_SCRN_RES",
> > +			   api_protocol_get_scrn_res},
> > +	[GET_TP_RES]	= {ILITEK_TP_CMD_GET_TP_RES, "GET_TP_RES",
> > +			   api_protocol_get_tp_res_v6},
> > +	[GET_IC_MODE]	= {ILITEK_TP_CMD_GET_IC_MODE, "GET_IC_MODE",
> > +			   api_protocol_get_ic_mode},
> > +	[GET_MCU_VER]	= {ILITEK_TP_CMD_GET_MCU_VER, "GET_MOD_NAME",
> > +			   api_protocol_get_mcu_ver},
> > +	[GET_KEY_INFO]	= {ILITEK_TP_CMD_GET_KEY_INFO, "GET_KEY_INFO",
> > +			   api_protocol_get_key_info_v6},
> > +};
> > +
> > +/* Probe APIs */
> > +static int ilitek_request_gpio(struct device *dev,
> > +			       struct ilitek_ts_data *ilitek_data)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!dev || !ilitek_data)
> > +		return -EINVAL;
> > +
> > +	ilitek_data->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ilitek_data->reset_gpio)) {
> > +		ret = PTR_ERR(ilitek_data->reset_gpio);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void ilitek_reset(struct ilitek_ts_data *ilitek_data,
> > +			 int delay, bool boot)
> > +{
> > +	bool init_stats = ilitek_data->irq_status;
> > +
> > +	if (!boot)
> > +		ilitek_irq_disable(ilitek_data);
> > +
> > +	if (ilitek_data->reset_gpio) {
> > +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> > +		mdelay(10);
> > +
> > +		gpiod_set_value(ilitek_data->reset_gpio, 0);
> > +		mdelay(10);
> > +
> > +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> > +		mdelay(delay);
> 
> I do not believe you mean to leave reset line activei here. Remember,
> gpiod is logical API, with 1 representing line being active, and 0 -
> inactive.  gpiolib internally takes into account line polarity, so for
> active-low GPIOs gpiod_set_value(gpio, 1) will drive the line low.

Agreed; reset pin should reset touch device when pin logically be low.
Should modified bindings to be active-low.
Then leave reset line inactive here after reset.
Should be updated in v4.

> 
> > +	}
> > +
> > +	if (!boot && init_stats)
> > +		ilitek_irq_enable(ilitek_data);
> > +}
> > +
> > +static int ilitek_protocol_init(struct ilitek_ts_data *ilitek_data)
> > +{
> > +	int ret = 0;
> > +	u8 outbuf[64] = {0};
> > +
> > +	if (!ilitek_data)
> > +		return -EINVAL;
> 
> How can it be NULL?

It should not be NULL in current patch.
Should be removed it and elsewhere below in v4.
> 
> > +
> > +	ilitek_data->ptl_cb_func = ptl_func_map_v6;
> > +
> > +	ret = api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ilitek_data->ptl.ver_major == 0x3 ||
> > +	    ilitek_data->ptl.ver == BL_V1_6 ||
> > +	    ilitek_data->ptl.ver == BL_V1_7) {
> > +		ilitek_data->process_and_report = NULL;
> > +		ilitek_data->ptl_cb_func = NULL;
> > +		ilitek_data->reset_time = 200;
> > +	} else {
> > +		ilitek_data->process_and_report = ilitek_process_and_report_v6;
> > +		ilitek_data->ptl_cb_func = ptl_func_map_v6;
> > +		ilitek_data->reset_time = 600;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ilitek_read_tp_info(struct ilitek_ts_data *ilitek_data, bool boot)
> > +{
> > +	u8 outbuf[256] = {0};
> > +
> > +	if (api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf) < 0)
> > +		return -EFAULT;
> > +	if (api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf) < 0)
> > +		return -EFAULT;
> > +	if (api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf) < 0)
> > +		return -EFAULT;
> > +	if (boot && api_protocol_set_cmd(ilitek_data, GET_SCRN_RES, NULL,
> > +					 outbuf) < 0)
> > +		return -EFAULT;
> > +	if (api_protocol_set_cmd(ilitek_data, GET_TP_RES, NULL, outbuf) < 0)
> > +		return -EFAULT;
> > +	if (api_protocol_set_cmd(ilitek_data, GET_IC_MODE, NULL, outbuf) < 0)
> > +		return -EFAULT;
> 
> EFAULT is weird error code to be used here, maybe EIO or EINVAL?

Agreed; will refactor it and return error code from api_* func. directly.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int ilitek_input_dev_init(struct device *dev,
> > +				 struct ilitek_ts_data *ilitek_data)
> > +{
> > +	int ret = 0, i = 0;
> > +	struct input_dev *input = NULL;
> > +
> > +	if (!dev || !ilitek_data)
> > +		return -EINVAL;
> 
> How can either these 2 be NULL?

It should not be NULL in current patch.
Should be removed in v4.

> 
> > +
> > +	input  = input_allocate_device();
> > +	if (!input)
> > +		return -ENOMEM;
> > +
> > +	input->name = ILITEK_TS_NAME;
> > +	input->id.bustype = BUS_I2C;
> > +
> > +	__set_bit(INPUT_PROP_DIRECT, input->propbit);
> > +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > +
> > +	input_set_abs_params(input, ABS_MT_POSITION_X,
> > +			     ilitek_data->screen_min_x,
> > +			     ilitek_data->screen_max_x, 0, 0);
> > +	input_set_abs_params(input, ABS_MT_POSITION_Y,
> > +			     ilitek_data->screen_min_y,
> > +			     ilitek_data->screen_max_y, 0, 0);
> > +	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +	input_set_abs_params(input, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
> 
> Please support common touchscreen properties by using
> touchscreen_parse_properties() and then touchscreen_set_mt_pos() and/or
> touchscreen_report_pos() when reporting contacts.

Agreed; should update driver flow and bindings in v4.

> 
> > +
> > +	input_mt_init_slots(input, ilitek_data->max_tp, INPUT_MT_DIRECT);
> 
> This needs error handling.

Agreed; will update in v4.

> 
> > +
> > +	for (i = 0; i < ilitek_data->keycount; i++)
> > +		set_bit(ilitek_data->keyinfo[i].id & KEY_MAX, input->keybit);
> 
> As I mentioned, soft keys are better handled in userspace.

As replied above, FW of touch device could report key id and region.
Is it not feasible for a touchscreen driver do key handling?

> 
> > +
> > +	ret = input_register_device(input);
> > +	if (ret)
> > +		dev_err(dev, "failed to register input device, err:%d\n", ret);
> > +
> > +	ilitek_data->input_dev = input;
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t ilitek_i2c_isr(int irq, void *dev_id)
> > +{
> > +	struct ilitek_ts_data *ilitek_data = dev_id;
> > +	int ret = 0;
> > +
> > +	if (!ilitek_data->process_and_report) {
> > +		dev_err(&ilitek_data->client->dev, "report touch cb is NULL\n");
> 
> How can this happen?

This driver should support two different protocol formats.
One for 231X/251X, the other for Lego 213X/2316/232X/252X.
This patch currently support Lego series, assigned in ilitek_protocol_init()
Then assign null func. ptr for the other one protocol format.

> 
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	ilitek_irq_disable(ilitek_data);
> 
> You are in the interrupt handler, why do you need to disable interrupts?
> Current one will not be acknowledged until you return from ISR.

To make sure IRQ number is disabled during ISR handling and avoid
ISR re-entry, and the IRQ is requested as one-shot; there should be
no need for irq number disable.

Agreed; should remove it and update in v4.

> 
> > +	mutex_lock(&ilitek_data->ilitek_mutex);
> 
> What does this mutex protect?

Mutex protect I2C write cmd and read data in concurrent situation.
Such as getting X/Y pos. data in IRQ handling and querying FW info.
It may result in double I2C write cmds, then read unexpected data.

However, current patch should not generate I2C concurrent situation.
(already remove the querying FW versiom from I2C comm.)
I think it could be removed, and patched if needed afterward.
Will update in v4.

> 
> > +	ret = ilitek_data->process_and_report(ilitek_data);
> > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > +	ilitek_irq_enable(ilitek_data);
> > +
> > +	if (ret < 0) {
> > +		dev_err(&ilitek_data->client->dev, "process and report point failed, err:%d\n",
> > +			ret);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int ilitek_request_irq(struct device *dev,
> > +			      struct ilitek_ts_data *ilitek_data)
> > +{
> > +	int ret = 0;
> > +
> > +	spin_lock_init(&ilitek_data->irq_lock);
> > +	ilitek_data->irq_status = true;
> > +
> > +	ret = request_threaded_irq(ilitek_data->client->irq, NULL,
> > +				   ilitek_i2c_isr,
> > +				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> 
> Please reply on platform (ACPI, DT, etc) to set up proper trigger for
> the interrupt line. Additionally, you should prefer having level
> interrupt, rather than edge, or you risk missing interrupts.

Agreed; will update here in v4.
Also, bindings will be modified to active_level_low in v4.

> > +				   "ilitek_touch_irq", ilitek_data);
> > +	if (ret)
> > +		dev_err(dev, "request threaded irq failed, err:%d\n", ret);
> 
> I am not sure what is the value of this wrapper.

It put irq-related manipulation together from probe flow.
And agreed; it's less necessary.
Will remove this wrapper in v4.

> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t firmware_version_show(struct device *dev,
> > +				     struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > +	int ret = 0;
> > +	u8 outbuf[64] = {0};
> > +
> > +	ilitek_irq_disable(ilitek_data);
> > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > +	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);
> 
> Do you need to query this all the time? Is it possible to collect this
> data at boot or after firmware flash and store for subsequent use?

Agreed; just return the stored FW version (queried at boot or after FW update).
The same modification in showing product_id below.
Will update in v4.

[Question]
Is it a kind of consensus that don't do I2C transaction from show/read function via "cat"?

> > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > +	ilitek_irq_enable(ilitek_data);
> > +
> > +	if (ret < 0) {
> > +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return scnprintf(buf, PAGE_SIZE,
> > +			 "ILITEK FW ver: %d.%d.%d.%d.%d.%d.%d.%d\n",
> > +			 ilitek_data->firmware_ver[0],
> > +			 ilitek_data->firmware_ver[1],
> > +			 ilitek_data->firmware_ver[2],
> > +			 ilitek_data->firmware_ver[3],
> > +			 ilitek_data->firmware_ver[4],
> > +			 ilitek_data->firmware_ver[5],
> > +			 ilitek_data->firmware_ver[6],
> > +			 ilitek_data->firmware_ver[7]);
> > +}
> > +static DEVICE_ATTR_RO(firmware_version);
> > +
> > +static ssize_t product_id_show(struct device *dev,
> > +			       struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > +	int ret = 0;
> > +	u8 outbuf[64] = {0};
> > +
> > +	ilitek_irq_disable(ilitek_data);
> > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > +	ret = api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf);
> > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > +	ilitek_irq_enable(ilitek_data);
> > +
> > +	if (ret < 0) {
> > +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "MCU: %s\n", ilitek_data->product_id);
> > +}
> > +static DEVICE_ATTR_RO(product_id);
> > +
> > +static struct attribute *ilitek_sysfs_attrs[] = {
> > +	&dev_attr_firmware_version.attr,
> > +	&dev_attr_product_id.attr,
> > +	NULL
> > +};
> > +
> > +static struct attribute_group ilitek_attrs_group[] = {
> > +	{.attrs = ilitek_sysfs_attrs},
> > +};
> > +
> > +static int ilitek_create_sysfsnode(struct device *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	ret = sysfs_create_group(&dev->kobj, ilitek_attrs_group);
> > +	if (ret)
> > +		dev_err(dev, "sysfs create group failed, err:%d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ilitek_ts_i2c_probe(struct i2c_client *client,
> > +			       const struct i2c_device_id *id)
> > +{
> > +	struct ilitek_ts_data *ilitek_data = NULL;
> > +	struct device *dev = &client->dev;
> > +	int ret = 0;
> > +
> > +	ilitek_data = kzalloc(sizeof(*ilitek_data), GFP_KERNEL);
> > +	if (!ilitek_data)
> > +		return -ENOMEM;
> 
> Please use devm APIs to make sure resources are freed automatically
> after errors. It will also allow you to get rid of
> ilitek_ts_i2c_remove().

Agreed; will update probe and remove function in v4.

> > +
> > +	ilitek_data->client = client;
> > +	i2c_set_clientdata(client, ilitek_data);
> > +
> > +	mutex_init(&ilitek_data->ilitek_mutex);
> > +
> > +	ret = ilitek_request_gpio(dev, ilitek_data);
> > +	if (ret < 0) {
> > +		dev_err(dev, "request gpio failed, err:%d", ret);
> > +		goto err_put_gpiod;
> > +	}
> > +
> > +	ilitek_reset(ilitek_data, 1000, true);
> > +	ret = ilitek_protocol_init(ilitek_data);
> > +	if (ret < 0) {
> > +		dev_err(dev, "protocol init failed, err:%d", ret);
> > +		goto err_put_gpiod;
> > +	}
> > +
> > +	ret = ilitek_read_tp_info(ilitek_data, true);
> > +	if (ret < 0) {
> > +		dev_err(dev, "read tp info failed, err:%d", ret);
> > +		goto err_put_gpiod;
> > +	}
> > +
> > +	ret = ilitek_input_dev_init(dev, ilitek_data);
> > +	if (ret < 0) {
> > +		dev_err(dev, "input dev init failed, err:%d", ret);
> > +		goto err_free_input_dev;
> > +	}
> > +
> > +	ret = ilitek_request_irq(dev, ilitek_data);
> > +	if (ret < 0) {
> > +		dev_err(dev, "request irq failed, err:%d", ret);
> > +		goto err_free_input_dev;
> > +	}
> > +
> > +	ret = ilitek_create_sysfsnode(dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "create sysfs node failed, err:%d", ret);
> > +		goto err_free_irq;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_free_irq:
> > +	free_irq(ilitek_data->client->irq, ilitek_data);
> > +
> > +	input_unregister_device(ilitek_data->input_dev);
> > +err_free_input_dev:
> > +	if (ilitek_data->input_dev)
> > +		input_free_device(ilitek_data->input_dev);
> > +err_put_gpiod:
> > +	if (!IS_ERR(ilitek_data->reset_gpio))
> > +		gpiod_put(ilitek_data->reset_gpio);
> > +	kfree(ilitek_data);
> > +	return ret;
> > +}
> > +
> > +static int ilitek_ts_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > +
> > +	sysfs_remove_group(&dev->kobj, ilitek_attrs_group);
> > +
> > +	free_irq(ilitek_data->client->irq, ilitek_data);
> > +
> > +	input_unregister_device(ilitek_data->input_dev);
> > +
> > +	if (!IS_ERR(ilitek_data->reset_gpio))
> > +		gpiod_put(ilitek_data->reset_gpio);
> > +
> > +	kfree(ilitek_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ilitek_ts_i2c_id[] = {
> > +	{ILITEK_TS_NAME, 0},
> > +	{ /* not omitted */ },
> 
> What does "not omitted" mean in this context?

It reminds developers that the empty brackets should not be removed.
Will remove the comment in v4.

> > +};
> > +MODULE_DEVICE_TABLE(i2c, ilitek_ts_i2c_id);
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id ilitekts_acpi_id[] = {
> > +	{ "ILTK0001", 0 },
> > +	{ /* not omitted */ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, ilitekts_acpi_id);
> > +#endif
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id ilitek_ts_i2c_match[] = {
> > +	{.compatible = "ilitek,ili2520",},
> > +	{.compatible = "ilitek,ili2510",},
> > +	{ /* not omitted */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ilitek_ts_i2c_match);
> > +#endif
> > +
> > +static struct i2c_driver ilitek_ts_i2c_driver = {
> > +	.driver = {
> > +		.name = ILITEK_TS_NAME,
> > +		.owner = THIS_MODULE,
> 
> No need to set owner.

Agreed; will update.

> > +		.of_match_table = of_match_ptr(ilitek_ts_i2c_match),
> > +		.acpi_match_table = ACPI_PTR(ilitekts_acpi_id),
> 
> You need power management (suspend/resume) handling too.
> 

Agreed; will update.

> > +	},
> > +	.probe = ilitek_ts_i2c_probe,
> > +	.remove = ilitek_ts_i2c_remove,
> > +	.id_table = ilitek_ts_i2c_id,
> > +};
> > +
> > +module_i2c_driver(ilitek_ts_i2c_driver);
> > +
> > +MODULE_AUTHOR("ILITEK");
> > +MODULE_DESCRIPTION("ILITEK I2C Touchscreen Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> > 
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Many thanks,

--
Joe Hung

Re: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Dmitry Torokhov]

Hi Joe,

On Tue, Feb 23, 2021 at 12:18:31PM +0000, Joe Hung (洪銘陽) wrote:
> Hi Dmitry,
> 
> Many thanks for your kind review, most of comments sound great to me.
> There some questions with "[Question]" prefix, response as below.
> 
> Looking forward to further discussion.
> 
> On Sat, Feb 20, 2021 at 21:51:58PM -0800, Dmitry Torokhov wrote:
> > Hi Joe,
> > 
> > On Thu, Feb 18, 2021 at 02:44:35PM +0800, Joe Hung wrote:
> > > Add support for ILITEK Lego series of touch devices.
> > > Lego series includes ILITEK 213X/2316/232X/252X.
> > > Designed in compatible way for ILITEK 23XX/251X ICs.
> > > 
> > > Tested and passed with evaluation board with ILI2520 IC.
> > 
> > Thank you for your submission.
> > 
> > > 
> > > Signed-off-by: Joe Hung <joe_hung@ilitek.com>
> > > ---
> > > Changes in v3:
> > >   - None
> > > 
> > > Changes in v2:
> > >   - Remove irq-gpio and related flow
> > > 
> > >  drivers/input/touchscreen/Kconfig         |   12 +
> > >  drivers/input/touchscreen/Makefile        |    1 +
> > >  drivers/input/touchscreen/ilitek_ts_i2c.c | 1010 +++++++++++++++++++++
> > >  3 files changed, 1023 insertions(+)
> > >  create mode 100644 drivers/input/touchscreen/ilitek_ts_i2c.c
> > > 
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index f012fe746df0..9dea7fd0d94f 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called zinitix.
> > > 
> > > +config TOUCHSCREEN_ILITEK
> > > +	tristate "Ilitek I2C 23XX/25XX/Lego Series Touch ICs"
> > > +	depends on I2C
> > > +	help
> > > +	  Say Y here if you have touchscreen with ILITEK touch IC,
> > > +	  it supports 23XX/25XX/other Lego series.
> > > +
> > > +	  If unsure, say N.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called ilitek_ts_i2c.
> > > +
> > >  endif
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 6233541e9173..6749bdc42f41 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
> > >  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
> > >  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
> > >  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> > > +obj-$(CONFIG_TOUCHSCREEN_ILITEK)        += ilitek_ts_i2c.o
> > > diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> > > new file mode 100644
> > > index 000000000000..72cb7c82170c
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> > > @@ -0,0 +1,1010 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * ILITEK Touch IC driver for 23XX, 25XX and Lego series
> > > + *
> > > + * Copyright (C) 2011 ILI Technology Corporation.
> > > + * Copyright (C) 2020 Luca Hsu <luca_hsu@ilitek.com>
> > > + * Copyright (C) 2021 Joe Hung <joe_hung@ilitek.com>
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/mt.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/vmalloc.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/kthread.h>
> > 
> > Are you sure you need this include?
> 
> Agreed; will remove it in v4.
> 
> > > +#include <linux/sched.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/cdev.h>
> > 
> > You do not create character devices in this driver.
> 
> Agreed; will remove it in v4.
> 
> > > +#include <linux/uaccess.h>
> > > +#include <linux/version.h>
> > 
> > Why do you need this include?
> 
> Agreed; will remove it in v4.
> 
> > > +#include <linux/rtc.h>
> > 
> > I do not think this driver uses rtc.
> 
> Agreed; will remove it in v4.
> 
> > > +#include <linux/gpio.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/proc_fs.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/pm_runtime.h>
> > 
> > I do not think your driver uses runtime PM.
> > 
> 
> Agreed; will remove it in v4.
> 
> > > +#include <linux/ctype.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/acpi.h>
> > 
> > Please double-check that you include only facilities that the driver
> > actually uses.
> 
> Agreed; will do.
> 
> > 
> > > +
> > > +#define ILITEK_TS_NAME					"ilitek_ts"
> > > +#define BL_V1_8						(0x10800)
> > > +#define BL_V1_7						(0x10700)
> > > +#define BL_V1_6						(0x10600)
> > > +
> > > +#define ILITEK_TP_CMD_GET_TP_RES			(0x20)
> > > +#define ILITEK_TP_CMD_GET_SCRN_RES			(0x21)
> > > +#define ILITEK_TP_CMD_GET_KEY_INFO			(0x22)
> > > +#define ILITEK_TP_CMD_GET_FW_VER			(0x40)
> > > +#define ILITEK_TP_CMD_GET_PRL_VER			(0x42)
> > > +#define ILITEK_TP_CMD_GET_MCU_VER			(0x61)
> > > +#define ILITEK_TP_CMD_GET_IC_MODE			(0xC0)
> > > +
> > > +#define REPORT_ADDRESS_COUNT				(61)
> > > +#define ILITEK_SUPPORT_MAX_POINT			(40)
> > > +#define ILITEK_SUPPORT_MAX_KEY_CNT			(50)
> > 
> > No need to use parenthesis for simple scalar values.
> > 
> 
> Agreed; will modified here in v4.
> 
> > > +
> > > +#define CEIL(n, d)	(((n) % (d)) ? ((n) / (d) + 1) : ((n) / (d)))
> > 
> > I think you want standard DIV_ROUND_UP().
> 
> Agreed; will use DIV_ROUND_UP directly in v4.
> 
> > 
> > > +
> > > +struct ilitek_protocol_info {
> > 
> > Let's use ili23xx instead of ilitek prefix throughout the file. Or
> > whatever the first generation of the product supported by this driver.
> > 
> 
> This driver is planned to support 213X/23XX/25XX.

OK, then I'd use ili213x as the prefix and would have source as
ili213x.c

> 
> [Question]
> Was wondering why ilitek prefix is not feasible ?

Just because the driver does not support all Ilitek devices and we
already have drivers/input/touchscreen/ili210x.c

> Is it fine that 23XX prefix that still support 25XX ICs ?

That is fine.

> 
> > > +	s32 ver;
> > > +	u8 ver_major;
> > > +	u8 ver_mid;
> > > +	u8 ver_minor;
> > > +};
> > > +
> > > +struct ilitek_key_info {
> > > +	s32 id;
> > > +	s32 x;
> > > +	s32 y;
> > > +	s32 status;
> > > +};
> > > +
> > > +struct ilitek_touch_info {
> > > +	u16 id;
> > > +	u16 x;
> > > +	u16 y;
> > > +	u16 p;
> > > +	u16 w;
> > > +	u16 h;
> > > +	u16 status;
> > > +};
> > > +
> > > +struct ilitek_ts_data {
> > > +	struct i2c_client		*client;
> > > +	struct gpio_desc		*reset_gpio;
> > > +	struct input_dev		*input_dev;
> > > +
> > > +	struct mutex			ilitek_mutex;
> > > +	spinlock_t			irq_lock;
> > > +	bool				irq_status;
> > > +	int (*process_and_report)(struct ilitek_ts_data *ilitek_data);
> > > +
> > > +	struct PROTOCOL_MAP		*ptl_cb_func;
> > > +	struct ilitek_protocol_info	ptl;
> > > +
> > > +
> > > +	struct ilitek_key_info		keyinfo[ILITEK_SUPPORT_MAX_KEY_CNT];
> > > +	struct ilitek_touch_info	tpinfo[ILITEK_SUPPORT_MAX_POINT];
> > > +
> > > +	bool				is_touched;
> > > +	bool				touch_key_hold_press;
> > > +
> > > +	bool				ilitek_repeat_start;
> > > +
> > > +	char				product_id[30];
> > > +	u8				mcu_ver[2];
> > > +	u8				ic_mode;
> > > +	u8				firmware_ver[8];
> > > +
> > > +	s32				reset_time;
> > > +	s32				tp_max_x;
> > > +	s32				tp_max_y;
> > > +	s32				screen_max_x;
> > > +	s32				screen_max_y;
> > > +	s32				screen_min_x;
> > > +	s32				screen_min_y;
> > > +	s32				max_tp;
> > > +	s32				x_ch;
> > > +	s32				y_ch;
> > > +	s32				slave_count;
> > 
> > What is the importance of this?
> 
> slave_count is different among different ILITEK touch ICs.
> It would be used during FW upgrade.
> We needs FW upgrade touch IC's slave also.

I do not see it currently being used anywhere, that is why I question.
If it is needed for future enhancements, I would prefer it was added
with them, not ahead of time.

> 
> > > +	s32				format;
> > > +	s32				keycount;
> > > +	s32				key_xlen;
> > > +	s32				key_ylen;
> > > +};
> > > +
> > > +enum ilitek_cmds_v6 {
> > > +	GET_PTL_VER = 0,
> > > +	GET_FW_VER,
> > > +	GET_SCRN_RES,
> > > +	GET_TP_RES,
> > > +	GET_IC_MODE,
> > > +	GET_MCU_VER,
> > > +	GET_KEY_INFO,
> > > +
> > > +	MAX_CMD_CNT	/* ALWAYS keep at the end */
> > > +};
> > > +
> > > +typedef int protocol_func(struct ilitek_ts_data *ilitek_data,
> > > +			  u16 cmd, uint8_t *inbuf, uint8_t *outbuf);
> > 
> > In kernel please use u8 instead of uint8_t, etc.
> 
> Agreed; should correct it in v4.
> 
> > 
> > > +
> > > +struct PROTOCOL_MAP {
> > > +	u16 cmd;
> > > +	const char *name;
> > > +	protocol_func *func;
> > > +};
> > > +
> > > +/* ILITEK I2C R/W APIs */
> > > +static int ilitek_i2c_transfer(struct i2c_client *client,
> > > +			       struct i2c_msg *msgs, int cnt)
> > > +{
> > > +	int ret = 0, retry = 4;
> > > +
> > > +	while (retry--) {
> > > +		ret = i2c_transfer(client->adapter, msgs, cnt);
> > > +		if (ret >= 0)
> > > +			break;
> > > +		mdelay(20);
> > > +	}
> > 
> > Why does it this device need retries?
> > 
> 
> I2C transfer may failed cause FW not ready for some reason.
> It should be fine for basic probe/ISR flow, and risky for FW update.
> 
> [Question]
> Should it be removed, and patched with FW update patch ?

I think I would prefer firmware update executing retries as needed, and
common paths not having them.

> 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ilitek_data,
> > > +				     u8 *cmd, int write_len, int delay,
> > > +				     u8 *data, int read_len)
> > > +{
> > > +	int ret = 0;
> > > +	struct i2c_client *client = ilitek_data->client;
> > > +	struct i2c_msg msgs[2] = {
> > > +		{.addr = client->addr, .flags = 0,
> > > +		 .len = write_len, .buf = cmd,},
> > > +		{.addr = client->addr, .flags = I2C_M_RD,
> > > +		 .len = read_len, .buf = data,}
> > > +	};
> > > +
> > > +	if (ilitek_data->ilitek_repeat_start && delay == 0 &&
> > 
> > Where is this being set? As far as I understand, repeated start is
> > property of the i2c controller, not peripheral device.
> > 
> > Also, please see if you can switch to using regmap, as it should take
> > care of selecting right mode, depending on controller's capabilities.
> > 
> 
> Some ilitek touch ICs may not support "I2C repeat start".

OK, then we need to somehow activate this feature, because as it is it
is a dead code.

> And agreed; it should be removed here in v4.
> 
> > > +		write_len > 0 && read_len > 0) {
> > > +		ret = ilitek_i2c_transfer(client, msgs, 2);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	} else {
> > > +		if (write_len > 0) {
> > > +			ret = ilitek_i2c_transfer(client, msgs, 1);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +		}
> > > +		if (delay > 0)
> > > +			mdelay(delay);
> > > +		if (read_len > 0) {
> > > +			ret = ilitek_i2c_transfer(client, msgs + 1, 1);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void ilitek_irq_enable(struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	unsigned long irqflag = 0;
> > > +
> > > +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> > > +	if (!ilitek_data->irq_status) {
> > > +		enable_irq(ilitek_data->client->irq);
> > > +		ilitek_data->irq_status = true;
> > > +	}
> > > +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
> > 
> > The driver should know at any time whether interrupts are disabled or
> > not, so I'd prefer if we did not have this wrapper, and the one below.
> > 
> 
> Agreed; will remove in v4.
> 
> > > +}
> > > +
> > > +static void ilitek_irq_disable(struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	unsigned long irqflag = 0;
> > > +
> > > +	spin_lock_irqsave(&ilitek_data->irq_lock, irqflag);
> > > +	if (ilitek_data->irq_status) {
> > > +		disable_irq_nosync(ilitek_data->client->irq);
> > > +		ilitek_data->irq_status = false;
> > > +	}
> > > +	spin_unlock_irqrestore(&ilitek_data->irq_lock, irqflag);
> > > +}
> > > +
> > > +/* ILITEK ISR APIs */
> > > +static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
> > > +			    int x, int y)
> > > +{
> > > +	int j = 0;
> > > +	int x_min;
> > > +	int x_max;
> > > +	int y_min;
> > > +	int y_max;
> > > +
> > > +	for (j = 0; j < ilitek_data->keycount; j++) {
> > > +		x_min = ilitek_data->keyinfo[j].x;
> > > +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> > > +		y_min = ilitek_data->keyinfo[j].y;
> > > +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> > > +
> > > +		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
> > > +			input_report_key(ilitek_data->input_dev,
> > > +					 ilitek_data->keyinfo[j].id, 1);
> > > +
> > > +			ilitek_data->keyinfo[j].status = 1;
> > > +			ilitek_data->touch_key_hold_press = true;
> > > +			ilitek_data->is_touched = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +}
> > 
> > This sounds like handling of "soft keys" which is better left for
> > userspace. OTOH the Linux keycode seems to be coming from controller
> > firmware. This is really weird.
> 
> It handles keys which key ID and region are get from touch device firmware.
> Then check region of touch points to trigger key event.
> 
> [Question]
> Is it not feasible for a touchscreen driver do key handling?

Typically this is better handled in userspace, as then it works across
various touch controllers, vs. needing this special one.

> 
> > > +
> > > +static int ilitek_key_release(struct ilitek_ts_data *ilitek_data,
> > > +			      int x, int y, bool check_point)
> > > +{
> > > +	int j = 0;
> > > +	int x_min;
> > > +	int x_max;
> > > +	int y_min;
> > > +	int y_max;
> > > +
> > > +	for (j = 0; j < ilitek_data->keycount; j++) {
> > > +		x_min = ilitek_data->keyinfo[j].x;
> > > +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> > > +		y_min = ilitek_data->keyinfo[j].y;
> > > +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> > > +
> > > +		if (ilitek_data->keyinfo[j].status == 1) {
> > > +			if (!check_point ||
> > > +			    (x < x_min || x > x_max ||
> > > +			     y < y_min || y > y_max)) {
> > > +				input_report_key(ilitek_data->input_dev,
> > > +						 ilitek_data->keyinfo[j].id, 0);
> > > +				ilitek_data->keyinfo[j].status = 0;
> > > +				ilitek_data->touch_key_hold_press = false;
> > > +			}
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static void ilitek_touch_down(struct ilitek_ts_data *ilitek_data,
> > > +			      int id, int x, int y, int p, int h, int w)
> > > +{
> > > +	struct input_dev *input = ilitek_data->input_dev;
> > > +
> > > +	input_report_key(input, BTN_TOUCH, 1);
> > > +
> > > +	input_mt_slot(input, id);
> > > +	input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
> > > +
> > > +	input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> > > +	input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> > > +
> > > +	input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, h);
> > > +	input_event(input, EV_ABS, ABS_MT_WIDTH_MAJOR, w);
> > > +
> > > +	ilitek_data->tpinfo[id].status = 1;
> > > +	ilitek_data->tpinfo[id].id = id;
> > > +	ilitek_data->tpinfo[id].x = x;
> > > +	ilitek_data->tpinfo[id].y = y;
> > > +	ilitek_data->tpinfo[id].p = p;
> > > +	ilitek_data->tpinfo[id].h = h;
> > > +	ilitek_data->tpinfo[id].w = w;
> > > +	ilitek_data->is_touched = true;
> > > +}
> > > +
> > > +static void ilitek_touch_release(struct ilitek_ts_data *ilitek_data, int id)
> > > +{
> > > +	struct input_dev *input = ilitek_data->input_dev;
> > > +
> > > +	if (ilitek_data->tpinfo[id].status == 1) {
> > > +		input_mt_slot(input, id);
> > > +		input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> > > +		ilitek_data->tpinfo[id].status = 0;
> > > +	}
> > > +}
> > > +
> > > +static void ilitek_touch_release_all(struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	struct input_dev *input = ilitek_data->input_dev;
> > > +	int i = 0;
> > > +
> > > +	for (i = 0; i < ilitek_data->max_tp; i++)
> > > +		ilitek_touch_release(ilitek_data, i);
> > > +	input_report_key(input, BTN_TOUCH, 0);
> > > +	input_sync(input);
> > > +
> > > +	ilitek_data->is_touched = false;
> > > +}
> > > +
> > > +static int ilitek_process_and_report_v6(struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[512] = {0};
> > > +	int packet_len = 5;
> > > +	int packet_max_point = 10;
> > > +	int report_max_point = 6;
> > > +	int release_point = 0;
> > > +	int i = 0, count = 0;
> > > +	struct input_dev *input = ilitek_data->input_dev;
> > > +	struct device *dev = &ilitek_data->client->dev;
> > > +	u16 x, y, p = 10, w = 10, h = 10, status, id;
> > > +
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0, buf, 64);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "get touch info failed, err:%d\n", ret);
> > > +		goto err_release_touch_and_key;
> > > +	}
> > > +
> > > +	report_max_point = buf[REPORT_ADDRESS_COUNT];
> > > +	if (report_max_point > ilitek_data->max_tp) {
> > > +		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
> > > +			report_max_point, ilitek_data->max_tp);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	count = CEIL(report_max_point, packet_max_point);
> > > +	for (i = 1; i < count; i++) {
> > > +		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
> > > +			buf + i * 64, 64);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "get touch info. err, count=%d\n", count);
> > > +			goto err_release_touch_and_key;
> > > +		}
> > > +	}
> > 
> > This means you have 1 + count transactions on i2c bus. Have you
> > considered reading max number of bytes in one transaction, and then soft
> > out how much of received data is valid?
> > 
> 
> Due to limitation on FW of touch device, data byte for each I2C transaction
> is limited. It would lead to I2C communication issue when large count of bytes
> are request.
> 
> Also, for only a few touch points,
> It should be less efficient to query max number of bytes.

I was thinking about having one read transaction for up to <max bytes
your devices can transmit>, then look at the header and decide how many,
if any, more requests you would need to issue. This way for majority of
cases I believe you would need only one read per interrupt.

> 
> > > +
> > > +	for (i = 0; i < report_max_point; i++) {
> > > +		status = buf[i * packet_len + 1] & 0x40;
> > > +		id = buf[i * packet_len + 1] & 0x3F;
> > > +
> > > +		if (!status) {
> > > +			release_point++;
> > > +			ilitek_touch_release(ilitek_data, id);
> > > +			continue;
> > > +		}
> > > +
> > > +		x = (buf[i * packet_len + 3] << 8) + buf[i * packet_len + 2];
> > > +		y = (buf[i * packet_len + 5] << 8) + buf[i * packet_len + 4];
> > 
> > 		x = le16_to_cpup(....);
> > 
> > Please adjust here and elsewhere. Also, if data may be unaligned, please
> > use get_unaligned_le16() instead.
> > 
> 
> Agreed; will check and update elsewhere in v4.
> 
> > > +
> > > +		if (!ilitek_data->is_touched)
> > > +			ilitek_key_down(ilitek_data, x, y);
> > > +
> > > +		if (!ilitek_data->touch_key_hold_press) {
> > > +			if (x > ilitek_data->screen_max_x ||
> > > +			    y > ilitek_data->screen_max_y ||
> > > +			    x < ilitek_data->screen_min_x ||
> > > +			    y < ilitek_data->screen_min_y)
> > > +				continue;
> > > +
> > > +			ilitek_touch_down(ilitek_data, id, x, y, p, h, w);
> > > +		} else {
> > > +			ilitek_key_release(ilitek_data, x, y, 1);
> > > +		}
> > > +	}
> > > +
> > > +	if (release_point == report_max_point) {
> > > +		ilitek_touch_release_all(ilitek_data);
> > > +		ilitek_key_release(ilitek_data, 0, 0, 0);
> > > +		ilitek_data->is_touched = false;
> > > +	}
> > > +	input_sync(input);
> > > +	return 0;
> > > +
> > > +err_release_touch_and_key:
> > > +	ilitek_touch_release_all(ilitek_data);
> > > +	ilitek_key_release(ilitek_data, 0, 0, 0);
> > > +	ilitek_data->is_touched = false;
> > > +	input_sync(input);
> > > +	return ret;
> > > +}
> > > +
> > > +
> > > +/* APIs of cmds for ILITEK Touch IC */
> > > +static int api_protocol_set_cmd(struct ilitek_ts_data *ilitek_data,
> > > +				u16 idx, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	u16 cmd;
> > > +	int ret = 0;
> > > +
> > > +	if (!ilitek_data || !ilitek_data->ptl_cb_func)
> > > +		return -EINVAL;
> > > +
> > > +	if (idx >= MAX_CMD_CNT)
> > > +		return -EINVAL;
> > > +
> > > +	cmd = ilitek_data->ptl_cb_func[idx].cmd;
> > > +	ret = ilitek_data->ptl_cb_func[idx].func(ilitek_data, cmd,
> > > +						 inbuf, outbuf);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int api_protocol_get_ptl_ver(struct ilitek_ts_data *ilitek_data,
> > > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = cmd;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 3);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ilitek_data->ptl.ver = (outbuf[0] << 16) + (outbuf[1] << 8);
> > > +	ilitek_data->ptl.ver_major = outbuf[0];
> > > +	ilitek_data->ptl.ver_mid = outbuf[1];
> > > +	ilitek_data->ptl.ver_minor = outbuf[2];
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_mcu_ver(struct ilitek_ts_data *ilitek_data,
> > > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_MCU_VER;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 32);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	memcpy(ilitek_data->mcu_ver, outbuf, 2);
> > > +	memset(ilitek_data->product_id, 0, sizeof(ilitek_data->product_id));
> > > +	memcpy(ilitek_data->product_id, outbuf+6, 26);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_fw_ver(struct ilitek_ts_data *ilitek_data,
> > > +				   u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_FW_VER;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	memcpy(ilitek_data->firmware_ver, outbuf, 8);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_scrn_res(struct ilitek_ts_data *ilitek_data,
> > > +				     u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_SCRN_RES;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 8);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ilitek_data->screen_min_x = (outbuf[1] << 8) + outbuf[0];
> > > +	ilitek_data->screen_min_y = (outbuf[3] << 8) + outbuf[2];
> > > +	ilitek_data->screen_max_x = (outbuf[5] << 8) + outbuf[4];
> > > +	ilitek_data->screen_max_y = (outbuf[7] << 8) + outbuf[6];
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_key_info_v6(struct ilitek_ts_data *ilitek_data,
> > > +					u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0, i = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_KEY_INFO;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 10,
> > > +					outbuf, ilitek_data->keycount * 5 + 5);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ilitek_data->key_xlen = (outbuf[2] << 8) + outbuf[1];
> > > +	ilitek_data->key_ylen = (outbuf[4] << 8) + outbuf[3];
> > > +
> > > +	for (i = 0; i < ilitek_data->keycount; i++) {
> > > +		ilitek_data->keyinfo[i].id = outbuf[i * 5 + 5];
> > > +		ilitek_data->keyinfo[i].x = (outbuf[i * 5 + 7] << 8) +
> > > +					    outbuf[i * 5 + 6];
> > > +		ilitek_data->keyinfo[i].y = (outbuf[i * 5 + 9] << 8) +
> > > +					    outbuf[i * 5 + 8];
> > > +		ilitek_data->keyinfo[i].status = 0;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_tp_res_v6(struct ilitek_ts_data *ilitek_data,
> > > +				      u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_TP_RES;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 15);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ilitek_data->tp_max_x = (outbuf[1] << 8) + outbuf[0];
> > > +	ilitek_data->tp_max_y = (outbuf[3] << 8) + outbuf[2];
> > > +	ilitek_data->x_ch = (outbuf[5] << 8) + outbuf[4];
> > > +	ilitek_data->y_ch = (outbuf[7] << 8) + outbuf[6];
> > > +	ilitek_data->max_tp = outbuf[8];
> > > +	ilitek_data->keycount = outbuf[9];
> > > +	ilitek_data->slave_count = outbuf[10];
> > > +	ilitek_data->format = outbuf[12];
> > > +
> > > +	if (ilitek_data->keycount > ILITEK_SUPPORT_MAX_KEY_CNT) {
> > > +		dev_err(&ilitek_data->client->dev, "Invalid KEY count:%d from FW\n",
> > > +			ilitek_data->keycount);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (ilitek_data->keycount > 0) {
> > > +		ret = api_protocol_set_cmd(ilitek_data, GET_KEY_INFO,
> > > +					   NULL, outbuf);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (ilitek_data->max_tp > ILITEK_SUPPORT_MAX_POINT) {
> > > +		dev_err(&ilitek_data->client->dev, "Invalid MAX_TP:%d from FW\n",
> > > +			ilitek_data->max_tp);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int api_protocol_get_ic_mode(struct ilitek_ts_data *ilitek_data,
> > > +				    u16 cmd, u8 *inbuf, u8 *outbuf)
> > > +{
> > > +	int ret = 0;
> > > +	u8 buf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	buf[0] = ILITEK_TP_CMD_GET_IC_MODE;
> > > +	ret = ilitek_i2c_write_and_read(ilitek_data, buf, 1, 5, outbuf, 2);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ilitek_data->ic_mode = outbuf[0];
> > > +	return ret;
> > > +}
> > > +
> > > +struct PROTOCOL_MAP ptl_func_map_v3[] = {
> > > +	{ /* V3 cmds */},
> > > +};
> > > +
> > > +struct PROTOCOL_MAP ptl_func_map_v6[] = {
> > > +	[GET_PTL_VER]	= {ILITEK_TP_CMD_GET_PRL_VER, "GET_PTL_VER",
> > > +			   api_protocol_get_ptl_ver},
> > > +	[GET_FW_VER]	= {ILITEK_TP_CMD_GET_FW_VER, "GET_FW_VER",
> > > +			   api_protocol_get_fw_ver},
> > > +	[GET_SCRN_RES]	= {ILITEK_TP_CMD_GET_SCRN_RES, "GET_SCRN_RES",
> > > +			   api_protocol_get_scrn_res},
> > > +	[GET_TP_RES]	= {ILITEK_TP_CMD_GET_TP_RES, "GET_TP_RES",
> > > +			   api_protocol_get_tp_res_v6},
> > > +	[GET_IC_MODE]	= {ILITEK_TP_CMD_GET_IC_MODE, "GET_IC_MODE",
> > > +			   api_protocol_get_ic_mode},
> > > +	[GET_MCU_VER]	= {ILITEK_TP_CMD_GET_MCU_VER, "GET_MOD_NAME",
> > > +			   api_protocol_get_mcu_ver},
> > > +	[GET_KEY_INFO]	= {ILITEK_TP_CMD_GET_KEY_INFO, "GET_KEY_INFO",
> > > +			   api_protocol_get_key_info_v6},
> > > +};
> > > +
> > > +/* Probe APIs */
> > > +static int ilitek_request_gpio(struct device *dev,
> > > +			       struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!dev || !ilitek_data)
> > > +		return -EINVAL;
> > > +
> > > +	ilitek_data->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(ilitek_data->reset_gpio)) {
> > > +		ret = PTR_ERR(ilitek_data->reset_gpio);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void ilitek_reset(struct ilitek_ts_data *ilitek_data,
> > > +			 int delay, bool boot)
> > > +{
> > > +	bool init_stats = ilitek_data->irq_status;
> > > +
> > > +	if (!boot)
> > > +		ilitek_irq_disable(ilitek_data);
> > > +
> > > +	if (ilitek_data->reset_gpio) {
> > > +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> > > +		mdelay(10);
> > > +
> > > +		gpiod_set_value(ilitek_data->reset_gpio, 0);
> > > +		mdelay(10);
> > > +
> > > +		gpiod_set_value(ilitek_data->reset_gpio, 1);
> > > +		mdelay(delay);
> > 
> > I do not believe you mean to leave reset line activei here. Remember,
> > gpiod is logical API, with 1 representing line being active, and 0 -
> > inactive.  gpiolib internally takes into account line polarity, so for
> > active-low GPIOs gpiod_set_value(gpio, 1) will drive the line low.
> 
> Agreed; reset pin should reset touch device when pin logically be low.
> Should modified bindings to be active-low.
> Then leave reset line inactive here after reset.
> Should be updated in v4.
> 
> > 
> > > +	}
> > > +
> > > +	if (!boot && init_stats)
> > > +		ilitek_irq_enable(ilitek_data);
> > > +}
> > > +
> > > +static int ilitek_protocol_init(struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	int ret = 0;
> > > +	u8 outbuf[64] = {0};
> > > +
> > > +	if (!ilitek_data)
> > > +		return -EINVAL;
> > 
> > How can it be NULL?
> 
> It should not be NULL in current patch.
> Should be removed it and elsewhere below in v4.
> > 
> > > +
> > > +	ilitek_data->ptl_cb_func = ptl_func_map_v6;
> > > +
> > > +	ret = api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (ilitek_data->ptl.ver_major == 0x3 ||
> > > +	    ilitek_data->ptl.ver == BL_V1_6 ||
> > > +	    ilitek_data->ptl.ver == BL_V1_7) {
> > > +		ilitek_data->process_and_report = NULL;
> > > +		ilitek_data->ptl_cb_func = NULL;
> > > +		ilitek_data->reset_time = 200;
> > > +	} else {
> > > +		ilitek_data->process_and_report = ilitek_process_and_report_v6;
> > > +		ilitek_data->ptl_cb_func = ptl_func_map_v6;
> > > +		ilitek_data->reset_time = 600;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ilitek_read_tp_info(struct ilitek_ts_data *ilitek_data, bool boot)
> > > +{
> > > +	u8 outbuf[256] = {0};
> > > +
> > > +	if (api_protocol_set_cmd(ilitek_data, GET_PTL_VER, NULL, outbuf) < 0)
> > > +		return -EFAULT;
> > > +	if (api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf) < 0)
> > > +		return -EFAULT;
> > > +	if (api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf) < 0)
> > > +		return -EFAULT;
> > > +	if (boot && api_protocol_set_cmd(ilitek_data, GET_SCRN_RES, NULL,
> > > +					 outbuf) < 0)
> > > +		return -EFAULT;
> > > +	if (api_protocol_set_cmd(ilitek_data, GET_TP_RES, NULL, outbuf) < 0)
> > > +		return -EFAULT;
> > > +	if (api_protocol_set_cmd(ilitek_data, GET_IC_MODE, NULL, outbuf) < 0)
> > > +		return -EFAULT;
> > 
> > EFAULT is weird error code to be used here, maybe EIO or EINVAL?
> 
> Agreed; will refactor it and return error code from api_* func. directly.
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ilitek_input_dev_init(struct device *dev,
> > > +				 struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	int ret = 0, i = 0;
> > > +	struct input_dev *input = NULL;
> > > +
> > > +	if (!dev || !ilitek_data)
> > > +		return -EINVAL;
> > 
> > How can either these 2 be NULL?
> 
> It should not be NULL in current patch.
> Should be removed in v4.
> 
> > 
> > > +
> > > +	input  = input_allocate_device();
> > > +	if (!input)
> > > +		return -ENOMEM;
> > > +
> > > +	input->name = ILITEK_TS_NAME;
> > > +	input->id.bustype = BUS_I2C;
> > > +
> > > +	__set_bit(INPUT_PROP_DIRECT, input->propbit);
> > > +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > > +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > > +
> > > +	input_set_abs_params(input, ABS_MT_POSITION_X,
> > > +			     ilitek_data->screen_min_x,
> > > +			     ilitek_data->screen_max_x, 0, 0);
> > > +	input_set_abs_params(input, ABS_MT_POSITION_Y,
> > > +			     ilitek_data->screen_min_y,
> > > +			     ilitek_data->screen_max_y, 0, 0);
> > > +	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > > +	input_set_abs_params(input, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
> > 
> > Please support common touchscreen properties by using
> > touchscreen_parse_properties() and then touchscreen_set_mt_pos() and/or
> > touchscreen_report_pos() when reporting contacts.
> 
> Agreed; should update driver flow and bindings in v4.
> 
> > 
> > > +
> > > +	input_mt_init_slots(input, ilitek_data->max_tp, INPUT_MT_DIRECT);
> > 
> > This needs error handling.
> 
> Agreed; will update in v4.
> 
> > 
> > > +
> > > +	for (i = 0; i < ilitek_data->keycount; i++)
> > > +		set_bit(ilitek_data->keyinfo[i].id & KEY_MAX, input->keybit);
> > 
> > As I mentioned, soft keys are better handled in userspace.
> 
> As replied above, FW of touch device could report key id and region.
> Is it not feasible for a touchscreen driver do key handling?
> 
> > 
> > > +
> > > +	ret = input_register_device(input);
> > > +	if (ret)
> > > +		dev_err(dev, "failed to register input device, err:%d\n", ret);
> > > +
> > > +	ilitek_data->input_dev = input;
> > > +	return ret;
> > > +}
> > > +
> > > +static irqreturn_t ilitek_i2c_isr(int irq, void *dev_id)
> > > +{
> > > +	struct ilitek_ts_data *ilitek_data = dev_id;
> > > +	int ret = 0;
> > > +
> > > +	if (!ilitek_data->process_and_report) {
> > > +		dev_err(&ilitek_data->client->dev, "report touch cb is NULL\n");
> > 
> > How can this happen?
> 
> This driver should support two different protocol formats.
> One for 231X/251X, the other for Lego 213X/2316/232X/252X.
> This patch currently support Lego series, assigned in ilitek_protocol_init()
> Then assign null func. ptr for the other one protocol format.
> 
> > 
> > > +		return IRQ_NONE;
> > > +	}
> > > +
> > > +	ilitek_irq_disable(ilitek_data);
> > 
> > You are in the interrupt handler, why do you need to disable interrupts?
> > Current one will not be acknowledged until you return from ISR.
> 
> To make sure IRQ number is disabled during ISR handling and avoid
> ISR re-entry, and the IRQ is requested as one-shot; there should be
> no need for irq number disable.
> 
> Agreed; should remove it and update in v4.
> 
> > 
> > > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > 
> > What does this mutex protect?
> 
> Mutex protect I2C write cmd and read data in concurrent situation.
> Such as getting X/Y pos. data in IRQ handling and querying FW info.
> It may result in double I2C write cmds, then read unexpected data.
> 
> However, current patch should not generate I2C concurrent situation.
> (already remove the querying FW versiom from I2C comm.)
> I think it could be removed, and patched if needed afterward.
> Will update in v4.
> 
> > 
> > > +	ret = ilitek_data->process_and_report(ilitek_data);
> > > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > > +	ilitek_irq_enable(ilitek_data);
> > > +
> > > +	if (ret < 0) {
> > > +		dev_err(&ilitek_data->client->dev, "process and report point failed, err:%d\n",
> > > +			ret);
> > > +		return IRQ_NONE;
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int ilitek_request_irq(struct device *dev,
> > > +			      struct ilitek_ts_data *ilitek_data)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	spin_lock_init(&ilitek_data->irq_lock);
> > > +	ilitek_data->irq_status = true;
> > > +
> > > +	ret = request_threaded_irq(ilitek_data->client->irq, NULL,
> > > +				   ilitek_i2c_isr,
> > > +				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > 
> > Please reply on platform (ACPI, DT, etc) to set up proper trigger for
> > the interrupt line. Additionally, you should prefer having level
> > interrupt, rather than edge, or you risk missing interrupts.
> 
> Agreed; will update here in v4.
> Also, bindings will be modified to active_level_low in v4.
> 
> > > +				   "ilitek_touch_irq", ilitek_data);
> > > +	if (ret)
> > > +		dev_err(dev, "request threaded irq failed, err:%d\n", ret);
> > 
> > I am not sure what is the value of this wrapper.
> 
> It put irq-related manipulation together from probe flow.
> And agreed; it's less necessary.
> Will remove this wrapper in v4.
> 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static ssize_t firmware_version_show(struct device *dev,
> > > +				     struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > > +	int ret = 0;
> > > +	u8 outbuf[64] = {0};
> > > +
> > > +	ilitek_irq_disable(ilitek_data);
> > > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > > +	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);
> > 
> > Do you need to query this all the time? Is it possible to collect this
> > data at boot or after firmware flash and store for subsequent use?
> 
> Agreed; just return the stored FW version (queried at boot or after FW update).
> The same modification in showing product_id below.
> Will update in v4.
> 
> [Question]
> Is it a kind of consensus that don't do I2C transaction from show/read function via "cat"?

It just seems simpler to me to do this once at probe time, since it is
not going to change unless firmware is updated. If amount of data was
large, I'd consider querying it dynamically in show() as then it would
be more expensive memory-wise to store it on the off-chance userspace
might want to see it.

> 
> > > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > > +	ilitek_irq_enable(ilitek_data);
> > > +
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return scnprintf(buf, PAGE_SIZE,
> > > +			 "ILITEK FW ver: %d.%d.%d.%d.%d.%d.%d.%d\n",
> > > +			 ilitek_data->firmware_ver[0],
> > > +			 ilitek_data->firmware_ver[1],
> > > +			 ilitek_data->firmware_ver[2],
> > > +			 ilitek_data->firmware_ver[3],
> > > +			 ilitek_data->firmware_ver[4],
> > > +			 ilitek_data->firmware_ver[5],
> > > +			 ilitek_data->firmware_ver[6],
> > > +			 ilitek_data->firmware_ver[7]);
> > > +}
> > > +static DEVICE_ATTR_RO(firmware_version);
> > > +
> > > +static ssize_t product_id_show(struct device *dev,
> > > +			       struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > > +	int ret = 0;
> > > +	u8 outbuf[64] = {0};
> > > +
> > > +	ilitek_irq_disable(ilitek_data);
> > > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > > +	ret = api_protocol_set_cmd(ilitek_data, GET_MCU_VER, NULL, outbuf);
> > > +	mutex_unlock(&ilitek_data->ilitek_mutex);
> > > +	ilitek_irq_enable(ilitek_data);
> > > +
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "api set cmd failed, err:%d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return scnprintf(buf, PAGE_SIZE, "MCU: %s\n", ilitek_data->product_id);
> > > +}
> > > +static DEVICE_ATTR_RO(product_id);
> > > +
> > > +static struct attribute *ilitek_sysfs_attrs[] = {
> > > +	&dev_attr_firmware_version.attr,
> > > +	&dev_attr_product_id.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static struct attribute_group ilitek_attrs_group[] = {
> > > +	{.attrs = ilitek_sysfs_attrs},
> > > +};
> > > +
> > > +static int ilitek_create_sysfsnode(struct device *dev)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	ret = sysfs_create_group(&dev->kobj, ilitek_attrs_group);
> > > +	if (ret)
> > > +		dev_err(dev, "sysfs create group failed, err:%d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int ilitek_ts_i2c_probe(struct i2c_client *client,
> > > +			       const struct i2c_device_id *id)
> > > +{
> > > +	struct ilitek_ts_data *ilitek_data = NULL;
> > > +	struct device *dev = &client->dev;
> > > +	int ret = 0;
> > > +
> > > +	ilitek_data = kzalloc(sizeof(*ilitek_data), GFP_KERNEL);
> > > +	if (!ilitek_data)
> > > +		return -ENOMEM;
> > 
> > Please use devm APIs to make sure resources are freed automatically
> > after errors. It will also allow you to get rid of
> > ilitek_ts_i2c_remove().
> 
> Agreed; will update probe and remove function in v4.
> 
> > > +
> > > +	ilitek_data->client = client;
> > > +	i2c_set_clientdata(client, ilitek_data);
> > > +
> > > +	mutex_init(&ilitek_data->ilitek_mutex);
> > > +
> > > +	ret = ilitek_request_gpio(dev, ilitek_data);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "request gpio failed, err:%d", ret);
> > > +		goto err_put_gpiod;
> > > +	}
> > > +
> > > +	ilitek_reset(ilitek_data, 1000, true);
> > > +	ret = ilitek_protocol_init(ilitek_data);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "protocol init failed, err:%d", ret);
> > > +		goto err_put_gpiod;
> > > +	}
> > > +
> > > +	ret = ilitek_read_tp_info(ilitek_data, true);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "read tp info failed, err:%d", ret);
> > > +		goto err_put_gpiod;
> > > +	}
> > > +
> > > +	ret = ilitek_input_dev_init(dev, ilitek_data);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "input dev init failed, err:%d", ret);
> > > +		goto err_free_input_dev;
> > > +	}
> > > +
> > > +	ret = ilitek_request_irq(dev, ilitek_data);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "request irq failed, err:%d", ret);
> > > +		goto err_free_input_dev;
> > > +	}
> > > +
> > > +	ret = ilitek_create_sysfsnode(dev);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "create sysfs node failed, err:%d", ret);
> > > +		goto err_free_irq;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_free_irq:
> > > +	free_irq(ilitek_data->client->irq, ilitek_data);
> > > +
> > > +	input_unregister_device(ilitek_data->input_dev);
> > > +err_free_input_dev:
> > > +	if (ilitek_data->input_dev)
> > > +		input_free_device(ilitek_data->input_dev);
> > > +err_put_gpiod:
> > > +	if (!IS_ERR(ilitek_data->reset_gpio))
> > > +		gpiod_put(ilitek_data->reset_gpio);
> > > +	kfree(ilitek_data);
> > > +	return ret;
> > > +}
> > > +
> > > +static int ilitek_ts_i2c_remove(struct i2c_client *client)
> > > +{
> > > +	struct device *dev = &client->dev;
> > > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > > +
> > > +	sysfs_remove_group(&dev->kobj, ilitek_attrs_group);
> > > +
> > > +	free_irq(ilitek_data->client->irq, ilitek_data);
> > > +
> > > +	input_unregister_device(ilitek_data->input_dev);
> > > +
> > > +	if (!IS_ERR(ilitek_data->reset_gpio))
> > > +		gpiod_put(ilitek_data->reset_gpio);
> > > +
> > > +	kfree(ilitek_data);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ilitek_ts_i2c_id[] = {
> > > +	{ILITEK_TS_NAME, 0},
> > > +	{ /* not omitted */ },
> > 
> > What does "not omitted" mean in this context?
> 
> It reminds developers that the empty brackets should not be removed.
> Will remove the comment in v4.

I think typical notation is "{ /* sentinel */ }" for this.

> 
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ilitek_ts_i2c_id);
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id ilitekts_acpi_id[] = {
> > > +	{ "ILTK0001", 0 },
> > > +	{ /* not omitted */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, ilitekts_acpi_id);
> > > +#endif
> > > +
> > > +#ifdef CONFIG_OF
> > > +static const struct of_device_id ilitek_ts_i2c_match[] = {
> > > +	{.compatible = "ilitek,ili2520",},
> > > +	{.compatible = "ilitek,ili2510",},
> > > +	{ /* not omitted */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ilitek_ts_i2c_match);
> > > +#endif
> > > +
> > > +static struct i2c_driver ilitek_ts_i2c_driver = {
> > > +	.driver = {
> > > +		.name = ILITEK_TS_NAME,
> > > +		.owner = THIS_MODULE,
> > 
> > No need to set owner.
> 
> Agreed; will update.
> 
> > > +		.of_match_table = of_match_ptr(ilitek_ts_i2c_match),
> > > +		.acpi_match_table = ACPI_PTR(ilitekts_acpi_id),
> > 
> > You need power management (suspend/resume) handling too.
> > 
> 
> Agreed; will update.
> 
> > > +	},
> > > +	.probe = ilitek_ts_i2c_probe,
> > > +	.remove = ilitek_ts_i2c_remove,
> > > +	.id_table = ilitek_ts_i2c_id,
> > > +};
> > > +
> > > +module_i2c_driver(ilitek_ts_i2c_driver);
> > > +
> > > +MODULE_AUTHOR("ILITEK");
> > > +MODULE_DESCRIPTION("ILITEK I2C Touchscreen Driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.25.1
> > > 
> > > 
> > 
> > Thanks.
> > 
> > -- 
> > Dmitry
> 
> Many thanks,
> 
> --
> Joe Hung

Thank you.

-- 
Dmitry

RE: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Joe Hung (洪銘陽)]

Hello Dmitry,

Thank you for the quick response.
Understand and agreed on all your comments.

Little question about the name of prefix and this driver file,
could it be named according to different protocol version named by ILITEK internally
as ilitek_p6x.c (as protocol version 6 for Lego series, and ili210x.c should be protocol 2).

In addition, could we named it like how touchscreen/elants_i2c.c and eftf2127.c worked?
Is there a way that we could keep ilitek as prefix and driver file's name?


On Wed, Feb 24, 2021 at 22:23:33PM -0800, Dmitry Torokhov wrote:
> Hi Joe,
> > > > +struct ilitek_protocol_info {
> > > 
> > > Let's use ili23xx instead of ilitek prefix throughout the file. Or
> > > whatever the first generation of the product supported by this driver.
> > > 
> > 
> > This driver is planned to support 213X/23XX/25XX.
> 
> OK, then I'd use ili213x as the prefix and would have source as
> ili213x.c
> 
> > 
> > [Question]
> > Was wondering why ilitek prefix is not feasible ?
> 
> Just because the driver does not support all Ilitek devices and we
> already have drivers/input/touchscreen/ili210x.c
> 
> > Is it fine that 23XX prefix that still support 25XX ICs ?
> 
> That is fine.

Understand, thank you very much.

> 
> > > > +	s32				screen_min_y;
> > > > +	s32				max_tp;
> > > > +	s32				x_ch;
> > > > +	s32				y_ch;
> > > > +	s32				slave_count;
> > > 
> > > What is the importance of this?
> > 
> > slave_count is different among different ILITEK touch ICs.
> > It would be used during FW upgrade.
> > We needs FW upgrade touch IC's slave also.
> 
> I do not see it currently being used anywhere, that is why I question.
> If it is needed for future enhancements, I would prefer it was added
> with them, not ahead of time.

Agreed; would also remove other unused item in v4.

> 
> > > > +/* ILITEK I2C R/W APIs */
> > > > +static int ilitek_i2c_transfer(struct i2c_client *client,
> > > > +			       struct i2c_msg *msgs, int cnt)
> > > > +{
> > > > +	int ret = 0, retry = 4;
> > > > +
> > > > +	while (retry--) {
> > > > +		ret = i2c_transfer(client->adapter, msgs, cnt);
> > > > +		if (ret >= 0)
> > > > +			break;
> > > > +		mdelay(20);
> > > > +	}
> > > 
> > > Why does it this device need retries?
> > > 
> > 
> > I2C transfer may failed cause FW not ready for some reason.
> > It should be fine for basic probe/ISR flow, and risky for FW update.
> > 
> > [Question]
> > Should it be removed, and patched with FW update patch ?
> 
> I think I would prefer firmware update executing retries as needed, and
> common paths not having them.
> 

Understand; would also remove this API in v4.

> > 
> > > > +
> > > > +	if (ilitek_data->ilitek_repeat_start && delay == 0 &&
> > > 
> > > Where is this being set? As far as I understand, repeated start is
> > > property of the i2c controller, not peripheral device.
> > > 
> > > Also, please see if you can switch to using regmap, as it should take
> > > care of selecting right mode, depending on controller's capabilities.
> > > 
> > 
> > Some ilitek touch ICs may not support "I2C repeat start".
> 
> OK, then we need to somehow activate this feature, because as it is it
> is a dead code.
> 

Agreed; would modified here in v4.
Current patch for Lego series ICs should all support repeated start.
Should remove it here.

> > And agreed; it should be removed here in v4.
> > 
> > > > +static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
> > > > +			    int x, int y)
> > > > +{
> > > > +	int j = 0;
> > > > +	int x_min;
> > > > +	int x_max;
> > > > +	int y_min;
> > > > +	int y_max;
> > > > +
> > > > +	for (j = 0; j < ilitek_data->keycount; j++) {
> > > > +		x_min = ilitek_data->keyinfo[j].x;
> > > > +		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
> > > > +		y_min = ilitek_data->keyinfo[j].y;
> > > > +		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
> > > > +
> > > > +		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
> > > > +			input_report_key(ilitek_data->input_dev,
> > > > +					 ilitek_data->keyinfo[j].id, 1);
> > > > +
> > > > +			ilitek_data->keyinfo[j].status = 1;
> > > > +			ilitek_data->touch_key_hold_press = true;
> > > > +			ilitek_data->is_touched = true;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +}
> > > 
> > > This sounds like handling of "soft keys" which is better left for
> > > userspace. OTOH the Linux keycode seems to be coming from controller
> > > firmware. This is really weird.
> > 
> > It handles keys which key ID and region are get from touch device firmware.
> > Then check region of touch points to trigger key event.
> > 
> > [Question]
> > Is it not feasible for a touchscreen driver do key handling?
> 
> Typically this is better handled in userspace, as then it works across
> various touch controllers, vs. needing this special one.
>

Agreed; should remove key-related flow in v4.

> > > > +	report_max_point = buf[REPORT_ADDRESS_COUNT];
> > > > +	if (report_max_point > ilitek_data->max_tp) {
> > > > +		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
> > > > +			report_max_point, ilitek_data->max_tp);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	count = CEIL(report_max_point, packet_max_point);
> > > > +	for (i = 1; i < count; i++) {
> > > > +		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
> > > > +			buf + i * 64, 64);
> > > > +		if (ret < 0) {
> > > > +			dev_err(dev, "get touch info. err, count=%d\n", count);
> > > > +			goto err_release_touch_and_key;
> > > > +		}
> > > > +	}
> > > 
> > > This means you have 1 + count transactions on i2c bus. Have you
> > > considered reading max number of bytes in one transaction, and then soft
> > > out how much of received data is valid?
> > > 
> > 
> > Due to limitation on FW of touch device, data byte for each I2C transaction
> > is limited. It would lead to I2C communication issue when large count of bytes
> > are request.
> > 
> > Also, for only a few touch points,
> > It should be less efficient to query max number of bytes.
> 
> I was thinking about having one read transaction for up to <max bytes
> your devices can transmit>, then look at the header and decide how many,
> if any, more requests you would need to issue. This way for majority of
> cases I believe you would need only one read per interrupt.

Understand; just have checked with device FW developer,
the max bytes devices when touch info. report is 64 bytes.
It should contain 10 touch point data.

More I2C read should be in the case for touch points > 10
and < 40, which touch device FW could support.

> > > 
> > > > +static ssize_t firmware_version_show(struct device *dev,
> > > > +				     struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > +	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
> > > > +	int ret = 0;
> > > > +	u8 outbuf[64] = {0};
> > > > +
> > > > +	ilitek_irq_disable(ilitek_data);
> > > > +	mutex_lock(&ilitek_data->ilitek_mutex);
> > > > +	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);
> > > 
> > > Do you need to query this all the time? Is it possible to collect this
> > > data at boot or after firmware flash and store for subsequent use?
> > 
> > Agreed; just return the stored FW version (queried at boot or after FW update).
> > The same modification in showing product_id below.
> > Will update in v4.
> > 
> > [Question]
> > Is it a kind of consensus that don't do I2C transaction from show/read function via "cat"?
> 
> It just seems simpler to me to do this once at probe time, since it is
> not going to change unless firmware is updated. If amount of data was
> large, I'd consider querying it dynamically in show() as then it would
> be more expensive memory-wise to store it on the off-chance userspace
> might want to see it.
> 

Understand; thank you for the explanation.
Will update in v4.

> > 
> > > > +
> > > > +static const struct i2c_device_id ilitek_ts_i2c_id[] = {
> > > > +	{ILITEK_TS_NAME, 0},
> > > > +	{ /* not omitted */ },
> > > 
> > > What does "not omitted" mean in this context?
> > 
> > It reminds developers that the empty brackets should not be removed.
> > Will remove the comment in v4.
> 
> I think typical notation is "{ /* sentinel */ }" for this.
> 

Agreed; would remove it as other drivers do in v4.

> Thank you.
> 
> -- 
> Dmitry

Thank you very much

--
Joe Hung

Re: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Dmitry Torokhov]

Hi Joe,

On Fri, Feb 26, 2021 at 08:30:25AM +0000, Joe Hung (洪銘陽) wrote:
> Hello Dmitry,
> 
> Thank you for the quick response.
> Understand and agreed on all your comments.
> 
> Little question about the name of prefix and this driver file,
> could it be named according to different protocol version named by ILITEK internally
> as ilitek_p6x.c (as protocol version 6 for Lego series, and ili210x.c should be protocol 2).
> 
> In addition, could we named it like how touchscreen/elants_i2c.c and eftf2127.c worked?

So elants_i2c vs eftf2127 is historical artefact with elants_i2c being
there first.

> Is there a way that we could keep ilitek as prefix and driver file's name?

OK, this is really a minor point as far as I am concerned and we spent
way too much time debating it. You may keep ilitek file name and prefix
if you would prefer. Just make sure you list supported
controllers/protocols in Kconfig and that should be enough.

Thanks.

-- 
Dmitry

RE: [PATCH v3 2/2] input: touchscreen: Add support for ILITEK Lego Series

[Joe Hung (洪銘陽)]

Hi Dmitry,

Thank you for the quick response.
Would update Kconfig also in v4.

On Sun, Feb 28, 2021 at 21:25:13PM -0800, Dmitry Torokhov wrote:
> Hi Joe,
> 
> On Fri, Feb 26, 2021 at 08:30:25AM +0000, Joe Hung (洪銘陽) wrote:
> > Hello Dmitry,
> > 
> > Thank you for the quick response.
> > Understand and agreed on all your comments.
> > 
> > Little question about the name of prefix and this driver file,
> > could it be named according to different protocol version named by ILITEK internally
> > as ilitek_p6x.c (as protocol version 6 for Lego series, and ili210x.c should be protocol 2).
> > 
> > In addition, could we named it like how touchscreen/elants_i2c.c and eftf2127.c worked?
> 
> So elants_i2c vs eftf2127 is historical artefact with elants_i2c being
> there first.
> 
> > Is there a way that we could keep ilitek as prefix and driver file's name?
> 
> OK, this is really a minor point as far as I am concerned and we spent
> way too much time debating it. You may keep ilitek file name and prefix
> if you would prefer. Just make sure you list supported
> controllers/protocols in Kconfig and that should be enough.

Agreed; thank you very much.

> 
> Thanks.
> 
> -- 
> Dmitry

Thank you very much.

--
Joe Hung

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: ilitek_ts_i2c: Add bindings

[Dmitry Torokhov]

Hi Joe,

On Thu, Feb 18, 2021 at 02:44:34PM +0800, Joe Hung wrote:
> Add binding documentation for ILITEK touch devices.
> 
> Signed-off-by: Joe Hung <joe_hung@ilitek.com>
> ---
> Changes in v3:
>   - Add include header in examples, and pass the dt binding check
> 
> Changes in v2:
>   - Convert to DT schema format
>   - Using interrupts instead of irq-gpios
> 
>  .../input/touchscreen/ilitek_ts_i2c.yaml      | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> new file mode 100644
> index 000000000000..5da9020ed272
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/ilitek_ts_i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ilitek I2C Touchscreen Controller
> +
> +maintainers:
> +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ilitek,ili2520
> +      - ilitek,ili2510
> +  reg:
> +    const: 0x41
> +  interrupts:
> +    maxItems: 1
> +  reset-gpios:
> +    maxItems: 1
> +
> +additionalProperties: false

I think you need to make sure that wakeup-source and various
touchscreen-* properties can be used in this binding.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - reset-gpios
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        touchscreen@41 {
> +                compatible = "ilitek,ili2520";
> +                reg = <0x41>;
> +                interrupt-parent = <&gpio1>;
> +                interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +                reset-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>;
> +        };
> +    };
> --
> 2.25.1
> 
> 

Thanks.

-- 
Dmitry

RE: [PATCH v3 1/2] dt-bindings: input: touchscreen: ilitek_ts_i2c: Add bindings

[Joe Hung (洪銘陽)]

Hi Dmitry,

Many thanks for your review.
Should be updated in v4 shortly.

On Sat, Feb 20, 2021 21:03:29PM -0800, Dmitry Torokhov wrote:
> Hi Joe,
> 
> On Thu, Feb 18, 2021 at 02:44:34PM +0800, Joe Hung wrote:
> > Add binding documentation for ILITEK touch devices.
> > 
> > Signed-off-by: Joe Hung <joe_hung@ilitek.com>
> > ---
> > Changes in v3:
> >   - Add include header in examples, and pass the dt binding check
> > 
> > Changes in v2:
> >   - Convert to DT schema format
> >   - Using interrupts instead of irq-gpios
> > 
> >  .../input/touchscreen/ilitek_ts_i2c.yaml      | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> > new file mode 100644
> > index 000000000000..5da9020ed272
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/ilitek_ts_i2c.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/touchscreen/ilitek_ts_i2c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ilitek I2C Touchscreen Controller
> > +
> > +maintainers:
> > +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ilitek,ili2520
> > +      - ilitek,ili2510
> > +  reg:
> > +    const: 0x41
> > +  interrupts:
> > +    maxItems: 1
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +additionalProperties: false
> 
> I think you need to make sure that wakeup-source and various
> touchscreen-* properties can be used in this binding.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - reset-gpios
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        touchscreen@41 {
> > +                compatible = "ilitek,ili2520";
> > +                reg = <0x41>;
> > +                interrupt-parent = <&gpio1>;
> > +                interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> > +                reset-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>;
> > +        };
> > +    };
> > --
> > 2.25.1
> > 
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Many thanks.

--
Joe Hung