[PATCH 1/3 v2] Input: atmel_mxt_ts - Make wait-after-reset period compatible with all chips

[PATCH 1/3 v2] Input: atmel_mxt_ts - Make wait-after-reset period compatible with all chips

[Iiro Valkonen]

The delay before the chip can be accessed after reset varies between different
chips in maXTouch family. Waiting for 200ms and then monitoring the CHG (chip
is ready when the line is low) is guaranteed to work with all chips.

v2: At Dmitry's suggestion, add a timeout so we are not stuck looping
endlessly in case the CHG is not going low. 

Signed-off-by: Iiro Valkonen <iiro.valkonen@atmel.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    9 ++++++++-
 include/linux/i2c/atmel_mxt_ts.h         |    1 +
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 1e61387..5469a29 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -170,7 +170,7 @@
 #define MXT_BOOT_VALUE		0xa5
 #define MXT_BACKUP_VALUE	0x55
 #define MXT_BACKUP_TIME		25	/* msec */
-#define MXT_RESET_TIME		65	/* msec */
+#define MXT_RESET_TIME		200	/* msec */
 
 #define MXT_FWRESET_TIME	175	/* msec */
 
@@ -792,6 +792,7 @@ static int mxt_initialize(struct mxt_data *data)
 	struct i2c_client *client = data->client;
 	struct mxt_info *info = &data->info;
 	int error;
+	int reset_timeout = 0;
 	u8 val;
 
 	error = mxt_get_info(data);
@@ -828,6 +829,12 @@ static int mxt_initialize(struct mxt_data *data)
 	mxt_write_object(data, MXT_GEN_COMMAND,
 			MXT_COMMAND_RESET, 1);
 	msleep(MXT_RESET_TIME);
+	while ((reset_timeout++ <= 100) && data->pdata->read_chg())
+		msleep(2);
+	if (reset_timeout >= 100) {
+		dev_err(&client->dev, "No response after reset!\n");
+		return -EIO;
+	}
 
 	/* Update matrix size at info struct */
 	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, &val);
diff --git a/include/linux/i2c/atmel_mxt_ts.h b/include/linux/i2c/atmel_mxt_ts.h
index f027f7a..ef59c22 100644
--- a/include/linux/i2c/atmel_mxt_ts.h
+++ b/include/linux/i2c/atmel_mxt_ts.h
@@ -39,6 +39,7 @@ struct mxt_platform_data {
 	unsigned int voltage;
 	unsigned char orient;
 	unsigned long irqflags;
+	u8(*read_chg) (void);
 };
 
 #endif /* __LINUX_ATMEL_MXT_TS_H */
-- 
1.7.0.4

Re: [PATCH 1/3 v2] Input: atmel_mxt_ts - Make wait-after-reset period compatible with all chips

[Joonyoung Shim]

Hi,

On 2011-07-04 오후 9:57, Iiro Valkonen wrote:
> The delay before the chip can be accessed after reset varies between different
> chips in maXTouch family. Waiting for 200ms and then monitoring the CHG (chip
> is ready when the line is low) is guaranteed to work with all chips.

I wonder 200ms waiting needs indeed, it is very long time.
If monitoring the CHG line can detect the completion of reset,
200ms waiting can be removed?

>
> v2: At Dmitry's suggestion, add a timeout so we are not stuck looping
> endlessly in case the CHG is not going low.
>
> Signed-off-by: Iiro Valkonen<iiro.valkonen@atmel.com>
> ---
>   drivers/input/touchscreen/atmel_mxt_ts.c |    9 ++++++++-
>   include/linux/i2c/atmel_mxt_ts.h         |    1 +
>   2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 1e61387..5469a29 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -170,7 +170,7 @@
>   #define MXT_BOOT_VALUE		0xa5
>   #define MXT_BACKUP_VALUE	0x55
>   #define MXT_BACKUP_TIME		25	/* msec */
> -#define MXT_RESET_TIME		65	/* msec */
> +#define MXT_RESET_TIME		200	/* msec */
>
>   #define MXT_FWRESET_TIME	175	/* msec */
>
> @@ -792,6 +792,7 @@ static int mxt_initialize(struct mxt_data *data)
>   	struct i2c_client *client = data->client;
>   	struct mxt_info *info =&data->info;
>   	int error;
> +	int reset_timeout = 0;
>   	u8 val;
>
>   	error = mxt_get_info(data);
> @@ -828,6 +829,12 @@ static int mxt_initialize(struct mxt_data *data)
>   	mxt_write_object(data, MXT_GEN_COMMAND,
>   			MXT_COMMAND_RESET, 1);
>   	msleep(MXT_RESET_TIME);
> +	while ((reset_timeout++<= 100)&&  data->pdata->read_chg())

If pdata->read_chg is NULL?

> +		msleep(2);
> +	if (reset_timeout>= 100) {
> +		dev_err(&client->dev, "No response after reset!\n");
> +		return -EIO;
> +	}
>
>   	/* Update matrix size at info struct */
>   	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE,&val);
> diff --git a/include/linux/i2c/atmel_mxt_ts.h b/include/linux/i2c/atmel_mxt_ts.h
> index f027f7a..ef59c22 100644
> --- a/include/linux/i2c/atmel_mxt_ts.h
> +++ b/include/linux/i2c/atmel_mxt_ts.h
> @@ -39,6 +39,7 @@ struct mxt_platform_data {
>   	unsigned int voltage;
>   	unsigned char orient;
>   	unsigned long irqflags;
> +	u8(*read_chg) (void);
>   };
>
>   #endif /* __LINUX_ATMEL_MXT_TS_H */

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] Input: atmel_mxt_ts - Make wait-after-reset period compatible with all chips

[Iiro Valkonen]

Hello,

On 07/05/2011 04:07 AM, Joonyoung Shim wrote:
> Hi,
> 
> On 2011-07-04 오후 9:57, Iiro Valkonen wrote:
>> The delay before the chip can be accessed after reset varies between different
>> chips in maXTouch family. Waiting for 200ms and then monitoring the CHG (chip
>> is ready when the line is low) is guaranteed to work with all chips.
> 
> I wonder 200ms waiting needs indeed, it is very long time.
> If monitoring the CHG line can detect the completion of reset,
> 200ms waiting can be removed?
> 

Yes, 200ms is a bit longish. But it is what we need to guarantee correct
functionality with all chips in all situations. I would prefer to see that
in the mainline version. Anyone worried about the delay could maybe adjust
the value to suit his/her needs, for example with mXT224 you could get away with
using just 65ms, like in the original version. Another option would be to
check the family ID in the driver, and wait just long enough. Drawback with
that is that the driver (possibly) needs updating with every new chip. I'd
like to keep this simple & robust, wait 200ms which works for every current
(and most likely with every upcoming) chip, and take the small delay penalty.

>> @@ -828,6 +829,12 @@ static int mxt_initialize(struct mxt_data *data)
>>       mxt_write_object(data, MXT_GEN_COMMAND,
>>               MXT_COMMAND_RESET, 1);
>>       msleep(MXT_RESET_TIME);
>> +    while ((reset_timeout++<= 100)&&  data->pdata->read_chg())
> 
> If pdata->read_chg is NULL?
> 

Hopefully it'll never be, but I'll add a check.

Thank you for the comments.

-- 
Iiro
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] Input: atmel_mxt_ts - Make wait-after-reset period compatible with all chips

[Joonyoung Shim]

On 2011-07-05 오후 4:12, Iiro Valkonen wrote:
> Hello,
>
> On 07/05/2011 04:07 AM, Joonyoung Shim wrote:
>> Hi,
>>
>> On 2011-07-04 오후 9:57, Iiro Valkonen wrote:
>>> The delay before the chip can be accessed after reset varies between different
>>> chips in maXTouch family. Waiting for 200ms and then monitoring the CHG (chip
>>> is ready when the line is low) is guaranteed to work with all chips.
>>
>> I wonder 200ms waiting needs indeed, it is very long time.
>> If monitoring the CHG line can detect the completion of reset,
>> 200ms waiting can be removed?
>>

My question is monitoring the CHG line can substitute 
"msleep(MXT_RESET_TIME)"? In other words, i want to remove
msleep(MXT_RESET_TIME) and add only waiting until CHG line is low.

>
> Yes, 200ms is a bit longish. But it is what we need to guarantee correct
> functionality with all chips in all situations. I would prefer to see that
> in the mainline version. Anyone worried about the delay could maybe adjust
> the value to suit his/her needs, for example with mXT224 you could get away with
> using just 65ms, like in the original version. Another option would be to
> check the family ID in the driver, and wait just long enough. Drawback with
> that is that the driver (possibly) needs updating with every new chip. I'd
> like to keep this simple&  robust, wait 200ms which works for every current
> (and most likely with every upcoming) chip, and take the small delay penalty.
>

The 200ms delay is sometimes a problem for fastbooting, and if each
chip has different delay value, i think it's better that driver supports
suitable delay value of the chip because it isn't difficult stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html