* [PATCH v4 0/4] Add a property to turn off the max touch controller if not used
@ 2024-04-17 9:05 Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Stefan Eichenberger
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Stefan Eichenberger @ 2024-04-17 9:05 UTC (permalink / raw
To: nick, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij
Cc: linux-input, devicetree, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
@ 2024-04-17 9:05 ` Stefan Eichenberger
2024-06-20 15:37 ` Dmitry Torokhov
2024-04-17 9:05 ` [PATCH v4 2/4] Input: atmel_mxt_ts - move calls to register the input device to separate function Stefan Eichenberger
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Stefan Eichenberger @ 2024-04-17 9:05 UTC (permalink / raw
To: nick, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij
Cc: linux-input, devicetree, linux-arm-kernel, linux-kernel,
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 2/4] Input: atmel_mxt_ts - move calls to register the input device to separate function
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Stefan Eichenberger
@ 2024-04-17 9:05 ` Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 3/4] dt-bindings: input: atmel,maxtouch: add poweroff-sleep property Stefan Eichenberger
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Eichenberger @ 2024-04-17 9:05 UTC (permalink / raw
To: nick, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij
Cc: linux-input, devicetree, linux-arm-kernel, linux-kernel,
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 3/4] dt-bindings: input: atmel,maxtouch: add poweroff-sleep property
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 2/4] Input: atmel_mxt_ts - move calls to register the input device to separate function Stefan Eichenberger
@ 2024-04-17 9:05 ` Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep Stefan Eichenberger
2024-04-17 11:12 ` [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Joao Paulo Goncalves
4 siblings, 0 replies; 10+ messages in thread
From: Stefan Eichenberger @ 2024-04-17 9:05 UTC (permalink / raw
To: nick, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij
Cc: linux-input, devicetree, linux-arm-kernel, linux-kernel,
Stefan Eichenberger, Krzysztof Kozlowski
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
` (2 preceding siblings ...)
2024-04-17 9:05 ` [PATCH v4 3/4] dt-bindings: input: atmel,maxtouch: add poweroff-sleep property Stefan Eichenberger
@ 2024-04-17 9:05 ` Stefan Eichenberger
2024-06-20 1:12 ` Dmitry Torokhov
2024-04-17 11:12 ` [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Joao Paulo Goncalves
4 siblings, 1 reply; 10+ messages in thread
From: Stefan Eichenberger @ 2024-04-17 9:05 UTC (permalink / raw
To: nick, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij
Cc: linux-input, devicetree, linux-arm-kernel, linux-kernel,
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v4 0/4] Add a property to turn off the max touch controller if not used
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
` (3 preceding siblings ...)
2024-04-17 9:05 ` [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep Stefan Eichenberger
@ 2024-04-17 11:12 ` Joao Paulo Goncalves
4 siblings, 0 replies; 10+ messages in thread
From: Joao Paulo Goncalves @ 2024-04-17 11:12 UTC (permalink / raw
To: eichest
Cc: alexandre.belloni, claudiu.beznea, conor+dt, devicetree,
dmitry.torokhov, krzysztof.kozlowski+dt, linus.walleij,
linux-arm-kernel, linux-input, linux-kernel, nick, nicolas.ferre,
robh, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep
2024-04-17 9:05 ` [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep Stefan Eichenberger
@ 2024-06-20 1:12 ` Dmitry Torokhov
2024-06-21 14:31 ` Stefan Eichenberger
0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2024-06-20 1:12 UTC (permalink / raw
To: Stefan Eichenberger
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions
2024-04-17 9:05 ` [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Stefan Eichenberger
@ 2024-06-20 15:37 ` Dmitry Torokhov
2024-06-21 14:38 ` Stefan Eichenberger
0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2024-06-20 15:37 UTC (permalink / raw
To: Stefan Eichenberger
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep
2024-06-20 1:12 ` Dmitry Torokhov
@ 2024-06-21 14:31 ` Stefan Eichenberger
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Eichenberger @ 2024-06-21 14:31 UTC (permalink / raw
To: Dmitry Torokhov
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions
2024-06-20 15:37 ` Dmitry Torokhov
@ 2024-06-21 14:38 ` Stefan Eichenberger
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Eichenberger @ 2024-06-21 14:38 UTC (permalink / raw
To: Dmitry Torokhov
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-06-21 14:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-17 9:05 [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Stefan Eichenberger
2024-06-20 15:37 ` Dmitry Torokhov
2024-06-21 14:38 ` Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 2/4] Input: atmel_mxt_ts - move calls to register the input device to separate function Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 3/4] dt-bindings: input: atmel,maxtouch: add poweroff-sleep property Stefan Eichenberger
2024-04-17 9:05 ` [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep Stefan Eichenberger
2024-06-20 1:12 ` Dmitry Torokhov
2024-06-21 14:31 ` Stefan Eichenberger
2024-04-17 11:12 ` [PATCH v4 0/4] Add a property to turn off the max touch controller if not used Joao Paulo Goncalves
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 read-only IMAP folder(s) and NNTP newsgroup(s).