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

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

[Stefan Eichenberger]

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

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.

Stefan Eichenberger (2):
  dt-bindings: input: atmel,maxtouch: add power-off-in-suspend property
  Input: atmel_mxt_ts - support power off 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 v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Stefan Eichenberger]

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

Add a new property to indicate that the device should be powered off in
suspend mode.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 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..047a5101341c 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-in-suspend:
+    description: |
+      When this property is set, all supplies are turned off when the system is
+      going to suspend.
+    type: boolean
+
   wakeup-source:
     type: boolean
 
-- 
2.40.1

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Krzysztof Kozlowski]

On 07/12/2023 12:12, Stefan Eichenberger wrote:
> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
> index c40799355ed7..047a5101341c 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-in-suspend:
> +    description: |
> +      When this property is set, all supplies are turned off when the system is
> +      going to suspend.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Stefan Eichenberger]

Hi Krzysztof,

On Thu, Dec 07, 2023 at 05:59:08PM +0100, Krzysztof Kozlowski wrote:
> On 07/12/2023 12:12, Stefan Eichenberger wrote:
> > diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
> > index c40799355ed7..047a5101341c 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-in-suspend:
> > +    description: |
> > +      When this property is set, all supplies are turned off when the system is
> > +      going to suspend.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.

Thanks a lot for the feedback. Would the following be okay as a
description?

vdda and vdd are switched off in suspend mode.

Best regards,
Stefan

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Krzysztof Kozlowski]

On 08/12/2023 13:37, Stefan Eichenberger wrote:
> Hi Krzysztof,
> 
> On Thu, Dec 07, 2023 at 05:59:08PM +0100, Krzysztof Kozlowski wrote:
>> On 07/12/2023 12:12, Stefan Eichenberger wrote:
>>> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
>>> index c40799355ed7..047a5101341c 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-in-suspend:
>>> +    description: |
>>> +      When this property is set, all supplies are turned off when the system is
>>> +      going to suspend.
>>
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
> 
> Thanks a lot for the feedback. Would the following be okay as a
> description?
> 
> vdda and vdd are switched off in suspend mode.

Are switched by who? Linux driver? Then nothing changed...

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Linus Walleij]

On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:

> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> Add a new property to indicate that the device should be powered off in
> suspend mode.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
(...)
> +  atmel,poweroff-in-suspend:
> +    description: |
> +      When this property is set, all supplies are turned off when the system is
> +      going to suspend.
> +    type: boolean
   wakeup-source:
     type: boolean

As Krzysztof says it seems you are describing an operating system feature.

I can't help but wonder: shouldn't that pretty much be the default behaviour
if wakeup-source is *not* specified?

I.e. the property kind of describes !wakeup-source.

Yours,
Linus Walleij

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Stefan Eichenberger]

Hi Krzysztof and Linux,

On Fri, Dec 08, 2023 at 01:54:21PM +0100, Linus Walleij wrote:
> On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> 
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > Add a new property to indicate that the device should be powered off in
> > suspend mode.
> >
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> (...)
> > +  atmel,poweroff-in-suspend:
> > +    description: |
> > +      When this property is set, all supplies are turned off when the system is
> > +      going to suspend.
> > +    type: boolean
>    wakeup-source:
>      type: boolean
> 
> As Krzysztof says it seems you are describing an operating system feature.
> 
> I can't help but wonder: shouldn't that pretty much be the default behaviour
> if wakeup-source is *not* specified?
> 
> I.e. the property kind of describes !wakeup-source.

The maxtouch controller has a deep sleep mode which is currently used
without powering down vdd and vdda. However, because we have a shared
regulator which powers other peripherals that we want to shut down in
suspend we need a way to power down vdd and vdda. However, I agree this
is not really a feature of the device. The feature would basically be
the internal deep sleep mode. I didn't want to change the default
behaviour of the driver, so I added this property but maybe I could
change it to:

atmel,deep-sleep:
  description: |
     Use the maxtouch deep-sleep mode instead of powering down vdd and
     vdda.

Or to not change the default behaviour:
atmel,no-deep-sleep:
  description: |
     Do not use the maxtouch deep-sleep mode but power down vdd and vdda
     in suspend.

As I understand the datasheet even if the maxtouch is using its deep
sleep mode it does not act as a wakeup source. It is just faster in
waking up because it can keep the configuration in memory.

Regards,
Stefan

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Linus Walleij]

On Fri, Dec 8, 2023 at 2:11 PM Stefan Eichenberger <eichest@gmail.com> wrote:

> > I can't help but wonder: shouldn't that pretty much be the default behaviour
> > if wakeup-source is *not* specified?
> >
> > I.e. the property kind of describes !wakeup-source.
>
> The maxtouch controller has a deep sleep mode which is currently used
> without powering down vdd and vdda. However, because we have a shared
> regulator which powers other peripherals that we want to shut down in
> suspend we need a way to power down vdd and vdda. However, I agree this
> is not really a feature of the device. The feature would basically be
> the internal deep sleep mode.

While it is of no concern to the device tree bindings, Linux regulators
are counting meaning that you need to make all peripherals disable
their regulators and it will come down.

> I didn't want to change the default
> behaviour of the driver, so I added this property but maybe I could
> change it to:
>
> atmel,deep-sleep:
>   description: |
>      Use the maxtouch deep-sleep mode instead of powering down vdd and
>      vdda.
>
> Or to not change the default behaviour:
> atmel,no-deep-sleep:
>   description: |
>      Do not use the maxtouch deep-sleep mode but power down vdd and vdda
>      in suspend.
>
> As I understand the datasheet even if the maxtouch is using its deep
> sleep mode it does not act as a wakeup source.

Do you mean it can still work as a wakeup source in deep sleep mode?
(there is a "not" too much above ...)

> It is just faster in
> waking up because it can keep the configuration in memory.

That sounds like a good reason to have the property, because that
means that if you can control the wakeup latency and specify in the binding
how much in absolute time units it is affected.

I would define it in positive terms instead of reverse "no-deep-sleep"
though such as "atmel,fast-wakeup".

And: If you disable the regulators it will probably *not* be able to wake the
system up, right? And that is just a few lines of code in the driver such as:

go_to_sleep():
  if (!wakeup_source):
     disable_regulators()

Yours,
Linus Walleij

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Stefan Eichenberger]

Hi Linus,

On Fri, Dec 08, 2023 at 03:23:54PM +0100, Linus Walleij wrote:
> On Fri, Dec 8, 2023 at 2:11 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> 
> > > I can't help but wonder: shouldn't that pretty much be the default behaviour
> > > if wakeup-source is *not* specified?
> > >
> > > I.e. the property kind of describes !wakeup-source.
> >
> > The maxtouch controller has a deep sleep mode which is currently used
> > without powering down vdd and vdda. However, because we have a shared
> > regulator which powers other peripherals that we want to shut down in
> > suspend we need a way to power down vdd and vdda. However, I agree this
> > is not really a feature of the device. The feature would basically be
> > the internal deep sleep mode.
> 
> While it is of no concern to the device tree bindings, Linux regulators
> are counting meaning that you need to make all peripherals disable
> their regulators and it will come down.

Yes we are working on that one. This is the last driver we need to allow
disabling the regulator, afterward it should hoepfully work on our
target system.

> 
> > I didn't want to change the default
> > behaviour of the driver, so I added this property but maybe I could
> > change it to:
> >
> > atmel,deep-sleep:
> >   description: |
> >      Use the maxtouch deep-sleep mode instead of powering down vdd and
> >      vdda.
> >
> > Or to not change the default behaviour:
> > atmel,no-deep-sleep:
> >   description: |
> >      Do not use the maxtouch deep-sleep mode but power down vdd and vdda
> >      in suspend.
> >
> > As I understand the datasheet even if the maxtouch is using its deep
> > sleep mode it does not act as a wakeup source.
> 
> Do you mean it can still work as a wakeup source in deep sleep mode?
> (there is a "not" too much above ...)

Sorry for the confusion. As it is configured now, it can not work as
wakeup source in deep sleep mode. Touch is completely disabled.

> > It is just faster in
> > waking up because it can keep the configuration in memory.
> 
> That sounds like a good reason to have the property, because that
> means that if you can control the wakeup latency and specify in the binding
> how much in absolute time units it is affected.
> 
> I would define it in positive terms instead of reverse "no-deep-sleep"
> though such as "atmel,fast-wakeup".
> 
> And: If you disable the regulators it will probably *not* be able to wake the
> system up, right? And that is just a few lines of code in the driver such as:
> 
> go_to_sleep():
>   if (!wakeup_source):
>      disable_regulators()

So to not change the default behaviour I would have to name it:
atmel,slow-wakeup
or maybe even full wakeup because it does a wakeup from the disabled
state?
atmel,full-wakeup

Exactly, the added code looks more or less exactly as you wrote. And on
resume it does the opposite + configure it:

resume():
  if (!wakeup_source):
     enable_regulators()
     configure_maxtouch()

Regards,
Stefan

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Dmitry Torokhov]

Hi Linus, Krzysztof,

On Fri, Dec 08, 2023 at 01:54:21PM +0100, Linus Walleij wrote:
> On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> 
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > Add a new property to indicate that the device should be powered off in
> > suspend mode.
> >
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> (...)
> > +  atmel,poweroff-in-suspend:
> > +    description: |
> > +      When this property is set, all supplies are turned off when the system is
> > +      going to suspend.
> > +    type: boolean
>    wakeup-source:
>      type: boolean
> 
> As Krzysztof says it seems you are describing an operating system feature.

It appears to be an OS feature, but I would argue that it is also a
property of a board. It is tempting to say that if DTS defines supplies
for the controller we should use them to power off the controller in
suspend, otherwise we should use the deep sleep functionality of the
controller. But a mere presence of regulators does not indicate if they
can actually be powered off in suspend (i.e. if controllers shares power
rails with another device that can be a wakeup source), so we need to
have additional hints on how OS should behave on a given device.

On top of that we have regulator framework supplying dummy regulators...

Thanks.

-- 
Dmitry

Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Fri, Dec 08, 2023 at 11:37:47PM +0000, Dmitry Torokhov wrote:
> Hi Linus, Krzysztof,
> 
> On Fri, Dec 08, 2023 at 01:54:21PM +0100, Linus Walleij wrote:
> > On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > 
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > >
> > > Add a new property to indicate that the device should be powered off in
> > > suspend mode.
> > >
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > (...)
> > > +  atmel,poweroff-in-suspend:
> > > +    description: |
> > > +      When this property is set, all supplies are turned off when the system is
> > > +      going to suspend.
> > > +    type: boolean
> >    wakeup-source:
> >      type: boolean
> > 
> > As Krzysztof says it seems you are describing an operating system feature.
> 
> It appears to be an OS feature, but I would argue that it is also a
> property of a board. It is tempting to say that if DTS defines supplies
> for the controller we should use them to power off the controller in
> suspend, otherwise we should use the deep sleep functionality of the
> controller. But a mere presence of regulators does not indicate if they
> can actually be powered off in suspend (i.e. if controllers shares power
> rails with another device that can be a wakeup source), so we need to
> have additional hints on how OS should behave on a given device.
> 
> On top of that we have regulator framework supplying dummy regulators...

Simply rephrasing the property might be sufficient? The current one
sounds making policy decisions with the "should be". Reframing the
commit message and property description etc in terms of what aspect of
the hardware the ability to turn off all supplies in suspend comes from
would make it more acceptable. Pretty much answering the question "why
can't we try and turn off all supplies on all systems with this device"
should get things rolling.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 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 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 20094b9899f0..7caa0d317d30 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_in_suspend;
 
 	/* 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;
 }
@@ -3328,6 +3332,8 @@ static int mxt_probe(struct i2c_client *client)
 		msleep(MXT_RESET_INVALID_CHG);
 	}
 
+	data->poweroff_in_suspend = device_property_read_bool(&client->dev,
+							       "atmel,poweroff-in-suspend");
 	/*
 	 * Controllers like mXT1386 have a dedicated WAKE line that could be
 	 * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
@@ -3390,12 +3396,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_in_suspend) {
+		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);
 
@@ -3411,14 +3426,37 @@ static int mxt_resume(struct device *dev)
 	if (!input_dev)
 		return 0;
 
-	enable_irq(data->irq);
+	if (!device_may_wakeup(dev) && data->poweroff_in_suspend) {
+		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