diff mbox series

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

Message ID 1624456597-9486-1-git-send-email-loic.poulain@linaro.org (mailing list archive)
State New
Headers show
Series [1/2] Input: atmel_mxt_ts: Add wake-up support | expand

Commit Message

Loic Poulain June 23, 2021, 1:56 p.m. UTC
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(-)

Comments

Dmitry Torokhov June 24, 2021, 12:41 a.m. UTC | #1
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.
Loic Poulain June 24, 2021, 7:56 a.m. UTC | #2
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
diff mbox series

Patch

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);