* [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support @ 2021-06-23 13:56 Loic Poulain 2021-06-23 13:56 ` [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss Loic Poulain 2021-06-24 0:41 ` [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Dmitry Torokhov 0 siblings, 2 replies; 7+ messages in thread From: Loic Poulain @ 2021-06-23 13:56 UTC (permalink / raw) To: nick, dmitry.torokhov; +Cc: linux-input, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss 2021-06-23 13:56 [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Loic Poulain @ 2021-06-23 13:56 ` Loic Poulain 2021-06-24 0:48 ` Dmitry Torokhov 2021-06-24 0:41 ` [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Dmitry Torokhov 1 sibling, 1 reply; 7+ messages in thread From: Loic Poulain @ 2021-06-23 13:56 UTC (permalink / raw) To: nick, dmitry.torokhov; +Cc: linux-input, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss 2021-06-23 13:56 ` [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss Loic Poulain @ 2021-06-24 0:48 ` Dmitry Torokhov 2021-06-24 5:57 ` Peter Hutterer 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2021-06-24 0:48 UTC (permalink / raw) To: Loic Poulain, Peter Hutterer; +Cc: nick, linux-input 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss 2021-06-24 0:48 ` Dmitry Torokhov @ 2021-06-24 5:57 ` Peter Hutterer 2021-06-24 8:05 ` Loic Poulain 0 siblings, 1 reply; 7+ messages in thread From: Peter Hutterer @ 2021-06-24 5:57 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Loic Poulain, nick, linux-input 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss 2021-06-24 5:57 ` Peter Hutterer @ 2021-06-24 8:05 ` Loic Poulain 0 siblings, 0 replies; 7+ messages in thread From: Loic Poulain @ 2021-06-24 8:05 UTC (permalink / raw) To: Peter Hutterer; +Cc: Dmitry Torokhov, nick, linux-input 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support 2021-06-23 13:56 [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Loic Poulain 2021-06-23 13:56 ` [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss Loic Poulain @ 2021-06-24 0:41 ` Dmitry Torokhov 2021-06-24 7:56 ` Loic Poulain 1 sibling, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2021-06-24 0:41 UTC (permalink / raw) To: Loic Poulain; +Cc: nick, linux-input 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support 2021-06-24 0:41 ` [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Dmitry Torokhov @ 2021-06-24 7:56 ` Loic Poulain 0 siblings, 0 replies; 7+ messages in thread From: Loic Poulain @ 2021-06-24 7:56 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: nick, linux-input 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-24 7:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-23 13:56 [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Loic Poulain 2021-06-23 13:56 ` [PATCH 2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss Loic Poulain 2021-06-24 0:48 ` Dmitry Torokhov 2021-06-24 5:57 ` Peter Hutterer 2021-06-24 8:05 ` Loic Poulain 2021-06-24 0:41 ` [PATCH 1/2] Input: atmel_mxt_ts: Add wake-up support Dmitry Torokhov 2021-06-24 7:56 ` Loic Poulain
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).