[PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Tom Rini]

This reverts commit 885f3fb9fa1f9e185e8a4e905157087495734349 due to this
change breaking the touchpad on the Chromebook Pixel 2015 on resume from
sleep or warm resets.

Cc: Olof Johansson <olof@lixom.net>
Cc: Nick Dyer <nick.dyer@itdev.co.uk>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2160512..9b92b60 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1098,9 +1098,7 @@ static int mxt_soft_reset(struct mxt_data *data)
 	struct device *dev = &data->client->dev;
 	int ret = 0;
 
-	dev_info(dev, "Resetting device\n");
-
-	disable_irq(data->irq);
+	dev_info(dev, "Resetting chip\n");
 
 	reinit_completion(&data->reset_completion);
 
@@ -1108,11 +1106,6 @@ static int mxt_soft_reset(struct mxt_data *data)
 	if (ret)
 		return ret;
 
-	/* Ignore CHG line for 100ms after reset */
-	msleep(100);
-
-	enable_irq(data->irq);
-
 	ret = mxt_wait_for_completion(data, &data->reset_completion,
 				      MXT_RESET_TIMEOUT);
 	if (ret)
-- 
1.7.9.5

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Nick Dyer]

On 2016-04-07 23:52, Tom Rini wrote:
> This reverts commit 885f3fb9fa1f9e185e8a4e905157087495734349 due to this
> change breaking the touchpad on the Chromebook Pixel 2015 on resume from
> sleep or warm resets.
> 
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Nick Dyer <nick.dyer@itdev.co.uk>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Henrik Rydberg <rydberg@bitmath.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Hi Tom-

Sorry that this has caused a problem. I suggest we try and find a 3rd
option other than reverting this patch, because otherwise we will cause
problems on other platforms.

I have a Pixel 2 here - can you advise how to reproduce?

> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 2160512..9b92b60 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1098,9 +1098,7 @@ static int mxt_soft_reset(struct mxt_data *data)
>  	struct device *dev = &data->client->dev;
>  	int ret = 0;
>  
> -	dev_info(dev, "Resetting device\n");
> -
> -	disable_irq(data->irq);
> +	dev_info(dev, "Resetting chip\n");
>  
>  	reinit_completion(&data->reset_completion);
>  
> @@ -1108,11 +1106,6 @@ static int mxt_soft_reset(struct mxt_data *data)
>  	if (ret)
>  		return ret;
>  
> -	/* Ignore CHG line for 100ms after reset */
> -	msleep(100);
> -
> -	enable_irq(data->irq);

I suspect what is going on here is that Pixel 2 is using edge triggering
and we are missing the edge during the 100ms delay. Could you try just
changing this from "enable_irq" to
 mxt_acquire_irq(data);

> -
>  	ret = mxt_wait_for_completion(data, &data->reset_completion,
>  				      MXT_RESET_TIMEOUT);
>  	if (ret)
> 

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

On Fri, Apr 08, 2016 at 10:10:06AM +0100, Nick Dyer wrote:
> On 2016-04-07 23:52, Tom Rini wrote:
> > This reverts commit 885f3fb9fa1f9e185e8a4e905157087495734349 due to this
> > change breaking the touchpad on the Chromebook Pixel 2015 on resume from
> > sleep or warm resets.
> > 
> > Cc: Olof Johansson <olof@lixom.net>
> > Cc: Nick Dyer <nick.dyer@itdev.co.uk>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Henrik Rydberg <rydberg@bitmath.org>
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> 
> Hi Tom-
> 
> Sorry that this has caused a problem. I suggest we try and find a 3rd
> option other than reverting this patch, because otherwise we will cause
> problems on other platforms.

Yeah, it would be good to fix all platforms.  I'm curious, was there an
observed problem on another platform or just a theoretical problem?

> I have a Pixel 2 here - can you advise how to reproduce?

I (and a bunch of other folks, the linux-samus people now point people
at using mxt-app every boot to reset the device) see this every time I
either suspend the laptop or do a warm boot into a new kernel (I didn't
try kexec but it too is probably broken).  Note that I'm not using
mainline to boot ChromeOS but I've got a regular Linux distro in ROOT-C.

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Nick Dyer]

On 2016-04-08 13:14, Tom Rini wrote:
> On Fri, Apr 08, 2016 at 10:10:06AM +0100, Nick Dyer wrote:
>> On 2016-04-07 23:52, Tom Rini wrote:
>>> This reverts commit 885f3fb9fa1f9e185e8a4e905157087495734349 due to this
>>> change breaking the touchpad on the Chromebook Pixel 2015 on resume from
>>> sleep or warm resets.
>>>
>>> Cc: Olof Johansson <olof@lixom.net>
>>> Cc: Nick Dyer <nick.dyer@itdev.co.uk>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Cc: Henrik Rydberg <rydberg@bitmath.org>
>>> Signed-off-by: Tom Rini <trini@konsulko.com>
>>
>> Hi Tom-
>>
>> Sorry that this has caused a problem. I suggest we try and find a 3rd
>> option other than reverting this patch, because otherwise we will cause
>> problems on other platforms.
> 
> Yeah, it would be good to fix all platforms.  I'm curious, was there an
> observed problem on another platform or just a theoretical problem?

Yes, reported by several customers and I've seen it myself in testing
(Beagleboard + MXT Evaluation Kit). What happens is that the interrupt
triggers too early during the power on sequence after a soft reset and you
get an error message when the ISR fails to communicate on the appmode
address. There have been some anecdotal reports that the device may end up
stuck in the bootloader mode if this happens, although I'm not 100%
convinced this is accurate.

>> I have a Pixel 2 here - can you advise how to reproduce?
> 
> I (and a bunch of other folks, the linux-samus people now point people
> at using mxt-app every boot to reset the device) see this every time I
> either suspend the laptop or do a warm boot into a new kernel (I didn't
> try kexec but it too is probably broken).  Note that I'm not using
> mainline to boot ChromeOS but I've got a regular Linux distro in ROOT-C.

OK. I will try it. My Pixel is running Ubuntu with a mainline kernel, so
should be able to repro.

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On Fri, Apr 08, 2016 at 01:26:30PM +0100, Nick Dyer wrote:
> On 2016-04-08 13:14, Tom Rini wrote:
> > On Fri, Apr 08, 2016 at 10:10:06AM +0100, Nick Dyer wrote:
> >> On 2016-04-07 23:52, Tom Rini wrote:
> >>> This reverts commit 885f3fb9fa1f9e185e8a4e905157087495734349 due to this
> >>> change breaking the touchpad on the Chromebook Pixel 2015 on resume from
> >>> sleep or warm resets.
> >>>
> >>> Cc: Olof Johansson <olof@lixom.net>
> >>> Cc: Nick Dyer <nick.dyer@itdev.co.uk>
> >>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>> Cc: Henrik Rydberg <rydberg@bitmath.org>
> >>> Signed-off-by: Tom Rini <trini@konsulko.com>
> >>
> >> Hi Tom-
> >>
> >> Sorry that this has caused a problem. I suggest we try and find a 3rd
> >> option other than reverting this patch, because otherwise we will cause
> >> problems on other platforms.
> > 
> > Yeah, it would be good to fix all platforms.  I'm curious, was there an
> > observed problem on another platform or just a theoretical problem?
> 
> Yes, reported by several customers and I've seen it myself in testing
> (Beagleboard + MXT Evaluation Kit). What happens is that the interrupt
> triggers too early during the power on sequence after a soft reset and you
> get an error message when the ISR fails to communicate on the appmode
> address. There have been some anecdotal reports that the device may end up
> stuck in the bootloader mode if this happens, although I'm not 100%
> convinced this is accurate.

Oh interesting, OK.

> >> I have a Pixel 2 here - can you advise how to reproduce?
> > 
> > I (and a bunch of other folks, the linux-samus people now point people
> > at using mxt-app every boot to reset the device) see this every time I
> > either suspend the laptop or do a warm boot into a new kernel (I didn't
> > try kexec but it too is probably broken).  Note that I'm not using
> > mainline to boot ChromeOS but I've got a regular Linux distro in ROOT-C.
> 
> OK. I will try it. My Pixel is running Ubuntu with a mainline kernel, so
> should be able to repro.

Thanks.  Happy to test patches when you get there and feel free to shoot
me patches to have more info get dumped out or whatever if needed.

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Nick Dyer]

On 2016-04-08 13:39, Tom Rini wrote:
>>>> I have a Pixel 2 here - can you advise how to reproduce?
>>>
>>> I (and a bunch of other folks, the linux-samus people now point people
>>> at using mxt-app every boot to reset the device) see this every time I
>>> either suspend the laptop or do a warm boot into a new kernel (I didn't
>>> try kexec but it too is probably broken).  Note that I'm not using
>>> mainline to boot ChromeOS but I've got a regular Linux distro in ROOT-C.
>>
>> OK. I will try it. My Pixel is running Ubuntu with a mainline kernel, so
>> should be able to repro.
> 
> Thanks.  Happy to test patches when you get there and feel free to shoot
> me patches to have more info get dumped out or whatever if needed.

Could you try the below patch to correctly acquire the IRQ after soft reset on
systems using IRQF_TRIGGER_FALLING.

Appears to work correctly on my Pixel 2 during a brief test.

A workaround also seems to be to reconfig T18 COMMSCONFIG to enable
the RETRIGEN bit using mxt-app:
    mxt-app -W -T18 44
    mxt-app --backup
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2160512e..5af7907 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1093,6 +1093,19 @@ static int mxt_t6_command(struct mxt_data *data, u16 cmd_offset,
 	return 0;
 }
 
+static int mxt_acquire_irq(struct mxt_data *data)
+{
+	int error;
+
+	enable_irq(data->irq);
+
+	error = mxt_process_messages_until_invalid(data);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static int mxt_soft_reset(struct mxt_data *data)
 {
 	struct device *dev = &data->client->dev;
@@ -1111,7 +1124,7 @@ static int mxt_soft_reset(struct mxt_data *data)
 	/* Ignore CHG line for 100ms after reset */
 	msleep(100);
 
-	enable_irq(data->irq);
+	mxt_acquire_irq(data);
 
 	ret = mxt_wait_for_completion(data, &data->reset_completion,
 				      MXT_RESET_TIMEOUT);
@@ -1466,19 +1479,6 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	return ret;
 }
 
-static int mxt_acquire_irq(struct mxt_data *data)
-{
-	int error;
-
-	enable_irq(data->irq);
-
-	error = mxt_process_messages_until_invalid(data);
-	if (error)
-		return error;
-
-	return 0;
-}
-
 static int mxt_get_info(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;

Re: [PATCH] Revert "Input: atmel_mxt_ts - disable interrupt for 50ms after reset"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

On Fri, Apr 08, 2016 at 10:30:02PM +0100, Nick Dyer wrote:
> On 2016-04-08 13:39, Tom Rini wrote:
> >>>> I have a Pixel 2 here - can you advise how to reproduce?
> >>>
> >>> I (and a bunch of other folks, the linux-samus people now point people
> >>> at using mxt-app every boot to reset the device) see this every time I
> >>> either suspend the laptop or do a warm boot into a new kernel (I didn't
> >>> try kexec but it too is probably broken).  Note that I'm not using
> >>> mainline to boot ChromeOS but I've got a regular Linux distro in ROOT-C.
> >>
> >> OK. I will try it. My Pixel is running Ubuntu with a mainline kernel, so
> >> should be able to repro.
> > 
> > Thanks.  Happy to test patches when you get there and feel free to shoot
> > me patches to have more info get dumped out or whatever if needed.
> 
> Could you try the below patch to correctly acquire the IRQ after soft reset on
> systems using IRQF_TRIGGER_FALLING.
> 
> Appears to work correctly on my Pixel 2 during a brief test.

This also works for me so:

Tested-by: Tom Rini <trini@konsulko.com>

... and adding in the linux-samus github project person so it can get
fixed there too.

On an unrelated note and since you have a Pixel 2 as well, the
touchscreen doesn't work for input after suspend (before and after this
patch) but is fine on cold and warm reboots.  Any chance you can debug
that one as well?  Thanks!

> 
> A workaround also seems to be to reconfig T18 COMMSCONFIG to enable
> the RETRIGEN bit using mxt-app:
>     mxt-app -W -T18 44
>     mxt-app --backup
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 2160512e..5af7907 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1093,6 +1093,19 @@ static int mxt_t6_command(struct mxt_data *data, u16 cmd_offset,
>  	return 0;
>  }
>  
> +static int mxt_acquire_irq(struct mxt_data *data)
> +{
> +	int error;
> +
> +	enable_irq(data->irq);
> +
> +	error = mxt_process_messages_until_invalid(data);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
>  static int mxt_soft_reset(struct mxt_data *data)
>  {
>  	struct device *dev = &data->client->dev;
> @@ -1111,7 +1124,7 @@ static int mxt_soft_reset(struct mxt_data *data)
>  	/* Ignore CHG line for 100ms after reset */
>  	msleep(100);
>  
> -	enable_irq(data->irq);
> +	mxt_acquire_irq(data);
>  
>  	ret = mxt_wait_for_completion(data, &data->reset_completion,
>  				      MXT_RESET_TIMEOUT);
> @@ -1466,19 +1479,6 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
>  	return ret;
>  }
>  
> -static int mxt_acquire_irq(struct mxt_data *data)
> -{
> -	int error;
> -
> -	enable_irq(data->irq);
> -
> -	error = mxt_process_messages_until_invalid(data);
> -	if (error)
> -		return error;
> -
> -	return 0;
> -}
> -
>  static int mxt_get_info(struct mxt_data *data)
>  {
>  	struct i2c_client *client = data->client;

-- 
Tom

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] Input: atmel_mxt_ts - use mxt_acquire_irq in mxt_soft_reset

[Nick Dyer]

If using IRQF_TRIGGER_FALLING, then there is a race here: if the reset
completes before we enable the IRQ, then CHG is already low and touch
will be broken.

This has been seen on Chromebook Pixel 2.

A workaround is to reconfig T18 COMMSCONFIG to enable the RETRIGEN bit
using mxt-app:
    mxt-app -W -T18 44
    mxt-app --backup

Tested-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2160512..5af7907 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1093,6 +1093,19 @@ static int mxt_t6_command(struct mxt_data *data, u16 cmd_offset,
 	return 0;
 }
 
+static int mxt_acquire_irq(struct mxt_data *data)
+{
+	int error;
+
+	enable_irq(data->irq);
+
+	error = mxt_process_messages_until_invalid(data);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static int mxt_soft_reset(struct mxt_data *data)
 {
 	struct device *dev = &data->client->dev;
@@ -1111,7 +1124,7 @@ static int mxt_soft_reset(struct mxt_data *data)
 	/* Ignore CHG line for 100ms after reset */
 	msleep(100);
 
-	enable_irq(data->irq);
+	mxt_acquire_irq(data);
 
 	ret = mxt_wait_for_completion(data, &data->reset_completion,
 				      MXT_RESET_TIMEOUT);
@@ -1466,19 +1479,6 @@ release_mem:
 	return ret;
 }
 
-static int mxt_acquire_irq(struct mxt_data *data)
-{
-	int error;
-
-	enable_irq(data->irq);
-
-	error = mxt_process_messages_until_invalid(data);
-	if (error)
-		return error;
-
-	return 0;
-}
-
 static int mxt_get_info(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
-- 
2.5.0

Re: [PATCH] Input: atmel_mxt_ts - use mxt_acquire_irq in mxt_soft_reset

[Dmitry Torokhov]

On Mon, Apr 11, 2016 at 10:31:45AM +0100, Nick Dyer wrote:
> If using IRQF_TRIGGER_FALLING, then there is a race here: if the reset
> completes before we enable the IRQ, then CHG is already low and touch
> will be broken.
> 
> This has been seen on Chromebook Pixel 2.
> 
> A workaround is to reconfig T18 COMMSCONFIG to enable the RETRIGEN bit
> using mxt-app:
>     mxt-app -W -T18 44
>     mxt-app --backup
> 
> Tested-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>

Applied, thank you.

> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 2160512..5af7907 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1093,6 +1093,19 @@ static int mxt_t6_command(struct mxt_data *data, u16 cmd_offset,
>  	return 0;
>  }
>  
> +static int mxt_acquire_irq(struct mxt_data *data)
> +{
> +	int error;
> +
> +	enable_irq(data->irq);
> +
> +	error = mxt_process_messages_until_invalid(data);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
>  static int mxt_soft_reset(struct mxt_data *data)
>  {
>  	struct device *dev = &data->client->dev;
> @@ -1111,7 +1124,7 @@ static int mxt_soft_reset(struct mxt_data *data)
>  	/* Ignore CHG line for 100ms after reset */
>  	msleep(100);
>  
> -	enable_irq(data->irq);
> +	mxt_acquire_irq(data);
>  
>  	ret = mxt_wait_for_completion(data, &data->reset_completion,
>  				      MXT_RESET_TIMEOUT);
> @@ -1466,19 +1479,6 @@ release_mem:
>  	return ret;
>  }
>  
> -static int mxt_acquire_irq(struct mxt_data *data)
> -{
> -	int error;
> -
> -	enable_irq(data->irq);
> -
> -	error = mxt_process_messages_until_invalid(data);
> -	if (error)
> -		return error;
> -
> -	return 0;
> -}
> -
>  static int mxt_get_info(struct mxt_data *data)
>  {
>  	struct i2c_client *client = data->client;
> -- 
> 2.5.0
> 

-- 
Dmitry