[PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Marek Vasut]

The ili251x firmware protocol permits readout of panel resolution,
implement this, but make it possible to override this value using
DT bindings. This way, older DTs which contain touchscreen-size-x
and touchscreen-size-y properties will behave just like before and
new DTs may avoid specifying these for ILI251x.

Note that the command format is different on other controllers, so
this functionality is isolated to ILI251x.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Hung <joe_hung@ilitek.com>
Cc: Luca Hsu <luca_hsu@ilitek.com>
---
V2: New patch
---
 drivers/input/touchscreen/ili210x.c | 31 +++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 30576a5f2f04..c46553ecabe6 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -323,6 +323,34 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
 	return IRQ_HANDLED;
 }
 
+static int ili251x_firmware_update_resolution(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u16 resx, resy;
+	u8 rs[10];
+	int ret;
+
+	/* The firmware update blob might have changed the resolution. */
+	ret = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs));
+	if (ret)
+		return ret;
+
+	resx = (rs[1] << 8U) | rs[0];
+	resy = (rs[3] << 8U) | rs[2];
+
+	/* The value reported by the firmware is invalid. */
+	if (!resx || resx == 0xffff || !resy || resy == 0xfff)
+		return -EINVAL;
+
+	priv->input->absinfo[ABS_X].maximum = resx - 1;
+	priv->input->absinfo[ABS_Y].maximum = resy - 1;
+	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
+	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
+
+	return 0;
+}
+
 static ssize_t ili210x_calibrate(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -449,6 +477,9 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, max_xy, 0, 0);
 	if (priv->chip->has_pressure_reg)
 		input_set_abs_params(input, ABS_MT_PRESSURE, 0, 0xa, 0, 0);
+	/* ILI251x does report valid resolution information, try it. */
+	if (priv->chip == &ili251x_chip)
+		ili251x_firmware_update_resolution(dev);
 	touchscreen_parse_properties(input, true, &priv->prop);
 
 	error = input_mt_init_slots(input, priv->chip->max_touches,
-- 
2.32.0

[PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Marek Vasut]

The ili251x firmware protocol permits readout of firmware version,
protocol version, mcu version and current mode (application, boot
loader, forced update). These information are useful when updating
the firmware on the il251x, e.g. to avoid updating the same firmware
into the device multiple times. The locking is now necessary to avoid
races between interrupt handler and the sysfs readouts.

Note that the protocol differs considerably between the ili2xxx devices,
this patch therefore implements this functionality only for ili251x that
I can test.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Hung <joe_hung@ilitek.com>
Cc: Luca Hsu <luca_hsu@ilitek.com>
---
V2: No change

NOTE: Regarding checkpatch warnings, Consider renaming function(s)
      'ili251x_firmware_version_show' to 'firmware_version_show',
      I considered it and decided against it, because grepping for
      ili251x in debug symbols gives far more accurate results than
      grepping for firmware_version.
---
 drivers/input/touchscreen/ili210x.c | 130 +++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index c46553ecabe6..7790ad000dc1 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -22,6 +22,12 @@
 /* Touchscreen commands */
 #define REG_TOUCHDATA		0x10
 #define REG_PANEL_INFO		0x20
+#define REG_FIRMWARE_VERSION	0x40
+#define REG_PROTOCOL_VERSION	0x42
+#define REG_KERNEL_VERSION	0x61
+#define REG_IC_MODE		0xc0
+#define REG_IC_MODE_AP		0x5a
+#define REG_IC_MODE_BL		0x55
 #define REG_CALIBRATE		0xcc
 
 struct ili2xxx_chip {
@@ -43,6 +49,7 @@ struct ili210x {
 	struct input_dev *input;
 	struct gpio_desc *reset_gpio;
 	struct touchscreen_properties prop;
+	struct mutex lock;
 	const struct ili2xxx_chip *chip;
 	bool stop;
 };
@@ -307,7 +314,9 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
 	int error;
 
 	do {
+		mutex_lock(&priv->lock);
 		error = chip->get_touch_data(client, touchdata);
+		mutex_unlock(&priv->lock);
 		if (error) {
 			dev_err(&client->dev,
 				"Unable to get touch data: %d\n", error);
@@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
 	return 0;
 }
 
+static ssize_t ili251x_firmware_version_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u8 fw[8];
+	int ret;
+
+	/* Get firmware version */
+	mutex_lock(&priv->lock);
+	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
+				   &fw, sizeof(fw));
+	mutex_unlock(&priv->lock);
+
+	if (ret)
+		return ret;
+
+	if (!ret) {
+		ret = scnprintf(buf, PAGE_SIZE,
+				"%02x%02x.%02x%02x.%02x%02x.%02x%02x\n",
+				fw[0], fw[1], fw[2], fw[3],
+				fw[4], fw[5], fw[6], fw[7]);
+	}
+
+	return ret;
+}
+static DEVICE_ATTR(firmware_version, 0444, ili251x_firmware_version_show, NULL);
+
+static ssize_t ili251x_kernel_version_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u8 kv[5];
+	int ret;
+
+	/* Get kernel version */
+	mutex_lock(&priv->lock);
+	ret = priv->chip->read_reg(client, REG_KERNEL_VERSION,
+				   &kv, sizeof(kv));
+	mutex_unlock(&priv->lock);
+
+	if (ret)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%02x.%02x.%02x.%02x.%02x\n",
+			 kv[0], kv[1], kv[2], kv[3], kv[4]);
+}
+static DEVICE_ATTR(kernel_version, 0444, ili251x_kernel_version_show, NULL);
+
+static ssize_t ili251x_protocol_version_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u8 pv[2];
+	int ret;
+
+	/* Get protocol version */
+	mutex_lock(&priv->lock);
+	ret = priv->chip->read_reg(client, REG_PROTOCOL_VERSION,
+				   &pv, sizeof(pv));
+	mutex_unlock(&priv->lock);
+
+	if (ret)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%02x.%02x\n", pv[0], pv[1]);
+}
+static DEVICE_ATTR(protocol_version, 0444, ili251x_protocol_version_show, NULL);
+
+static ssize_t ili251x_mode_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	char *mode = "AP";
+	u8 md[2];
+	int ret;
+
+	/* Get mode */
+	mutex_lock(&priv->lock);
+	ret = priv->chip->read_reg(client, REG_IC_MODE, &md, sizeof(md));
+	mutex_unlock(&priv->lock);
+
+	if (ret)
+		return ret;
+
+	if (md[0] == REG_IC_MODE_AP)		/* Application Mode */
+		mode = "AP";
+	else if (md[0] == REG_IC_MODE_BL)	/* BootLoader Mode */
+		mode = "BL";
+	else					/* Unknown Mode */
+		mode = "??";
+
+	return scnprintf(buf, PAGE_SIZE, "%02x.%02x:%s\n", md[0], md[1], mode);
+}
+static DEVICE_ATTR(mode, 0444, ili251x_mode_show, NULL);
+
 static ssize_t ili210x_calibrate(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -379,22 +490,34 @@ static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
 
 static struct attribute *ili210x_attributes[] = {
 	&dev_attr_calibrate.attr,
+	&dev_attr_firmware_version.attr,
+	&dev_attr_kernel_version.attr,
+	&dev_attr_protocol_version.attr,
+	&dev_attr_mode.attr,
 	NULL,
 };
 
-static umode_t ili210x_calibrate_visible(struct kobject *kobj,
+static umode_t ili210x_attributes_visible(struct kobject *kobj,
 					  struct attribute *attr, int index)
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ili210x *priv = i2c_get_clientdata(client);
 
-	return priv->chip->has_calibrate_reg ? attr->mode : 0;
+	/* Calibrate is present on all ILI2xxx which have calibrate register */
+	if (attr == &dev_attr_calibrate.attr)
+		return priv->chip->has_calibrate_reg ? attr->mode : 0;
+
+	/* Firmware/Kernel/Protocol/BootMode is implememted only for ILI251x */
+	if (priv->chip != &ili251x_chip)
+		return 0;
+
+	return attr->mode;
 }
 
 static const struct attribute_group ili210x_attr_group = {
 	.attrs = ili210x_attributes,
-	.is_visible = ili210x_calibrate_visible,
+	.is_visible = ili210x_attributes_visible,
 };
 
 static void ili210x_power_down(void *data)
@@ -465,6 +588,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	priv->input = input;
 	priv->reset_gpio = reset_gpio;
 	priv->chip = chip;
+	mutex_init(&priv->lock);
 	i2c_set_clientdata(client, priv);
 
 	/* Setup input device */
-- 
2.32.0

[PATCH v2 3/3] Input: ili210x - add ili251x firmware update support

[Marek Vasut]

The ili251x firmware can be updated, this is used when switching between
different modes of operation of the touch surface, e.g. glove operation.
This patch implements the firmware update mechanism triggered by a write
of firmware name into an sysfs attribute.

The firmware itself is distributed as an intel hex file with non-standard
types. The first two lines are of type 0xad, which indicates the start of
DataFlash payload, that is always at address 0xf000 on the ili251x, so it
can be dropped, and 0xac which indicates the position of firmware info in
the Application payload, that is always at address 0x2020 on the ili251x
and we do not care. The rest of the firmware is data of type 0x00, and we
care about that. To convert the firmware hex file into something usable
by the kernel, remove the first two lines and then use ihex2fw:

 $ tail -n +3 input.hex > temp.hex
 $ ./tools/firmware/ihex2fw temp.hex firmware/touch.bin

To trigger the firmware update, place touch.bin or whichever file name
you pick into /lib/firmware/, write that zero terminated file name into
firmware_update sysfs attribute, wait about 30-40 seconds. The firmware
update is slow. Then verify firmware_version and mode sysfs attributes
to verify whether the firmware got updated and the controller switched
back to application (AP) mode by reading out firmware_version attribute
in sysfs.

Note that the content of firmware_version, e.g. 0600.0005.abcd.aa04 can
be matched to the content of the firmware hex file. The first four bytes,
0x06 0x00 0x00 0x05 can be found at ^:102030 00 05000006, the next four
bytes 0xab 0xcd 0xaa 0x04 at ^:10F000 00 nnnnnnnn ABCDAA04.

Note that the protocol differs considerably between the ili2xxx devices,
this patch therefore implements this functionality only for ili251x that
I can test.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Hung <joe_hung@ilitek.com>
Cc: Luca Hsu <luca_hsu@ilitek.com>
---
V2: - Rename REG_IC_MODE to REG_GET_MODE, since it is pair command to REG_SET_MODE
    - Replace 0xc7 in code with REG_READ_DATA_CRC macro
    - Handle firmware name with newline at the end
    - Update X and Y resolution after firmware update, the FW could have
      changed the resolution
---
 drivers/input/touchscreen/Kconfig   |   1 +
 drivers/input/touchscreen/ili210x.c | 323 +++++++++++++++++++++++++++-
 2 files changed, 318 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index ad454cd2855a..4d34043cdf01 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -425,6 +425,7 @@ config TOUCHSCREEN_HYCON_HY46XX
 config TOUCHSCREEN_ILI210X
 	tristate "Ilitek ILI210X based touchscreen"
 	depends on I2C
+	select CRC_CCITT
 	help
 	  Say Y here if you have a ILI210X based touchscreen
 	  controller. This driver supports models ILI2102,
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 7790ad000dc1..c4a9bd3f67e7 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/crc-ccitt.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/ihex.h>
 #include <linux/input.h>
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
@@ -25,9 +27,14 @@
 #define REG_FIRMWARE_VERSION	0x40
 #define REG_PROTOCOL_VERSION	0x42
 #define REG_KERNEL_VERSION	0x61
-#define REG_IC_MODE		0xc0
-#define REG_IC_MODE_AP		0x5a
-#define REG_IC_MODE_BL		0x55
+#define REG_GET_MODE		0xc0
+#define REG_GET_MODE_AP		0x5a
+#define REG_GET_MODE_BL		0x55
+#define REG_SET_MODE_AP		0xc1
+#define REG_SET_MODE_BL		0xc2
+#define REG_WRITE_DATA		0xc3
+#define REG_WRITE_ENABLE	0xc4
+#define REG_READ_DATA_CRC	0xc7
 #define REG_CALIBRATE		0xcc
 
 struct ili2xxx_chip {
@@ -445,15 +452,15 @@ static ssize_t ili251x_mode_show(struct device *dev,
 
 	/* Get mode */
 	mutex_lock(&priv->lock);
-	ret = priv->chip->read_reg(client, REG_IC_MODE, &md, sizeof(md));
+	ret = priv->chip->read_reg(client, REG_GET_MODE, &md, sizeof(md));
 	mutex_unlock(&priv->lock);
 
 	if (ret)
 		return ret;
 
-	if (md[0] == REG_IC_MODE_AP)		/* Application Mode */
+	if (md[0] == REG_GET_MODE_AP)		/* Application Mode */
 		mode = "AP";
-	else if (md[0] == REG_IC_MODE_BL)	/* BootLoader Mode */
+	else if (md[0] == REG_GET_MODE_BL)	/* BootLoader Mode */
 		mode = "BL";
 	else					/* Unknown Mode */
 		mode = "??";
@@ -488,8 +495,312 @@ static ssize_t ili210x_calibrate(struct device *dev,
 }
 static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
 
+static int ili251x_firmware_to_buffer(struct device *dev,
+				      const char *fwname, u8 **buf,
+				      u16 *ac_end, u16 *df_end)
+{
+	const struct firmware *fw = NULL;
+	const struct ihex_binrec *rec;
+	u32 fw_addr, fw_last_addr = 0;
+	u16 fw_len;
+	u8 *fw_buf;
+	int ret;
+
+	ret = request_ihex_firmware(&fw, fwname, dev);
+	if (ret) {
+		dev_err(dev, "Failed to request firmware %s, ret=%d\n", fwname, ret);
+		return ret;
+	}
+
+	/*
+	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
+	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
+	 * once, copy them all into this buffer at the right locations, and then
+	 * do all operations on this linear buffer.
+	 */
+	fw_buf = kcalloc(1, SZ_64K, GFP_KERNEL);
+	if (!fw_buf) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	rec = (const struct ihex_binrec *)fw->data;
+	while (rec) {
+		fw_addr = be32_to_cpu(rec->addr);
+		fw_len = be16_to_cpu(rec->len);
+
+		if (fw_addr + fw_len > SZ_64K) {
+			ret = -EFBIG;
+			goto err_big;
+		}
+
+		/* Find the last address before DF start address, that is AC end */
+		if (fw_addr == 0xf000)
+			*ac_end = fw_last_addr;
+		fw_last_addr = fw_addr + fw_len;
+
+		memcpy(fw_buf + fw_addr, rec->data, fw_len);
+		rec = ihex_next_binrec(rec);
+	}
+
+	/* DF end address is the last address in the firmware blob */
+	*df_end = fw_addr + fw_len;
+	*buf = fw_buf;
+	release_firmware(fw);
+	return 0;
+err_big:
+	kfree(fw_buf);
+err_alloc:
+	release_firmware(fw);
+	return ret;
+}
+
+/* Switch mode between Application and BootLoader */
+static int ili251x_switch_ic_mode(struct i2c_client *client, u8 cmd_mode)
+{
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u8 cmd_wren[3] = { REG_WRITE_ENABLE, 0x5a, 0xa5 };
+	u8 md[2];
+	int ret;
+
+	ret = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
+	if (ret)
+		return ret;
+	/* Mode already set */
+	if ((cmd_mode == REG_SET_MODE_AP && md[0] == REG_GET_MODE_AP) ||
+	    (cmd_mode == REG_SET_MODE_BL && md[0] == REG_GET_MODE_BL))
+		return 0;
+
+	/* Unlock writes */
+	ret = i2c_master_send(client, cmd_wren, sizeof(cmd_wren));
+	if (ret != sizeof(cmd_wren))
+		return -EINVAL;
+
+	mdelay(20);
+
+	/* Select mode (BootLoader or Application) */
+	ret = i2c_master_send(client, &cmd_mode, 1);
+	if (ret != 1)
+		return -EINVAL;
+
+	mdelay(200);	/* Reboot into bootloader takes a lot of time ... */
+
+	/* Read back mode */
+	ret = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
+	if (ret)
+		return ret;
+	/* Check if mode is correct now. */
+	if ((cmd_mode == REG_SET_MODE_AP && md[0] == REG_GET_MODE_AP) ||
+	    (cmd_mode == REG_SET_MODE_BL && md[0] == REG_GET_MODE_BL))
+		return 0;
+
+	return -EINVAL;
+}
+
+static int ili251x_firmware_busy(struct i2c_client *client)
+{
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int ret, i = 0;
+	u8 data;
+
+	do {
+		/* The read_reg already contains suitable delay */
+		ret = priv->chip->read_reg(client, 0x80, &data, 1);
+		if (ret)
+			return ret;
+		if (i++ == 100000)
+			return -ETIMEDOUT;
+	} while (data != 0x50);
+
+	return 0;
+}
+
+static int ili251x_firmware_write_to_ic(struct device *dev, u8 *fwbuf,
+					u16 start, u16 end, u8 dataflash)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	u8 cmd_crc = REG_READ_DATA_CRC;
+	u8 crcrb[4] = { 0 };
+	u8 fw_data[33];
+	u16 fw_addr;
+	int ret;
+
+	/*
+	 * The DF (dataflash) needs 2 bytes offset for unknown reasons,
+	 * the AC (application) has 2 bytes CRC16-CCITT at the end.
+	 */
+	u16 crc = crc_ccitt(0, fwbuf + start + (dataflash ? 2 : 0),
+			    end - start - 2);
+
+	/* Unlock write to either AC (application) or DF (dataflash) area */
+	u8 cmd_wr[10] = {
+		REG_WRITE_ENABLE, 0x5a, 0xa5, dataflash,
+		(end >> 16) & 0xff, (end >> 8) & 0xff, end & 0xff,
+		(crc >> 16) & 0xff, (crc >> 8) & 0xff, crc & 0xff
+	};
+
+	ret = i2c_master_send(client, cmd_wr, sizeof(cmd_wr));
+	if (ret != sizeof(cmd_wr))
+		return -EINVAL;
+
+	ret = ili251x_firmware_busy(client);
+	if (ret)
+		return ret;
+
+	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
+		fw_data[0] = REG_WRITE_DATA;
+		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);
+		ret = i2c_master_send(client, fw_data, 33);
+		if (ret != sizeof(fw_data))
+			return ret;
+		ret = ili251x_firmware_busy(client);
+		if (ret)
+			return ret;
+	}
+
+	ret = i2c_master_send(client, &cmd_crc, 1);
+	if (ret != 1)
+		return -EINVAL;
+
+	ret = ili251x_firmware_busy(client);
+	if (ret)
+		return ret;
+
+	ret = priv->chip->read_reg(client, REG_READ_DATA_CRC,
+				   &crcrb, sizeof(crcrb));
+	if (ret)
+		return ret;
+
+	/* Check CRC readback */
+	if ((crcrb[0] != (crc & 0xff)) || crcrb[1] != ((crc >> 8) & 0xff))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ili251x_firmware_reset(struct i2c_client *client)
+{
+	u8 cmd_reset[2] = { 0xf2, 0x01 };
+	int ret;
+
+	ret = i2c_master_send(client, cmd_reset, sizeof(cmd_reset));
+	if (ret != sizeof(cmd_reset))
+		return -EINVAL;
+
+	return ili251x_firmware_busy(client);
+}
+
+static void ili251x_hardware_reset(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+
+	/* Reset the controller */
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	usleep_range(10000, 15000);
+	gpiod_set_value_cansleep(priv->reset_gpio, 0);
+	msleep(300);
+}
+
+static ssize_t ili210x_firmware_update_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	char fwname[NAME_MAX];
+	u16 ac_end, df_end;
+	u8 *fwbuf;
+	int ret;
+	int i;
+
+	if (count >= NAME_MAX) {
+		dev_err(dev, "File name too long\n");
+		return -EINVAL;
+	}
+
+	memcpy(fwname, buf, count);
+	if (fwname[count - 1] == '\n')
+		fwname[count - 1] = '\0';
+	else
+		fwname[count] = '\0';
+
+	ret = ili251x_firmware_to_buffer(dev, fwname, &fwbuf, &ac_end, &df_end);
+	if (ret)
+		return ret;
+
+	mutex_lock(&priv->lock);
+
+	dev_info(dev, "Firmware update started, firmware=%s\n", fwname);
+
+	ili251x_hardware_reset(dev);
+
+	ret = ili251x_firmware_reset(client);
+	if (ret)
+		goto exit;
+
+	/* This may not succeed on first try, so re-try a few times. */
+	for (i = 0; i < 5; i++) {
+		ret = ili251x_switch_ic_mode(client, REG_SET_MODE_BL);
+		if (!ret)
+			break;
+	}
+
+	if (ret)
+		goto exit;
+
+	dev_info(dev, "IC is now in BootLoader mode\n");
+
+	msleep(200);	/* The bootloader seems to need some time too. */
+
+	ret = ili251x_firmware_write_to_ic(dev, fwbuf, 0xf000, df_end, 1);
+	if (ret) {
+		dev_err(dev, "DF firmware update failed, ret=%d\n", ret);
+		goto exit;
+	}
+
+	dev_info(dev, "DataFlash firmware written\n");
+
+	ret = ili251x_firmware_write_to_ic(dev, fwbuf, 0x2000, ac_end, 0);
+	if (ret) {
+		dev_err(dev, "AC firmware update failed, ret=%d\n", ret);
+		goto exit;
+	}
+
+	dev_info(dev, "Application firmware written\n");
+
+	/* This may not succeed on first try, so re-try a few times. */
+	for (i = 0; i < 5; i++) {
+		ret = ili251x_switch_ic_mode(client, REG_SET_MODE_AP);
+		if (!ret)
+			break;
+	}
+
+	if (ret)
+		goto exit;
+
+	dev_info(dev, "IC is now in Application mode\n");
+
+	ret = ili251x_firmware_update_resolution(dev);
+	if (ret)
+		goto exit;
+
+	ret = count;
+
+exit:
+	ili251x_hardware_reset(dev);
+	dev_info(dev, "Firmware update ended, ret=%i\n", ret);
+	mutex_unlock(&priv->lock);
+	kfree(fwbuf);
+	return ret;
+}
+
+static DEVICE_ATTR(firmware_update, 0200, NULL, ili210x_firmware_update_store);
+
 static struct attribute *ili210x_attributes[] = {
 	&dev_attr_calibrate.attr,
+	&dev_attr_firmware_update.attr,
 	&dev_attr_firmware_version.attr,
 	&dev_attr_kernel_version.attr,
 	&dev_attr_protocol_version.attr,
-- 
2.32.0

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Dmitry Torokhov]

Hi Marek,

On Fri, Aug 27, 2021 at 11:12:56PM +0200, Marek Vasut wrote:
> The ili251x firmware protocol permits readout of panel resolution,
> implement this, but make it possible to override this value using
> DT bindings. This way, older DTs which contain touchscreen-size-x
> and touchscreen-size-y properties will behave just like before and
> new DTs may avoid specifying these for ILI251x.
> 
> Note that the command format is different on other controllers, so
> this functionality is isolated to ILI251x.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
> V2: New patch
> ---
>  drivers/input/touchscreen/ili210x.c | 31 +++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 30576a5f2f04..c46553ecabe6 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -323,6 +323,34 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ili251x_firmware_update_resolution(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	u16 resx, resy;
> +	u8 rs[10];
> +	int ret;
> +
> +	/* The firmware update blob might have changed the resolution. */
> +	ret = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs));
> +	if (ret)
> +		return ret;
> +
> +	resx = (rs[1] << 8U) | rs[0];
> +	resy = (rs[3] << 8U) | rs[2];

Why do we need the 'U' specifier here? Actually, let's use
le16_to_cpup() or get_unaligned_le16().

> +
> +	/* The value reported by the firmware is invalid. */
> +	if (!resx || resx == 0xffff || !resy || resy == 0xfff)

Not 0xffff for resy?

> +		return -EINVAL;
> +
> +	priv->input->absinfo[ABS_X].maximum = resx - 1;
> +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
> +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
> +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;

There is

	input_set_abs_max(priv->input, ABS_X, resx - 1);

> +
> +	return 0;
> +}
> +
>  static ssize_t ili210x_calibrate(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
> @@ -449,6 +477,9 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, max_xy, 0, 0);
>  	if (priv->chip->has_pressure_reg)
>  		input_set_abs_params(input, ABS_MT_PRESSURE, 0, 0xa, 0, 0);
> +	/* ILI251x does report valid resolution information, try it. */
> +	if (priv->chip == &ili251x_chip)
> +		ili251x_firmware_update_resolution(dev);

Would prefer a flag in chip features vs matching on chip.

>  	touchscreen_parse_properties(input, true, &priv->prop);
>  
>  	error = input_mt_init_slots(input, priv->chip->max_touches,
> -- 
> 2.32.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Dmitry Torokhov]

On Fri, Aug 27, 2021 at 11:12:57PM +0200, Marek Vasut wrote:
> The ili251x firmware protocol permits readout of firmware version,
> protocol version, mcu version and current mode (application, boot
> loader, forced update). These information are useful when updating
> the firmware on the il251x, e.g. to avoid updating the same firmware
> into the device multiple times. The locking is now necessary to avoid
> races between interrupt handler and the sysfs readouts.
> 
> Note that the protocol differs considerably between the ili2xxx devices,
> this patch therefore implements this functionality only for ili251x that
> I can test.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
> V2: No change
> 
> NOTE: Regarding checkpatch warnings, Consider renaming function(s)
>       'ili251x_firmware_version_show' to 'firmware_version_show',
>       I considered it and decided against it, because grepping for
>       ili251x in debug symbols gives far more accurate results than
>       grepping for firmware_version.

Yep, I completely agree here. I wish checkpatch did not complain about
this.

> ---
>  drivers/input/touchscreen/ili210x.c | 130 +++++++++++++++++++++++++++-
>  1 file changed, 127 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index c46553ecabe6..7790ad000dc1 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -22,6 +22,12 @@
>  /* Touchscreen commands */
>  #define REG_TOUCHDATA		0x10
>  #define REG_PANEL_INFO		0x20
> +#define REG_FIRMWARE_VERSION	0x40
> +#define REG_PROTOCOL_VERSION	0x42
> +#define REG_KERNEL_VERSION	0x61
> +#define REG_IC_MODE		0xc0
> +#define REG_IC_MODE_AP		0x5a
> +#define REG_IC_MODE_BL		0x55
>  #define REG_CALIBRATE		0xcc
>  
>  struct ili2xxx_chip {
> @@ -43,6 +49,7 @@ struct ili210x {
>  	struct input_dev *input;
>  	struct gpio_desc *reset_gpio;
>  	struct touchscreen_properties prop;
> +	struct mutex lock;
>  	const struct ili2xxx_chip *chip;
>  	bool stop;
>  };
> @@ -307,7 +314,9 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
>  	int error;
>  
>  	do {
> +		mutex_lock(&priv->lock);
>  		error = chip->get_touch_data(client, touchdata);
> +		mutex_unlock(&priv->lock);
>  		if (error) {
>  			dev_err(&client->dev,
>  				"Unable to get touch data: %d\n", error);
> @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
>  	return 0;
>  }
>  
> +static ssize_t ili251x_firmware_version_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	u8 fw[8];
> +	int ret;
> +
> +	/* Get firmware version */
> +	mutex_lock(&priv->lock);
> +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
> +				   &fw, sizeof(fw));
> +	mutex_unlock(&priv->lock);

Could we query firmware version and mode at probe time (and also later
after firmware update attempt) so that we do not need to introduce
locking against interrupt handler?

> +
> +	if (ret)
> +		return ret;
> +
> +	if (!ret) {
> +		ret = scnprintf(buf, PAGE_SIZE,
> +				"%02x%02x.%02x%02x.%02x%02x.%02x%02x\n",
> +				fw[0], fw[1], fw[2], fw[3],
> +				fw[4], fw[5], fw[6], fw[7]);
> +	}

I think there is sysfs_emit() that is preferred now.

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/3] Input: ili210x - add ili251x firmware update support

[Dmitry Torokhov]

On Fri, Aug 27, 2021 at 11:12:58PM +0200, Marek Vasut wrote:
> The ili251x firmware can be updated, this is used when switching between
> different modes of operation of the touch surface, e.g. glove operation.
> This patch implements the firmware update mechanism triggered by a write
> of firmware name into an sysfs attribute.
> 
> The firmware itself is distributed as an intel hex file with non-standard
> types. The first two lines are of type 0xad, which indicates the start of
> DataFlash payload, that is always at address 0xf000 on the ili251x, so it
> can be dropped, and 0xac which indicates the position of firmware info in
> the Application payload, that is always at address 0x2020 on the ili251x
> and we do not care. The rest of the firmware is data of type 0x00, and we
> care about that. To convert the firmware hex file into something usable
> by the kernel, remove the first two lines and then use ihex2fw:
> 
>  $ tail -n +3 input.hex > temp.hex
>  $ ./tools/firmware/ihex2fw temp.hex firmware/touch.bin
> 
> To trigger the firmware update, place touch.bin or whichever file name
> you pick into /lib/firmware/, write that zero terminated file name into
> firmware_update sysfs attribute, wait about 30-40 seconds. The firmware
> update is slow. Then verify firmware_version and mode sysfs attributes
> to verify whether the firmware got updated and the controller switched
> back to application (AP) mode by reading out firmware_version attribute
> in sysfs.
> 
> Note that the content of firmware_version, e.g. 0600.0005.abcd.aa04 can
> be matched to the content of the firmware hex file. The first four bytes,
> 0x06 0x00 0x00 0x05 can be found at ^:102030 00 05000006, the next four
> bytes 0xab 0xcd 0xaa 0x04 at ^:10F000 00 nnnnnnnn ABCDAA04.
> 
> Note that the protocol differs considerably between the ili2xxx devices,
> this patch therefore implements this functionality only for ili251x that
> I can test.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
> V2: - Rename REG_IC_MODE to REG_GET_MODE, since it is pair command to REG_SET_MODE
>     - Replace 0xc7 in code with REG_READ_DATA_CRC macro
>     - Handle firmware name with newline at the end
>     - Update X and Y resolution after firmware update, the FW could have
>       changed the resolution
> ---
>  drivers/input/touchscreen/Kconfig   |   1 +
>  drivers/input/touchscreen/ili210x.c | 323 +++++++++++++++++++++++++++-
>  2 files changed, 318 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index ad454cd2855a..4d34043cdf01 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -425,6 +425,7 @@ config TOUCHSCREEN_HYCON_HY46XX
>  config TOUCHSCREEN_ILI210X
>  	tristate "Ilitek ILI210X based touchscreen"
>  	depends on I2C
> +	select CRC_CCITT
>  	help
>  	  Say Y here if you have a ILI210X based touchscreen
>  	  controller. This driver supports models ILI2102,
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 7790ad000dc1..c4a9bd3f67e7 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/crc-ccitt.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> +#include <linux/ihex.h>
>  #include <linux/input.h>
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
> @@ -25,9 +27,14 @@
>  #define REG_FIRMWARE_VERSION	0x40
>  #define REG_PROTOCOL_VERSION	0x42
>  #define REG_KERNEL_VERSION	0x61
> -#define REG_IC_MODE		0xc0
> -#define REG_IC_MODE_AP		0x5a
> -#define REG_IC_MODE_BL		0x55
> +#define REG_GET_MODE		0xc0
> +#define REG_GET_MODE_AP		0x5a
> +#define REG_GET_MODE_BL		0x55
> +#define REG_SET_MODE_AP		0xc1
> +#define REG_SET_MODE_BL		0xc2
> +#define REG_WRITE_DATA		0xc3
> +#define REG_WRITE_ENABLE	0xc4
> +#define REG_READ_DATA_CRC	0xc7
>  #define REG_CALIBRATE		0xcc
>  
>  struct ili2xxx_chip {
> @@ -445,15 +452,15 @@ static ssize_t ili251x_mode_show(struct device *dev,
>  
>  	/* Get mode */
>  	mutex_lock(&priv->lock);
> -	ret = priv->chip->read_reg(client, REG_IC_MODE, &md, sizeof(md));
> +	ret = priv->chip->read_reg(client, REG_GET_MODE, &md, sizeof(md));
>  	mutex_unlock(&priv->lock);
>  
>  	if (ret)
>  		return ret;
>  
> -	if (md[0] == REG_IC_MODE_AP)		/* Application Mode */
> +	if (md[0] == REG_GET_MODE_AP)		/* Application Mode */
>  		mode = "AP";
> -	else if (md[0] == REG_IC_MODE_BL)	/* BootLoader Mode */
> +	else if (md[0] == REG_GET_MODE_BL)	/* BootLoader Mode */
>  		mode = "BL";
>  	else					/* Unknown Mode */
g>  		mode = "??";
> @@ -488,8 +495,312 @@ static ssize_t ili210x_calibrate(struct device *dev,
>  }
>  static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
>  
> +static int ili251x_firmware_to_buffer(struct device *dev,
> +				      const char *fwname, u8 **buf,
> +				      u16 *ac_end, u16 *df_end)
> +{
> +	const struct firmware *fw = NULL;
> +	const struct ihex_binrec *rec;
> +	u32 fw_addr, fw_last_addr = 0;
> +	u16 fw_len;
> +	u8 *fw_buf;
> +	int ret;
> +
> +	ret = request_ihex_firmware(&fw, fwname, dev);

Let's call variables that carry error code or 0 as success 'error' and
reserve 'ret' for cases when there can be actual data.

> +	if (ret) {
> +		dev_err(dev, "Failed to request firmware %s, ret=%d\n", fwname, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
> +	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
> +	 * once, copy them all into this buffer at the right locations, and then
> +	 * do all operations on this linear buffer.
> +	 */
> +	fw_buf = kcalloc(1, SZ_64K, GFP_KERNEL);

Why kcalloc and not kzalloc?

> +	if (!fw_buf) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	rec = (const struct ihex_binrec *)fw->data;
> +	while (rec) {
> +		fw_addr = be32_to_cpu(rec->addr);
> +		fw_len = be16_to_cpu(rec->len);
> +
> +		if (fw_addr + fw_len > SZ_64K) {
> +			ret = -EFBIG;
> +			goto err_big;
> +		}
> +
> +		/* Find the last address before DF start address, that is AC end */
> +		if (fw_addr == 0xf000)
> +			*ac_end = fw_last_addr;
> +		fw_last_addr = fw_addr + fw_len;
> +
> +		memcpy(fw_buf + fw_addr, rec->data, fw_len);
> +		rec = ihex_next_binrec(rec);
> +	}
> +
> +	/* DF end address is the last address in the firmware blob */
> +	*df_end = fw_addr + fw_len;
> +	*buf = fw_buf;
> +	release_firmware(fw);
> +	return 0;
> +err_big:
> +	kfree(fw_buf);
> +err_alloc:
> +	release_firmware(fw);
> +	return ret;
> +}
> +
> +/* Switch mode between Application and BootLoader */
> +static int ili251x_switch_ic_mode(struct i2c_client *client, u8 cmd_mode)
> +{
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	u8 cmd_wren[3] = { REG_WRITE_ENABLE, 0x5a, 0xa5 };
> +	u8 md[2];
> +	int ret;
> +
> +	ret = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
> +	if (ret)
> +		return ret;
> +	/* Mode already set */
> +	if ((cmd_mode == REG_SET_MODE_AP && md[0] == REG_GET_MODE_AP) ||
> +	    (cmd_mode == REG_SET_MODE_BL && md[0] == REG_GET_MODE_BL))
> +		return 0;
> +
> +	/* Unlock writes */
> +	ret = i2c_master_send(client, cmd_wren, sizeof(cmd_wren));
> +	if (ret != sizeof(cmd_wren))
> +		return -EINVAL;
> +
> +	mdelay(20);
> +
> +	/* Select mode (BootLoader or Application) */
> +	ret = i2c_master_send(client, &cmd_mode, 1);
> +	if (ret != 1)
> +		return -EINVAL;
> +
> +	mdelay(200);	/* Reboot into bootloader takes a lot of time ... */
> +
> +	/* Read back mode */
> +	ret = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
> +	if (ret)
> +		return ret;
> +	/* Check if mode is correct now. */
> +	if ((cmd_mode == REG_SET_MODE_AP && md[0] == REG_GET_MODE_AP) ||
> +	    (cmd_mode == REG_SET_MODE_BL && md[0] == REG_GET_MODE_BL))
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int ili251x_firmware_busy(struct i2c_client *client)
> +{
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	int ret, i = 0;
> +	u8 data;
> +
> +	do {
> +		/* The read_reg already contains suitable delay */
> +		ret = priv->chip->read_reg(client, 0x80, &data, 1);

Can we have symbolic name for this register (and others).

> +		if (ret)
> +			return ret;
> +		if (i++ == 100000)
> +			return -ETIMEDOUT;
> +	} while (data != 0x50);
> +
> +	return 0;
> +}
> +
> +static int ili251x_firmware_write_to_ic(struct device *dev, u8 *fwbuf,
> +					u16 start, u16 end, u8 dataflash)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	u8 cmd_crc = REG_READ_DATA_CRC;
> +	u8 crcrb[4] = { 0 };
> +	u8 fw_data[33];
> +	u16 fw_addr;
> +	int ret;
> +
> +	/*
> +	 * The DF (dataflash) needs 2 bytes offset for unknown reasons,
> +	 * the AC (application) has 2 bytes CRC16-CCITT at the end.
> +	 */
> +	u16 crc = crc_ccitt(0, fwbuf + start + (dataflash ? 2 : 0),
> +			    end - start - 2);
> +
> +	/* Unlock write to either AC (application) or DF (dataflash) area */
> +	u8 cmd_wr[10] = {
> +		REG_WRITE_ENABLE, 0x5a, 0xa5, dataflash,
> +		(end >> 16) & 0xff, (end >> 8) & 0xff, end & 0xff,
> +		(crc >> 16) & 0xff, (crc >> 8) & 0xff, crc & 0xff
> +	};
> +
> +	ret = i2c_master_send(client, cmd_wr, sizeof(cmd_wr));
> +	if (ret != sizeof(cmd_wr))
> +		return -EINVAL;
> +
> +	ret = ili251x_firmware_busy(client);
> +	if (ret)
> +		return ret;
> +
> +	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
> +		fw_data[0] = REG_WRITE_DATA;
> +		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);

Is it guaranteed that we have enough data (32 bytes) and we will not
reach past the buffer?

> +		ret = i2c_master_send(client, fw_data, 33);
> +		if (ret != sizeof(fw_data))
> +			return ret;
> +		ret = ili251x_firmware_busy(client);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = i2c_master_send(client, &cmd_crc, 1);
> +	if (ret != 1)
> +		return -EINVAL;
> +
> +	ret = ili251x_firmware_busy(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = priv->chip->read_reg(client, REG_READ_DATA_CRC,
> +				   &crcrb, sizeof(crcrb));
> +	if (ret)
> +		return ret;
> +
> +	/* Check CRC readback */
> +	if ((crcrb[0] != (crc & 0xff)) || crcrb[1] != ((crc >> 8) & 0xff))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ili251x_firmware_reset(struct i2c_client *client)
> +{
> +	u8 cmd_reset[2] = { 0xf2, 0x01 };
> +	int ret;
> +
> +	ret = i2c_master_send(client, cmd_reset, sizeof(cmd_reset));
> +	if (ret != sizeof(cmd_reset))
> +		return -EINVAL;
> +
> +	return ili251x_firmware_busy(client);
> +}
> +
> +static void ili251x_hardware_reset(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +
> +	/* Reset the controller */
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	usleep_range(10000, 15000);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +	msleep(300);
> +}
> +
> +static ssize_t ili210x_firmware_update_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	char fwname[NAME_MAX];
> +	u16 ac_end, df_end;
> +	u8 *fwbuf;
> +	int ret;
> +	int i;
> +
> +	if (count >= NAME_MAX) {
> +		dev_err(dev, "File name too long\n");
> +		return -EINVAL;
> +	}
> +
> +	memcpy(fwname, buf, count);
> +	if (fwname[count - 1] == '\n')
> +		fwname[count - 1] = '\0';
> +	else
> +		fwname[count] = '\0';

I believe the practice is to use constant firmware name based on driver
or chip name. If there is desire to allow dynamic firmware name for
given device I think it should be handled at firmware loader core.

> +
> +	ret = ili251x_firmware_to_buffer(dev, fwname, &fwbuf, &ac_end, &df_end);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	dev_info(dev, "Firmware update started, firmware=%s\n", fwname);

dev_dbg() please.

> +
> +	ili251x_hardware_reset(dev);
> +
> +	ret = ili251x_firmware_reset(client);
> +	if (ret)
> +		goto exit;
> +
> +	/* This may not succeed on first try, so re-try a few times. */
> +	for (i = 0; i < 5; i++) {
> +		ret = ili251x_switch_ic_mode(client, REG_SET_MODE_BL);
> +		if (!ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		goto exit;
> +
> +	dev_info(dev, "IC is now in BootLoader mode\n");
> +
> +	msleep(200);	/* The bootloader seems to need some time too. */
> +
> +	ret = ili251x_firmware_write_to_ic(dev, fwbuf, 0xf000, df_end, 1);
> +	if (ret) {
> +		dev_err(dev, "DF firmware update failed, ret=%d\n", ret);
> +		goto exit;
> +	}
> +
> +	dev_info(dev, "DataFlash firmware written\n");
> +
> +	ret = ili251x_firmware_write_to_ic(dev, fwbuf, 0x2000, ac_end, 0);
> +	if (ret) {
> +		dev_err(dev, "AC firmware update failed, ret=%d\n", ret);
> +		goto exit;
> +	}
> +
> +	dev_info(dev, "Application firmware written\n");
> +
> +	/* This may not succeed on first try, so re-try a few times. */
> +	for (i = 0; i < 5; i++) {
> +		ret = ili251x_switch_ic_mode(client, REG_SET_MODE_AP);
> +		if (!ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		goto exit;
> +
> +	dev_info(dev, "IC is now in Application mode\n");
> +
> +	ret = ili251x_firmware_update_resolution(dev);
> +	if (ret)
> +		goto exit;
> +
> +	ret = count;
> +
> +exit:
> +	ili251x_hardware_reset(dev);
> +	dev_info(dev, "Firmware update ended, ret=%i\n", ret);
> +	mutex_unlock(&priv->lock);
> +	kfree(fwbuf);
> +	return ret;
> +}
> +
> +static DEVICE_ATTR(firmware_update, 0200, NULL, ili210x_firmware_update_store);
> +
>  static struct attribute *ili210x_attributes[] = {
>  	&dev_attr_calibrate.attr,
> +	&dev_attr_firmware_update.attr,
>  	&dev_attr_firmware_version.attr,
>  	&dev_attr_kernel_version.attr,
>  	&dev_attr_protocol_version.attr,
> -- 
> 2.32.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Marek Vasut]

On 8/30/21 10:55 PM, Dmitry Torokhov wrote:

[...]

>> +		return -EINVAL;
>> +
>> +	priv->input->absinfo[ABS_X].maximum = resx - 1;
>> +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
>> +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
>> +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
> 
> There is
> 
> 	input_set_abs_max(priv->input, ABS_X, resx - 1);

git grep finds nothing in linux-next / your tree on k.org / patchwork. 
Where is that ?

[...]

Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Marek Vasut]

On 8/30/21 11:01 PM, Dmitry Torokhov wrote:

[...]

>> @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
>>   	return 0;
>>   }
>>   
>> +static ssize_t ili251x_firmware_version_show(struct device *dev,
>> +					     struct device_attribute *attr,
>> +					     char *buf)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct ili210x *priv = i2c_get_clientdata(client);
>> +	u8 fw[8];
>> +	int ret;
>> +
>> +	/* Get firmware version */
>> +	mutex_lock(&priv->lock);
>> +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
>> +				   &fw, sizeof(fw));
>> +	mutex_unlock(&priv->lock);
> 
> Could we query firmware version and mode at probe time (and also later
> after firmware update attempt) so that we do not need to introduce
> locking against interrupt handler?

This is a threaded interrupt handler and I don't expect much lock 
contention here.

The sysfs attribute readout would race with the interrupt handler and if 
it wasn't for the firmware update support, we could very well cache all 
those values. Except, the firmware update can change them all. Worse, if 
the interrupt were to fire during an update, it could break that update, 
and I want to prevent that from happening.

[...]

Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 01:02:57AM +0200, Marek Vasut wrote:
> On 8/30/21 11:01 PM, Dmitry Torokhov wrote:
> 
> [...]
> 
> > > @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
> > >   	return 0;
> > >   }
> > > +static ssize_t ili251x_firmware_version_show(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     char *buf)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	u8 fw[8];
> > > +	int ret;
> > > +
> > > +	/* Get firmware version */
> > > +	mutex_lock(&priv->lock);
> > > +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
> > > +				   &fw, sizeof(fw));
> > > +	mutex_unlock(&priv->lock);
> > 
> > Could we query firmware version and mode at probe time (and also later
> > after firmware update attempt) so that we do not need to introduce
> > locking against interrupt handler?
> 
> This is a threaded interrupt handler and I don't expect much lock contention
> here.
> 
> The sysfs attribute readout would race with the interrupt handler and if it
> wasn't for the firmware update support, we could very well cache all those
> values. Except, the firmware update can change them all. Worse, if the
> interrupt were to fire during an update, it could break that update, and I
> want to prevent that from happening.

Usually we simply disable interrupts from the device when updating
firmware.

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 12:49:49AM +0200, Marek Vasut wrote:
> On 8/30/21 10:55 PM, Dmitry Torokhov wrote:
> 
> [...]
> 
> > > +		return -EINVAL;
> > > +
> > > +	priv->input->absinfo[ABS_X].maximum = resx - 1;
> > > +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
> > > +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
> > > +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
> > 
> > There is
> > 
> > 	input_set_abs_max(priv->input, ABS_X, resx - 1);
> 
> git grep finds nothing in linux-next / your tree on k.org / patchwork. Where
> is that ?

Look for INPUT_GENERATE_ABS_ACCESSORS in include/linux/input.h

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/3] Input: ili210x - add ili251x firmware update support

[Marek Vasut]

On 8/30/21 11:14 PM, Dmitry Torokhov wrote:

[...]

>> +	if (ret) {
>> +		dev_err(dev, "Failed to request firmware %s, ret=%d\n", fwname, ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
>> +	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
>> +	 * once, copy them all into this buffer at the right locations, and then
>> +	 * do all operations on this linear buffer.
>> +	 */
>> +	fw_buf = kcalloc(1, SZ_64K, GFP_KERNEL);
> 
> Why kcalloc and not kzalloc?

Because the firmware blob might be sparse (with gaps in it) and those 
gaps should be zeroed out instead of random data (see your question 
about the 32 byte long memcpy() below (*) as well).

>> +	if (!fw_buf) {
>> +		ret = -ENOMEM;
>> +		goto err_alloc;
>> +	}
>> +
>> +	rec = (const struct ihex_binrec *)fw->data;
>> +	while (rec) {
>> +		fw_addr = be32_to_cpu(rec->addr);
>> +		fw_len = be16_to_cpu(rec->len);
>> +
>> +		if (fw_addr + fw_len > SZ_64K) {
>> +			ret = -EFBIG;
>> +			goto err_big;
>> +		}
>> +
>> +		/* Find the last address before DF start address, that is AC end */
>> +		if (fw_addr == 0xf000)
>> +			*ac_end = fw_last_addr;
>> +		fw_last_addr = fw_addr + fw_len;
>> +
>> +		memcpy(fw_buf + fw_addr, rec->data, fw_len);
>> +		rec = ihex_next_binrec(rec);
>> +	}

[...]

>> +static int ili251x_firmware_busy(struct i2c_client *client)
>> +{
>> +	struct ili210x *priv = i2c_get_clientdata(client);
>> +	int ret, i = 0;
>> +	u8 data;
>> +
>> +	do {
>> +		/* The read_reg already contains suitable delay */
>> +		ret = priv->chip->read_reg(client, 0x80, &data, 1);
> 
> Can we have symbolic name for this register (and others).

The name of this one isn't part of the example code, so ... I can call 
it something, but who knows what it really is.

I believe I did manage to find what the other registers are called already.

[...]

>> +	ret = ili251x_firmware_busy(client);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
>> +		fw_data[0] = REG_WRITE_DATA;
>> +		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);
> 
> Is it guaranteed that we have enough data (32 bytes) and we will not
> reach past the buffer?

Yes, see above (*). If the firmware blob entry were too short, this 
would pull in zeroes.

I tried iterating through the firmware file, but using linear buffer for 
the firmware results in far less convoluted code, and considering it is 
not performance critical, I think this is ok.

[...]

>> +static ssize_t ili210x_firmware_update_store(struct device *dev,
>> +					     struct device_attribute *attr,
>> +					     const char *buf, size_t count)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct ili210x *priv = i2c_get_clientdata(client);
>> +	char fwname[NAME_MAX];
>> +	u16 ac_end, df_end;
>> +	u8 *fwbuf;
>> +	int ret;
>> +	int i;
>> +
>> +	if (count >= NAME_MAX) {
>> +		dev_err(dev, "File name too long\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	memcpy(fwname, buf, count);
>> +	if (fwname[count - 1] == '\n')
>> +		fwname[count - 1] = '\0';
>> +	else
>> +		fwname[count] = '\0';
> 
> I believe the practice is to use constant firmware name based on driver
> or chip name. If there is desire to allow dynamic firmware name for
> given device I think it should be handled at firmware loader core.

There are a couple of input devices which do similar thing, see e.g.:
drivers/input/mouse/cyapa.c
drivers/input/rmi4/rmi_f34.c

The ilitek firmware contains both firmware and calibration data, so you 
might end up with a usecase where you switch between different firmware 
blobs at runtime.

That's why it is implemented this way.

[...]

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Marek Vasut]

On 8/31/21 1:21 AM, Dmitry Torokhov wrote:
> On Tue, Aug 31, 2021 at 12:49:49AM +0200, Marek Vasut wrote:
>> On 8/30/21 10:55 PM, Dmitry Torokhov wrote:
>>
>> [...]
>>
>>>> +		return -EINVAL;
>>>> +
>>>> +	priv->input->absinfo[ABS_X].maximum = resx - 1;
>>>> +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
>>>> +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
>>>> +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
>>>
>>> There is
>>>
>>> 	input_set_abs_max(priv->input, ABS_X, resx - 1);
>>
>> git grep finds nothing in linux-next / your tree on k.org / patchwork. Where
>> is that ?
> 
> Look for INPUT_GENERATE_ABS_ACCESSORS in include/linux/input.h

Oh, input_abs_set_max, thanks.

Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Marek Vasut]

On 8/31/21 1:20 AM, Dmitry Torokhov wrote:
> On Tue, Aug 31, 2021 at 01:02:57AM +0200, Marek Vasut wrote:
>> On 8/30/21 11:01 PM, Dmitry Torokhov wrote:
>>
>> [...]
>>
>>>> @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
>>>>    	return 0;
>>>>    }
>>>> +static ssize_t ili251x_firmware_version_show(struct device *dev,
>>>> +					     struct device_attribute *attr,
>>>> +					     char *buf)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>>> +	u8 fw[8];
>>>> +	int ret;
>>>> +
>>>> +	/* Get firmware version */
>>>> +	mutex_lock(&priv->lock);
>>>> +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
>>>> +				   &fw, sizeof(fw));
>>>> +	mutex_unlock(&priv->lock);
>>>
>>> Could we query firmware version and mode at probe time (and also later
>>> after firmware update attempt) so that we do not need to introduce
>>> locking against interrupt handler?
>>
>> This is a threaded interrupt handler and I don't expect much lock contention
>> here.
>>
>> The sysfs attribute readout would race with the interrupt handler and if it
>> wasn't for the firmware update support, we could very well cache all those
>> values. Except, the firmware update can change them all. Worse, if the
>> interrupt were to fire during an update, it could break that update, and I
>> want to prevent that from happening.
> 
> Usually we simply disable interrupts from the device when updating
> firmware.

I don't think this touch controller has any "disable interrupts" bit.
So the only option here would be some disable_irq() on the IRQ line itself ?

Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 01:33:10AM +0200, Marek Vasut wrote:
> On 8/31/21 1:20 AM, Dmitry Torokhov wrote:
> > On Tue, Aug 31, 2021 at 01:02:57AM +0200, Marek Vasut wrote:
> > > On 8/30/21 11:01 PM, Dmitry Torokhov wrote:
> > > 
> > > [...]
> > > 
> > > > > @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
> > > > >    	return 0;
> > > > >    }
> > > > > +static ssize_t ili251x_firmware_version_show(struct device *dev,
> > > > > +					     struct device_attribute *attr,
> > > > > +					     char *buf)
> > > > > +{
> > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > > > +	u8 fw[8];
> > > > > +	int ret;
> > > > > +
> > > > > +	/* Get firmware version */
> > > > > +	mutex_lock(&priv->lock);
> > > > > +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
> > > > > +				   &fw, sizeof(fw));
> > > > > +	mutex_unlock(&priv->lock);
> > > > 
> > > > Could we query firmware version and mode at probe time (and also later
> > > > after firmware update attempt) so that we do not need to introduce
> > > > locking against interrupt handler?
> > > 
> > > This is a threaded interrupt handler and I don't expect much lock contention
> > > here.
> > > 
> > > The sysfs attribute readout would race with the interrupt handler and if it
> > > wasn't for the firmware update support, we could very well cache all those
> > > values. Except, the firmware update can change them all. Worse, if the
> > > interrupt were to fire during an update, it could break that update, and I
> > > want to prevent that from happening.
> > 
> > Usually we simply disable interrupts from the device when updating
> > firmware.
> 
> I don't think this touch controller has any "disable interrupts" bit.
> So the only option here would be some disable_irq() on the IRQ line itself ?

Yes I believe so.

-- 
Dmitry

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 01:31:06AM +0200, Marek Vasut wrote:
> On 8/31/21 1:21 AM, Dmitry Torokhov wrote:
> > On Tue, Aug 31, 2021 at 12:49:49AM +0200, Marek Vasut wrote:
> > > On 8/30/21 10:55 PM, Dmitry Torokhov wrote:
> > > 
> > > [...]
> > > 
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv->input->absinfo[ABS_X].maximum = resx - 1;
> > > > > +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
> > > > > +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
> > > > > +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
> > > > 
> > > > There is
> > > > 
> > > > 	input_set_abs_max(priv->input, ABS_X, resx - 1);
> > > 
> > > git grep finds nothing in linux-next / your tree on k.org / patchwork. Where
> > > is that ?
> > 
> > Look for INPUT_GENERATE_ABS_ACCESSORS in include/linux/input.h
> 
> Oh, input_abs_set_max, thanks.

Ah, sorry, mistyped the name first time around.

-- 
Dmitry

Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

[Marek Vasut]

On 8/31/21 1:46 AM, Dmitry Torokhov wrote:
> On Tue, Aug 31, 2021 at 01:31:06AM +0200, Marek Vasut wrote:
>> On 8/31/21 1:21 AM, Dmitry Torokhov wrote:
>>> On Tue, Aug 31, 2021 at 12:49:49AM +0200, Marek Vasut wrote:
>>>> On 8/30/21 10:55 PM, Dmitry Torokhov wrote:
>>>>
>>>> [...]
>>>>
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	priv->input->absinfo[ABS_X].maximum = resx - 1;
>>>>>> +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
>>>>>> +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
>>>>>> +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;
>>>>>
>>>>> There is
>>>>>
>>>>> 	input_set_abs_max(priv->input, ABS_X, resx - 1);
>>>>
>>>> git grep finds nothing in linux-next / your tree on k.org / patchwork. Where
>>>> is that ?
>>>
>>> Look for INPUT_GENERATE_ABS_ACCESSORS in include/linux/input.h
>>
>> Oh, input_abs_set_max, thanks.
> 
> Ah, sorry, mistyped the name first time around.

Yep, no worries, this will be fixed in V3.

Re: [PATCH v2 3/3] Input: ili210x - add ili251x firmware update support

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 01:27:21AM +0200, Marek Vasut wrote:
> On 8/30/21 11:14 PM, Dmitry Torokhov wrote:
> 
> [...]
> 
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to request firmware %s, ret=%d\n", fwname, ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
> > > +	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
> > > +	 * once, copy them all into this buffer at the right locations, and then
> > > +	 * do all operations on this linear buffer.
> > > +	 */
> > > +	fw_buf = kcalloc(1, SZ_64K, GFP_KERNEL);
> > 
> > Why kcalloc and not kzalloc?
> 
> Because the firmware blob might be sparse (with gaps in it) and those gaps
> should be zeroed out instead of random data (see your question about the 32
> byte long memcpy() below (*) as well).

That is the exact purpose of kZalloc - to ZERO-out allocation (as
compared to kmalloc() which leaves memory uninitialized).

> 
> > > +	if (!fw_buf) {
> > > +		ret = -ENOMEM;
> > > +		goto err_alloc;
> > > +	}
> > > +
> > > +	rec = (const struct ihex_binrec *)fw->data;
> > > +	while (rec) {
> > > +		fw_addr = be32_to_cpu(rec->addr);
> > > +		fw_len = be16_to_cpu(rec->len);
> > > +
> > > +		if (fw_addr + fw_len > SZ_64K) {
> > > +			ret = -EFBIG;
> > > +			goto err_big;
> > > +		}
> > > +
> > > +		/* Find the last address before DF start address, that is AC end */
> > > +		if (fw_addr == 0xf000)
> > > +			*ac_end = fw_last_addr;
> > > +		fw_last_addr = fw_addr + fw_len;
> > > +
> > > +		memcpy(fw_buf + fw_addr, rec->data, fw_len);
> > > +		rec = ihex_next_binrec(rec);
> > > +	}
> 
> [...]
> 
> > > +static int ili251x_firmware_busy(struct i2c_client *client)
> > > +{
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	int ret, i = 0;
> > > +	u8 data;
> > > +
> > > +	do {
> > > +		/* The read_reg already contains suitable delay */
> > > +		ret = priv->chip->read_reg(client, 0x80, &data, 1);
> > 
> > Can we have symbolic name for this register (and others).
> 
> The name of this one isn't part of the example code, so ... I can call it
> something, but who knows what it really is.
> 
> I believe I did manage to find what the other registers are called already.

OK.

> 
> [...]
> 
> > > +	ret = ili251x_firmware_busy(client);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
> > > +		fw_data[0] = REG_WRITE_DATA;
> > > +		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);
> > 
> > Is it guaranteed that we have enough data (32 bytes) and we will not
> > reach past the buffer?
> 
> Yes, see above (*). If the firmware blob entry were too short, this would
> pull in zeroes.
> 
> I tried iterating through the firmware file, but using linear buffer for the
> firmware results in far less convoluted code, and considering it is not
> performance critical, I think this is ok.

Could you drop a comment to that effect.

> 
> [...]
> 
> > > +static ssize_t ili210x_firmware_update_store(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     const char *buf, size_t count)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	char fwname[NAME_MAX];
> > > +	u16 ac_end, df_end;
> > > +	u8 *fwbuf;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	if (count >= NAME_MAX) {
> > > +		dev_err(dev, "File name too long\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	memcpy(fwname, buf, count);
> > > +	if (fwname[count - 1] == '\n')
> > > +		fwname[count - 1] = '\0';
> > > +	else
> > > +		fwname[count] = '\0';
> > 
> > I believe the practice is to use constant firmware name based on driver
> > or chip name. If there is desire to allow dynamic firmware name for
> > given device I think it should be handled at firmware loader core.
> 
> There are a couple of input devices which do similar thing, see e.g.:
> drivers/input/mouse/cyapa.c
> drivers/input/rmi4/rmi_f34.c
> 
> The ilitek firmware contains both firmware and calibration data, so you
> might end up with a usecase where you switch between different firmware
> blobs at runtime.
> 
> That's why it is implemented this way.

Right, and I'd rather not proliferate this any further. Can you drop the
filename support from this patch so it can be merged easily, and then we
can continue discussion on this topic separately?

Thanks.

-- 
Dmitry