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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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