Hello Dmitry,
Thanks for your quick response, agreed on all your comments.
Some explanation replied blow.
All comments should be took and modified in PATCH v7 shortly.
On Tue, Mar 23, 2021 at 16:13:25PM -0700, Dmitry Torokhov wrote
> Hi Joe,
>
> On Mon, Mar 22, 2021 at 06:41:31PM +0800, Joe Hung wrote:
> > Add support for ILITEK Lego series of touch devices.
> > Lego series includes ILITEK 213X/23XX/25XX.
> >
> > Tested/passed with evaluation board with ILI2520/2322 IC.
>
> The driver looks pretty good, a few more comments though.
>
> >
> > Signed-off-by: Joe Hung <joe_hung@xxxxxxxxxx>
> > ---
> > Changes in v6:
> > - Modified print message from sysfs file node
> > - Add i2c functionality check in probe function
> > - Add single touch ABS_X/ABS_Y registration
> > - Remove TOUCH_MAJOR/WIDTH_MAJOR
> >
> > Changes in v5:
> > - None
> >
> > Changes in v4:
> > - Remove unused inlcude header file
> > - Remove parenthesis for scalar values
> > - Place to use standard macro DIV_ROUND_UP
> > - Remove unused/unrequired member of struct
> > - Remove retries when I2C transfer
> > - Remove irq_disable/enable wrapper
> > - Remove key handler
> > - Adjust to use get_unaligned_le16/be16
> > - Modify ilitek_reset() to leave reset gpio in-active finally
> > - Remove null check for input argument that should not happen
> > - Modify return value for read_tp_info()
> > - Modify to use common touchscreen_* api
> > - Add error handling for input_mt_init_slots
> > - Modify input flag for irq request, and parse it from ACPI/DTS
> > - Return stored value instead of querying via I2C in *_show api
> > - Modify to use devm_* APIs and get rid of remove api
> > - Add PM (suspend/resume) handling
> >
> > 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 | 753
> > ++++++++++++++++++++++
> > 3 files changed, 766 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..03a16852d4bc 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 213X/23XX/25XX/Lego Series Touch ICs"
> > + depends on I2C
> > + help
> > + Say Y here if you have touchscreen with ILITEK touch IC,
> > + it supports 213X/23XX/25XX and 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..1622e66c4eaa 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..507ab735f2b6
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> > @@ -0,0 +1,753 @@
> > +// 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@xxxxxxxxxx>
> > + * Copyright (C) 2021 Joe Hung <joe_hung@xxxxxxxxxx> */
> > +
> > +#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/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/errno.h>
> > +#include <linux/acpi.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <asm/unaligned.h>
> > +
> > +
> > +#define ILITEK_TS_NAME "ilitek_ts"
> > +#define BL_V1_8 0x108
> > +#define BL_V1_7 0x107
> > +#define BL_V1_6 0x106
> > +
> > +#define ILITEK_TP_CMD_GET_TP_RES 0x20
> > +#define ILITEK_TP_CMD_GET_SCRN_RES 0x21
> > +#define ILITEK_TP_CMD_SET_IC_SLEEP 0x30
> > +#define ILITEK_TP_CMD_SET_IC_WAKE 0x31
> > +#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 PTL_V3 0x03
> > +#define PTL_V6 0x06
>
> Are these needed? Do you plan bringing in V3 protocol support?
Yes, we plan to support V3 protocol in the following upstreaming patch.
But agreed, this patch is planned to support V6 (Lego series) only.
Will remove protocol related manipulation in PATCH v7.
>
> > +
> > +#define MOD_AP 0x5A
> > +#define MOD_BL 0x55
>
> These are not used?
Yes and agreed, it should be removed in PATCH v7.
>
> > +
> > +struct ilitek_protocol_info {
> > + u16 ver;
> > + u8 ver_major;
> > + u8 flag;
> > +};
> > +
> > +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 touchscreen_properties prop;
> > +
> > + int (*process_and_report)(struct ilitek_ts_data *ts);
>
> Until the driver supports multiple protocol versions there is no point in having this pointer.
Agreed, it should be removed in PATCH v7.
>
> > +
> > + struct PROTOCOL_MAP *ptl_cb_func;
> > + struct ilitek_protocol_info ptl;
> > +
> > + struct ilitek_touch_info tpinfo[ILITEK_SUPPORT_MAX_POINT];
> > +
> > + char product_id[30];
> > + u16 mcu_ver;
> > + u8 ic_mode;
> > + u8 firmware_ver[8];
> > +
> > + s32 reset_time;
> > + s32 screen_max_x;
> > + s32 screen_max_y;
> > + s32 screen_min_x;
> > + s32 screen_min_y;
> > + s32 max_tp;
> > +};
> > +
> > +enum ilitek_cmds {
> > + /* common cmds */
> > + GET_PTL_VER = 0,
> > + GET_FW_VER,
> > + GET_SCRN_RES,
> > + GET_TP_RES,
> > + GET_IC_MODE,
> > + GET_MCU_VER,
> > + SET_IC_SLEEP,
> > + SET_IC_WAKE,
> > +
> > + /* ALWAYS keep at the end */
> > + MAX_CMD_CNT
> > +};
> > +
> > +typedef int protocol_func(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf);
> > +
> > +struct PROTOCOL_MAP {
> > + u16 cmd;
> > + const char *name;
> > + protocol_func *func;
> > + u8 flag;
> > +};
> > +
> > +/* ILITEK I2C R/W APIs */
> > +static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ts,
> > + u8 *cmd, int write_len, int delay,
> > + u8 *data, int read_len)
> > +{
> > + int ret = 0;
> > + struct i2c_client *client = ts->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 (delay == 0 && write_len > 0 && read_len > 0) {
> > + ret = i2c_transfer(client->adapter, msgs, 2);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + if (write_len > 0) {
> > + ret = i2c_transfer(client->adapter, msgs, 1);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + if (delay > 0)
> > + mdelay(delay);
> > + if (read_len > 0) {
> > + ret = i2c_transfer(client->adapter, msgs + 1, 1);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* ILITEK ISR APIs */
> > +static void ilitek_touch_down(struct ilitek_ts_data *ts,
> > + int id, int x, int y, int p, int h, int w) {
> > + struct input_dev *input = ts->input_dev;
> > +
> > + input_report_key(input, BTN_TOUCH, 1);
> > +
> > + input_mt_slot(input, id);
> > + input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
> > +
> > + touchscreen_report_pos(input, &ts->prop, x, y, true);
> > +
> > + ts->tpinfo[id].status = 1;
> > + ts->tpinfo[id].id = id;
> > + ts->tpinfo[id].x = x;
> > + ts->tpinfo[id].y = y;
> > + ts->tpinfo[id].p = p;
> > + ts->tpinfo[id].h = h;
> > + ts->tpinfo[id].w = w;
> > +}
> > +
> > +static void ilitek_touch_release(struct ilitek_ts_data *ts, int id) {
> > + struct input_dev *input = ts->input_dev;
> > +
> > + if (ts->tpinfo[id].status == 1) {
> > + input_mt_slot(input, id);
> > + input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> > + ts->tpinfo[id].status = 0;
> > + }
> > +}
> > +
> > +static void ilitek_touch_release_all(struct ilitek_ts_data *ts) {
> > + struct input_dev *input = ts->input_dev;
> > + int i = 0;
> > +
> > + for (i = 0; i < ts->max_tp; i++)
> > + ilitek_touch_release(ts, i);
> > + input_report_key(input, BTN_TOUCH, 0); }
> > +
> > +static int ilitek_process_and_report_v6(struct ilitek_ts_data *ts) {
> > + 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 = ts->input_dev;
> > + struct device *dev = &ts->client->dev;
> > + u16 x, y, p = 10, w = 10, h = 10, status, id;
> > +
> > + ret = ilitek_i2c_write_and_read(ts, 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 > ts->max_tp) {
> > + dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
> > + report_max_point, ts->max_tp);
> > + ret = -EINVAL;
> > + goto err_release_touch_and_key;
> > + }
> > +
> > + count = DIV_ROUND_UP(report_max_point, packet_max_point);
> > + for (i = 1; i < count; i++) {
> > + ret = ilitek_i2c_write_and_read(ts, 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(ts, id);
> > + continue;
> > + }
> > +
> > + x = get_unaligned_le16(buf + i * packet_len + 2);
> > + y = get_unaligned_le16(buf + i * packet_len + 4);
> > +
> > + if (x > ts->screen_max_x || x < ts->screen_min_x ||
> > + y > ts->screen_max_y || y < ts->screen_min_y) {
> > + dev_warn(dev, "invalid position, X[%d,%d,%d], Y[%d,%d,%d]\n",
> > + ts->screen_min_x, x, ts->screen_max_x,
> > + ts->screen_min_y, y, ts->screen_max_y);
> > + continue;
> > + }
> > + ilitek_touch_down(ts, id, x, y, p, h, w);
> > + }
> > +
> > +err_release_touch_and_key:
> > + if (ret < 0 || release_point == report_max_point)
> > + ilitek_touch_release_all(ts);
>
> If you use input_mt_sync_frame() here you will not need to are about releasing BTN_TOUCH or doing single-touch emulation.
Agreed, would modify in PATCH v7.
>
> > + input_sync(input);
> > + return (ret < 0) ? ret : 0;
> > +}
> > +
> > +
> > +/* APIs of cmds for ILITEK Touch IC */ static int
> > +api_protocol_set_cmd(struct ilitek_ts_data *ts,
> > + u16 idx, u8 *inbuf, u8 *outbuf)
> > +{
> > + u16 cmd;
> > + int ret = 0;
> > +
> > + if (idx >= MAX_CMD_CNT)
> > + return -EINVAL;
> > +
> > + /* check if cmd is supported by its protocol version */
> > + if (!(ts->ptl.flag & ts->ptl_cb_func[idx].flag))
> > + return -EINVAL;
> > +
> > + cmd = ts->ptl_cb_func[idx].cmd;
> > + ret = ts->ptl_cb_func[idx].func(ts, cmd, inbuf, outbuf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_ptl_ver(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 3);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ts->ptl.ver = get_unaligned_be16(outbuf);
> > + ts->ptl.ver_major = outbuf[0];
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_mcu_ver(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 32);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ts->mcu_ver = get_unaligned_le16(outbuf);
> > + memset(ts->product_id, 0, sizeof(ts->product_id));
> > + memcpy(ts->product_id, outbuf+6, 26);
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_fw_ver(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 8);
> > + if (ret < 0)
> > + return ret;
> > +
> > + memcpy(ts->firmware_ver, outbuf, 8);
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_scrn_res(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 8);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ts->screen_min_x = get_unaligned_le16(outbuf);
> > + ts->screen_min_y = get_unaligned_le16(outbuf + 2);
> > + ts->screen_max_x = get_unaligned_le16(outbuf + 4);
> > + ts->screen_max_y = get_unaligned_le16(outbuf + 6);
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_tp_res(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 15);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ts->max_tp = outbuf[8];
> > +
> > + if (ts->max_tp > ILITEK_SUPPORT_MAX_POINT) {
> > + dev_err(&ts->client->dev, "Invalid MAX_TP:%d from FW\n",
> > + ts->max_tp);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int api_protocol_get_ic_mode(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + int ret = 0;
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + ret = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ts->ic_mode = outbuf[0];
> > + return 0;
> > +}
> > +
> > +static int api_protocol_set_ic_sleep(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + return ilitek_i2c_write_and_read(ts, buf, 1, 0, NULL, 0); }
> > +
> > +static int api_protocol_set_ic_wake(struct ilitek_ts_data *ts,
> > + u16 cmd, u8 *inbuf, u8 *outbuf) {
> > + u8 buf[64] = {0};
> > +
> > + buf[0] = cmd;
> > + return ilitek_i2c_write_and_read(ts, buf, 1, 0, NULL, 0); }
> > +
> > +struct PROTOCOL_MAP ptl_func_map[] = {
> > + /* common cmds */
> > + [GET_PTL_VER] = {ILITEK_TP_CMD_GET_PRL_VER, "GET_PTL_VER",
> > + api_protocol_get_ptl_ver, PTL_V3 | PTL_V6},
> > + [GET_FW_VER] = {ILITEK_TP_CMD_GET_FW_VER, "GET_FW_VER",
> > + api_protocol_get_fw_ver, PTL_V3 | PTL_V6},
> > + [GET_SCRN_RES] = {ILITEK_TP_CMD_GET_SCRN_RES, "GET_SCRN_RES",
> > + api_protocol_get_scrn_res, PTL_V3 | PTL_V6},
> > + [GET_TP_RES] = {ILITEK_TP_CMD_GET_TP_RES, "GET_TP_RES",
> > + api_protocol_get_tp_res, PTL_V3 | PTL_V6},
> > + [GET_IC_MODE] = {ILITEK_TP_CMD_GET_IC_MODE, "GET_IC_MODE",
> > + api_protocol_get_ic_mode, PTL_V3 | PTL_V6},
> > + [GET_MCU_VER] = {ILITEK_TP_CMD_GET_MCU_VER, "GET_MOD_VER",
> > + api_protocol_get_mcu_ver, PTL_V3 | PTL_V6},
> > + [SET_IC_SLEEP] = {ILITEK_TP_CMD_SET_IC_SLEEP, "SET_IC_SLEEP",
> > + api_protocol_set_ic_sleep, PTL_V3 | PTL_V6},
> > + [SET_IC_WAKE] = {ILITEK_TP_CMD_SET_IC_WAKE, "SET_IC_WAKE",
> > + api_protocol_set_ic_wake, PTL_V3 | PTL_V6}, };
> > +
> > +/* Probe APIs */
> > +static void ilitek_reset(struct ilitek_ts_data *ts, int delay, bool
> > +boot) {
> > + if (!boot)
> > + disable_irq_nosync(ts->client->irq);
>
> I am confused about this flag. ilitek_reset() is called from probe and resume handlers, and in resume interrupts are already disabled, so why are you trying to disable them again here?
Reset touch controller will trigger IRQ during firmware initialization,
This triggered irq is not used for reporting touch event,
so we disable irq as a kind of prevention.
But agreed, ilitek_reset() in this patch should work fine,
because irq should be disabled well when calling
ilitek_reset() only in probe and resume handlers.
So agreed, should remove it in PATCH v7.
>
> > +
> > + if (ts->reset_gpio) {
> > + gpiod_set_value(ts->reset_gpio, 1);
> > + mdelay(10);
> > + gpiod_set_value(ts->reset_gpio, 0);
> > + mdelay(delay);
> > + }
> > +
> > + if (!boot)
> > + enable_irq(ts->client->irq);
> > +}
> > +
> > +static int ilitek_protocol_init(struct ilitek_ts_data *ts) {
> > + int ret = 0;
> > + u8 outbuf[64] = {0};
> > +
> > + ts->ptl.flag = PTL_V6;
> > + ts->ptl_cb_func = ptl_func_map;
> > + ts->process_and_report = ilitek_process_and_report_v6;
> > + ts->reset_time = 600;
> > +
> > + ret = api_protocol_set_cmd(ts, GET_PTL_VER, NULL, outbuf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Protocol v3 is not support currently */
> > + if (ts->ptl.ver_major == 0x3 ||
> > + ts->ptl.ver == BL_V1_6 ||
> > + ts->ptl.ver == BL_V1_7)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int ilitek_read_tp_info(struct ilitek_ts_data *ts, bool boot)
> > +{
> > + u8 outbuf[256] = {0};
> > + int error = 0;
> > +
> > + error = api_protocol_set_cmd(ts, GET_PTL_VER, NULL, outbuf);
> > + if (error < 0)
> > + return error;
>
>
> api_protocol_set_cmd() returns normalized (-<rc>, 0], i.e. it is never positive, so simply
>
> if (error)
> return error;
Agreed, should modified whole file where calling it in PATCH v7
>
> > +
> > + error = api_protocol_set_cmd(ts, GET_MCU_VER, NULL, outbuf);
> > + if (error < 0)
> > + return error;
> > +
> > + error = api_protocol_set_cmd(ts, GET_FW_VER, NULL, outbuf);
> > + if (error < 0)
> > + return error;
> > +
> > + if (boot) {
> > + error = api_protocol_set_cmd(ts, GET_SCRN_RES, NULL,
> > + outbuf);
> > + if (error < 0)
> > + return error;
> > + }
> > +
> > + error = api_protocol_set_cmd(ts, GET_TP_RES, NULL, outbuf);
> > + if (error < 0)
> > + return error;
> > +
> > + error = api_protocol_set_cmd(ts, GET_IC_MODE, NULL, outbuf);
> > + if (error < 0)
> > + return error;
> > +
> > + return 0;
> > +}
> > +
> > +static int ilitek_input_dev_init(struct device *dev, struct
> > +ilitek_ts_data *ts) {
> > + int ret = 0;
> > + struct input_dev *input = NULL;
>
> No need to initialize variables like this, it only hurts, as otherwise compiler can warn you if you forger to assign a value to a variable and try to use it.
Agreed, should modified in PATCH v7.
>
> > +
> > + input = devm_input_allocate_device(dev);
> > + if (!input)
> > + return -ENOMEM;
> > +
> > + ts->input_dev = input;
> > + 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);
>
> I do not think anything of this besides INPUT_PROP_DIRECT is needed, as input_set_abs_params, and input_mt_init_slots will set needed capabilities.
Agreed, should remove those in PATCH v7.
>
> > +
> > + /* Single touch input setup */
> > + input_set_abs_params(input, ABS_X, ts->screen_min_x,
> > + ts->screen_max_x, 0, 0);
> > + input_set_abs_params(input, ABS_Y, ts->screen_min_y,
> > + ts->screen_max_y, 0, 0);
> > +
> > + /* Multi-touch input setup */
> > + input_set_abs_params(input, ABS_MT_POSITION_X,
> > + ts->screen_min_x,
> > + ts->screen_max_x, 0, 0);
> > + input_set_abs_params(input, ABS_MT_POSITION_Y,
> > + ts->screen_min_y,
> > + ts->screen_max_y, 0, 0);
> > +
> > + touchscreen_parse_properties(input, true, &ts->prop);
> > +
> > + ret = input_mt_init_slots(input, ts->max_tp, INPUT_MT_DIRECT);
>
> I prefer variables that hold error codes to be called "error".
Understand, should check and modify whole file in PATCH v7.
>
> Also, if you specify INPUT_MT_DROP_UNUSED as an additional flag for
> input_mt_init_slots() and use input_mt_sync_frame() when reporting events you would not need to track the state of contacts in your driver and explicitly release them.
Agreed, should modify and remove related flow in PATCH v7.
>
>
> > + if (ret) {
> > + dev_err(dev, "failed to initialize MT slots, err:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = input_register_device(input);
> > + if (ret) {
> > + dev_err(dev, "failed to register input device, err:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t ilitek_i2c_isr(int irq, void *dev_id) {
> > + struct ilitek_ts_data *ts = dev_id;
> > + int ret = 0;
> > +
> > + ret = ts->process_and_report(ts);
> > +
> > + if (ret < 0) {
> > + dev_err(&ts->client->dev, "[%s] err:%d\n", __func__, ret);
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +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 *ts = i2c_get_clientdata(client);
> > +
> > + return scnprintf(buf, PAGE_SIZE,
> > + "fw version: [%02X%02X.%02X%02X.%02X%02X.%02X%02X]\n",
> > + ts->firmware_ver[0], ts->firmware_ver[1],
> > + ts->firmware_ver[2], ts->firmware_ver[3],
> > + ts->firmware_ver[4], ts->firmware_ver[5],
> > + ts->firmware_ver[6], ts->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 *ts = i2c_get_clientdata(client);
> > +
> > + return scnprintf(buf, PAGE_SIZE, "product id: [%04X], module: [%s]\n",
> > + ts->mcu_ver, ts->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_ts_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id) {
> > + struct ilitek_ts_data *ts = NULL;
> > + struct device *dev = &client->dev;
> > + int ret = 0;
>
> Please drop unneeded initializations, s/ret/error/
Agreed, should check and modify whole file in PATCH v7.
>
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > + dev_err(dev, "%s: i2c check functionality failed\n",
> > + ILITEK_TS_NAME);
>
> I do not think you need to have ILITEK_TS_NAME as device and driver name are already printed by dev_err.
Agreed, should modified in PATCH v7.
>
> > + return -ENXIO;
> > + }
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > +
> > + ts->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>
> Make it devm_gpiod_get_optional() as you already handle it being not present in ilitek_reset().
Agreed, should modify in PATCH v7
>
> > + if (IS_ERR(ts->reset_gpio)) {
> > + ret = PTR_ERR(ts->reset_gpio);
> > + dev_err(dev, "request gpiod failed, err:%d", ret);
> > + return ret;
> > + }
> > +
> > + ilitek_reset(ts, 1000, true);
> > + ret = ilitek_protocol_init(ts);
> > + if (ret < 0) {
> > + dev_err(dev, "protocol init failed, err:%d", ret);
> > + return ret;
> > + }
> > +
> > + ret = ilitek_read_tp_info(ts, true);
> > + if (ret < 0) {
> > + dev_err(dev, "read tp info failed, err:%d", ret);
> > + return ret;
> > + }
> > +
> > + ret = ilitek_input_dev_init(dev, ts);
> > + if (ret < 0) {
> > + dev_err(dev, "input dev init failed, err:%d", ret);
> > + return ret;
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, ts->client->irq, NULL,
> > + ilitek_i2c_isr, IRQF_ONESHOT,
> > + "ilitek_touch_irq", ts);
> > + if (ret) {
> > + dev_err(dev, "request threaded irq failed, err:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = devm_device_add_group(dev, ilitek_attrs_group);
> > + if (ret) {
> > + dev_err(dev, "sysfs create group failed, err:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused ilitek_suspend(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ilitek_ts_data *ts = i2c_get_clientdata(client);
> > + int error = 0;
> > +
> > + disable_irq(client->irq);
> > +
> > + if (device_may_wakeup(dev)) {
> > + enable_irq_wake(client->irq);
>
> You do not need to enable irq wake here, i2c core will do it for you.
Understand, should modify in PATCH v7.
>
> > + } else {
> > + error = api_protocol_set_cmd(ts, SET_IC_SLEEP, NULL, NULL);
> > + if (error < 0)
> > + return error;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused ilitek_resume(struct device *dev) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ilitek_ts_data *ts = i2c_get_clientdata(client);
> > + int error = 0;
> > +
> > + if (device_may_wakeup(dev)) {
> > + disable_irq_wake(client->irq);
> > + } else {
> > + error = api_protocol_set_cmd(ts, SET_IC_WAKE, NULL, NULL);
> > + if (error < 0)
> > + return error;
> > + }
> > +
> > + ilitek_reset(ts, ts->reset_time, false);
>
> Are you sure you need to perform reset when device is a wakeup source?
> I'd expect in that case it would stay operational.
It should perform reset after SET_IC_WAKE, should modify in PATCH v7.
>
> > + enable_irq(client->irq);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(ilitek_pm_ops, ilitek_suspend,
> > +ilitek_resume);
> > +
> > +static const struct i2c_device_id ilitek_ts_i2c_id[] = {
> > + {ILITEK_TS_NAME, 0},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ilitek_ts_i2c_id);
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id ilitekts_acpi_id[] = {
> > + { "ILTK0001", 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, ilitekts_acpi_id); #endif
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id ilitek_ts_i2c_match[] = {
> > + {.compatible = "ilitek,ili2130",},
> > + {.compatible = "ilitek,ili2131",},
> > + {.compatible = "ilitek,ili2132",},
> > + {.compatible = "ilitek,ili2316",},
> > + {.compatible = "ilitek,ili2322",},
> > + {.compatible = "ilitek,ili2323",},
> > + {.compatible = "ilitek,ili2326",},
> > + {.compatible = "ilitek,ili2520",},
> > + {.compatible = "ilitek,ili2521",},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, ilitek_ts_i2c_match); #endif
> > +
> > +static struct i2c_driver ilitek_ts_i2c_driver = {
> > + .driver = {
> > + .name = ILITEK_TS_NAME,
> > + .pm = &ilitek_pm_ops,
> > + .of_match_table = of_match_ptr(ilitek_ts_i2c_match),
> > + .acpi_match_table = ACPI_PTR(ilitekts_acpi_id),
> > + },
> > + .probe = ilitek_ts_i2c_probe,
> > + .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
Thank you very much.
--
Joe Hung
