[PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support

[PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support

[Loic Poulain]

Add capability for the touchscreen to wakeup the host on touch event.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 05de92c..807f449 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3223,6 +3223,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return error;
 	}
 
+	device_set_wakeup_capable(&client->dev, true);
+
 	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
 				      data->regulators);
 	if (error) {
@@ -3309,8 +3311,12 @@ static int __maybe_unused mxt_suspend(struct device *dev)
 
 	mutex_lock(&input_dev->mutex);
 
-	if (input_device_enabled(input_dev))
-		mxt_stop(data);
+	if (input_device_enabled(input_dev)) {
+		if (device_may_wakeup(dev))
+			enable_irq_wake(data->irq);
+		else
+			mxt_stop(data);
+	}
 
 	mutex_unlock(&input_dev->mutex);
 
@@ -3332,8 +3338,12 @@ static int __maybe_unused mxt_resume(struct device *dev)
 
 	mutex_lock(&input_dev->mutex);
 
-	if (input_device_enabled(input_dev))
-		mxt_start(data);
+	if (input_device_enabled(input_dev)) {
+		if (device_may_wakeup(dev))
+			disable_irq_wake(data->irq);
+		else
+			mxt_start(data);
+	}
 
 	mutex_unlock(&input_dev->mutex);
 
-- 
2.7.4

[PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss

[Loic Poulain]

If both touch events and release are part of the same report,
userspace will not consider it as a touch-down & touch-up but as
a non-action. That can happen on resume when 'buffered' events are
dequeued in a row.

Make sure that release always causes previous events to be synced
before being reported.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 807f449..e05ec30 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
 		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
 		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
 	} else {
+		/*
+		 * Always sync input before reporting release, to be sure
+		 * previous event(s) are taking into account by user side.
+		 */
+		if (data->update_input)
+			mxt_input_sync(data);
+
 		dev_dbg(dev, "[%u] release\n", id);
 
 		/* close out slot */
-- 
2.7.4

Re: [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support

[Dmitry Torokhov]

Hi Loic,

On Wed, Jun 23, 2021 at 03:56:36PM +0200, Loic Poulain wrote:
> Add capability for the touchscreen to wakeup the host on touch event.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 05de92c..807f449 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3223,6 +3223,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return error;
>  	}
>  
> +	device_set_wakeup_capable(&client->dev, true);

We do not want to make the touch controller be wakeup source
unconditionally. I2C core recognized "wakeup-source" in device tree,
other platforms may employ different techniques setting I2C_CLIENT_WAKE
when registering I2C devices to mark them as wakeup capable/enabled.

> +
>  	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
>  				      data->regulators);
>  	if (error) {
> @@ -3309,8 +3311,12 @@ static int __maybe_unused mxt_suspend(struct device *dev)
>  
>  	mutex_lock(&input_dev->mutex);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_stop(data);
> +	if (input_device_enabled(input_dev)) {
> +		if (device_may_wakeup(dev))
> +			enable_irq_wake(data->irq);

For devices that are registered as I2C_CLIENT_WAKE i2c core ensures that
their interrupts are configured for wakeup when system transitions to
sleep state, so you do not need to call enable_irq_wake() and
disable_irq_wake().

You also need to make sure the controller is powered up when it is
configured for wakeup.

> +		else
> +			mxt_stop(data);
> +	}
>  
>  	mutex_unlock(&input_dev->mutex);
>  
> @@ -3332,8 +3338,12 @@ static int __maybe_unused mxt_resume(struct device *dev)
>  
>  	mutex_lock(&input_dev->mutex);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_start(data);
> +	if (input_device_enabled(input_dev)) {
> +		if (device_may_wakeup(dev))
> +			disable_irq_wake(data->irq);
> +		else
> +			mxt_start(data);
> +	}
>  
>  	mutex_unlock(&input_dev->mutex);
>  
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss

[Dmitry Torokhov]

On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> If both touch events and release are part of the same report,
> userspace will not consider it as a touch-down & touch-up but as
> a non-action. That can happen on resume when 'buffered' events are
> dequeued in a row.
> 
> Make sure that release always causes previous events to be synced
> before being reported.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 807f449..e05ec30 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
>  		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
>  		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
>  	} else {
> +		/*
> +		 * Always sync input before reporting release, to be sure
> +		 * previous event(s) are taking into account by user side.
> +		 */
> +		if (data->update_input)
> +			mxt_input_sync(data);

That means we sync for every contact release, whereas I think ideal
would be to only sync when we observe touch-down and touch-up in the
same slot.

Let's also add Peter to the conversation...

> +
>  		dev_dbg(dev, "[%u] release\n", id);
>  
>  		/* close out slot */
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss

[Peter Hutterer]

On Wed, Jun 23, 2021 at 05:48:49PM -0700, Dmitry Torokhov wrote:
> On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> > If both touch events and release are part of the same report,
> > userspace will not consider it as a touch-down & touch-up but as
> > a non-action. That can happen on resume when 'buffered' events are
> > dequeued in a row.
> > 
> > Make sure that release always causes previous events to be synced
> > before being reported.
> > 
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 807f449..e05ec30 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
> >  		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
> >  		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
> >  	} else {
> > +		/*
> > +		 * Always sync input before reporting release, to be sure
> > +		 * previous event(s) are taking into account by user side.
> > +		 */
> > +		if (data->update_input)
> > +			mxt_input_sync(data);
> 
> That means we sync for every contact release, whereas I think ideal
> would be to only sync when we observe touch-down and touch-up in the
> same slot.
> 
> Let's also add Peter to the conversation...

Thanks for the CC.

FTR, this is expected userspace behaviour, the device state is only looked
at during SYN_REPORT. Where you send event E=1 and E=0 in the same frame,
the state at SYN_REPORT time is 0, the 1 never happened.

The only device we (as in: libinput) make an exception for here are
keyboards because too many drivers get it wrong and it's too hard to fix all
of them. But especially for touch devices (and tablets!) we don't really
have any choice but to look at the state of the device at the end of the
frame.

So, yes, this patch is needed but I agree with Dmitry that you should only
send this for the special case that requires it.

Cheers,
   Peter

Re: [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support

[Loic Poulain]

Hi Dmitry,

On Thu, 24 Jun 2021 at 02:41, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> Hi Loic,
>
> On Wed, Jun 23, 2021 at 03:56:36PM +0200, Loic Poulain wrote:
> > Add capability for the touchscreen to wakeup the host on touch event.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 05de92c..807f449 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -3223,6 +3223,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >               return error;
> >       }
> >
> > +     device_set_wakeup_capable(&client->dev, true);
>
> We do not want to make the touch controller be wakeup source
> unconditionally. I2C core recognized "wakeup-source" in device tree,
> other platforms may employ different techniques setting I2C_CLIENT_WAKE
> when registering I2C devices to mark them as wakeup capable/enabled.

Contrary to device_init_wakeup(), used in some other input drivers,
device_set_wakeup_capable() does not enable the device as a wakeup
source but just sets it as wakeup capable, and it's up to the user or
distro policy to enable it as a wakeup source or not. It's a quite
common way to do, and it does not change the behavior of this driver.
The I2C_CLIENT_WAKE forces enabling wakeup source, which is maybe not
what we want by default for a touchscreen. remote-wakeup enabling is a
device configuration not a hardware property. Thoughts?

I should probably also add dev_pm_set_wake_irq() for auto-enabling
wake on suspend instead of doing it manually.

Regards,
Loic

Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss

[Loic Poulain]

Hi Peter, Dmitry,

On Thu, 24 Jun 2021 at 07:58, Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Wed, Jun 23, 2021 at 05:48:49PM -0700, Dmitry Torokhov wrote:
> > On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> > > If both touch events and release are part of the same report,
> > > userspace will not consider it as a touch-down & touch-up but as
> > > a non-action. That can happen on resume when 'buffered' events are
> > > dequeued in a row.
> > >
> > > Make sure that release always causes previous events to be synced
> > > before being reported.
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > ---
> > >  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > index 807f449..e05ec30 100644
> > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
> > >             input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
> > >             input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
> > >     } else {
> > > +           /*
> > > +            * Always sync input before reporting release, to be sure
> > > +            * previous event(s) are taking into account by user side.
> > > +            */
> > > +           if (data->update_input)
> > > +                   mxt_input_sync(data);
> >
> > That means we sync for every contact release, whereas I think ideal
> > would be to only sync when we observe touch-down and touch-up in the
> > same slot.
> >
> > Let's also add Peter to the conversation...
>
> Thanks for the CC.
>
> FTR, this is expected userspace behaviour, the device state is only looked
> at during SYN_REPORT. Where you send event E=1 and E=0 in the same frame,
> the state at SYN_REPORT time is 0, the 1 never happened.
>
> The only device we (as in: libinput) make an exception for here are
> keyboards because too many drivers get it wrong and it's too hard to fix all
> of them. But especially for touch devices (and tablets!) we don't really
> have any choice but to look at the state of the device at the end of the
> frame.
>
> So, yes, this patch is needed but I agree with Dmitry that you should only
> send this for the special case that requires it.

I'm not really familiar with the input framework, so thanks for the
clarification. In that patch _sync() is 'forced' if there is any
previous event in the report, but from what you say, I should only
sync if one of the previous events is a touch-down, so a transition
E=1? right?

Regards,
Loic