[PATCH RESEND v3 0/2] Add a property to turn off the max touch controller in suspend mode

[PATCH RESEND v3 0/2] Add a property to turn off the max touch controller in suspend mode

[Stefan Eichenberger]

Our hardware has a shared regulator that powers various peripherals such
as the display, touch, USB hub, etc. Since the Maxtouch controller
doesn't currently allow it to be turned off, this regulator has to stay
on in suspend mode. This increases the overall power consumption. In
order to turn off the controller when the system goes into suspend mode,
this series adds a device tree property to the maxtouch driver that
allows the controller to be turned off completely and ensurs that it can
resume from the power off state.

Resend v3:
- Previously I messed up the series because send-email crashed. This is
  a resend of the original series:
  https://lore.kernel.org/linux-input/20240209084543.14726-1-eichest@gmail.com/T/#t
  https://lore.kernel.org/linux-input/20240209084818.14925-1-eichest@gmail.com/T/#u

Changes since v2:
- Add Reviewed-by tags from Linus and Krzysztof to the dt-bindings patch

Changes since v1:
- Rename the property and change the description (Krzysztof, Linus,
Dmitry, Conor)

Stefan Eichenberger (2):
  dt-bindings: input: atmel,maxtouch: add poweroff-sleep property
  Input: atmel_mxt_ts - support poweroff in suspend

 .../bindings/input/atmel,maxtouch.yaml        |  6 ++
 drivers/input/touchscreen/atmel_mxt_ts.c      | 72 ++++++++++++++-----
 2 files changed, 61 insertions(+), 17 deletions(-)

-- 
2.40.1

[PATCH RESEND v3 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-sleep property

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Add a new property to indicate that the device should power off rather
than use deep sleep. Deep sleep is a feature of the controller that
expects the controller to remain powered in suspend. However, if a
display shares its regulator with the touch controller, we may want to
do a power off so that the display and touch controller do not use any
power.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/input/atmel,maxtouch.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
index c40799355ed7..8de5f539b30e 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
@@ -87,6 +87,12 @@ properties:
       - 2 # ATMEL_MXT_WAKEUP_GPIO
     default: 0
 
+  atmel,poweroff-sleep:
+    description: |
+      Instead of using the deep sleep feature of the maXTouch controller,
+      poweroff the regulators.
+    type: boolean
+
   wakeup-source:
     type: boolean
 
-- 
2.40.1

[PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in suspend

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Add a new device tree property to indicate that the device should be
powered off in suspend mode. We have a shared regulator that powers the
display, a USB hub and some other peripherals. The maXTouch controller
doesn't normally disable the regulator in suspend mode, so our extra
peripherals stay powered on. This is not desirable as it consumes more
power. With this patch we add the option to disable the regulator in
suspend mode for the maXTouch and accept the longer initialisation time.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 72 ++++++++++++++++++------
 1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 542a31448c8f..2d5655385702 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -317,6 +317,7 @@ struct mxt_data {
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *wake_gpio;
 	bool use_retrigen_workaround;
+	bool poweroff_sleep;
 
 	/* Cached parameters from object table */
 	u16 T5_address;
@@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data,
 			dev_warn(dev, "Error %d updating config\n", error);
 	}
 
-	if (data->multitouch) {
-		error = mxt_initialize_input_device(data);
-		if (error)
-			return error;
-	} else {
-		dev_warn(dev, "No touch object detected\n");
-	}
+	/* If input device is not already registered */
+	if (!data->input_dev) {
+		if (data->multitouch) {
+			error = mxt_initialize_input_device(data);
+			if (error)
+				return error;
+		} else {
+			dev_warn(dev, "No touch object detected\n");
+		}
 
-	mxt_debug_init(data);
+		mxt_debug_init(data);
+	}
 
 	return 0;
 }
@@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client)
 		msleep(MXT_RESET_INVALID_CHG);
 	}
 
+	data->poweroff_sleep = device_property_read_bool(&client->dev,
+							 "atmel,poweroff-sleep");
 	/*
 	 * Controllers like mXT1386 have a dedicated WAKE line that could be
 	 * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
@@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev)
 	if (!input_dev)
 		return 0;
 
-	mutex_lock(&input_dev->mutex);
+	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
+		if (data->reset_gpio)
+			gpiod_set_value(data->reset_gpio, 1);
 
-	if (input_device_enabled(input_dev))
-		mxt_stop(data);
+		regulator_bulk_disable(ARRAY_SIZE(data->regulators),
+				data->regulators);
+		data->T44_address = 0;
+	} else {
+		mutex_lock(&input_dev->mutex);
+
+		if (input_device_enabled(input_dev))
+			mxt_stop(data);
 
-	mutex_unlock(&input_dev->mutex);
+		mutex_unlock(&input_dev->mutex);
+	}
 
 	disable_irq(data->irq);
 
@@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev)
 	if (!input_dev)
 		return 0;
 
-	enable_irq(data->irq);
+	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
+		int ret;
 
-	mutex_lock(&input_dev->mutex);
+		ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				data->regulators);
+		if (ret) {
+			dev_err(dev, "failed to enable regulators: %d\n",
+					ret);
+			return ret;
+		}
+		msleep(MXT_BACKUP_TIME);
 
-	if (input_device_enabled(input_dev))
-		mxt_start(data);
+		if (data->reset_gpio) {
+			/* Wait a while and then de-assert the RESET GPIO line */
+			msleep(MXT_RESET_GPIO_TIME);
+			gpiod_set_value(data->reset_gpio, 0);
+			msleep(MXT_RESET_INVALID_CHG);
+		}
 
-	mutex_unlock(&input_dev->mutex);
+		/* This also enables the irq again */
+		mxt_initialize(data);
+	} else {
+		enable_irq(data->irq);
+
+		mutex_lock(&input_dev->mutex);
+
+		if (input_device_enabled(input_dev))
+			mxt_start(data);
+
+		mutex_unlock(&input_dev->mutex);
+	}
 
 	return 0;
 }
-- 
2.40.1

Re: [PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in suspend

[Dmitry Torokhov]

Hi Stefan,

On Fri, Feb 09, 2024 at 11:50:12AM +0100, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Add a new device tree property to indicate that the device should be
> powered off in suspend mode. We have a shared regulator that powers the
> display, a USB hub and some other peripherals. The maXTouch controller
> doesn't normally disable the regulator in suspend mode, so our extra
> peripherals stay powered on. This is not desirable as it consumes more
> power. With this patch we add the option to disable the regulator in
> suspend mode for the maXTouch and accept the longer initialisation time.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 72 ++++++++++++++++++------
>  1 file changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 542a31448c8f..2d5655385702 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -317,6 +317,7 @@ struct mxt_data {
>  	struct gpio_desc *reset_gpio;
>  	struct gpio_desc *wake_gpio;
>  	bool use_retrigen_workaround;
> +	bool poweroff_sleep;
>  
>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data,
>  			dev_warn(dev, "Error %d updating config\n", error);
>  	}
>  
> -	if (data->multitouch) {
> -		error = mxt_initialize_input_device(data);
> -		if (error)
> -			return error;
> -	} else {
> -		dev_warn(dev, "No touch object detected\n");
> -	}
> +	/* If input device is not already registered */
> +	if (!data->input_dev) {
> +		if (data->multitouch) {
> +			error = mxt_initialize_input_device(data);
> +			if (error)
> +				return error;
> +		} else {
> +			dev_warn(dev, "No touch object detected\n");
> +		}
>  
> -	mxt_debug_init(data);
> +		mxt_debug_init(data);
> +	}
>  
>  	return 0;
>  }
> @@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client)
>  		msleep(MXT_RESET_INVALID_CHG);
>  	}
>  
> +	data->poweroff_sleep = device_property_read_bool(&client->dev,
> +							 "atmel,poweroff-sleep");
>  	/*
>  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
>  	 * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
> @@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev)
>  	if (!input_dev)
>  		return 0;
>  
> -	mutex_lock(&input_dev->mutex);
> +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> +		if (data->reset_gpio)
> +			gpiod_set_value(data->reset_gpio, 1);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_stop(data);
> +		regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> +				data->regulators);
> +		data->T44_address = 0;
> +	} else {
> +		mutex_lock(&input_dev->mutex);
> +
> +		if (input_device_enabled(input_dev))
> +			mxt_stop(data);
>  
> -	mutex_unlock(&input_dev->mutex);
> +		mutex_unlock(&input_dev->mutex);
> +	}

This all should go into mxt_stop(), so that if device is closed, or
inhibited, you power it down as well (if you can).

>  
>  	disable_irq(data->irq);
>  
> @@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev)
>  	if (!input_dev)
>  		return 0;
>  
> -	enable_irq(data->irq);
> +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> +		int ret;
>  
> -	mutex_lock(&input_dev->mutex);
> +		ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				data->regulators);
> +		if (ret) {
> +			dev_err(dev, "failed to enable regulators: %d\n",
> +					ret);
> +			return ret;
> +		}
> +		msleep(MXT_BACKUP_TIME);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_start(data);
> +		if (data->reset_gpio) {
> +			/* Wait a while and then de-assert the RESET GPIO line */
> +			msleep(MXT_RESET_GPIO_TIME);
> +			gpiod_set_value(data->reset_gpio, 0);
> +			msleep(MXT_RESET_INVALID_CHG);
> +		}
>  
> -	mutex_unlock(&input_dev->mutex);
> +		/* This also enables the irq again */
> +		mxt_initialize(data);

And this needs to go into mxt_start(). Also, we should make sure that
once resume operation completes the device is fully operational. That
means you should not request the firmware asynchronously in
mxt_initialize() in case you are in the resume path. I think you should
also unwind mxt_initialize() and mxt_configure_objects() to make it
clear what is the part of initial initialization and what is part of
re-initializing during resume. The configuration that is exposed to
userspace (resolution, number of objects, other properties) should stay
the same, the configuration of the chip itself (power mode, etc) should
be restored.

Thanks.

-- 
Dmitry

Re: [PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in suspend

[Stefan Eichenberger]

Hi Dmitry,

Thanks for the feedback, I had a first look at the changes and I'm not
sure if we would break some use cases. Therfore, here some questions.

On Fri, Mar 15, 2024 at 11:51:19AM -0700, Dmitry Torokhov wrote:
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 542a31448c8f..2d5655385702 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -317,6 +317,7 @@ struct mxt_data {
> >  	struct gpio_desc *reset_gpio;
> >  	struct gpio_desc *wake_gpio;
> >  	bool use_retrigen_workaround;
> > +	bool poweroff_sleep;
> >  
> >  	/* Cached parameters from object table */
> >  	u16 T5_address;
> > @@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data,
> >  			dev_warn(dev, "Error %d updating config\n", error);
> >  	}
> >  
> > -	if (data->multitouch) {
> > -		error = mxt_initialize_input_device(data);
> > -		if (error)
> > -			return error;
> > -	} else {
> > -		dev_warn(dev, "No touch object detected\n");
> > -	}
> > +	/* If input device is not already registered */
> > +	if (!data->input_dev) {
> > +		if (data->multitouch) {
> > +			error = mxt_initialize_input_device(data);
> > +			if (error)
> > +				return error;
> > +		} else {
> > +			dev_warn(dev, "No touch object detected\n");
> > +		}
> >  
> > -	mxt_debug_init(data);
> > +		mxt_debug_init(data);
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client)
> >  		msleep(MXT_RESET_INVALID_CHG);
> >  	}
> >  
> > +	data->poweroff_sleep = device_property_read_bool(&client->dev,
> > +							 "atmel,poweroff-sleep");
> >  	/*
> >  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
> >  	 * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
> > @@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev)
> >  	if (!input_dev)
> >  		return 0;
> >  
> > -	mutex_lock(&input_dev->mutex);
> > +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> > +		if (data->reset_gpio)
> > +			gpiod_set_value(data->reset_gpio, 1);
> >  
> > -	if (input_device_enabled(input_dev))
> > -		mxt_stop(data);
> > +		regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > +				data->regulators);
> > +		data->T44_address = 0;
> > +	} else {
> > +		mutex_lock(&input_dev->mutex);
> > +
> > +		if (input_device_enabled(input_dev))
> > +			mxt_stop(data);
> >  
> > -	mutex_unlock(&input_dev->mutex);
> > +		mutex_unlock(&input_dev->mutex);
> > +	}
> 
> This all should go into mxt_stop(), so that if device is closed, or
> inhibited, you power it down as well (if you can).

We would then have to power it up during probe to see if the device is
threre, read the configuration and power it down again afterwards until
the device is opened. If the device is in bootloader mode we would most
likely have to keep the power on all the time and never turn it off,
right?

> 
> >  
> >  	disable_irq(data->irq);
> >  
> > @@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev)
> >  	if (!input_dev)
> >  		return 0;
> >  
> > -	enable_irq(data->irq);
> > +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> > +		int ret;
> >  
> > -	mutex_lock(&input_dev->mutex);
> > +		ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > +				data->regulators);
> > +		if (ret) {
> > +			dev_err(dev, "failed to enable regulators: %d\n",
> > +					ret);
> > +			return ret;
> > +		}
> > +		msleep(MXT_BACKUP_TIME);
> >  
> > -	if (input_device_enabled(input_dev))
> > -		mxt_start(data);
> > +		if (data->reset_gpio) {
> > +			/* Wait a while and then de-assert the RESET GPIO line */
> > +			msleep(MXT_RESET_GPIO_TIME);
> > +			gpiod_set_value(data->reset_gpio, 0);
> > +			msleep(MXT_RESET_INVALID_CHG);
> > +		}
> >  
> > -	mutex_unlock(&input_dev->mutex);
> > +		/* This also enables the irq again */
> > +		mxt_initialize(data);
> 
> And this needs to go into mxt_start(). Also, we should make sure that
> once resume operation completes the device is fully operational. That
> means you should not request the firmware asynchronously in
> mxt_initialize() in case you are in the resume path. I think you should
> also unwind mxt_initialize() and mxt_configure_objects() to make it
> clear what is the part of initial initialization and what is part of
> re-initializing during resume. The configuration that is exposed to
> userspace (resolution, number of objects, other properties) should stay
> the same, the configuration of the chip itself (power mode, etc) should
> be restored.

Here we would most likely have to load the firmware (configuration)
synchronously all the time if the poweroff_sleep flag is set. Ths makes
sure that the device is ready when we open the device. Would this delay
be acceptable when opening the input device? Normally the configuration
is not that big and should load quite fast. 

Regards,
Stefan