[PATCH 00/10] Input: atmel_mxt_ts - make fw update more robust

[PATCH 00/10] Input: atmel_mxt_ts - make fw update more robust

[Daniel Kurtz]

This patchset attempts to make the atmel fw update a bit more robust by.
Some highlights:
 1) handle more errors during fw update
 2) use the CHG (interrupt) line to detect state transitions (instead of open
    waiting)
 3) handle cases where the device is in bootloader at probe time
 4) tear down the input device before fw update and recreate on success. 

Applies cleanly to Dmitry's input/next branch.

Benson Leung (4):
  Input: atmel_mxt_ts - refactor bootloader entry/exit
  Input: atmel_mxt_ts - wait for CHG assert in mxt_check_bootloader
  Input: atmel_mxt_ts - wait for CHG after bootloader resets
  Input: atmel_mxt_ts - increase FWRESET_TIME

Daniel Kurtz (6):
  Input: atmel_mxt_ts - refactor i2c error handling
  Input: atmel_mxt_ts - register input device before request_irq
  Input: atmel_mxt_ts - refactor input device creation
  Input: atmel_mxt_ts - handle bootloader mode at probe
  Input: atmel_mxt_ts - handle errors during fw update
  Input: atmel_mxt_ts - destroy state before fw update and restore after
 drivers/input/touchscreen/atmel_mxt_ts.c | 423 ++++++++++++++++++++++---------
 1 file changed, 300 insertions(+), 123 deletions(-)

-- 
1.8.1

[PATCH 01/10] Input: atmel_mxt_ts - refactor i2c error handling

[Daniel Kurtz]

A recent patch refactored i2c error handling in the register read/write
path.  This adds similar handling to the other i2c paths used in fw_update
and bootloader state detection.

The generic i2c layer can return values indicating a partial transaction.
>From the atmel_mxt driver's perspective, this is an IO error, so we use
some helper functions to convert these partial transfers to -EIO in a
uniform way.  Other error codes might still be useful, though, so we pass
them up unmodified.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 92 +++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 38 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d04f810..a222bd8 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -324,16 +324,62 @@ static void mxt_dump_message(struct device *dev,
 		message->reportid, 7, message->message);
 }
 
+static int mxt_i2c_recv(struct i2c_client *client, u8 *buf, size_t count)
+{
+	int ret;
+
+	ret = i2c_master_recv(client, buf, count);
+	if (ret == count) {
+		ret = 0;
+	} else if (ret != count) {
+		ret = (ret < 0) ? ret : -EIO;
+		dev_err(&client->dev, "i2c recv failed (%d)\n", ret);
+	}
+
+	return ret;
+}
+
+static int mxt_i2c_send(struct i2c_client *client, const u8 *buf, size_t count)
+{
+	int ret;
+
+	ret = i2c_master_send(client, buf, count);
+	if (ret == count) {
+		ret = 0;
+	} else if (ret != count) {
+		ret = (ret < 0) ? ret : -EIO;
+		dev_err(&client->dev, "i2c send failed (%d)\n", ret);
+	}
+
+	return ret;
+}
+
+static int mxt_i2c_transfer(struct i2c_client *client, struct i2c_msg *msgs,
+		size_t count)
+{
+	int ret;
+
+	ret = i2c_transfer(client->adapter, msgs, count);
+	if (ret == count) {
+		ret = 0;
+	} else {
+		ret = (ret < 0) ? ret : -EIO;
+		dev_err(&client->dev, "i2c transfer failed (%d)\n", ret);
+	}
+
+	return ret;
+}
+
 static int mxt_check_bootloader(struct i2c_client *client,
 				     unsigned int state)
 {
 	u8 val;
+	int ret;
 
 recheck:
-	if (i2c_master_recv(client, &val, 1) != 1) {
-		dev_err(&client->dev, "%s: i2c recv failed\n", __func__);
-		return -EIO;
-	}
+	ret = mxt_i2c_recv(client, &val, 1);
+	if (ret)
+		return ret;
 
 	switch (state) {
 	case MXT_WAITING_BOOTLOAD_CMD:
@@ -363,23 +409,13 @@ static int mxt_unlock_bootloader(struct i2c_client *client)
 	buf[0] = MXT_UNLOCK_CMD_LSB;
 	buf[1] = MXT_UNLOCK_CMD_MSB;
 
-	if (i2c_master_send(client, buf, 2) != 2) {
-		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
-		return -EIO;
-	}
-
-	return 0;
+	return mxt_i2c_send(client, buf, 2);
 }
 
 static int mxt_fw_write(struct i2c_client *client,
 			     const u8 *data, unsigned int frame_size)
 {
-	if (i2c_master_send(client, data, frame_size) != frame_size) {
-		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
-		return -EIO;
-	}
-
-	return 0;
+	return mxt_i2c_send(client, data, frame_size);
 }
 
 static int __mxt_read_reg(struct i2c_client *client,
@@ -387,7 +423,6 @@ static int __mxt_read_reg(struct i2c_client *client,
 {
 	struct i2c_msg xfer[2];
 	u8 buf[2];
-	int ret;
 
 	buf[0] = reg & 0xff;
 	buf[1] = (reg >> 8) & 0xff;
@@ -404,17 +439,7 @@ static int __mxt_read_reg(struct i2c_client *client,
 	xfer[1].len = len;
 	xfer[1].buf = val;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret == 2) {
-		ret = 0;
-	} else {
-		if (ret >= 0)
-			ret = -EIO;
-		dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
-			__func__, ret);
-	}
-
-	return ret;
+	return mxt_i2c_transfer(client, xfer, 2);
 }
 
 static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
@@ -438,16 +463,7 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	buf[1] = (reg >> 8) & 0xff;
 	memcpy(&buf[2], val, len);
 
-	ret = i2c_master_send(client, buf, count);
-	if (ret == count) {
-		ret = 0;
-	} else {
-		if (ret >= 0)
-			ret = -EIO;
-		dev_err(&client->dev, "%s: i2c send failed (%d)\n",
-			__func__, ret);
-	}
-
+	ret = mxt_i2c_send(client, buf, count);
 	kfree(buf);
 	return ret;
 }
-- 
1.8.1

[PATCH 02/10] Input: atmel_mxt_ts - register input device before request_irq

[Daniel Kurtz]

As soon as the irq is request, input event interrupts could occur that
the isr should handle.  Similarly, if there are input events queued up
in the device output buffer, it will send them immediately when we
drain the message buffer with mxt_handle_messages.

Therefore, register the input device before enabling the irq (or handling
messages).

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a222bd8..84f0408 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1124,8 +1124,13 @@ static int mxt_probe(struct i2c_client *client,
 		return -EINVAL;
 
 	data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&client->dev, "Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
 	input_dev = input_allocate_device();
-	if (!data || !input_dev) {
+	if (!input_dev) {
 		dev_err(&client->dev, "Failed to allocate memory\n");
 		error = -ENOMEM;
 		goto err_free_mem;
@@ -1167,8 +1172,11 @@ static int mxt_probe(struct i2c_client *client,
 	/* For multi touch */
 	num_mt_slots = data->T9_reportid_max - data->T9_reportid_min + 1;
 	error = input_mt_init_slots(input_dev, num_mt_slots, 0);
-	if (error)
+	if (error) {
+		input_free_device(input_dev);
 		goto err_free_object;
+	}
+
 	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
 			     0, MXT_MAX_AREA, 0, 0);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
@@ -1181,37 +1189,37 @@ static int mxt_probe(struct i2c_client *client,
 	input_set_drvdata(input_dev, data);
 	i2c_set_clientdata(client, data);
 
+	error = input_register_device(input_dev);
+	if (error) {
+		input_free_device(input_dev);
+		goto err_free_object;
+	}
+
 	error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
 				     pdata->irqflags | IRQF_ONESHOT,
 				     client->name, data);
 	if (error) {
 		dev_err(&client->dev, "Failed to register interrupt\n");
-		goto err_free_object;
+		goto err_unregister_device;
 	}
 
 	error = mxt_make_highchg(data);
 	if (error)
 		goto err_free_irq;
 
-	error = input_register_device(input_dev);
-	if (error)
-		goto err_free_irq;
-
 	error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
 	if (error)
-		goto err_unregister_device;
+		goto err_free_irq;
 
 	return 0;
 
-err_unregister_device:
-	input_unregister_device(input_dev);
-	input_dev = NULL;
 err_free_irq:
 	free_irq(client->irq, data);
+err_unregister_device:
+	input_unregister_device(data->input_dev);
 err_free_object:
 	kfree(data->object_table);
 err_free_mem:
-	input_free_device(input_dev);
 	kfree(data);
 	return error;
 }
-- 
1.8.1

[PATCH 03/10] Input: atmel_mxt_ts - refactor input device creation

[Daniel Kurtz]

Move input device initialization to its own helper function.
This is in preparation of a future patch that makes input device
conditional on the device not being in its bootloader.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 91 ++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 41 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 84f0408..9afc26e 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1111,52 +1111,23 @@ static void mxt_input_close(struct input_dev *dev)
 	mxt_stop(data);
 }
 
-static int mxt_probe(struct i2c_client *client,
-		const struct i2c_device_id *id)
+static int mxt_input_dev_create(struct mxt_data *data)
 {
-	const struct mxt_platform_data *pdata = client->dev.platform_data;
-	struct mxt_data *data;
 	struct input_dev *input_dev;
 	int error;
 	unsigned int num_mt_slots;
 
-	if (!pdata)
-		return -EINVAL;
-
-	data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
-	if (!data) {
-		dev_err(&client->dev, "Failed to allocate memory\n");
+	data->input_dev = input_dev = input_allocate_device();
+	if (!input_dev)
 		return -ENOMEM;
-	}
-
-	input_dev = input_allocate_device();
-	if (!input_dev) {
-		dev_err(&client->dev, "Failed to allocate memory\n");
-		error = -ENOMEM;
-		goto err_free_mem;
-	}
 
 	input_dev->name = "Atmel maXTouch Touchscreen";
-	snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
-		 client->adapter->nr, client->addr);
 	input_dev->phys = data->phys;
-
 	input_dev->id.bustype = BUS_I2C;
-	input_dev->dev.parent = &client->dev;
+	input_dev->dev.parent = &data->client->dev;
 	input_dev->open = mxt_input_open;
 	input_dev->close = mxt_input_close;
 
-	data->client = client;
-	data->input_dev = input_dev;
-	data->pdata = pdata;
-	data->irq = client->irq;
-
-	mxt_calc_resolution(data);
-
-	error = mxt_initialize(data);
-	if (error)
-		goto err_free_mem;
-
 	__set_bit(EV_ABS, input_dev->evbit);
 	__set_bit(EV_KEY, input_dev->evbit);
 	__set_bit(BTN_TOUCH, input_dev->keybit);
@@ -1172,10 +1143,8 @@ static int mxt_probe(struct i2c_client *client,
 	/* For multi touch */
 	num_mt_slots = data->T9_reportid_max - data->T9_reportid_min + 1;
 	error = input_mt_init_slots(input_dev, num_mt_slots, 0);
-	if (error) {
-		input_free_device(input_dev);
-		goto err_free_object;
-	}
+	if (error)
+		goto err_free_device;
 
 	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
 			     0, MXT_MAX_AREA, 0, 0);
@@ -1187,14 +1156,54 @@ static int mxt_probe(struct i2c_client *client,
 			     0, 255, 0, 0);
 
 	input_set_drvdata(input_dev, data);
-	i2c_set_clientdata(client, data);
 
 	error = input_register_device(input_dev);
-	if (error) {
-		input_free_device(input_dev);
-		goto err_free_object;
+	if (error)
+		goto err_free_device;
+
+	return 0;
+
+err_free_device:
+	input_free_device(data->input_dev);
+	data->input_dev = NULL;
+	return error;
+}
+
+static int __devinit mxt_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	const struct mxt_platform_data *pdata = client->dev.platform_data;
+	struct mxt_data *data;
+	int error;
+
+	if (!pdata)
+		return -EINVAL;
+
+	data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&client->dev, "Failed to allocate memory\n");
+		return -ENOMEM;
 	}
 
+	snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
+		 client->adapter->nr, client->addr);
+
+	data->client = client;
+	i2c_set_clientdata(client, data);
+
+	data->pdata = pdata;
+	data->irq = client->irq;
+
+	mxt_calc_resolution(data);
+
+	error = mxt_initialize(data);
+	if (error)
+		goto err_free_mem;
+
+	error = mxt_input_dev_create(data);
+	if (error)
+		goto err_free_object;
+
 	error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
 				     pdata->irqflags | IRQF_ONESHOT,
 				     client->name, data);
-- 
1.8.1

[PATCH 04/10] Input: atmel_mxt_ts - handle bootloader mode at probe

[Daniel Kurtz]

In some cases it is possible for a device to be in its bootloader at
driver probe time.  This is detected by the driver when probe() is called
with an i2c_client which has one of the Atmel Bootloader i2c addresses.

In this case, we should load enough driver functionality to still loading
new firmware using:
  echo 1 > update_fw

However, we must be very careful not to follow any code paths that would try
to access the as-yet uninitialized object table or input device.
In particular:
 1) mxt_remove calls input_unregister_device on input_dev.
 2) mxt_suspend/resume reads and writes from the object table.
 3) Spurious or bootloader induced interrupts

Signed-off-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 51 +++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 9afc26e..6c2c712 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -324,6 +324,12 @@ static void mxt_dump_message(struct device *dev,
 		message->reportid, 7, message->message);
 }
 
+static bool mxt_in_bootloader(struct mxt_data *data)
+{
+	struct i2c_client *client = data->client;
+	return (client->addr == MXT_BOOT_LOW || client->addr == MXT_BOOT_HIGH);
+}
+
 static int mxt_i2c_recv(struct i2c_client *client, u8 *buf, size_t count)
 {
 	int ret;
@@ -584,6 +590,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 	u8 reportid;
 	bool update_input = false;
 
+	if (mxt_in_bootloader(data))
+		goto end;
+
 	do {
 		if (mxt_read_message(data, &message)) {
 			dev_err(dev, "Failed to read message\n");
@@ -975,6 +984,9 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		return ret;
 	}
 
+	if (mxt_in_bootloader(data))
+		goto bootloader_ready;
+
 	/* Change to the bootloader mode */
 	mxt_write_object(data, MXT_GEN_COMMAND_T6,
 			MXT_COMMAND_RESET, MXT_BOOT_VALUE);
@@ -986,6 +998,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 	else
 		client->addr = MXT_BOOT_HIGH;
 
+bootloader_ready:
 	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
 	if (ret)
 		goto out;
@@ -1196,25 +1209,34 @@ static int __devinit mxt_probe(struct i2c_client *client,
 
 	mxt_calc_resolution(data);
 
-	error = mxt_initialize(data);
-	if (error)
-		goto err_free_mem;
+	if (mxt_in_bootloader(data)) {
+		dev_info(&client->dev, "Device in bootloader at probe\n");
+	} else {
+		error = mxt_initialize(data);
+		if (error)
+			goto err_free_mem;
 
-	error = mxt_input_dev_create(data);
-	if (error)
-		goto err_free_object;
+		error = mxt_input_dev_create(data);
+		if (error)
+			goto err_free_object;
+	}
 
 	error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
 				     pdata->irqflags | IRQF_ONESHOT,
 				     client->name, data);
 	if (error) {
 		dev_err(&client->dev, "Failed to register interrupt\n");
-		goto err_unregister_device;
+		if (mxt_in_bootloader(data))
+			goto err_free_mem;
+		else
+			goto err_unregister_device;
 	}
 
-	error = mxt_make_highchg(data);
-	if (error)
-		goto err_free_irq;
+	if (!mxt_in_bootloader(data)) {
+		error = mxt_make_highchg(data);
+		if (error)
+			goto err_free_irq;
+	}
 
 	error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
 	if (error)
@@ -1239,7 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
 
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	free_irq(data->irq, data);
-	input_unregister_device(data->input_dev);
+	if (data->input_dev)
+		input_unregister_device(data->input_dev);
 	kfree(data->object_table);
 	kfree(data);
 
@@ -1253,6 +1276,9 @@ static int mxt_suspend(struct device *dev)
 	struct mxt_data *data = i2c_get_clientdata(client);
 	struct input_dev *input_dev = data->input_dev;
 
+	if (mxt_in_bootloader(data))
+		return 0;
+
 	mutex_lock(&input_dev->mutex);
 
 	if (input_dev->users)
@@ -1269,6 +1295,9 @@ static int mxt_resume(struct device *dev)
 	struct mxt_data *data = i2c_get_clientdata(client);
 	struct input_dev *input_dev = data->input_dev;
 
+	if (mxt_in_bootloader(data))
+		return 0;
+
 	/* Soft reset */
 	mxt_write_object(data, MXT_GEN_COMMAND_T6,
 			MXT_COMMAND_RESET, 1);
-- 
1.8.1

[PATCH 05/10] Input: atmel_mxt_ts - handle errors during fw update

[Daniel Kurtz]

If there are any (i2c) errors during fw update, abort the update, but
leave the i2c address assigned to the bootloader address.

Note that an error when trying to reset the device into the bootloader
will leave the i2c address assigned to the application address.

Signed-off-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 6c2c712..76a25d3 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -988,8 +988,10 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		goto bootloader_ready;
 
 	/* Change to the bootloader mode */
-	mxt_write_object(data, MXT_GEN_COMMAND_T6,
+	ret = mxt_write_object(data, MXT_GEN_COMMAND_T6,
 			MXT_COMMAND_RESET, MXT_BOOT_VALUE);
+	if (ret)
+		goto out;
 	msleep(MXT_RESET_TIME);
 
 	/* Change to slave address of bootloader */
@@ -1004,7 +1006,9 @@ bootloader_ready:
 		goto out;
 
 	/* Unlock bootloader */
-	mxt_unlock_bootloader(client);
+	ret = mxt_unlock_bootloader(client);
+	if (ret)
+		goto out;
 
 	while (pos < fw->size) {
 		ret = mxt_check_bootloader(client,
@@ -1020,7 +1024,9 @@ bootloader_ready:
 		frame_size += 2;
 
 		/* Write one frame to device */
-		mxt_fw_write(client, fw->data + pos, frame_size);
+		ret = mxt_fw_write(client, fw->data + pos, frame_size);
+		if (ret)
+			goto out;
 
 		ret = mxt_check_bootloader(client,
 						MXT_FRAME_CRC_PASS);
@@ -1032,14 +1038,13 @@ bootloader_ready:
 		dev_dbg(dev, "Updated %d bytes / %zd bytes\n", pos, fw->size);
 	}
 
-out:
-	release_firmware(fw);
-
 	/* Change to slave address of application */
 	if (client->addr == MXT_BOOT_LOW)
 		client->addr = MXT_APP_LOW;
 	else
 		client->addr = MXT_APP_HIGH;
+out:
+	release_firmware(fw);
 
 	return ret;
 }
-- 
1.8.1

[PATCH 06/10] Input: atmel_mxt_ts - destroy state before fw update and restore after

[Daniel Kurtz]

After firmware update, the device may have a completely different object
table which corresponds to an input device with different properties.
So, destroy the old state before firmware update, and completely
reinitialize the driver afterward.

Two benefits of this:
 1) Since there is no input device during fw update, no need to worry
about device open/close events.
 2) If firmware update fails, the device and driver will still be in
bootloader mode and an improperly configured input device will not exist.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 76a25d3..c74f5a5 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1001,6 +1001,13 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		client->addr = MXT_BOOT_HIGH;
 
 bootloader_ready:
+	/* Free any driver state. It will get reinitialized after fw update. */
+	mxt_free_object_table(data);
+	if (data->input_dev) {
+		input_unregister_device(data->input_dev);
+		data->input_dev = NULL;
+	}
+
 	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
 	if (ret)
 		goto out;
@@ -1068,9 +1075,8 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 		/* Wait for reset */
 		msleep(MXT_FWRESET_TIME);
 
-		mxt_free_object_table(data);
-
 		mxt_initialize(data);
+		mxt_input_dev_create(data);
 	}
 
 	enable_irq(data->irq);
-- 
1.8.1

[PATCH 07/10] Input: atmel_mxt_ts - refactor bootloader entry/exit

[Daniel Kurtz]

From: Benson Leung <bleung@chromium.org>

Refactor bootloading into a three parts:
 1) bl enter that only happens when device is not yet in bl.
    bl enter frees old driver state and switches to BL i2c addr.
 2) the actual fw_update
 3) bl exit that only happens if fw update is successful.
    bl exit switches to APP i2c addr and reloads object table and creates
    a new input device.

Signed-off-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Yufeng Shen <miletus@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 128 +++++++++++++++++++++----------
 1 file changed, 87 insertions(+), 41 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index c74f5a5..be96be3 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -253,6 +253,11 @@ struct mxt_data {
 	u8 T9_reportid_max;
 };
 
+static void mxt_free_object_table(struct mxt_data *data);
+static int mxt_initialize(struct mxt_data *data);
+static int mxt_input_dev_create(struct mxt_data *data);
+static int mxt_make_highchg(struct mxt_data *data);
+
 static bool mxt_object_readable(unsigned int type)
 {
 	switch (type) {
@@ -402,6 +407,8 @@ recheck:
 
 	if (val != state) {
 		dev_err(&client->dev, "Unvalid bootloader mode state\n");
+		dev_err(&client->dev, "Invalid bootloader mode state %d, %d\n",
+			val, state);
 		return -EINVAL;
 	}
 
@@ -581,6 +588,81 @@ static bool mxt_is_T9_message(struct mxt_data *data, struct mxt_message *msg)
 	return (id >= data->T9_reportid_min && id <= data->T9_reportid_max);
 }
 
+static int mxt_enter_bl(struct mxt_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	if (mxt_in_bootloader(data))
+		return 0;
+
+	disable_irq(data->irq);
+
+	/* Change to the bootloader mode */
+	ret = mxt_write_object(data, MXT_GEN_COMMAND_T6,
+			       MXT_COMMAND_RESET, MXT_BOOT_VALUE);
+	if (ret) {
+		enable_irq(data->irq);
+		return ret;
+	}
+
+	/* Change to slave address of bootloader */
+	if (client->addr == MXT_APP_LOW)
+		client->addr = MXT_BOOT_LOW;
+	else
+		client->addr = MXT_BOOT_HIGH;
+
+	/* Free any driver state. It will get reinitialized after fw update. */
+	mxt_free_object_table(data);
+	if (data->input_dev) {
+		input_unregister_device(data->input_dev);
+		data->input_dev = NULL;
+	}
+
+	enable_irq(data->irq);
+	msleep(MXT_RESET_TIME);
+	return 0;
+}
+
+static void mxt_exit_bl(struct mxt_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
+	int error;
+
+	if (!mxt_in_bootloader(data))
+		return;
+
+	disable_irq(data->irq);
+	/* Wait for reset */
+	msleep(MXT_FWRESET_TIME);
+
+	if (client->addr == MXT_BOOT_LOW)
+		client->addr = MXT_APP_LOW;
+	else
+		client->addr = MXT_APP_HIGH;
+
+	error = mxt_initialize(data);
+	if (error) {
+		dev_err(dev, "Failed to initialize on exit bl. error = %d\n",
+			error);
+		return;
+	}
+
+	error = mxt_input_dev_create(data);
+	if (error) {
+		dev_err(dev, "Create input dev failed after init. error = %d\n",
+			error);
+		return;
+	}
+
+	error = mxt_make_highchg(data);
+	if (error)
+		dev_err(dev, "Failed to clear CHG after init. error = %d\n",
+			error);
+	enable_irq(data->irq);
+}
+
 static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 {
 	struct mxt_data *data = dev_id;
@@ -984,28 +1066,10 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		return ret;
 	}
 
-	if (mxt_in_bootloader(data))
-		goto bootloader_ready;
-
-	/* Change to the bootloader mode */
-	ret = mxt_write_object(data, MXT_GEN_COMMAND_T6,
-			MXT_COMMAND_RESET, MXT_BOOT_VALUE);
-	if (ret)
+	ret = mxt_enter_bl(data);
+	if (ret) {
+		dev_err(dev, "Failed to reset to bootloader.\n");
 		goto out;
-	msleep(MXT_RESET_TIME);
-
-	/* Change to slave address of bootloader */
-	if (client->addr == MXT_APP_LOW)
-		client->addr = MXT_BOOT_LOW;
-	else
-		client->addr = MXT_BOOT_HIGH;
-
-bootloader_ready:
-	/* Free any driver state. It will get reinitialized after fw update. */
-	mxt_free_object_table(data);
-	if (data->input_dev) {
-		input_unregister_device(data->input_dev);
-		data->input_dev = NULL;
 	}
 
 	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
@@ -1045,11 +1109,8 @@ bootloader_ready:
 		dev_dbg(dev, "Updated %d bytes / %zd bytes\n", pos, fw->size);
 	}
 
-	/* Change to slave address of application */
-	if (client->addr == MXT_BOOT_LOW)
-		client->addr = MXT_APP_LOW;
-	else
-		client->addr = MXT_APP_HIGH;
+	/* Device exits bl mode to app mode only if successful */
+	mxt_exit_bl(data);
 out:
 	release_firmware(fw);
 
@@ -1060,31 +1121,16 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t count)
 {
-	struct mxt_data *data = dev_get_drvdata(dev);
 	int error;
 
-	disable_irq(data->irq);
-
 	error = mxt_load_fw(dev, MXT_FW_NAME);
 	if (error) {
 		dev_err(dev, "The firmware update failed(%d)\n", error);
 		count = error;
 	} else {
 		dev_dbg(dev, "The firmware update succeeded\n");
-
-		/* Wait for reset */
-		msleep(MXT_FWRESET_TIME);
-
-		mxt_initialize(data);
-		mxt_input_dev_create(data);
 	}
 
-	enable_irq(data->irq);
-
-	error = mxt_make_highchg(data);
-	if (error)
-		return error;
-
 	return count;
 }
 
-- 
1.8.1

[PATCH 08/10] Input: atmel_mxt_ts - wait for CHG assert in mxt_check_bootloader

[Daniel Kurtz]

From: Benson Leung <bleung@chromium.org>

The driver should not immediately read bootloader status when
in Application Update Mode. The CHG line will assert when the device
has made a state transition and is ready to report a new status
via i2c.

This change adds a wait for completion in mxt_check_bootloader,
and changes the mxt_interrupt handler to signal the completion.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 63 ++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index be96be3..d0f91ff 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -13,6 +13,7 @@
 
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
@@ -251,6 +252,9 @@ struct mxt_data {
 	u8 T6_reportid;
 	u8 T9_reportid_min;
 	u8 T9_reportid_max;
+
+	/* for fw update in bootloader */
+	struct completion bl_completion;
 };
 
 static void mxt_free_object_table(struct mxt_data *data);
@@ -381,19 +385,58 @@ static int mxt_i2c_transfer(struct i2c_client *client, struct i2c_msg *msgs,
 	return ret;
 }
 
-static int mxt_check_bootloader(struct i2c_client *client,
-				     unsigned int state)
+static int mxt_wait_for_chg(struct mxt_data *data, unsigned int timeout_ms)
 {
+	struct device *dev = &data->client->dev;
+	struct completion *comp = &data->bl_completion;
+	unsigned long timeout = msecs_to_jiffies(timeout_ms);
+	long ret;
+
+	ret = wait_for_completion_interruptible_timeout(comp, timeout);
+	if (ret < 0) {
+		dev_err(dev, "Wait for completion interrupted.\n");
+		/*
+		 * TODO: handle -EINTR better by terminating fw update process
+		 * before returning to userspace by writing length 0x000 to
+		 * device (iff we are in WAITING_FRAME_DATA state).
+		 */
+		return -EINTR;
+	} else if (ret == 0) {
+		dev_err(dev, "Wait for completion timed out.\n");
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mxt_check_bootloader(struct mxt_data *data, unsigned int state)
+{
+	struct i2c_client *client = data->client;
 	u8 val;
 	int ret;
 
 recheck:
+	if (state != MXT_WAITING_BOOTLOAD_CMD) {
+		/*
+		 * In application update mode, the interrupt
+		 * line signals state transitions. We must wait for the
+		 * CHG assertion before reading the status byte.
+		 * Once the status byte has been read, the line is deasserted.
+		 */
+		int ret = mxt_wait_for_chg(data, 300);
+		if (ret) {
+			dev_err(&client->dev, "Update wait error %d\n", ret);
+			return ret;
+		}
+	}
+
 	ret = mxt_i2c_recv(client, &val, 1);
 	if (ret)
 		return ret;
 
 	switch (state) {
 	case MXT_WAITING_BOOTLOAD_CMD:
+		dev_info(&client->dev, "bootloader version: %d\n",
+			 val & MXT_BOOT_STATUS_MASK);
 	case MXT_WAITING_FRAME_DATA:
 		val &= ~MXT_BOOT_STATUS_MASK;
 		break;
@@ -672,8 +715,11 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 	u8 reportid;
 	bool update_input = false;
 
-	if (mxt_in_bootloader(data))
+	if (mxt_in_bootloader(data)) {
+		/* bootloader state transition completion */
+		complete(&data->bl_completion);
 		goto end;
+	}
 
 	do {
 		if (mxt_read_message(data, &message)) {
@@ -1072,18 +1118,18 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		goto out;
 	}
 
-	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
+	ret = mxt_check_bootloader(data, MXT_WAITING_BOOTLOAD_CMD);
 	if (ret)
 		goto out;
 
+	INIT_COMPLETION(data->bl_completion);
 	/* Unlock bootloader */
 	ret = mxt_unlock_bootloader(client);
 	if (ret)
 		goto out;
 
 	while (pos < fw->size) {
-		ret = mxt_check_bootloader(client,
-						MXT_WAITING_FRAME_DATA);
+		ret = mxt_check_bootloader(data, MXT_WAITING_FRAME_DATA);
 		if (ret)
 			goto out;
 
@@ -1099,8 +1145,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		if (ret)
 			goto out;
 
-		ret = mxt_check_bootloader(client,
-						MXT_FRAME_CRC_PASS);
+		ret = mxt_check_bootloader(data, MXT_FRAME_CRC_PASS);
 		if (ret)
 			goto out;
 
@@ -1264,6 +1309,8 @@ static int __devinit mxt_probe(struct i2c_client *client,
 	data->pdata = pdata;
 	data->irq = client->irq;
 
+	init_completion(&data->bl_completion);
+
 	mxt_calc_resolution(data);
 
 	if (mxt_in_bootloader(data)) {
-- 
1.8.1

[PATCH 09/10] Input: atmel_mxt_ts - wait for CHG after bootloader resets

[Daniel Kurtz]

From: Benson Leung <bleung@chromium.org>

Rather than msleep for MXT_RESET_TIME and MXT_FWRESET_TIME
during the transition to bootloader mode and the transition
back from app, wait for the CHG assert to indicate that the
transition is done.

This change replaces the msleep with a wait for completion that
the mxt_interrupt handler signals.

This improves firmware update time by 300 ms as we no longer
wait longer than necessary for each reset.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d0f91ff..ef867d3 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -634,6 +634,7 @@ static bool mxt_is_T9_message(struct mxt_data *data, struct mxt_message *msg)
 static int mxt_enter_bl(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 	int ret;
 
 	if (mxt_in_bootloader(data))
@@ -662,8 +663,19 @@ static int mxt_enter_bl(struct mxt_data *data)
 		data->input_dev = NULL;
 	}
 
+	INIT_COMPLETION(data->bl_completion);
 	enable_irq(data->irq);
-	msleep(MXT_RESET_TIME);
+
+	/* Wait for CHG assert to indicate successful reset into bootloader */
+	ret = mxt_wait_for_chg(data, MXT_RESET_TIME);
+	if (ret) {
+		dev_err(dev, "Failed waiting for reset to bootloader.\n");
+		if (client->addr == MXT_BOOT_LOW)
+			client->addr = MXT_APP_LOW;
+		else
+			client->addr = MXT_APP_HIGH;
+		return ret;
+	}
 	return 0;
 }
 
@@ -676,10 +688,10 @@ static void mxt_exit_bl(struct mxt_data *data)
 	if (!mxt_in_bootloader(data))
 		return;
 
-	disable_irq(data->irq);
 	/* Wait for reset */
-	msleep(MXT_FWRESET_TIME);
+	mxt_wait_for_chg(data, MXT_FWRESET_TIME);
 
+	disable_irq(data->irq);
 	if (client->addr == MXT_BOOT_LOW)
 		client->addr = MXT_APP_LOW;
 	else
@@ -1122,7 +1134,6 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 	if (ret)
 		goto out;
 
-	INIT_COMPLETION(data->bl_completion);
 	/* Unlock bootloader */
 	ret = mxt_unlock_bootloader(client);
 	if (ret)
-- 
1.8.1

[PATCH 10/10] Input: atmel_mxt_ts - increase FWRESET_TIME

[Daniel Kurtz]

From: Benson Leung <bleung@chromium.org>

175ms is not enough time to update the firmware. Set to
500ms.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index ef867d3..0cf9ff1 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -180,7 +180,7 @@
 #define MXT_BACKUP_TIME		25	/* msec */
 #define MXT_RESET_TIME		65	/* msec */
 
-#define MXT_FWRESET_TIME	175	/* msec */
+#define MXT_FWRESET_TIME	500	/* msec */
 
 /* Command to unlock bootloader */
 #define MXT_UNLOCK_CMD_MSB	0xaa
-- 
1.8.1

Re: [PATCH 01/10] Input: atmel_mxt_ts - refactor i2c error handling

[Henrik Rydberg]

Hi Daniel,

> A recent patch refactored i2c error handling in the register read/write
> path.  This adds similar handling to the other i2c paths used in fw_update
> and bootloader state detection.
> 
> The generic i2c layer can return values indicating a partial transaction.
> From the atmel_mxt driver's perspective, this is an IO error, so we use
> some helper functions to convert these partial transfers to -EIO in a
> uniform way.  Other error codes might still be useful, though, so we pass
> them up unmodified.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 92 +++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 38 deletions(-)

Nice cleanup, but some comments below.

> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index d04f810..a222bd8 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -324,16 +324,62 @@ static void mxt_dump_message(struct device *dev,
>  		message->reportid, 7, message->message);
>  }
>  
> +static int mxt_i2c_recv(struct i2c_client *client, u8 *buf, size_t count)
> +{
> +	int ret;
> +
> +	ret = i2c_master_recv(client, buf, count);
> +	if (ret == count) {
> +		ret = 0;
> +	} else if (ret != count) {

Eh, implied logic. Also, no need for an else here, just return 0 above.

> +		ret = (ret < 0) ? ret : -EIO;
> +		dev_err(&client->dev, "i2c recv failed (%d)\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mxt_i2c_send(struct i2c_client *client, const u8 *buf, size_t count)
> +{
> +	int ret;
> +
> +	ret = i2c_master_send(client, buf, count);
> +	if (ret == count) {
> +		ret = 0;
> +	} else if (ret != count) {

Ditto...

> +		ret = (ret < 0) ? ret : -EIO;
> +		dev_err(&client->dev, "i2c send failed (%d)\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mxt_i2c_transfer(struct i2c_client *client, struct i2c_msg *msgs,
> +		size_t count)
> +{
> +	int ret;
> +
> +	ret = i2c_transfer(client->adapter, msgs, count);
> +	if (ret == count) {
> +		ret = 0;

return 0;

> +	} else {

skip

> +		ret = (ret < 0) ? ret : -EIO;
> +		dev_err(&client->dev, "i2c transfer failed (%d)\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
>  static int mxt_check_bootloader(struct i2c_client *client,
>  				     unsigned int state)
>  {
>  	u8 val;
> +	int ret;
>  
>  recheck:
> -	if (i2c_master_recv(client, &val, 1) != 1) {
> -		dev_err(&client->dev, "%s: i2c recv failed\n", __func__);
> -		return -EIO;
> -	}
> +	ret = mxt_i2c_recv(client, &val, 1);
> +	if (ret)
> +		return ret;
>  
>  	switch (state) {
>  	case MXT_WAITING_BOOTLOAD_CMD:
> @@ -363,23 +409,13 @@ static int mxt_unlock_bootloader(struct i2c_client *client)
>  	buf[0] = MXT_UNLOCK_CMD_LSB;
>  	buf[1] = MXT_UNLOCK_CMD_MSB;
>  
> -	if (i2c_master_send(client, buf, 2) != 2) {
> -		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> -		return -EIO;
> -	}
> -
> -	return 0;
> +	return mxt_i2c_send(client, buf, 2);
>  }
>  
>  static int mxt_fw_write(struct i2c_client *client,
>  			     const u8 *data, unsigned int frame_size)
>  {
> -	if (i2c_master_send(client, data, frame_size) != frame_size) {
> -		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> -		return -EIO;
> -	}
> -
> -	return 0;
> +	return mxt_i2c_send(client, data, frame_size);
>  }
>  
>  static int __mxt_read_reg(struct i2c_client *client,
> @@ -387,7 +423,6 @@ static int __mxt_read_reg(struct i2c_client *client,
>  {
>  	struct i2c_msg xfer[2];
>  	u8 buf[2];
> -	int ret;
>  
>  	buf[0] = reg & 0xff;
>  	buf[1] = (reg >> 8) & 0xff;
> @@ -404,17 +439,7 @@ static int __mxt_read_reg(struct i2c_client *client,
>  	xfer[1].len = len;
>  	xfer[1].buf = val;
>  
> -	ret = i2c_transfer(client->adapter, xfer, 2);
> -	if (ret == 2) {
> -		ret = 0;
> -	} else {
> -		if (ret >= 0)
> -			ret = -EIO;
> -		dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
> -			__func__, ret);
> -	}
> -
> -	return ret;
> +	return mxt_i2c_transfer(client, xfer, 2);
>  }
>  
>  static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
> @@ -438,16 +463,7 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
>  	buf[1] = (reg >> 8) & 0xff;
>  	memcpy(&buf[2], val, len);
>  
> -	ret = i2c_master_send(client, buf, count);
> -	if (ret == count) {
> -		ret = 0;
> -	} else {
> -		if (ret >= 0)
> -			ret = -EIO;
> -		dev_err(&client->dev, "%s: i2c send failed (%d)\n",
> -			__func__, ret);
> -	}
> -
> +	ret = mxt_i2c_send(client, buf, count);
>  	kfree(buf);
>  	return ret;
>  }
> -- 
> 1.8.1
> 

Thanks,
Henrik

Re: [PATCH 02/10] Input: atmel_mxt_ts - register input device before request_irq

[Dmitry Torokhov]

Hi Daniel,

On Fri, Feb 01, 2013 at 04:11:44PM +0800, Daniel Kurtz wrote:
> As soon as the irq is request, input event interrupts could occur that
> the isr should handle.  Similarly, if there are input events queued up
> in the device output buffer, it will send them immediately when we
> drain the message buffer with mxt_handle_messages.
> 
> Therefore, register the input device before enabling the irq (or handling
> messages).

I do not see the point. The allocated device can withstand
input_events() calls even without being registered and I am not sure why
anyone would be particularly interested in events that happened before
or during device registration, espceially for a touchscreen device.

Thanks.

-- 
Dmitry

Re: [PATCH 04/10] Input: atmel_mxt_ts - handle bootloader mode at probe

[Dmitry Torokhov]

On Fri, Feb 01, 2013 at 04:11:46PM +0800, Daniel Kurtz wrote:
> In some cases it is possible for a device to be in its bootloader at
> driver probe time.  This is detected by the driver when probe() is called
> with an i2c_client which has one of the Atmel Bootloader i2c addresses.
> 
> In this case, we should load enough driver functionality to still loading
> new firmware using:
>   echo 1 > update_fw
> 
> However, we must be very careful not to follow any code paths that would try
> to access the as-yet uninitialized object table or input device.
> In particular:
>  1) mxt_remove calls input_unregister_device on input_dev.
>  2) mxt_suspend/resume reads and writes from the object table.
>  3) Spurious or bootloader induced interrupts
> 
> Signed-off-by: Benson Leung <bleung@chromium.org>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 51 +++++++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 9afc26e..6c2c712 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -324,6 +324,12 @@ static void mxt_dump_message(struct device *dev,
>  		message->reportid, 7, message->message);
>  }
>  
> +static bool mxt_in_bootloader(struct mxt_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	return (client->addr == MXT_BOOT_LOW || client->addr == MXT_BOOT_HIGH);
> +}
> +
>  static int mxt_i2c_recv(struct i2c_client *client, u8 *buf, size_t count)
>  {
>  	int ret;
> @@ -584,6 +590,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>  	u8 reportid;
>  	bool update_input = false;
>  
> +	if (mxt_in_bootloader(data))
> +		goto end;
> +
>  	do {
>  		if (mxt_read_message(data, &message)) {
>  			dev_err(dev, "Failed to read message\n");
> @@ -975,6 +984,9 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>  		return ret;
>  	}
>  
> +	if (mxt_in_bootloader(data))
> +		goto bootloader_ready;
> +
>  	/* Change to the bootloader mode */
>  	mxt_write_object(data, MXT_GEN_COMMAND_T6,
>  			MXT_COMMAND_RESET, MXT_BOOT_VALUE);
> @@ -986,6 +998,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>  	else
>  		client->addr = MXT_BOOT_HIGH;
>  
> +bootloader_ready:
>  	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
>  	if (ret)
>  		goto out;
> @@ -1196,25 +1209,34 @@ static int __devinit mxt_probe(struct i2c_client *client,
>  
>  	mxt_calc_resolution(data);
>  
> -	error = mxt_initialize(data);
> -	if (error)
> -		goto err_free_mem;
> +	if (mxt_in_bootloader(data)) {
> +		dev_info(&client->dev, "Device in bootloader at probe\n");
> +	} else {
> +		error = mxt_initialize(data);
> +		if (error)
> +			goto err_free_mem;
>  
> -	error = mxt_input_dev_create(data);
> -	if (error)
> -		goto err_free_object;
> +		error = mxt_input_dev_create(data);
> +		if (error)
> +			goto err_free_object;
> +	}
>  
>  	error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>  				     pdata->irqflags | IRQF_ONESHOT,
>  				     client->name, data);
>  	if (error) {
>  		dev_err(&client->dev, "Failed to register interrupt\n");
> -		goto err_unregister_device;
> +		if (mxt_in_bootloader(data))
> +			goto err_free_mem;
> +		else
> +			goto err_unregister_device;
>  	}
>  
> -	error = mxt_make_highchg(data);
> -	if (error)
> -		goto err_free_irq;
> +	if (!mxt_in_bootloader(data)) {
> +		error = mxt_make_highchg(data);
> +		if (error)
> +			goto err_free_irq;
> +	}
>  
>  	error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
>  	if (error)
> @@ -1239,7 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
>  
>  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
>  	free_irq(data->irq, data);
> -	input_unregister_device(data->input_dev);
> +	if (data->input_dev)
> +		input_unregister_device(data->input_dev);
>  	kfree(data->object_table);
>  	kfree(data);
>  
> @@ -1253,6 +1276,9 @@ static int mxt_suspend(struct device *dev)
>  	struct mxt_data *data = i2c_get_clientdata(client);
>  	struct input_dev *input_dev = data->input_dev;
>  
> +	if (mxt_in_bootloader(data))
> +		return 0;
> +

Hmm, so what happens if we suspend while in bootloader and maybe even
trying to flash the device? Maybe we should refuse suspend instead?

>  	mutex_lock(&input_dev->mutex);
>  
>  	if (input_dev->users)
> @@ -1269,6 +1295,9 @@ static int mxt_resume(struct device *dev)
>  	struct mxt_data *data = i2c_get_clientdata(client);
>  	struct input_dev *input_dev = data->input_dev;
>  
> +	if (mxt_in_bootloader(data))
> +		return 0;
> +
>  	/* Soft reset */
>  	mxt_write_object(data, MXT_GEN_COMMAND_T6,
>  			MXT_COMMAND_RESET, 1);
> -- 
> 1.8.1
> 

-- 
Dmitry

Re: [PATCH 06/10] Input: atmel_mxt_ts - destroy state before fw update and restore after

[Dmitry Torokhov]

On Fri, Feb 01, 2013 at 04:11:48PM +0800, Daniel Kurtz wrote:
> After firmware update, the device may have a completely different object
> table which corresponds to an input device with different properties.
> So, destroy the old state before firmware update, and completely
> reinitialize the driver afterward.
> 
> Two benefits of this:
>  1) Since there is no input device during fw update, no need to worry
> about device open/close events.
>  2) If firmware update fails, the device and driver will still be in
> bootloader mode and an improperly configured input device will not exist.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 76a25d3..c74f5a5 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1001,6 +1001,13 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>  		client->addr = MXT_BOOT_HIGH;
>  
>  bootloader_ready:
> +	/* Free any driver state. It will get reinitialized after fw update. */
> +	mxt_free_object_table(data);
> +	if (data->input_dev) {
> +		input_unregister_device(data->input_dev);
> +		data->input_dev = NULL;
> +	}
> +
>  	ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
>  	if (ret)
>  		goto out;
> @@ -1068,9 +1075,8 @@ static ssize_t mxt_update_fw_store(struct device *dev,
>  		/* Wait for reset */
>  		msleep(MXT_FWRESET_TIME);
>  
> -		mxt_free_object_table(data);
> -
>  		mxt_initialize(data);
> +		mxt_input_dev_create(data);

What if it fails?

>  	}
>  
>  	enable_irq(data->irq);
> -- 
> 1.8.1
> 

-- 
Dmitry

Re: [PATCH 04/10] Input: atmel_mxt_ts - handle bootloader mode at probe

[Benson Leung]

On Wed, Feb 13, 2013 at 1:46 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Feb 01, 2013 at 04:11:46PM +0800, Daniel Kurtz wrote:
>> In some cases it is possible for a device to be in its bootloader at
>> driver probe time.  This is detected by the driver when probe() is called
>> with an i2c_client which has one of the Atmel Bootloader i2c addresses.
>>
>> In this case, we should load enough driver functionality to still loading
>> new firmware using:
>>   echo 1 > update_fw
>>
>> However, we must be very careful not to follow any code paths that would try
>> to access the as-yet uninitialized object table or input device.
>> In particular:
>>  1) mxt_remove calls input_unregister_device on input_dev.
>>  2) mxt_suspend/resume reads and writes from the object table.
>>  3) Spurious or bootloader induced interrupts
>>
>> Signed-off-by: Benson Leung <bleung@chromium.org>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/touchscreen/atmel_mxt_ts.c | 51 +++++++++++++++++++++++++-------
>>  1 file changed, 40 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 9afc26e..6c2c712 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -324,6 +324,12 @@ static void mxt_dump_message(struct device *dev,
>>               message->reportid, 7, message->message);
>>  }
>>
>> +static bool mxt_in_bootloader(struct mxt_data *data)
>> +{
>> +     struct i2c_client *client = data->client;
>> +     return (client->addr == MXT_BOOT_LOW || client->addr == MXT_BOOT_HIGH);
>> +}
>> +
>>  static int mxt_i2c_recv(struct i2c_client *client, u8 *buf, size_t count)
>>  {
>>       int ret;
>> @@ -584,6 +590,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>>       u8 reportid;
>>       bool update_input = false;
>>
>> +     if (mxt_in_bootloader(data))
>> +             goto end;
>> +
>>       do {
>>               if (mxt_read_message(data, &message)) {
>>                       dev_err(dev, "Failed to read message\n");
>> @@ -975,6 +984,9 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>>               return ret;
>>       }
>>
>> +     if (mxt_in_bootloader(data))
>> +             goto bootloader_ready;
>> +
>>       /* Change to the bootloader mode */
>>       mxt_write_object(data, MXT_GEN_COMMAND_T6,
>>                       MXT_COMMAND_RESET, MXT_BOOT_VALUE);
>> @@ -986,6 +998,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>>       else
>>               client->addr = MXT_BOOT_HIGH;
>>
>> +bootloader_ready:
>>       ret = mxt_check_bootloader(client, MXT_WAITING_BOOTLOAD_CMD);
>>       if (ret)
>>               goto out;
>> @@ -1196,25 +1209,34 @@ static int __devinit mxt_probe(struct i2c_client *client,
>>
>>       mxt_calc_resolution(data);
>>
>> -     error = mxt_initialize(data);
>> -     if (error)
>> -             goto err_free_mem;
>> +     if (mxt_in_bootloader(data)) {
>> +             dev_info(&client->dev, "Device in bootloader at probe\n");
>> +     } else {
>> +             error = mxt_initialize(data);
>> +             if (error)
>> +                     goto err_free_mem;
>>
>> -     error = mxt_input_dev_create(data);
>> -     if (error)
>> -             goto err_free_object;
>> +             error = mxt_input_dev_create(data);
>> +             if (error)
>> +                     goto err_free_object;
>> +     }
>>
>>       error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>>                                    pdata->irqflags | IRQF_ONESHOT,
>>                                    client->name, data);
>>       if (error) {
>>               dev_err(&client->dev, "Failed to register interrupt\n");
>> -             goto err_unregister_device;
>> +             if (mxt_in_bootloader(data))
>> +                     goto err_free_mem;
>> +             else
>> +                     goto err_unregister_device;
>>       }
>>
>> -     error = mxt_make_highchg(data);
>> -     if (error)
>> -             goto err_free_irq;
>> +     if (!mxt_in_bootloader(data)) {
>> +             error = mxt_make_highchg(data);
>> +             if (error)
>> +                     goto err_free_irq;
>> +     }
>>
>>       error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
>>       if (error)
>> @@ -1239,7 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
>>
>>       sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
>>       free_irq(data->irq, data);
>> -     input_unregister_device(data->input_dev);
>> +     if (data->input_dev)
>> +             input_unregister_device(data->input_dev);
>>       kfree(data->object_table);
>>       kfree(data);
>>
>> @@ -1253,6 +1276,9 @@ static int mxt_suspend(struct device *dev)
>>       struct mxt_data *data = i2c_get_clientdata(client);
>>       struct input_dev *input_dev = data->input_dev;
>>
>> +     if (mxt_in_bootloader(data))
>> +             return 0;
>> +
>
> Hmm, so what happens if we suspend while in bootloader and maybe even
> trying to flash the device? Maybe we should refuse suspend instead?
>

That is a good idea. It does make sense to refuse the suspend in the
event that there is an active firmware update in progress.

I should point out that before this patch, it would have been possible
to suspend during a firmware update, and all sorts of bad things would
happen because mxt_suspend tries to use the (now nonexistent) object
in mxt_save_regs. The check here for whether we are in the bootloader
makes this safer.

Furthermore, it's still possible to shut the system down during a
firmware update, which we do not prevent. Our approach has been to let
events like that occur, which may leave the firmware in an invalid
state, and to recover the next time we probe for the device.

If we fail a firmware update because we suspend or we shut down, the
next time the device will appear at the bootloader address, which is
what this patch seeks to handle.

>>       mutex_lock(&input_dev->mutex);
>>
>>       if (input_dev->users)
>> @@ -1269,6 +1295,9 @@ static int mxt_resume(struct device *dev)
>>       struct mxt_data *data = i2c_get_clientdata(client);
>>       struct input_dev *input_dev = data->input_dev;
>>
>> +     if (mxt_in_bootloader(data))
>> +             return 0;
>> +
>>       /* Soft reset */
>>       mxt_write_object(data, MXT_GEN_COMMAND_T6,
>>                       MXT_COMMAND_RESET, 1);
>> --
>> 1.8.1
>>
>
> --
> Dmitry



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