[PATCH 0/4] Input: cyapa - Add firmware update feature

[PATCH 0/4] Input: cyapa - Add firmware update feature

[Benson Leung]

Add a feature to update trackpad firmware via request_firmware to cyapa.
The last patch in the series adds sysfs attributses useful for version
and product identification that a user mode utility will need to
keep track of the current firmware on the device.

This is used today on Chromebook systems with cypress trackpads:
Samsung Series 5 550 Chromebook
Acer C7
HP Pavilion 14
Samsung Chromebook (2012, ARM version)

[PATCH 1/4] Input: cyapa - Move common initialization to cyapa_detect
[PATCH 2/4] Input: cyapa - Firmware update via request firmware
[PATCH 3/4] Input: cyapa - Allow filename to be changed in update_fw
[PATCH 4/4] Input: cyapa - Add sysfs attrs for product_id and fw_version

[PATCH 1/4] Input: cyapa - Move common initialization to cyapa_detect

[Benson Leung]

cyapa_check_is_operational and cyapa_create_input_dev are common to
the probe and firmware update paths. Pull those out into
cyapa_detect.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/mouse/cyapa.c | 57 +++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index b409c3d..a631aca 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -823,6 +823,40 @@ err_free_device:
 	return ret;
 }
 
+static void cyapa_detect(struct cyapa *cyapa)
+{
+	struct device *dev = &cyapa->client->dev;
+	int ret;
+
+	ret = cyapa_check_is_operational(cyapa);
+	if (ret == -ETIMEDOUT)
+		dev_err(dev, "no device detected, %d\n", ret);
+	else if (ret)
+		dev_err(dev, "device detected, but not operational, %d\n", ret);
+
+	if (!cyapa->input) {
+		ret = cyapa_create_input_dev(cyapa);
+		if (ret)
+			dev_err(dev, "create input_dev instance failed, %d\n",
+				ret);
+
+		enable_irq(cyapa->irq);
+		/*
+		 * On some systems, a system crash / warm boot does not reset
+		 * the device's current power mode to FULL_ACTIVE.
+		 * If such an event happens during suspend, after the device
+		 * has been put in a low power mode, the device will still be
+		 * in low power mode on a subsequent boot, since there was
+		 * never a matching resume().
+		 * Handle this by always forcing full power here, when a
+		 * device is first detected to be in operational mode.
+		 */
+		ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
+		if (ret)
+			dev_warn(dev, "set active power failed, %d\n", ret);
+	}
+}
+
 static int cyapa_probe(struct i2c_client *client,
 		       const struct i2c_device_id *dev_id)
 {
@@ -853,23 +887,8 @@ static int cyapa_probe(struct i2c_client *client,
 	if (adapter_func == CYAPA_ADAPTER_FUNC_SMBUS)
 		cyapa->smbus = true;
 	cyapa->state = CYAPA_STATE_NO_DEVICE;
-	ret = cyapa_check_is_operational(cyapa);
-	if (ret) {
-		dev_err(dev, "device not operational, %d\n", ret);
-		goto err_mem_free;
-	}
 
-	ret = cyapa_create_input_dev(cyapa);
-	if (ret) {
-		dev_err(dev, "create input_dev instance failed, %d\n", ret);
-		goto err_mem_free;
-	}
-
-	ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
-	if (ret) {
-		dev_err(dev, "set active power failed, %d\n", ret);
-		goto err_unregister_device;
-	}
+	cyapa_detect(cyapa);
 
 	cyapa->irq = client->irq;
 	ret = request_threaded_irq(cyapa->irq,
@@ -886,8 +905,8 @@ static int cyapa_probe(struct i2c_client *client,
 	return 0;
 
 err_unregister_device:
-	input_unregister_device(cyapa->input);
-err_mem_free:
+	if (cyapa->input)
+		input_unregister_device(cyapa->input);
 	kfree(cyapa);
 
 	return ret;
@@ -937,6 +956,8 @@ static int cyapa_resume(struct device *dev)
 	if (device_may_wakeup(dev) && cyapa->irq_wake)
 		disable_irq_wake(cyapa->irq);
 
+	cyapa_detect(cyapa);
+
 	ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
 	if (ret)
 		dev_warn(dev, "resume active power failed, %d\n", ret);
-- 
1.8.1.3

Re: [PATCH 1/4] Input: cyapa - Move common initialization to cyapa_detect

[Henrik Rydberg]

Hi Benson,

> cyapa_check_is_operational and cyapa_create_input_dev are common to
> the probe and firmware update paths. Pull those out into
> cyapa_detect.
> 
> Signed-off-by: Benson Leung <bleung@chromium.org>
> ---
>  drivers/input/mouse/cyapa.c | 57 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> index b409c3d..a631aca 100644
> --- a/drivers/input/mouse/cyapa.c
> +++ b/drivers/input/mouse/cyapa.c
> @@ -823,6 +823,40 @@ err_free_device:
>  	return ret;
>  }
>  
> +static void cyapa_detect(struct cyapa *cyapa)

No error return here?

> +{
> +	struct device *dev = &cyapa->client->dev;
> +	int ret;
> +
> +	ret = cyapa_check_is_operational(cyapa);
> +	if (ret == -ETIMEDOUT)
> +		dev_err(dev, "no device detected, %d\n", ret);
> +	else if (ret)
> +		dev_err(dev, "device detected, but not operational, %d\n", ret);
> +
> +	if (!cyapa->input) {

Return here saves you the ugly indentation.

> +		ret = cyapa_create_input_dev(cyapa);
> +		if (ret)
> +			dev_err(dev, "create input_dev instance failed, %d\n",
> +				ret);
> +
> +		enable_irq(cyapa->irq);
> +		/*
> +		 * On some systems, a system crash / warm boot does not reset
> +		 * the device's current power mode to FULL_ACTIVE.
> +		 * If such an event happens during suspend, after the device
> +		 * has been put in a low power mode, the device will still be
> +		 * in low power mode on a subsequent boot, since there was
> +		 * never a matching resume().
> +		 * Handle this by always forcing full power here, when a
> +		 * device is first detected to be in operational mode.
> +		 */
> +		ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
> +		if (ret)
> +			dev_warn(dev, "set active power failed, %d\n", ret);
> +	}
> +}
> +
>  static int cyapa_probe(struct i2c_client *client,
>  		       const struct i2c_device_id *dev_id)
>  {
> @@ -853,23 +887,8 @@ static int cyapa_probe(struct i2c_client *client,
>  	if (adapter_func == CYAPA_ADAPTER_FUNC_SMBUS)
>  		cyapa->smbus = true;
>  	cyapa->state = CYAPA_STATE_NO_DEVICE;
> -	ret = cyapa_check_is_operational(cyapa);
> -	if (ret) {
> -		dev_err(dev, "device not operational, %d\n", ret);
> -		goto err_mem_free;
> -	}
>  
> -	ret = cyapa_create_input_dev(cyapa);
> -	if (ret) {
> -		dev_err(dev, "create input_dev instance failed, %d\n", ret);
> -		goto err_mem_free;
> -	}
> -
> -	ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
> -	if (ret) {
> -		dev_err(dev, "set active power failed, %d\n", ret);
> -		goto err_unregister_device;
> -	}
> +	cyapa_detect(cyapa);

Replacing proper error handling with a silent function seems wrong.

>  
>  	cyapa->irq = client->irq;
>  	ret = request_threaded_irq(cyapa->irq,
> @@ -886,8 +905,8 @@ static int cyapa_probe(struct i2c_client *client,
>  	return 0;
>  
>  err_unregister_device:
> -	input_unregister_device(cyapa->input);
> -err_mem_free:
> +	if (cyapa->input)
> +		input_unregister_device(cyapa->input);
>  	kfree(cyapa);
>  
>  	return ret;
> @@ -937,6 +956,8 @@ static int cyapa_resume(struct device *dev)
>  	if (device_may_wakeup(dev) && cyapa->irq_wake)
>  		disable_irq_wake(cyapa->irq);
>  
> +	cyapa_detect(cyapa);
> +

Ditto.

>  	ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
>  	if (ret)
>  		dev_warn(dev, "resume active power failed, %d\n", ret);
> -- 
> 1.8.1.3
> 

Thanks,
Henrik

[PATCH 2/4] Input: cyapa - Firmware update via request firmware

[Benson Leung]

From: Daniel Kurtz <djkurtz@chromium.org>

Use the kernel request_firmware API to allow a hotplug script to load new
firmware into CYAPA device.  When request_firmware is called by a driver,
the kernel creates 'loading' and 'data' sysfs entries, and generates a
firmware udev event containing the name of a file ("cyapa.bin").

A udev rule can detect this event and use the sysfs entries to provide the
driver with the contents of a binary image firmware image. For instance,
the gentoo distribution includes /lib/udev/rules.d/50-firmware.rules,
which looks in /lib/firmware/ for a file matching the requested filename,
and sends the file to the driver using its sysfs entries.

Userspace can initiate the firmware update procedure by copying cyapa.bin
to /lib/firmware, and then writing anything to the device sysfs attribute:
  /sys/bus/i2c/devices/7-0067/update_fw

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/mouse/cyapa.c | 351 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 351 insertions(+)

diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index a631aca..e622c25 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/input/mt.h>
@@ -116,6 +117,9 @@
 #define BL_STATUS_SIZE  3  /* length of bootloader status registers */
 #define BLK_HEAD_BYTES 32
 
+/* Macro for register map group offset. */
+#define CYAPA_REG_MAP_SIZE  256
+
 #define PRODUCT_ID_SIZE  16
 #define QUERY_DATA_SIZE  27
 #define REG_PROTOCOL_GEN_QUERY_OFFSET  20
@@ -220,6 +224,8 @@ struct cyapa {
 	int physical_size_y;
 };
 
+static const u8 bl_activate[] = { 0x00, 0xff, 0x38, 0x00, 0x01, 0x02, 0x03,
+		0x04, 0x05, 0x06, 0x07 };
 static const u8 bl_deactivate[] = { 0x00, 0xff, 0x3b, 0x00, 0x01, 0x02, 0x03,
 		0x04, 0x05, 0x06, 0x07 };
 static const u8 bl_exit[] = { 0x00, 0xff, 0xa5, 0x00, 0x01, 0x02, 0x03, 0x04,
@@ -316,6 +322,41 @@ static const struct cyapa_cmd_len cyapa_smbus_cmds[] = {
 	{ CYAPA_SMBUS_BLK_HEAD, 16 },
 };
 
+#define CYAPA_FW_NAME		"cyapa.bin"
+#define CYAPA_FW_BLOCK_SIZE	64
+#define CYAPA_FW_HDR_START	0x0780
+#define CYAPA_FW_HDR_BLOCK_COUNT  2
+#define CYAPA_FW_HDR_BLOCK_START  (CYAPA_FW_HDR_START / CYAPA_FW_BLOCK_SIZE)
+#define CYAPA_FW_HDR_SIZE	(CYAPA_FW_HDR_BLOCK_COUNT * \
+				 CYAPA_FW_BLOCK_SIZE)
+#define CYAPA_FW_DATA_START	0x0800
+#define CYAPA_FW_DATA_BLOCK_COUNT  480
+#define CYAPA_FW_DATA_BLOCK_START  (CYAPA_FW_DATA_START / CYAPA_FW_BLOCK_SIZE)
+#define CYAPA_FW_DATA_SIZE	(CYAPA_FW_DATA_BLOCK_COUNT * \
+				 CYAPA_FW_BLOCK_SIZE)
+#define CYAPA_FW_SIZE		(CYAPA_FW_HDR_SIZE + CYAPA_FW_DATA_SIZE)
+#define CYAPA_CMD_LEN		16
+
+#define BYTE_PER_LINE  8
+void cyapa_dump_data(struct cyapa *cyapa, size_t length, const u8 *data)
+{
+#ifdef DEBUG
+	struct device *dev = &cyapa->client->dev;
+	int i;
+	char buf[BYTE_PER_LINE * 3 + 1];
+	char *s = buf;
+
+	for (i = 0; i < length; i++) {
+		s += sprintf(s, " %02x", data[i]);
+		if ((i + 1) == length || ((i + 1) % BYTE_PER_LINE) == 0) {
+			dev_dbg(dev, "%s\n", buf);
+			s = buf;
+		}
+	}
+#endif
+}
+#undef BYTE_PER_LINE
+
 static ssize_t cyapa_i2c_reg_read_block(struct cyapa *cyapa, u8 reg, size_t len,
 					u8 *values)
 {
@@ -498,6 +539,75 @@ static int cyapa_poll_state(struct cyapa *cyapa, unsigned int timeout)
 	return (ret == -EAGAIN || ret == -ETIMEDOUT) ? -ETIMEDOUT : ret;
 }
 
+/*
+ * Enter bootloader by soft resetting the device.
+ *
+ * If device is already in the bootloader, the function just returns.
+ * Otherwise, reset the device; after reset, device enters bootloader idle
+ * state immediately.
+ *
+ * Also, if device was unregister device from input core.  Device will
+ * re-register after it is detected following resumption of operational mode.
+ *
+ * Returns:
+ *   0 on success
+ *   -EAGAIN  device was reset, but is not now in bootloader idle state
+ *   < 0 if the device never responds within the timeout
+ */
+static int cyapa_bl_enter(struct cyapa *cyapa)
+{
+	int ret;
+
+	if (cyapa->input) {
+		disable_irq(cyapa->irq);
+		input_unregister_device(cyapa->input);
+		cyapa->input = NULL;
+	}
+
+	ret = cyapa_get_state(cyapa);
+	if (ret < 0)
+		return ret;
+	if (cyapa->state == CYAPA_STATE_BL_IDLE)
+		return 0;
+
+	if (cyapa->state != CYAPA_STATE_OP)
+		return -EAGAIN;
+
+	cyapa->state = CYAPA_STATE_NO_DEVICE;
+	ret = cyapa_write_byte(cyapa, CYAPA_CMD_SOFT_RESET, 0x01);
+	if (ret < 0)
+		return -EIO;
+
+	usleep_range(25000, 50000);
+	ret = cyapa_get_state(cyapa);
+	if (ret < 0)
+		return ret;
+	if (cyapa->state != CYAPA_STATE_BL_IDLE)
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int cyapa_bl_activate(struct cyapa *cyapa)
+{
+	int ret;
+
+	ret = cyapa_i2c_reg_write_block(cyapa, 0, sizeof(bl_activate),
+					bl_activate);
+	if (ret < 0)
+		return ret;
+
+	/* Wait for bootloader to activate; takes between 2 and 12 seconds */
+	msleep(2000);
+	ret = cyapa_poll_state(cyapa, 10000);
+	if (ret < 0)
+		return ret;
+	if (cyapa->state != CYAPA_STATE_BL_ACTIVE)
+		return -EAGAIN;
+
+	return 0;
+}
+
 static int cyapa_bl_deactivate(struct cyapa *cyapa)
 {
 	int ret;
@@ -682,6 +792,154 @@ static int cyapa_check_is_operational(struct cyapa *cyapa)
 	return 0;
 }
 
+static u16 cyapa_csum(const u8 *buf, size_t count)
+{
+	int i;
+	u16 csum = 0;
+
+	for (i = 0; i < count; i++)
+		csum += buf[i];
+
+	return csum;
+}
+
+/*
+ * Write a |len| byte long buffer |buf| to the device, by chopping it up into a
+ * sequence of smaller |CYAPA_CMD_LEN|-length write commands.
+ *
+ * The data bytes for a write command are prepended with the 1-byte offset
+ * of the data relative to the start of |buf|.
+ */
+static int cyapa_write_buffer(struct cyapa *cyapa, const u8 *buf, size_t len)
+{
+	int ret;
+	size_t i;
+	unsigned char cmd[CYAPA_CMD_LEN + 1];
+	size_t cmd_len;
+
+	for (i = 0; i < len; i += CYAPA_CMD_LEN) {
+		const u8 *payload = &buf[i];
+		cmd_len = (len - i >= CYAPA_CMD_LEN) ? CYAPA_CMD_LEN : len - i;
+		cmd[0] = i;
+		memcpy(&cmd[1], payload, cmd_len);
+
+		ret = cyapa_i2c_reg_write_block(cyapa, 0, cmd_len + 1, cmd);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+/*
+ * A firmware block write command writes 64 bytes of data to a single flash
+ * page in the device.  The 78-byte block write command has the format:
+ *   <0xff> <CMD> <Key> <Start> <Data> <Data-Checksum> <CMD Checksum>
+ *
+ *  <0xff>  - every command starts with 0xff
+ *  <CMD>   - the write command value is 0x39
+ *  <Key>   - write commands include an 8-byte key: { 00 01 02 03 04 05 06 07 }
+ *  <Block> - Memory Block number (address / 64) (16-bit, big-endian)
+ *  <Data>  - 64 bytes of firmware image data
+ *  <Data Checksum> - sum of 64 <Data> bytes, modulo 0xff
+ *  <CMD Checksum> - sum of 77 bytes, from 0xff to <Data Checksum>
+ *
+ * Each write command is split into 5 i2c write transactions of up to 16 bytes.
+ * Each transaction starts with an i2c register offset: (00, 10, 20, 30, 40).
+ */
+static int cyapa_write_fw_block(struct cyapa *cyapa, u16 block, const u8 *data)
+{
+	int ret;
+	u8 cmd[78];
+	u8 status[BL_STATUS_SIZE];
+	int tries = 3;
+
+	/* set write command and security key bytes. */
+	cmd[0] = 0xff;
+	cmd[1] = 0x39;
+	cmd[2] = 0x00;
+	cmd[3] = 0x01;
+	cmd[4] = 0x02;
+	cmd[5] = 0x03;
+	cmd[6] = 0x04;
+	cmd[7] = 0x05;
+	cmd[8] = 0x06;
+	cmd[9] = 0x07;
+	cmd[10] = block >> 8;
+	cmd[11] = block;
+	memcpy(&cmd[12], data, CYAPA_FW_BLOCK_SIZE);
+	cmd[76] = cyapa_csum(data, CYAPA_FW_BLOCK_SIZE);
+	cmd[77] = cyapa_csum(cmd, sizeof(cmd) - 1);
+
+	ret = cyapa_write_buffer(cyapa, cmd, sizeof(cmd));
+	if (ret)
+		return ret;
+
+	/* wait for write to finish */
+	do {
+		usleep_range(10000, 20000);
+
+		/* check block write command result status. */
+		ret = cyapa_i2c_reg_read_block(cyapa, BL_HEAD_OFFSET,
+					       BL_STATUS_SIZE, status);
+		if (ret != BL_STATUS_SIZE)
+			return (ret < 0) ? ret : -EIO;
+		ret = (status[1] == 0x10 && status[2] == 0x20) ? 0 : -EIO;
+	} while (--tries && ret);
+
+	return ret;
+}
+
+/*
+ * Verify the integrity of a CYAPA firmware image file.
+ *
+ * The firmware image file is 30848 bytes, composed of 482 64-byte blocks.
+ *
+ * The first 2 blocks are the firmware header.
+ * The next 480 blocks are the firmware image.
+ *
+ * The first two bytes of the header hold the header checksum, computed by
+ * summing the other 126 bytes of the header.
+ * The last two bytes of the header hold the firmware image checksum, computed
+ * by summing the 30720 bytes of the image modulo 0xffff.
+ *
+ * Both checksums are stored little-endian.
+ */
+static int cyapa_check_fw(struct cyapa *cyapa, const struct firmware *fw)
+{
+	struct device *dev = &cyapa->client->dev;
+	u16 csum;
+	u16 csum_expected;
+
+	/* Firmware must match exact 30848 bytes = 482 64-byte blocks. */
+	if (fw->size != CYAPA_FW_SIZE) {
+		dev_err(dev, "invalid firmware size = %zu, expected %u.\n",
+			fw->size, CYAPA_FW_SIZE);
+		return -EINVAL;
+	}
+
+	/* Verify header block */
+	csum_expected = (fw->data[0] << 8) | fw->data[1];
+	csum = cyapa_csum(&fw->data[2], CYAPA_FW_HDR_SIZE - 2);
+	if (csum != csum_expected) {
+		dev_err(dev,
+			"invalid fw header checksum = %04x, expected: %04x\n",
+			csum, csum_expected);
+		return -EINVAL;
+	}
+
+	/* Verify firmware image */
+	csum_expected = (fw->data[CYAPA_FW_HDR_SIZE - 2] << 8) |
+			 fw->data[CYAPA_FW_HDR_SIZE - 1];
+	csum = cyapa_csum(&fw->data[CYAPA_FW_HDR_SIZE], CYAPA_FW_DATA_SIZE);
+	if (csum != csum_expected) {
+		dev_err(dev,
+			"invalid fw header checksum = %04x, expected: %04x\n",
+			csum, csum_expected);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static irqreturn_t cyapa_irq(int irq, void *dev_id)
 {
 	struct cyapa *cyapa = dev_id;
@@ -857,6 +1115,94 @@ static void cyapa_detect(struct cyapa *cyapa)
 	}
 }
 
+static int cyapa_firmware(struct cyapa *cyapa, const char *fw_name)
+{
+	struct device *dev = &cyapa->client->dev;
+	int ret;
+	const struct firmware *fw;
+	int i;
+
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret) {
+		dev_err(dev, "Could not load firmware from %s, %d\n",
+			fw_name, ret);
+		return ret;
+	}
+
+	ret = cyapa_check_fw(cyapa, fw);
+	if (ret) {
+		dev_err(dev, "Invalid CYAPA firmware image: %s\n", fw_name);
+		goto done;
+	}
+
+	ret = cyapa_bl_enter(cyapa);
+	if (ret)
+		goto err_detect;
+
+	ret = cyapa_bl_activate(cyapa);
+	if (ret)
+		goto err_detect;
+
+	/* First write data, starting at byte 128  of fw->data */
+	for (i = 0; i < CYAPA_FW_DATA_BLOCK_COUNT; i++) {
+		size_t block = CYAPA_FW_DATA_BLOCK_START + i;
+		size_t addr = (i + CYAPA_FW_HDR_BLOCK_COUNT) *
+				CYAPA_FW_BLOCK_SIZE;
+		const u8 *data = &fw->data[addr];
+		ret = cyapa_write_fw_block(cyapa, block, data);
+		if (ret)
+			goto err_detect;
+	}
+
+	/* Then write checksum */
+	for (i = 0; i < CYAPA_FW_HDR_BLOCK_COUNT; i++) {
+		size_t block = CYAPA_FW_HDR_BLOCK_START + i;
+		size_t addr = i * CYAPA_FW_BLOCK_SIZE;
+		const u8 *data = &fw->data[addr];
+		ret = cyapa_write_fw_block(cyapa, block, data);
+		if (ret)
+			goto err_detect;
+	}
+
+err_detect:
+	cyapa_detect(cyapa);
+
+done:
+	release_firmware(fw);
+	return ret;
+}
+
+/*
+ * Sysfs Interface.
+ */
+static ssize_t cyapa_update_fw_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct cyapa *cyapa = dev_get_drvdata(dev);
+	const char *fw_name = CYAPA_FW_NAME;
+	int ret;
+
+	ret = cyapa_firmware(cyapa, fw_name);
+	if (ret)
+		dev_err(dev, "firmware update failed, %d\n", ret);
+	else
+		dev_dbg(dev, "firmware update succeeded\n");
+
+	return ret ? ret : count;
+}
+
+static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
+
+static struct attribute *cyapa_sysfs_entries[] = {
+	&dev_attr_update_fw.attr,
+	NULL,
+};
+
+static const struct attribute_group cyapa_sysfs_group = {
+	.attrs = cyapa_sysfs_entries,
+};
+
 static int cyapa_probe(struct i2c_client *client,
 		       const struct i2c_device_id *dev_id)
 {
@@ -902,6 +1248,9 @@ static int cyapa_probe(struct i2c_client *client,
 		goto err_unregister_device;
 	}
 
+	if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
+		dev_warn(dev, "error creating sysfs entries.\n");
+
 	return 0;
 
 err_unregister_device:
@@ -916,6 +1265,8 @@ static int cyapa_remove(struct i2c_client *client)
 {
 	struct cyapa *cyapa = i2c_get_clientdata(client);
 
+	sysfs_remove_group(&client->dev.kobj, &cyapa_sysfs_group);
+
 	free_irq(cyapa->irq, cyapa);
 	input_unregister_device(cyapa->input);
 	cyapa_set_power_mode(cyapa, PWR_MODE_OFF);
-- 
1.8.1.3

Re: [PATCH 2/4] Input: cyapa - Firmware update via request firmware

[Dmitry Torokhov]

On Wed, Mar 13, 2013 at 04:50:49PM -0700, Benson Leung wrote:
> +
> +#define BYTE_PER_LINE  8
> +void cyapa_dump_data(struct cyapa *cyapa, size_t length, const u8 *data)
> +{
> +#ifdef DEBUG
> +	struct device *dev = &cyapa->client->dev;
> +	int i;
> +	char buf[BYTE_PER_LINE * 3 + 1];
> +	char *s = buf;
> +
> +	for (i = 0; i < length; i++) {
> +		s += sprintf(s, " %02x", data[i]);
> +		if ((i + 1) == length || ((i + 1) % BYTE_PER_LINE) == 0) {
> +			dev_dbg(dev, "%s\n", buf);
> +			s = buf;
> +		}
> +	}
> +#endif
> +}
> +#undef BYTE_PER_LINE

We have dev_dbg("%*ph\n", BYTES_PER_LINE, data[...]); for this.

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: cyapa - Firmware update via request firmware

[Henrik Rydberg]

Hi Benson,

> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Use the kernel request_firmware API to allow a hotplug script to load new
> firmware into CYAPA device.  When request_firmware is called by a driver,
> the kernel creates 'loading' and 'data' sysfs entries, and generates a
> firmware udev event containing the name of a file ("cyapa.bin").
> 
> A udev rule can detect this event and use the sysfs entries to provide the
> driver with the contents of a binary image firmware image. For instance,
> the gentoo distribution includes /lib/udev/rules.d/50-firmware.rules,
> which looks in /lib/firmware/ for a file matching the requested filename,
> and sends the file to the driver using its sysfs entries.
> 
> Userspace can initiate the firmware update procedure by copying cyapa.bin
> to /lib/firmware, and then writing anything to the device sysfs attribute:
>   /sys/bus/i2c/devices/7-0067/update_fw

Why do you need to trigger this from userland via sysfs?

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Benson Leung <bleung@chromium.org>
> ---
>  drivers/input/mouse/cyapa.c | 351 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 351 insertions(+)
> 
> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> index a631aca..e622c25 100644
> --- a/drivers/input/mouse/cyapa.c
> +++ b/drivers/input/mouse/cyapa.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/firmware.h>
>  #include <linux/i2c.h>
>  #include <linux/input.h>
>  #include <linux/input/mt.h>
> @@ -116,6 +117,9 @@
>  #define BL_STATUS_SIZE  3  /* length of bootloader status registers */
>  #define BLK_HEAD_BYTES 32
>  
> +/* Macro for register map group offset. */
> +#define CYAPA_REG_MAP_SIZE  256
> +
>  #define PRODUCT_ID_SIZE  16
>  #define QUERY_DATA_SIZE  27
>  #define REG_PROTOCOL_GEN_QUERY_OFFSET  20
> @@ -220,6 +224,8 @@ struct cyapa {
>  	int physical_size_y;
>  };
>  
> +static const u8 bl_activate[] = { 0x00, 0xff, 0x38, 0x00, 0x01, 0x02, 0x03,
> +		0x04, 0x05, 0x06, 0x07 };
>  static const u8 bl_deactivate[] = { 0x00, 0xff, 0x3b, 0x00, 0x01, 0x02, 0x03,
>  		0x04, 0x05, 0x06, 0x07 };
>  static const u8 bl_exit[] = { 0x00, 0xff, 0xa5, 0x00, 0x01, 0x02, 0x03, 0x04,
> @@ -316,6 +322,41 @@ static const struct cyapa_cmd_len cyapa_smbus_cmds[] = {
>  	{ CYAPA_SMBUS_BLK_HEAD, 16 },
>  };
>  
> +#define CYAPA_FW_NAME		"cyapa.bin"
> +#define CYAPA_FW_BLOCK_SIZE	64
> +#define CYAPA_FW_HDR_START	0x0780
> +#define CYAPA_FW_HDR_BLOCK_COUNT  2
> +#define CYAPA_FW_HDR_BLOCK_START  (CYAPA_FW_HDR_START / CYAPA_FW_BLOCK_SIZE)
> +#define CYAPA_FW_HDR_SIZE	(CYAPA_FW_HDR_BLOCK_COUNT * \
> +				 CYAPA_FW_BLOCK_SIZE)
> +#define CYAPA_FW_DATA_START	0x0800
> +#define CYAPA_FW_DATA_BLOCK_COUNT  480
> +#define CYAPA_FW_DATA_BLOCK_START  (CYAPA_FW_DATA_START / CYAPA_FW_BLOCK_SIZE)
> +#define CYAPA_FW_DATA_SIZE	(CYAPA_FW_DATA_BLOCK_COUNT * \
> +				 CYAPA_FW_BLOCK_SIZE)
> +#define CYAPA_FW_SIZE		(CYAPA_FW_HDR_SIZE + CYAPA_FW_DATA_SIZE)
> +#define CYAPA_CMD_LEN		16
> +
> +#define BYTE_PER_LINE  8
> +void cyapa_dump_data(struct cyapa *cyapa, size_t length, const u8 *data)
> +{
> +#ifdef DEBUG
> +	struct device *dev = &cyapa->client->dev;
> +	int i;
> +	char buf[BYTE_PER_LINE * 3 + 1];
> +	char *s = buf;
> +
> +	for (i = 0; i < length; i++) {
> +		s += sprintf(s, " %02x", data[i]);
> +		if ((i + 1) == length || ((i + 1) % BYTE_PER_LINE) == 0) {
> +			dev_dbg(dev, "%s\n", buf);
> +			s = buf;
> +		}
> +	}
> +#endif
> +}
> +#undef BYTE_PER_LINE
> +
>  static ssize_t cyapa_i2c_reg_read_block(struct cyapa *cyapa, u8 reg, size_t len,
>  					u8 *values)
>  {
> @@ -498,6 +539,75 @@ static int cyapa_poll_state(struct cyapa *cyapa, unsigned int timeout)
>  	return (ret == -EAGAIN || ret == -ETIMEDOUT) ? -ETIMEDOUT : ret;
>  }
>  
> +/*
> + * Enter bootloader by soft resetting the device.
> + *
> + * If device is already in the bootloader, the function just returns.
> + * Otherwise, reset the device; after reset, device enters bootloader idle
> + * state immediately.
> + *
> + * Also, if device was unregister device from input core.  Device will
> + * re-register after it is detected following resumption of operational mode.
> + *
> + * Returns:
> + *   0 on success
> + *   -EAGAIN  device was reset, but is not now in bootloader idle state
> + *   < 0 if the device never responds within the timeout
> + */
> +static int cyapa_bl_enter(struct cyapa *cyapa)
> +{
> +	int ret;
> +
> +	if (cyapa->input) {
> +		disable_irq(cyapa->irq);
> +		input_unregister_device(cyapa->input);
> +		cyapa->input = NULL;
> +	}
> +
> +	ret = cyapa_get_state(cyapa);
> +	if (ret < 0)
> +		return ret;
> +	if (cyapa->state == CYAPA_STATE_BL_IDLE)
> +		return 0;
> +
> +	if (cyapa->state != CYAPA_STATE_OP)
> +		return -EAGAIN;
> +
> +	cyapa->state = CYAPA_STATE_NO_DEVICE;
> +	ret = cyapa_write_byte(cyapa, CYAPA_CMD_SOFT_RESET, 0x01);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	usleep_range(25000, 50000);
> +	ret = cyapa_get_state(cyapa);
> +	if (ret < 0)
> +		return ret;
> +	if (cyapa->state != CYAPA_STATE_BL_IDLE)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static int cyapa_bl_activate(struct cyapa *cyapa)
> +{
> +	int ret;
> +
> +	ret = cyapa_i2c_reg_write_block(cyapa, 0, sizeof(bl_activate),
> +					bl_activate);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Wait for bootloader to activate; takes between 2 and 12 seconds */
> +	msleep(2000);
> +	ret = cyapa_poll_state(cyapa, 10000);
> +	if (ret < 0)
> +		return ret;
> +	if (cyapa->state != CYAPA_STATE_BL_ACTIVE)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
>  static int cyapa_bl_deactivate(struct cyapa *cyapa)
>  {
>  	int ret;
> @@ -682,6 +792,154 @@ static int cyapa_check_is_operational(struct cyapa *cyapa)
>  	return 0;
>  }
>  
> +static u16 cyapa_csum(const u8 *buf, size_t count)
> +{
> +	int i;
> +	u16 csum = 0;
> +
> +	for (i = 0; i < count; i++)
> +		csum += buf[i];
> +
> +	return csum;
> +}
> +
> +/*
> + * Write a |len| byte long buffer |buf| to the device, by chopping it up into a
> + * sequence of smaller |CYAPA_CMD_LEN|-length write commands.
> + *
> + * The data bytes for a write command are prepended with the 1-byte offset
> + * of the data relative to the start of |buf|.
> + */
> +static int cyapa_write_buffer(struct cyapa *cyapa, const u8 *buf, size_t len)
> +{
> +	int ret;
> +	size_t i;
> +	unsigned char cmd[CYAPA_CMD_LEN + 1];
> +	size_t cmd_len;
> +
> +	for (i = 0; i < len; i += CYAPA_CMD_LEN) {
> +		const u8 *payload = &buf[i];
> +		cmd_len = (len - i >= CYAPA_CMD_LEN) ? CYAPA_CMD_LEN : len - i;
> +		cmd[0] = i;
> +		memcpy(&cmd[1], payload, cmd_len);
> +
> +		ret = cyapa_i2c_reg_write_block(cyapa, 0, cmd_len + 1, cmd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * A firmware block write command writes 64 bytes of data to a single flash
> + * page in the device.  The 78-byte block write command has the format:
> + *   <0xff> <CMD> <Key> <Start> <Data> <Data-Checksum> <CMD Checksum>
> + *
> + *  <0xff>  - every command starts with 0xff
> + *  <CMD>   - the write command value is 0x39
> + *  <Key>   - write commands include an 8-byte key: { 00 01 02 03 04 05 06 07 }
> + *  <Block> - Memory Block number (address / 64) (16-bit, big-endian)
> + *  <Data>  - 64 bytes of firmware image data
> + *  <Data Checksum> - sum of 64 <Data> bytes, modulo 0xff
> + *  <CMD Checksum> - sum of 77 bytes, from 0xff to <Data Checksum>
> + *
> + * Each write command is split into 5 i2c write transactions of up to 16 bytes.
> + * Each transaction starts with an i2c register offset: (00, 10, 20, 30, 40).
> + */
> +static int cyapa_write_fw_block(struct cyapa *cyapa, u16 block, const u8 *data)
> +{
> +	int ret;
> +	u8 cmd[78];
> +	u8 status[BL_STATUS_SIZE];
> +	int tries = 3;
> +
> +	/* set write command and security key bytes. */
> +	cmd[0] = 0xff;
> +	cmd[1] = 0x39;
> +	cmd[2] = 0x00;
> +	cmd[3] = 0x01;
> +	cmd[4] = 0x02;
> +	cmd[5] = 0x03;
> +	cmd[6] = 0x04;
> +	cmd[7] = 0x05;
> +	cmd[8] = 0x06;
> +	cmd[9] = 0x07;

Can't this be a command string somewhere as well?

> +	cmd[10] = block >> 8;
> +	cmd[11] = block;
> +	memcpy(&cmd[12], data, CYAPA_FW_BLOCK_SIZE);
> +	cmd[76] = cyapa_csum(data, CYAPA_FW_BLOCK_SIZE);
> +	cmd[77] = cyapa_csum(cmd, sizeof(cmd) - 1);
> +
> +	ret = cyapa_write_buffer(cyapa, cmd, sizeof(cmd));
> +	if (ret)
> +		return ret;
> +
> +	/* wait for write to finish */
> +	do {
> +		usleep_range(10000, 20000);
> +
> +		/* check block write command result status. */
> +		ret = cyapa_i2c_reg_read_block(cyapa, BL_HEAD_OFFSET,
> +					       BL_STATUS_SIZE, status);
> +		if (ret != BL_STATUS_SIZE)
> +			return (ret < 0) ? ret : -EIO;
> +		ret = (status[1] == 0x10 && status[2] == 0x20) ? 0 : -EIO;
> +	} while (--tries && ret);
> +
> +	return ret;
> +}
> +
> +/*
> + * Verify the integrity of a CYAPA firmware image file.
> + *
> + * The firmware image file is 30848 bytes, composed of 482 64-byte blocks.
> + *
> + * The first 2 blocks are the firmware header.
> + * The next 480 blocks are the firmware image.
> + *
> + * The first two bytes of the header hold the header checksum, computed by
> + * summing the other 126 bytes of the header.
> + * The last two bytes of the header hold the firmware image checksum, computed
> + * by summing the 30720 bytes of the image modulo 0xffff.
> + *
> + * Both checksums are stored little-endian.
> + */
> +static int cyapa_check_fw(struct cyapa *cyapa, const struct firmware *fw)
> +{
> +	struct device *dev = &cyapa->client->dev;
> +	u16 csum;
> +	u16 csum_expected;
> +
> +	/* Firmware must match exact 30848 bytes = 482 64-byte blocks. */
> +	if (fw->size != CYAPA_FW_SIZE) {
> +		dev_err(dev, "invalid firmware size = %zu, expected %u.\n",
> +			fw->size, CYAPA_FW_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	/* Verify header block */
> +	csum_expected = (fw->data[0] << 8) | fw->data[1];
> +	csum = cyapa_csum(&fw->data[2], CYAPA_FW_HDR_SIZE - 2);
> +	if (csum != csum_expected) {
> +		dev_err(dev,
> +			"invalid fw header checksum = %04x, expected: %04x\n",
> +			csum, csum_expected);
> +		return -EINVAL;
> +	}
> +
> +	/* Verify firmware image */
> +	csum_expected = (fw->data[CYAPA_FW_HDR_SIZE - 2] << 8) |
> +			 fw->data[CYAPA_FW_HDR_SIZE - 1];
> +	csum = cyapa_csum(&fw->data[CYAPA_FW_HDR_SIZE], CYAPA_FW_DATA_SIZE);
> +	if (csum != csum_expected) {
> +		dev_err(dev,
> +			"invalid fw header checksum = %04x, expected: %04x\n",
> +			csum, csum_expected);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static irqreturn_t cyapa_irq(int irq, void *dev_id)
>  {
>  	struct cyapa *cyapa = dev_id;
> @@ -857,6 +1115,94 @@ static void cyapa_detect(struct cyapa *cyapa)
>  	}
>  }
>  
> +static int cyapa_firmware(struct cyapa *cyapa, const char *fw_name)
> +{
> +	struct device *dev = &cyapa->client->dev;
> +	int ret;
> +	const struct firmware *fw;
> +	int i;
> +
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "Could not load firmware from %s, %d\n",
> +			fw_name, ret);
> +		return ret;
> +	}
> +
> +	ret = cyapa_check_fw(cyapa, fw);
> +	if (ret) {
> +		dev_err(dev, "Invalid CYAPA firmware image: %s\n", fw_name);
> +		goto done;
> +	}
> +
> +	ret = cyapa_bl_enter(cyapa);
> +	if (ret)
> +		goto err_detect;
> +
> +	ret = cyapa_bl_activate(cyapa);
> +	if (ret)
> +		goto err_detect;
> +
> +	/* First write data, starting at byte 128  of fw->data */
> +	for (i = 0; i < CYAPA_FW_DATA_BLOCK_COUNT; i++) {
> +		size_t block = CYAPA_FW_DATA_BLOCK_START + i;
> +		size_t addr = (i + CYAPA_FW_HDR_BLOCK_COUNT) *
> +				CYAPA_FW_BLOCK_SIZE;
> +		const u8 *data = &fw->data[addr];
> +		ret = cyapa_write_fw_block(cyapa, block, data);
> +		if (ret)
> +			goto err_detect;
> +	}
> +
> +	/* Then write checksum */
> +	for (i = 0; i < CYAPA_FW_HDR_BLOCK_COUNT; i++) {
> +		size_t block = CYAPA_FW_HDR_BLOCK_START + i;
> +		size_t addr = i * CYAPA_FW_BLOCK_SIZE;
> +		const u8 *data = &fw->data[addr];
> +		ret = cyapa_write_fw_block(cyapa, block, data);
> +		if (ret)
> +			goto err_detect;
> +	}
> +
> +err_detect:
> +	cyapa_detect(cyapa);
> +
> +done:
> +	release_firmware(fw);
> +	return ret;
> +}
> +
> +/*
> + * Sysfs Interface.
> + */
> +static ssize_t cyapa_update_fw_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct cyapa *cyapa = dev_get_drvdata(dev);
> +	const char *fw_name = CYAPA_FW_NAME;
> +	int ret;
> +
> +	ret = cyapa_firmware(cyapa, fw_name);
> +	if (ret)
> +		dev_err(dev, "firmware update failed, %d\n", ret);
> +	else
> +		dev_dbg(dev, "firmware update succeeded\n");
> +
> +	return ret ? ret : count;
> +}
> +
> +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
> +
> +static struct attribute *cyapa_sysfs_entries[] = {
> +	&dev_attr_update_fw.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group cyapa_sysfs_group = {
> +	.attrs = cyapa_sysfs_entries,
> +};
> +
>  static int cyapa_probe(struct i2c_client *client,
>  		       const struct i2c_device_id *dev_id)
>  {
> @@ -902,6 +1248,9 @@ static int cyapa_probe(struct i2c_client *client,
>  		goto err_unregister_device;
>  	}
>  
> +	if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
> +		dev_warn(dev, "error creating sysfs entries.\n");
> +

Would it not be neat to invoke the firmware update automatically instead?

>  	return 0;
>  
>  err_unregister_device:
> @@ -916,6 +1265,8 @@ static int cyapa_remove(struct i2c_client *client)
>  {
>  	struct cyapa *cyapa = i2c_get_clientdata(client);
>  
> +	sysfs_remove_group(&client->dev.kobj, &cyapa_sysfs_group);
> +
>  	free_irq(cyapa->irq, cyapa);
>  	input_unregister_device(cyapa->input);
>  	cyapa_set_power_mode(cyapa, PWR_MODE_OFF);
> -- 
> 1.8.1.3
> 

Thanks,
Henrik

Re: [PATCH 2/4] Input: cyapa - Firmware update via request firmware

[Benson Leung]

On Sat, Mar 16, 2013 at 1:00 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> Userspace can initiate the firmware update procedure by copying cyapa.bin
>> to /lib/firmware, and then writing anything to the device sysfs attribute:
>>   /sys/bus/i2c/devices/7-0067/update_fw
>
> Why do you need to trigger this from userland via sysfs?
>

It's a confluence of factors that drives the decision trigger from userland.
- As a part of the cold-boot probe steps, the trackpad's bootloader
will load an operational firmware from its flash memory. In the
average case, the trackpad driver does not need to update the firmware
because it's current. (the non-average cases are cases where on device
firmware is corrupted or out of date)
- The cypress firmware payloads we are working with here do not
contain an indication of firmware version in the payload itself, so
the driver can't tell at probe time whether what it would get if it
request_firmware is newer, older, or the same version as what's
already loaded on the trackpad.
- On a 400kHz i2c bus, it takes 13 seconds to send the 31K payload to
the device and to wait for it to jump into the operational mode
firmware. This is too long to block probe every time.
- Because it takes so long to do a firmware update, our user space
would like to put up a warning message anyhow to tell the user that
the touchpad is being updated, and not to disturb the system.

>> +     if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
>> +             dev_warn(dev, "error creating sysfs entries.\n");
>> +
>
> Would it not be neat to invoke the firmware update automatically instead?

The one situation where I could see an automatic firmware update being
useful would be cases where the driver, during probe, discovered that
the trackpad is in Bootloader mode because no valid firmware was found
(a case of corruption of some kind due to the system powering down
during a previous fw update, or example). In that case, the driver can
request firmware and update it without user space having to trigger it
later.

Another driver in input does it the same way. atmel_mxt_ts.c, which
our team is actively working on as well, also has an update_fw sysfs
for a lot of the same reasons.

Thanks,

--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH 2/4] Input: cyapa - Firmware update via request firmware

[Henrik Rydberg]

Hi Benson,

> On Sat, Mar 16, 2013 at 1:00 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> >> Userspace can initiate the firmware update procedure by copying cyapa.bin
> >> to /lib/firmware, and then writing anything to the device sysfs attribute:
> >>   /sys/bus/i2c/devices/7-0067/update_fw
> >
> > Why do you need to trigger this from userland via sysfs?
> >
> 
> It's a confluence of factors that drives the decision trigger from userland.
> - As a part of the cold-boot probe steps, the trackpad's bootloader
> will load an operational firmware from its flash memory. In the
> average case, the trackpad driver does not need to update the firmware
> because it's current. (the non-average cases are cases where on device
> firmware is corrupted or out of date)
> - The cypress firmware payloads we are working with here do not
> contain an indication of firmware version in the payload itself, so
> the driver can't tell at probe time whether what it would get if it
> request_firmware is newer, older, or the same version as what's
> already loaded on the trackpad.
> - On a 400kHz i2c bus, it takes 13 seconds to send the 31K payload to
> the device and to wait for it to jump into the operational mode
> firmware. This is too long to block probe every time.
> - Because it takes so long to do a firmware update, our user space
> would like to put up a warning message anyhow to tell the user that
> the touchpad is being updated, and not to disturb the system.
> 
> >> +     if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
> >> +             dev_warn(dev, "error creating sysfs entries.\n");
> >> +
> >
> > Would it not be neat to invoke the firmware update automatically instead?
> 
> The one situation where I could see an automatic firmware update being
> useful would be cases where the driver, during probe, discovered that
> the trackpad is in Bootloader mode because no valid firmware was found
> (a case of corruption of some kind due to the system powering down
> during a previous fw update, or example). In that case, the driver can
> request firmware and update it without user space having to trigger it
> later.
> 
> Another driver in input does it the same way. atmel_mxt_ts.c, which
> our team is actively working on as well, also has an update_fw sysfs
> for a lot of the same reasons.

Thanks for the rationale, it looks compelling enough for me.

The cyapa_firmware() function has loops that looks like they could be
simplified. With that and the previous comments said,

    Acked-by: Henrik Rydberg <rydberg@euromail.se>

Henrik

[PATCH 3/4] Input: cyapa - Allow filename to be changed in update_fw

[Benson Leung]

Allow the name of the designated firmware to be passed as the
argument to update_fw from user space. This will allow user space
to specify which firmware to load.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/mouse/cyapa.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index e622c25..51e9d44 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -1180,9 +1180,16 @@ static ssize_t cyapa_update_fw_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct cyapa *cyapa = dev_get_drvdata(dev);
-	const char *fw_name = CYAPA_FW_NAME;
+	const char *fw_name;
 	int ret;
 
+	/* Do not allow paths that step out of /lib/firmware  */
+	if (strstr(buf, "../") != NULL)
+		return -EINVAL;
+
+	fw_name = !strncmp(buf, "1", count) ||
+		  !strncmp(buf, "1\n", count) ? CYAPA_FW_NAME : buf;
+
 	ret = cyapa_firmware(cyapa, fw_name);
 	if (ret)
 		dev_err(dev, "firmware update failed, %d\n", ret);
-- 
1.8.1.3

[PATCH 4/4] Input: cyapa - Add sysfs attrs for product_id and fw version

[Benson Leung]

Add attributes to expose the product_id and firmware_version read
from the device firmware.

Signed-off-by: Dudley Du <dudl@cypress.com>
Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/mouse/cyapa.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index 51e9d44..97828f6 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -216,6 +216,8 @@ struct cyapa {
 
 	/* read from query data region. */
 	char product_id[16];
+	u8 fw_maj_ver;  /* firmware major version. */
+	u8 fw_min_ver;  /* firmware minor version. */
 	u8 btn_capability;
 	u8 gen;
 	int max_abs_x;
@@ -714,6 +716,9 @@ static int cyapa_get_query_data(struct cyapa *cyapa)
 	memcpy(&cyapa->product_id[13], &query_data[11], 2);
 	cyapa->product_id[15] = '\0';
 
+	cyapa->fw_maj_ver = query_data[15];
+	cyapa->fw_min_ver = query_data[16];
+
 	cyapa->btn_capability = query_data[19] & CAPABILITY_BTN_MASK;
 
 	cyapa->gen = query_data[20] & 0x0f;
@@ -1175,6 +1180,21 @@ done:
 /*
  * Sysfs Interface.
  */
+static ssize_t cyapa_show_fm_ver(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct cyapa *cyapa = dev_get_drvdata(dev);
+	return scnprintf(buf, PAGE_SIZE, "%d.%d\n", cyapa->fw_maj_ver,
+			 cyapa->fw_min_ver);
+}
+
+static ssize_t cyapa_show_product_id(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct cyapa *cyapa = dev_get_drvdata(dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", cyapa->product_id);
+}
+
 static ssize_t cyapa_update_fw_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -1199,9 +1219,13 @@ static ssize_t cyapa_update_fw_store(struct device *dev,
 	return ret ? ret : count;
 }
 
+static DEVICE_ATTR(firmware_version, S_IRUGO, cyapa_show_fm_ver, NULL);
+static DEVICE_ATTR(product_id, S_IRUGO, cyapa_show_product_id, NULL);
 static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
 
 static struct attribute *cyapa_sysfs_entries[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_product_id.attr,
 	&dev_attr_update_fw.attr,
 	NULL,
 };
-- 
1.8.1.3