* [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
@ 2019-09-24 21:52 Evan Green
2019-09-28 0:16 ` Dmitry Torokhov
0 siblings, 1 reply; 5+ messages in thread
From: Evan Green @ 2019-09-24 21:52 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov
Cc: Jongpil Jung, Furquan Shaikh, Rajat Jain, Evan Green,
linux-kernel, linux-input
Across suspend and resume, we are seeing error messages like the following:
atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
This occurs because the driver leaves its IRQ enabled. Upon resume, there
is an IRQ pending, but the interrupt is serviced before both the driver and
the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
Disable the IRQ in suspend, and re-enable it on resume. If there are cases
where the driver enters suspend with interrupts disabled, that's a bug we
should fix separately.
Signed-off-by: Evan Green <evgreen@chromium.org>
---
Changes in v2:
- Enable and disable unconditionally (Dmitry)
drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 24c4b691b1c9..a58092488679 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev)
mxt_stop(data);
mutex_unlock(&input_dev->mutex);
+ disable_irq(data->irq);
return 0;
}
@@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev)
mxt_start(data);
mutex_unlock(&input_dev->mutex);
+ enable_irq(data->irq);
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
2019-09-24 21:52 [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend Evan Green
@ 2019-09-28 0:16 ` Dmitry Torokhov
2019-09-28 0:16 ` Dmitry Torokhov
2019-09-30 15:29 ` Evan Green
0 siblings, 2 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2019-09-28 0:16 UTC (permalink / raw)
To: Evan Green
Cc: Nick Dyer, Jongpil Jung, Furquan Shaikh, Rajat Jain, linux-kernel,
linux-input
Hi Evan,
On Tue, Sep 24, 2019 at 02:52:38PM -0700, Evan Green wrote:
> Across suspend and resume, we are seeing error messages like the following:
>
> atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
> atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
>
> This occurs because the driver leaves its IRQ enabled. Upon resume, there
> is an IRQ pending, but the interrupt is serviced before both the driver and
> the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
>
> Disable the IRQ in suspend, and re-enable it on resume. If there are cases
> where the driver enters suspend with interrupts disabled, that's a bug we
> should fix separately.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
> - Enable and disable unconditionally (Dmitry)
>
> drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 24c4b691b1c9..a58092488679 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev)
> mxt_stop(data);
>
> mutex_unlock(&input_dev->mutex);
> + disable_irq(data->irq);
>
> return 0;
> }
> @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev)
> mxt_start(data);
>
> mutex_unlock(&input_dev->mutex);
> + enable_irq(data->irq);
At least for older devices that do soft reset on resume we need
interrupts to already work when we call mxt_start().
In general, order of resume steps should mirror suspend.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
2019-09-28 0:16 ` Dmitry Torokhov
@ 2019-09-28 0:16 ` Dmitry Torokhov
2019-09-30 15:29 ` Evan Green
1 sibling, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2019-09-28 0:16 UTC (permalink / raw)
To: Evan Green
Cc: Nick Dyer, Jongpil Jung, Furquan Shaikh, Rajat Jain, linux-kernel,
linux-input
Hi Evan,
On Tue, Sep 24, 2019 at 02:52:38PM -0700, Evan Green wrote:
> Across suspend and resume, we are seeing error messages like the following:
>
> atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
> atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
>
> This occurs because the driver leaves its IRQ enabled. Upon resume, there
> is an IRQ pending, but the interrupt is serviced before both the driver and
> the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
>
> Disable the IRQ in suspend, and re-enable it on resume. If there are cases
> where the driver enters suspend with interrupts disabled, that's a bug we
> should fix separately.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
> - Enable and disable unconditionally (Dmitry)
>
> drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 24c4b691b1c9..a58092488679 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev)
> mxt_stop(data);
>
> mutex_unlock(&input_dev->mutex);
> + disable_irq(data->irq);
>
> return 0;
> }
> @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev)
> mxt_start(data);
>
> mutex_unlock(&input_dev->mutex);
> + enable_irq(data->irq);
At least for older devices that do soft reset on resume we need
interrupts to already work when we call mxt_start().
In general, order of resume steps should mirror suspend.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
2019-09-28 0:16 ` Dmitry Torokhov
2019-09-28 0:16 ` Dmitry Torokhov
@ 2019-09-30 15:29 ` Evan Green
2019-09-30 15:29 ` Evan Green
1 sibling, 1 reply; 5+ messages in thread
From: Evan Green @ 2019-09-30 15:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Nick Dyer, Jongpil Jung, Furquan Shaikh, Rajat Jain, LKML,
linux-input
On Fri, Sep 27, 2019 at 5:16 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Evan,
>
> On Tue, Sep 24, 2019 at 02:52:38PM -0700, Evan Green wrote:
> > Across suspend and resume, we are seeing error messages like the following:
> >
> > atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
> > atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
> >
> > This occurs because the driver leaves its IRQ enabled. Upon resume, there
> > is an IRQ pending, but the interrupt is serviced before both the driver and
> > the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
> >
> > Disable the IRQ in suspend, and re-enable it on resume. If there are cases
> > where the driver enters suspend with interrupts disabled, that's a bug we
> > should fix separately.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Enable and disable unconditionally (Dmitry)
> >
> > drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 24c4b691b1c9..a58092488679 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev)
> > mxt_stop(data);
> >
> > mutex_unlock(&input_dev->mutex);
> > + disable_irq(data->irq);
> >
> > return 0;
> > }
> > @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev)
> > mxt_start(data);
> >
> > mutex_unlock(&input_dev->mutex);
> > + enable_irq(data->irq);
>
> At least for older devices that do soft reset on resume we need
> interrupts to already work when we call mxt_start().
>
> In general, order of resume steps should mirror suspend.
Ok, I can move the enable_irq up towards the top of resume. I was
worried that a pending IRQ might not get handled correctly if
mxt_start() hadn't been called yet. But if we need IRQs for
mxt_start() to run anyway, then I guess it should be handled.
-Evan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
2019-09-30 15:29 ` Evan Green
@ 2019-09-30 15:29 ` Evan Green
0 siblings, 0 replies; 5+ messages in thread
From: Evan Green @ 2019-09-30 15:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Nick Dyer, Jongpil Jung, Furquan Shaikh, Rajat Jain, LKML,
linux-input
On Fri, Sep 27, 2019 at 5:16 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Evan,
>
> On Tue, Sep 24, 2019 at 02:52:38PM -0700, Evan Green wrote:
> > Across suspend and resume, we are seeing error messages like the following:
> >
> > atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
> > atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
> >
> > This occurs because the driver leaves its IRQ enabled. Upon resume, there
> > is an IRQ pending, but the interrupt is serviced before both the driver and
> > the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
> >
> > Disable the IRQ in suspend, and re-enable it on resume. If there are cases
> > where the driver enters suspend with interrupts disabled, that's a bug we
> > should fix separately.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Enable and disable unconditionally (Dmitry)
> >
> > drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 24c4b691b1c9..a58092488679 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev)
> > mxt_stop(data);
> >
> > mutex_unlock(&input_dev->mutex);
> > + disable_irq(data->irq);
> >
> > return 0;
> > }
> > @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev)
> > mxt_start(data);
> >
> > mutex_unlock(&input_dev->mutex);
> > + enable_irq(data->irq);
>
> At least for older devices that do soft reset on resume we need
> interrupts to already work when we call mxt_start().
>
> In general, order of resume steps should mirror suspend.
Ok, I can move the enable_irq up towards the top of resume. I was
worried that a pending IRQ might not get handled correctly if
mxt_start() hadn't been called yet. But if we need IRQs for
mxt_start() to run anyway, then I guess it should be handled.
-Evan
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-30 15:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-24 21:52 [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend Evan Green
2019-09-28 0:16 ` Dmitry Torokhov
2019-09-28 0:16 ` Dmitry Torokhov
2019-09-30 15:29 ` Evan Green
2019-09-30 15:29 ` Evan Green
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).