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

[PATCH v3 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
V3: - Use le16_to_cpup() byte-swap resolution data
    - Check Y-resolution range up to 0xffff too
    - Use .has_firmware_proto flag to discern supported MCU protocol
    - Use input_abs_set_max() per include/linux/input.h INPUT_GENERATE_ABS_ACCESSORS
    - Rename variable ret to error
    - Add a wrapper function ili251x_firmware_update_cached_state(),
      which would call pull other cacheable state from the controller
      in subsequent patch (hence also the ret variable in it which
      looks like it could be removed, this will reduce the number of
      changes in the next patch).
    - Wait for the firmware to fully stabilize itself after reset.
      No, those 200 milliseconds is not a mistake, but a value taken
      from example code. Anything less sometimes does report partly
      invalid values.
---
 drivers/input/touchscreen/ili210x.c | 56 +++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 30576a5f2f04..baaddf95dd92 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -35,6 +35,7 @@ struct ili2xxx_chip {
 	unsigned int max_touches;
 	unsigned int resolution;
 	bool has_calibrate_reg;
+	bool has_firmware_proto;
 	bool has_pressure_reg;
 };
 
@@ -268,6 +269,7 @@ static const struct ili2xxx_chip ili251x_chip = {
 	.continue_polling	= ili251x_check_continue_polling,
 	.max_touches		= 10,
 	.has_calibrate_reg	= true,
+	.has_firmware_proto	= true,
 	.has_pressure_reg	= true,
 };
 
@@ -323,6 +325,54 @@ 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);
+	__le16 resx, resy;
+	u8 rs[10];
+	int error;
+
+	/* The firmware update blob might have changed the resolution. */
+	error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs));
+	if (error)
+		return error;
+
+	resx = le16_to_cpup((__le16 *)rs);
+	resy = le16_to_cpup((__le16 *)(rs + 2));
+
+	/* The value reported by the firmware is invalid. */
+	if (!resx || resx == 0xffff || !resy || resy == 0xffff)
+		return -EINVAL;
+
+	input_abs_set_max(priv->input, ABS_X, resx - 1);
+	input_abs_set_max(priv->input, ABS_Y, resy - 1);
+	input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1);
+	input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1);
+
+	return 0;
+}
+
+static int ili251x_firmware_update_cached_state(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int ret;
+
+	if (!priv->chip->has_firmware_proto)
+		return 0;
+
+	/* Wait for firmware to boot and stabilize itself. */
+	msleep(200);
+
+	/* Firmware does report valid information. */
+	ret = ili251x_firmware_update_resolution(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static ssize_t ili210x_calibrate(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -449,6 +499,12 @@ 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);
+	error = ili251x_firmware_update_cached_state(dev);
+	if (error) {
+		dev_err(dev, "Unable to cache firmware information, err: %d\n",
+			error);
+		return error;
+	}
 	touchscreen_parse_properties(input, true, &priv->prop);
 
 	error = input_mt_init_slots(input, priv->chip->max_touches,
-- 
2.33.0

[PATCH v3 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
V3: - Use .has_firmware_proto flag to discern supported MCU protocol
    - Rename REG_IC_MODE to REG_GET_MODE in this patch
    - Use sysfs_emit()
    - Rename variable ret to error
    - Cache firmware version information to avoid mutex in IRQ handler

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 | 165 +++++++++++++++++++++++++++-
 1 file changed, 162 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index baaddf95dd92..a4da753d9355 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_GET_MODE		0xc0
+#define REG_GET_MODE_AP		0x5a
+#define REG_GET_MODE_BL		0x55
 #define REG_CALIBRATE		0xcc
 
 struct ili2xxx_chip {
@@ -45,6 +51,10 @@ struct ili210x {
 	struct gpio_desc *reset_gpio;
 	struct touchscreen_properties prop;
 	const struct ili2xxx_chip *chip;
+	u8 version_firmware[8];
+	u8 version_kernel[5];
+	u8 version_proto[2];
+	u8 ic_mode[2];
 	bool stop;
 };
 
@@ -353,6 +363,69 @@ static int ili251x_firmware_update_resolution(struct device *dev)
 	return 0;
 }
 
+static ssize_t ili251x_firmware_update_firmware_version(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int error;
+	u8 fw[8];
+
+	/* Get firmware version */
+	error = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
+				     &fw, sizeof(fw));
+	if (!error)
+		memcpy(priv->version_firmware, fw, sizeof(fw));
+
+	return error;
+}
+
+static ssize_t ili251x_firmware_update_kernel_version(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int error;
+	u8 kv[5];
+
+	/* Get kernel version */
+	error = priv->chip->read_reg(client, REG_KERNEL_VERSION,
+				     &kv, sizeof(kv));
+	if (!error)
+		memcpy(priv->version_kernel, kv, sizeof(kv));
+
+	return error;
+}
+
+static ssize_t ili251x_firmware_update_protocol_version(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int error;
+	u8 pv[2];
+
+	/* Get protocol version */
+	error = priv->chip->read_reg(client, REG_PROTOCOL_VERSION,
+				     &pv, sizeof(pv));
+	if (!error)
+		memcpy(priv->version_proto, pv, sizeof(pv));
+
+	return error;
+}
+
+static ssize_t ili251x_firmware_update_ic_mode(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+	int error;
+	u8 md[2];
+
+	/* Get chip boot mode */
+	error = priv->chip->read_reg(client, REG_GET_MODE, &md, sizeof(md));
+	if (!error)
+		memcpy(priv->ic_mode, md, sizeof(md));
+
+	return error;
+}
+
 static int ili251x_firmware_update_cached_state(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -370,9 +443,83 @@ static int ili251x_firmware_update_cached_state(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = ili251x_firmware_update_firmware_version(dev);
+	if (ret)
+		return ret;
+
+	ret = ili251x_firmware_update_kernel_version(dev);
+	if (ret)
+		return ret;
+
+	ret = ili251x_firmware_update_protocol_version(dev);
+	if (ret)
+		return ret;
+
+	ret = ili251x_firmware_update_ic_mode(dev);
+	if (ret)
+		return ret;
+
 	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 = priv->version_firmware;
+
+	return sysfs_emit(buf, "%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]);
+}
+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 = priv->version_kernel;
+
+	return sysfs_emit(buf, "%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 = priv->version_proto;
+
+	return sysfs_emit(buf, "%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);
+	u8 *md = priv->ic_mode;
+	char *mode = "AP";
+
+	if (md[0] == REG_GET_MODE_AP)		/* Application Mode */
+		mode = "AP";
+	else if (md[0] == REG_GET_MODE_BL)	/* BootLoader Mode */
+		mode = "BL";
+	else					/* Unknown Mode */
+		mode = "??";
+
+	return sysfs_emit(buf, "%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)
@@ -401,22 +548,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->has_firmware_proto)
+		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)
-- 
2.33.0

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

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 10:25:05PM +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>

Applied, thank you.

-- 
Dmitry

[PATCH v3 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
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/ilitek/ili251x.bin

To trigger the firmware update, place firmware file ilitek/ili251x.bin
into /lib/firmware/, write into firmware_update sysfs attribute and wait
about 30-40 seconds. The firmware update is slow. Afterward, verify the
firmware_version and mode sysfs attributes to check whether the firmware
got updated and the controller switched back to application (AP) mode by
reading out 'mode' 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
V3: - Rename variable ret to error
    - Use kzalloc()
    - Fix last firmware block check, abort of last firmware block
      in the firmware blob is above 0xffe0, i.e. less than 32 bytes
      from the end of the max 64 kiB firmware blob. Add comment about
      this.
    - Pick some sort of symbolic name for the register 0x80 (BUSY)
    - Replace dev_info() with dev_dbg()
    - Call ili251x_firmware_update_cached_state() to update all the
      cached state after firmware update
    - Use disable_irq()/enable_irq() to prevent the IRQ handler from
      firing during firmware update.
    - Drop custom firmware filename support for now, use ili251x.bin
      for the firmware filename.
---
 drivers/input/touchscreen/Kconfig   |   1 +
 drivers/input/touchscreen/ili210x.c | 312 ++++++++++++++++++++++++++++
 2 files changed, 313 insertions(+)

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 a4da753d9355..9a8bd901c9d3 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,11 +27,20 @@
 #define REG_FIRMWARE_VERSION	0x40
 #define REG_PROTOCOL_VERSION	0x42
 #define REG_KERNEL_VERSION	0x61
+#define REG_IC_BUSY		0x80
+#define REG_IC_BUSY_NOT_BUSY	0x50
 #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
 
+#define ILI251X_FW_FILENAME	"ilitek/ili251x.bin"
+
 struct ili2xxx_chip {
 	int (*read_reg)(struct i2c_client *client, u8 reg,
 			void *buf, size_t len);
@@ -546,8 +557,309 @@ 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 error;
+
+	error = request_ihex_firmware(&fw, fwname, dev);
+	if (error) {
+		dev_err(dev, "Failed to request firmware %s, error=%d\n",
+			fwname, error);
+		return error;
+	}
+
+	/*
+	 * 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 = kzalloc(SZ_64K, GFP_KERNEL);
+	if (!fw_buf) {
+		error = -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);
+
+		/* The last 32 Byte firmware block can be 0xffe0 */
+		if (fw_addr + fw_len > SZ_64K || fw_addr > SZ_64K - 32) {
+			error = -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 error;
+}
+
+/* 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 error;
+
+	error = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
+	if (error)
+		return error;
+	/* 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 */
+	error = i2c_master_send(client, cmd_wren, sizeof(cmd_wren));
+	if (error != sizeof(cmd_wren))
+		return -EINVAL;
+
+	mdelay(20);
+
+	/* Select mode (BootLoader or Application) */
+	error = i2c_master_send(client, &cmd_mode, 1);
+	if (error != 1)
+		return -EINVAL;
+
+	mdelay(200);	/* Reboot into bootloader takes a lot of time ... */
+
+	/* Read back mode */
+	error = priv->chip->read_reg(client, REG_GET_MODE, md, sizeof(md));
+	if (error)
+		return error;
+	/* 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 error, i = 0;
+	u8 data;
+
+	do {
+		/* The read_reg already contains suitable delay */
+		error = priv->chip->read_reg(client, REG_IC_BUSY, &data, 1);
+		if (error)
+			return error;
+		if (i++ == 100000)
+			return -ETIMEDOUT;
+	} while (data != REG_IC_BUSY_NOT_BUSY);
+
+	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 error;
+
+	/*
+	 * 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
+	};
+
+	error = i2c_master_send(client, cmd_wr, sizeof(cmd_wr));
+	if (error != sizeof(cmd_wr))
+		return -EINVAL;
+
+	error = ili251x_firmware_busy(client);
+	if (error)
+		return error;
+
+	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
+		fw_data[0] = REG_WRITE_DATA;
+		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);
+		error = i2c_master_send(client, fw_data, 33);
+		if (error != sizeof(fw_data))
+			return error;
+		error = ili251x_firmware_busy(client);
+		if (error)
+			return error;
+	}
+
+	error = i2c_master_send(client, &cmd_crc, 1);
+	if (error != 1)
+		return -EINVAL;
+
+	error = ili251x_firmware_busy(client);
+	if (error)
+		return error;
+
+	error = priv->chip->read_reg(client, REG_READ_DATA_CRC,
+				   &crcrb, sizeof(crcrb));
+	if (error)
+		return error;
+
+	/* 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 error;
+
+	error = i2c_master_send(client, cmd_reset, sizeof(cmd_reset));
+	if (error != 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);
+	const char *fwname = ILI251X_FW_FILENAME;
+	u16 ac_end, df_end;
+	u8 *fwbuf;
+	int error;
+	int i;
+
+	error = ili251x_firmware_to_buffer(dev, fwname, &fwbuf, &ac_end, &df_end);
+	if (error)
+		return error;
+
+	/*
+	 * Disable touchscreen IRQ, so that we would not get spurious touch
+	 * interrupt during firmware update, and so that the IRQ handler won't
+	 * trigger and interfere with the firmware update. There is no bit in
+	 * the touch controller to disable the IRQs during update, so we have
+	 * to do it this way here.
+	 */
+	disable_irq(client->irq);
+
+	dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
+
+	ili251x_hardware_reset(dev);
+
+	error = ili251x_firmware_reset(client);
+	if (error)
+		goto exit;
+
+	/* This may not succeed on first try, so re-try a few times. */
+	for (i = 0; i < 5; i++) {
+		error = ili251x_switch_ic_mode(client, REG_SET_MODE_BL);
+		if (!error)
+			break;
+	}
+
+	if (error)
+		goto exit;
+
+	dev_dbg(dev, "IC is now in BootLoader mode\n");
+
+	msleep(200);	/* The bootloader seems to need some time too. */
+
+	error = ili251x_firmware_write_to_ic(dev, fwbuf, 0xf000, df_end, 1);
+	if (error) {
+		dev_err(dev, "DF firmware update failed, error=%d\n", error);
+		goto exit;
+	}
+
+	dev_dbg(dev, "DataFlash firmware written\n");
+
+	error = ili251x_firmware_write_to_ic(dev, fwbuf, 0x2000, ac_end, 0);
+	if (error) {
+		dev_err(dev, "AC firmware update failed, error=%d\n", error);
+		goto exit;
+	}
+
+	dev_dbg(dev, "Application firmware written\n");
+
+	/* This may not succeed on first try, so re-try a few times. */
+	for (i = 0; i < 5; i++) {
+		error = ili251x_switch_ic_mode(client, REG_SET_MODE_AP);
+		if (!error)
+			break;
+	}
+
+	if (error)
+		goto exit;
+
+	dev_dbg(dev, "IC is now in Application mode\n");
+
+	error = ili251x_firmware_update_cached_state(dev);
+	if (error)
+		goto exit;
+
+	error = count;
+
+exit:
+	ili251x_hardware_reset(dev);
+	dev_dbg(dev, "Firmware update ended, error=%i\n", error);
+	enable_irq(client->irq);
+	kfree(fwbuf);
+	return error;
+}
+
+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.33.0

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

[Dmitry Torokhov]

On Tue, Aug 31, 2021 at 10:25:06PM +0200, Marek Vasut wrote:
> +
> +	/* 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 error;

I do not quite like that we have to release firmware in both success
and error paths, so I moved the call to request_ihex_firmware() to the
caller of this function and release it before checking the result of
loading the firmware to buffer, and applied.

Thank you.

-- 
Dmitry

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

[Marek Vasut]

On 10/17/21 7:26 AM, Dmitry Torokhov wrote:
> On Tue, Aug 31, 2021 at 10:25:06PM +0200, Marek Vasut wrote:
>> +
>> +	/* 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 error;
> 
> I do not quite like that we have to release firmware in both success
> and error paths, so I moved the call to request_ihex_firmware() to the
> caller of this function and release it before checking the result of
> loading the firmware to buffer, and applied.

Hmmm, looking at the result, maybe it would be better to do the entire 
request_ihex_firmware() / release_firmware) in 
ili251x_firmware_to_buffer(). I'll try and see how that looks and maybe 
send you a subsequent patch if it looks better.

Otherwise, thanks for picking those up.

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

[Marek Vasut]

On 8/31/21 10:25 PM, 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
> V3: - Use le16_to_cpup() byte-swap resolution data
>      - Check Y-resolution range up to 0xffff too
>      - Use .has_firmware_proto flag to discern supported MCU protocol
>      - Use input_abs_set_max() per include/linux/input.h INPUT_GENERATE_ABS_ACCESSORS
>      - Rename variable ret to error
>      - Add a wrapper function ili251x_firmware_update_cached_state(),
>        which would call pull other cacheable state from the controller
>        in subsequent patch (hence also the ret variable in it which
>        looks like it could be removed, this will reduce the number of
>        changes in the next patch).
>      - Wait for the firmware to fully stabilize itself after reset.
>        No, those 200 milliseconds is not a mistake, but a value taken
>        from example code. Anything less sometimes does report partly
>        invalid values.

I hope I addressed all the feedback from V2 in this V3. I haven't heard 
anything about these patches for a long time. Do you think they can be 
applied now ?

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

[Dmitry Torokhov]

Hi Marek,

On Tue, Aug 31, 2021 at 10:25:04PM +0200, Marek Vasut wrote:
> +static int ili251x_firmware_update_resolution(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	__le16 resx, resy;

These are normal variables, so I changed it to u16, otherwise sparse is
rightfully unhappy.

> +	u8 rs[10];
> +	int error;
> +
> +	/* The firmware update blob might have changed the resolution. */
> +	error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs));
> +	if (error)
> +		return error;
> +
> +	resx = le16_to_cpup((__le16 *)rs);
> +	resy = le16_to_cpup((__le16 *)(rs + 2));
> +
> +	/* The value reported by the firmware is invalid. */
> +	if (!resx || resx == 0xffff || !resy || resy == 0xffff)
> +		return -EINVAL;
> +
> +	input_abs_set_max(priv->input, ABS_X, resx - 1);
> +	input_abs_set_max(priv->input, ABS_Y, resy - 1);
> +	input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1);
> +	input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1);
> +
> +	return 0;
> +}
> +
> +static int ili251x_firmware_update_cached_state(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	int ret;

Changed this to "error" and applied, thank you.

-- 
Dmitry