[PATCH v4 0/4] Add a property to turn off the max touch controller if not used

[PATCH v4 0/4] Add a property to turn off the max touch controller if not used

[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 when not used. This increases the overall power consumption. In order
to turn off the controller when the system does not use it, 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.

Changes since v3:
- Move the power on part to mxt_start and the power off part to
  mxt_stop. This allows to turn the touch controller off even when not
  in use and not only when being suspended (Dmitry)

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 (4):
  Input: atmel_mxt_ts - add power off and power on functions
  Input: atmel_mxt_ts - move calls to register the input device to
    separate function
  dt-bindings: input: atmel,maxtouch: add poweroff-sleep property
  Input: atmel_mxt_ts - add support for poweroff-sleep

 .../bindings/input/atmel,maxtouch.yaml        |   6 +
 drivers/input/touchscreen/atmel_mxt_ts.c      | 162 +++++++++++++-----
 2 files changed, 124 insertions(+), 44 deletions(-)

-- 
2.40.1

[PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions

[Stefan Eichenberger]

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

Add a separate function for power off and power on instead of calling
regulator_bulk_enable and regulator_bulk_disable directly.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 542a31448c8f..52867ce3b9b6 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1307,6 +1307,38 @@ static int mxt_soft_reset(struct mxt_data *data)
 	return 0;
 }
 
+static int mxt_power_on(struct mxt_data *data)
+{
+	int error;
+
+	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (error) {
+		dev_err(&data->client->dev, "failed to enable regulators: %d\n",
+			error);
+		return error;
+	}
+
+	msleep(MXT_BACKUP_TIME);
+
+	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);
+	}
+
+	return 0;
+}
+
+static void mxt_power_off(struct mxt_data *data)
+{
+	if (data->reset_gpio)
+		gpiod_set_value(data->reset_gpio, 1);
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
 static void mxt_update_crc(struct mxt_data *data, u8 cmd, u8 value)
 {
 	/*
@@ -3305,25 +3337,9 @@ static int mxt_probe(struct i2c_client *client)
 		return error;
 	}
 
-	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
-				      data->regulators);
-	if (error) {
-		dev_err(&client->dev, "failed to enable regulators: %d\n",
-			error);
+	error = mxt_power_on(data);
+	if (error)
 		return error;
-	}
-	/*
-	 * The device takes 40ms to come up after power-on according
-	 * to the mXT224 datasheet, page 13.
-	 */
-	msleep(MXT_BACKUP_TIME);
-
-	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);
-	}
 
 	/*
 	 * Controllers like mXT1386 have a dedicated WAKE line that could be
@@ -3361,8 +3377,8 @@ static int mxt_probe(struct i2c_client *client)
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
 err_disable_regulators:
-	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
-			       data->regulators);
+	mxt_power_off(data);
+
 	return error;
 }
 
@@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
-	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
-			       data->regulators);
+	mxt_power_off(data);
 }
 
 static int mxt_suspend(struct device *dev)
-- 
2.40.1

Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions

[Dmitry Torokhov]

Hi Stefan,

On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Add a separate function for power off and power on instead of calling
> regulator_bulk_enable and regulator_bulk_disable directly.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 59 +++++++++++++++---------
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 542a31448c8f..52867ce3b9b6 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1307,6 +1307,38 @@ static int mxt_soft_reset(struct mxt_data *data)
>  	return 0;
>  }
>  
> +static int mxt_power_on(struct mxt_data *data)
> +{
> +	int error;
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&data->client->dev, "failed to enable regulators: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	msleep(MXT_BACKUP_TIME);
> +
> +	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);
> +	}
> +
> +	return 0;
> +}
> +
> +static void mxt_power_off(struct mxt_data *data)
> +{
> +	if (data->reset_gpio)
> +		gpiod_set_value(data->reset_gpio, 1);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}
> +
>  static void mxt_update_crc(struct mxt_data *data, u8 cmd, u8 value)
>  {
>  	/*
> @@ -3305,25 +3337,9 @@ static int mxt_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> -	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> -				      data->regulators);
> -	if (error) {
> -		dev_err(&client->dev, "failed to enable regulators: %d\n",
> -			error);
> +	error = mxt_power_on(data);
> +	if (error)
>  		return error;
> -	}
> -	/*
> -	 * The device takes 40ms to come up after power-on according
> -	 * to the mXT224 datasheet, page 13.
> -	 */
> -	msleep(MXT_BACKUP_TIME);
> -
> -	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);
> -	}
>  
>  	/*
>  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
> @@ -3361,8 +3377,8 @@ static int mxt_probe(struct i2c_client *client)
>  	mxt_free_input_device(data);
>  	mxt_free_object_table(data);
>  err_disable_regulators:
> -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> -			       data->regulators);
> +	mxt_power_off(data);
> +
>  	return error;
>  }
>  
> @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
>  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
>  	mxt_free_input_device(data);
>  	mxt_free_object_table(data);
> -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> -			       data->regulators);
> +	mxt_power_off(data);

This change means that on unbind we will leave with GPIO line asserted.
Won't this potentially cause some current leakage? Why do we need to
have reset asserted here?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions

[Stefan Eichenberger]

Hi Dmitry

On Thu, Jun 20, 2024 at 08:37:40AM -0700, Dmitry Torokhov wrote:
> Hi Stefan,
> 
> On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > Add a separate function for power off and power on instead of calling
> > regulator_bulk_enable and regulator_bulk_disable directly.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 59 +++++++++++++++---------
> >  1 file changed, 37 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 542a31448c8f..52867ce3b9b6 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -1307,6 +1307,38 @@ static int mxt_soft_reset(struct mxt_data *data)
> >  	return 0;
> >  }
> >  
> > +static int mxt_power_on(struct mxt_data *data)
> > +{
> > +	int error;
> > +
> > +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > +				      data->regulators);
> > +	if (error) {
> > +		dev_err(&data->client->dev, "failed to enable regulators: %d\n",
> > +			error);
> > +		return error;
> > +	}
> > +
> > +	msleep(MXT_BACKUP_TIME);
> > +
> > +	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);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mxt_power_off(struct mxt_data *data)
> > +{
> > +	if (data->reset_gpio)
> > +		gpiod_set_value(data->reset_gpio, 1);
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> > +}
> > +
> >  static void mxt_update_crc(struct mxt_data *data, u8 cmd, u8 value)
> >  {
> >  	/*
> > @@ -3305,25 +3337,9 @@ static int mxt_probe(struct i2c_client *client)
> >  		return error;
> >  	}
> >  
> > -	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > -				      data->regulators);
> > -	if (error) {
> > -		dev_err(&client->dev, "failed to enable regulators: %d\n",
> > -			error);
> > +	error = mxt_power_on(data);
> > +	if (error)
> >  		return error;
> > -	}
> > -	/*
> > -	 * The device takes 40ms to come up after power-on according
> > -	 * to the mXT224 datasheet, page 13.
> > -	 */
> > -	msleep(MXT_BACKUP_TIME);
> > -
> > -	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);
> > -	}
> >  
> >  	/*
> >  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
> > @@ -3361,8 +3377,8 @@ static int mxt_probe(struct i2c_client *client)
> >  	mxt_free_input_device(data);
> >  	mxt_free_object_table(data);
> >  err_disable_regulators:
> > -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > -			       data->regulators);
> > +	mxt_power_off(data);
> > +
> >  	return error;
> >  }
> >  
> > @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
> >  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> >  	mxt_free_input_device(data);
> >  	mxt_free_object_table(data);
> > -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > -			       data->regulators);
> > +	mxt_power_off(data);
> 
> This change means that on unbind we will leave with GPIO line asserted.
> Won't this potentially cause some current leakage? Why do we need to
> have reset asserted here?

This is correct, but I checked the datasheet of three different maxTouch
models and all of them have the reset line low active. This means we had
a current leakage before this patch. Now it is fixed because we assert
the reset line, which sets the pin to 0. I also think it makes sense if
we look at the power on sequence. There we first power on the controller
before we release the reset line. Without asserting it on unbind this
would never trigger a reset after a power on.

Regards,
Stefan

Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions

[Dmitry Torokhov]

Hi Stefan,

On Fri, Jun 21, 2024 at 04:38:25PM +0200, Stefan Eichenberger wrote:
> Hi Dmitry
> 
> On Thu, Jun 20, 2024 at 08:37:40AM -0700, Dmitry Torokhov wrote:
> > Hi Stefan,
> > 
> > On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote:
> > > @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
> > >  	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > >  	mxt_free_input_device(data);
> > >  	mxt_free_object_table(data);
> > > -	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> > > -			       data->regulators);
> > > +	mxt_power_off(data);
> > 
> > This change means that on unbind we will leave with GPIO line asserted.
> > Won't this potentially cause some current leakage? Why do we need to
> > have reset asserted here?
> 
> This is correct, but I checked the datasheet of three different maxTouch
> models and all of them have the reset line low active. This means we had
> a current leakage before this patch.

No, the reset line would be either set or pulled high, which is the
normal state for the device.

> Now it is fixed because we assert
> the reset line, which sets the pin to 0. I also think it makes sense if
> we look at the power on sequence. There we first power on the controller
> before we release the reset line. Without asserting it on unbind this
> would never trigger a reset after a power on.

Please see that in mxt_probe() we do:

	/* Request the RESET line as asserted so we go into reset */
	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
						   "reset", GPIOD_OUT_HIGH);

If the reset GPIO is annotated as "active low" (as it should unless
there are inverters on the line) this will cause the line
be driven to 0, putting the chip into the reset state. Then we enable
regulators and deassert the reset GPIO with this bit of code:

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

So the line here will be left at "1" state (logical off). It should stay
this way until we need to go through the reset sequence again.

I can see that you need the power on sequence to be executed again after
probing is done. I recommend you make it something like this:

static int mxt_power_on()
{
	int error;

	if (data->reset_gpio)
		gpiod_set_value_cansleep(data->reset_gpio, 1);


	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
				      data->regulators);
	if (error) {
		dev_err(&client->dev, "failed to enable regulators: %d\n",
			error);
		return error;
	}

	/*
	 * The device takes 40ms to come up after power-on according
	 * to the mXT224 datasheet, page 13.
	 */
	msleep(MXT_BACKUP_TIME);

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

	return 0;
}

And then mxt_power_off() should only disable regulators, and leave the
reset line alone. This way first time around the first
"piod_set_value_cansleep(data->reset_gpio, 1)" will be effectively a
noop, but on subsequent calls it will ensure that you have the
transition inactive->active->inactive for the reset line.

I wonder if we need both MXT_BACKUP_TIME and MXT_RESET_GPIO_TIME
though...

Thanks.

-- 
Dmitry

[PATCH v4 2/4] Input: atmel_mxt_ts - move calls to register the input device to separate function

[Stefan Eichenberger]

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

The calls to register the input device are moved to a separate function
so that we can call it without having to confiugre the device. This is
necessary if we want to power on the device only when it is opened.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 52867ce3b9b6..7c807d1f1f9b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -2277,6 +2277,28 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
 	release_firmware(cfg);
 }
 
+static void mxt_debug_init(struct mxt_data *data);
+
+static int mxt_device_register(struct mxt_data *data)
+{
+	int error;
+
+	/* 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(&data->client->dev, "No touch object detected\n");
+		}
+
+		mxt_debug_init(data);
+	}
+
+	return 0;
+}
+
 static int mxt_initialize(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
@@ -2831,15 +2853,9 @@ 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");
-	}
-
-	mxt_debug_init(data);
+	error = mxt_device_register(data);
+	if (error)
+		return error;
 
 	return 0;
 }
-- 
2.40.1

[PATCH v4 3/4] 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 v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep

[Stefan Eichenberger]

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

Add support for poweroff-sleep to the Atmel maXTouch driver. This allows
us to power off the input device entirely and only power it on when it
is opened. This will also automatically power it off when we suspend the
system.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7c807d1f1f9b..f92808be3f5b 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;
@@ -2277,6 +2278,19 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
 	release_firmware(cfg);
 }
 
+static int mxt_initialize_after_resume(struct mxt_data *data)
+{
+	const struct firmware *fw;
+
+	mxt_acquire_irq(data);
+
+	firmware_request_nowarn(&fw, MXT_CFG_NAME, &data->client->dev);
+
+	mxt_config_cb(fw, data);
+
+	return 0;
+}
+
 static void mxt_debug_init(struct mxt_data *data);
 
 static int mxt_device_register(struct mxt_data *data)
@@ -2341,17 +2355,23 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
-	error = mxt_acquire_irq(data);
-	if (error)
-		return error;
+	if (data->poweroff_sleep) {
+		error = mxt_device_register(data);
+		if (error)
+			return error;
+	} else {
+		error = mxt_acquire_irq(data);
+		if (error)
+			return error;
 
-	error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
-					&client->dev, GFP_KERNEL, data,
-					mxt_config_cb);
-	if (error) {
-		dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
-			error);
-		return error;
+		error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
+						&client->dev, GFP_KERNEL, data,
+						mxt_config_cb);
+		if (error) {
+			dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
+				error);
+			return error;
+		}
 	}
 
 	return 0;
@@ -3089,6 +3109,9 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 	struct mxt_data *data = dev_get_drvdata(dev);
 	int error;
 
+	if (data->poweroff_sleep && !data->in_bootloader)
+		mxt_power_on(data);
+
 	error = mxt_load_fw(dev, MXT_FW_NAME);
 	if (error) {
 		dev_err(dev, "The firmware update failed(%d)\n", error);
@@ -3101,6 +3124,9 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 			return error;
 	}
 
+	if (data->poweroff_sleep && !data->in_bootloader)
+		mxt_power_off(data);
+
 	return count;
 }
 
@@ -3123,7 +3149,12 @@ static const struct attribute_group mxt_attr_group = {
 
 static void mxt_start(struct mxt_data *data)
 {
-	mxt_wakeup_toggle(data->client, true, false);
+	if (data->poweroff_sleep) {
+		mxt_power_on(data);
+		mxt_initialize_after_resume(data);
+	} else {
+		mxt_wakeup_toggle(data->client, true, false);
+	}
 
 	switch (data->suspend_mode) {
 	case MXT_SUSPEND_T9_CTRL:
@@ -3160,7 +3191,12 @@ static void mxt_stop(struct mxt_data *data)
 		break;
 	}
 
-	mxt_wakeup_toggle(data->client, false, false);
+	if (data->poweroff_sleep) {
+		disable_irq(data->irq);
+		mxt_power_off(data);
+	} else {
+		mxt_wakeup_toggle(data->client, false, false);
+	}
 }
 
 static int mxt_input_open(struct input_dev *dev)
@@ -3357,6 +3393,8 @@ static int mxt_probe(struct i2c_client *client)
 	if (error)
 		return error;
 
+	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,6 +3425,9 @@ static int mxt_probe(struct i2c_client *client)
 		goto err_free_object;
 	}
 
+	if (data->poweroff_sleep && !data->in_bootloader)
+		mxt_power_off(data);
+
 	return 0;
 
 err_free_object:
@@ -3406,7 +3447,8 @@ static void mxt_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
-	mxt_power_off(data);
+	if (!data->poweroff_sleep)
+		mxt_power_off(data);
 }
 
 static int mxt_suspend(struct device *dev)
@@ -3439,7 +3481,8 @@ static int mxt_resume(struct device *dev)
 	if (!input_dev)
 		return 0;
 
-	enable_irq(data->irq);
+	if (!data->poweroff_sleep)
+		enable_irq(data->irq);
 
 	mutex_lock(&input_dev->mutex);
 
-- 
2.40.1

Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep

[Dmitry Torokhov]

Hi Stefan,

On Wed, Apr 17, 2024 at 11:05:27AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Add support for poweroff-sleep to the Atmel maXTouch driver. This allows
> us to power off the input device entirely and only power it on when it
> is opened. This will also automatically power it off when we suspend the
> system.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 71 +++++++++++++++++++-----
>  1 file changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7c807d1f1f9b..f92808be3f5b 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;

Why is this separate from "enum mxt_suspend_mode suspend_mode"? Can we
make MXT_SUSPEND_POWEROFF and use it in mxt_start() and others? It still
can be driven by the "atmel,poweroff-sleep" device property. 

>  
>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -2277,6 +2278,19 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>  	release_firmware(cfg);
>  }
>  
> +static int mxt_initialize_after_resume(struct mxt_data *data)
> +{
> +	const struct firmware *fw;
> +
> +	mxt_acquire_irq(data);
> +
> +	firmware_request_nowarn(&fw, MXT_CFG_NAME, &data->client->dev);
> +
> +	mxt_config_cb(fw, data);

Is this really required? As far as I know all maXTouch controllers have
NVRAM for their configs and should not lose configuration even if power
is cut off. In fact, the whole automatic request of firmware/config upon
probe I think was a mistake and I would like to get rid of it. In fact,
on Chrome OS the version of the driver in use does not do that and
instead relies on userspace to check if firmware update is needed.

If this is actually required you need to add error handling.

Thanks.

-- 
Dmitry

Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep

[Stefan Eichenberger]

Hi Dimitry,

On Wed, Jun 19, 2024 at 06:12:45PM -0700, Dmitry Torokhov wrote:
> Hi Stefan,
> 
> On Wed, Apr 17, 2024 at 11:05:27AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > Add support for poweroff-sleep to the Atmel maXTouch driver. This allows
> > us to power off the input device entirely and only power it on when it
> > is opened. This will also automatically power it off when we suspend the
> > system.
> > 
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 71 +++++++++++++++++++-----
> >  1 file changed, 57 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 7c807d1f1f9b..f92808be3f5b 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;
> 
> Why is this separate from "enum mxt_suspend_mode suspend_mode"? Can we
> make MXT_SUSPEND_POWEROFF and use it in mxt_start() and others? It still
> can be driven by the "atmel,poweroff-sleep" device property. 

I agree and will change this in the next version.

> >  
> >  	/* Cached parameters from object table */
> >  	u16 T5_address;
> > @@ -2277,6 +2278,19 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
> >  	release_firmware(cfg);
> >  }
> >  
> > +static int mxt_initialize_after_resume(struct mxt_data *data)
> > +{
> > +	const struct firmware *fw;
> > +
> > +	mxt_acquire_irq(data);
> > +
> > +	firmware_request_nowarn(&fw, MXT_CFG_NAME, &data->client->dev);
> > +
> > +	mxt_config_cb(fw, data);
> 
> Is this really required? As far as I know all maXTouch controllers have
> NVRAM for their configs and should not lose configuration even if power
> is cut off. In fact, the whole automatic request of firmware/config upon
> probe I think was a mistake and I would like to get rid of it. In fact,
> on Chrome OS the version of the driver in use does not do that and
> instead relies on userspace to check if firmware update is needed.
> 
> If this is actually required you need to add error handling.

You are right, and the configuration goes to non-volatile memory, so
this is not needed. I will remove the firmware loading here and improve
the error handling. I will keep the firmware loading in the initial
probe for now, to be consistent with the non poweroff-sleep version.

Regards,
Stefan

RE: [PATCH v4 0/4] Add a property to turn off the max touch controller if not used

[Joao Paulo Goncalves]

> 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 when not used. This increases the overall power consumption. In order
> to turn off the controller when the system does not use it, 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.
>
> Changes since v3:
> - Move the power on part to mxt_start and the power off part to
>   mxt_stop. This allows to turn the touch controller off even when not
>   in use and not only when being suspended (Dmitry)
>
> 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 (4):
>   Input: atmel_mxt_ts - add power off and power on functions
>   Input: atmel_mxt_ts - move calls to register the input device to
>     separate function
>   dt-bindings: input: atmel,maxtouch: add poweroff-sleep property
>   Input: atmel_mxt_ts - add support for poweroff-sleep
>
>  .../bindings/input/atmel,maxtouch.yaml        |   6 +
>  drivers/input/touchscreen/atmel_mxt_ts.c      | 162 +++++++++++++-----
>  2 files changed, 124 insertions(+), 44 deletions(-)
>
> --
> 2.40.1
>

Reviewed-by: Joao Paulo Goncalves <joao.goncalves@toradex.com>

Regards,
Joao Paulo