* Re: [PATCH] input: goodix: Add option to power off the controller during suspend
2023-03-10 17:00 [PATCH] input: goodix: Add option to power off the controller during suspend Jan Jasper de Kroon
@ 2023-03-10 21:19 ` Ondřej Jirman
2023-03-11 13:15 ` [PATCH v2] input: touchscreen: goodix: Hold controller in reset " Jan Jasper de Kroon
2023-03-16 15:22 ` [PATCH v4 2/2] " Jan Jasper de Kroon
2 siblings, 0 replies; 8+ messages in thread
From: Ondřej Jirman @ 2023-03-10 21:19 UTC (permalink / raw)
To: Jan Jasper de Kroon; +Cc: linux-input
Hi Jan,
On Fri, Mar 10, 2023 at 06:00:26PM +0100, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> For whatever reason the controller is not suspended on Pinephone
> by the default procedure. It consumes quite a bit of power (~40mW)
> during system sleep, and more when the screen is touched.
>
> Let's power off the controller during system sleep instead.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>
> input: goodix: Don't disable regulators during suspend
I don't think you should concatenate commit messages like this.
Also what the change actually does is that it holds the controller
in reset during sleep, and doesn't try to power it off.
> It does no harm to disable them, but on Pinephone we have other
> peripherals attached to the same power supplies, whose drivers
> will not reference the regulator, so powering down the regulators
> from Goodix driver will break those other devices.
>
> Until those drivers gain the regulator support, don't disable
> the regulators in Goodix driver.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> ---
> drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
> drivers/input/touchscreen/goodix.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b348172f19c3..e6d054ede960 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
>
> static int goodix_ts_probe(struct i2c_client *client)
> {
> + struct device_node *np = client->dev.of_node;
> struct goodix_ts_data *ts;
> const char *cfg_name;
> int error;
> @@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
> i2c_set_clientdata(client, ts);
> init_completion(&ts->firmware_loading_complete);
> ts->contact_size = GOODIX_CONTACT_SIZE;
> + ts->poweroff_in_suspend = of_property_read_bool(np, "poweroff-in-suspend");
If you're adding a new DT property, you need to document it in the device tree
bindings of the driver.
> error = goodix_get_gpio_config(ts);
> if (error)
> @@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
> if (ts->load_cfg_from_disk)
> wait_for_completion(&ts->firmware_loading_complete);
>
> + if (ts->poweroff_in_suspend) {
> + goodix_free_irq(ts);
> + goodix_irq_direction_output(ts, 0);
> + gpiod_direction_output(ts->gpiod_rst, 0);
> + return 0;
> + }
> +
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> disable_irq(client->irq);
> @@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
> u8 config_ver;
> int error;
>
> + if (ts->poweroff_in_suspend) {
> + error = goodix_reset(ts);
> + if (error) {
> + dev_err(dev, "Controller reset failed.\n");
> + return error;
> + }
> +
> + error = goodix_request_irq(ts);
> + if (error)
> + return error;
> +
> + return 0;
> + }
> +
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> enable_irq(client->irq);
> return 0;
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..dc358bcda698 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -104,6 +104,7 @@ struct goodix_ts_data {
> u8 main_clk[GOODIX_MAIN_CLK_LEN];
> int bak_ref_len;
> u8 *bak_ref;
> + bool poweroff_in_suspend;
Hold reset in suspend?
kind regards,
o.
> };
>
> int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
> --
> 2.34.3
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-10 17:00 [PATCH] input: goodix: Add option to power off the controller during suspend Jan Jasper de Kroon
2023-03-10 21:19 ` Ondřej Jirman
@ 2023-03-11 13:15 ` Jan Jasper de Kroon
2023-03-12 18:40 ` [PATCH v3 2/2] " Jan Jasper de Kroon
2023-03-13 0:44 ` [PATCH v2] " Jarrah
2023-03-16 15:22 ` [PATCH v4 2/2] " Jan Jasper de Kroon
2 siblings, 2 replies; 8+ messages in thread
From: Jan Jasper de Kroon @ 2023-03-11 13:15 UTC (permalink / raw)
To: jajadekroon; +Cc: linux-input, megi
From: Ondřej Jirman <megi@xff.cz>
The Goodix touchscreen controller used in PinePhone is not properly
suspended during system sleep, leading to high power consumption. This
commit modifies the driver to hold the controller in reset during
system sleep, reducing power consumption by around 40mW.
The original patch also disabled the regulators during system sleep, but
this could cause issues with other peripherals attached to the same power
supplies. Hence, this commit only holds the controller in reset during
system sleep and does not attempt to power it off.
A separate commit adds a new device tree property "hold-in-reset-in-suspend"
to control the hold-in-reset behavior of the controller during system sleep.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
V1 -> V2: Rewrote the commit message and function name to better reflect it's behavior.
drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
drivers/input/touchscreen/goodix.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..a35a203e04bf 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
static int goodix_ts_probe(struct i2c_client *client)
{
+ struct device_node *np = client->dev.of_node;
struct goodix_ts_data *ts;
const char *cfg_name;
int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
init_completion(&ts->firmware_loading_complete);
ts->contact_size = GOODIX_CONTACT_SIZE;
+ ts->hold_in_reset_in_suspend = of_property_read_bool(np, "hold-in-reset-in-suspend");
error = goodix_get_gpio_config(ts);
if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->hold_in_reset_in_suspend) {
+ goodix_free_irq(ts);
+ goodix_irq_direction_output(ts, 0);
+ gpiod_direction_output(ts->gpiod_rst, 0);
+ return 0;
+ }
+
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
u8 config_ver;
int error;
+ if (ts->hold_in_reset_in_suspend) {
+ error = goodix_reset(ts);
+ if (error) {
+ dev_err(dev, "Controller reset failed.\n");
+ return error;
+ }
+
+ error = goodix_request_irq(ts);
+ if (error)
+ return error;
+
+ return 0;
+ }
+
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
enable_irq(client->irq);
return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..c515d03b742c 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ bool hold_in_reset_in_suspend;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-11 13:15 ` [PATCH v2] input: touchscreen: goodix: Hold controller in reset " Jan Jasper de Kroon
@ 2023-03-12 18:40 ` Jan Jasper de Kroon
2023-03-12 18:59 ` Hans de Goede
2023-03-13 0:44 ` [PATCH v2] " Jarrah
1 sibling, 1 reply; 8+ messages in thread
From: Jan Jasper de Kroon @ 2023-03-12 18:40 UTC (permalink / raw)
To: jajadekroon
Cc: linux-input, megi, hadess, hdegoede, dmitry.torokhov,
linux-kernel
From: Ondřej Jirman <megi@xff.cz>
The Goodix touchscreen controller used in PinePhone is not properly
suspended during system sleep, leading to high power consumption. This
commit modifies the driver to hold the controller in reset during
system sleep, reducing power consumption by around 40mW.
The original patch also disabled the regulators during system sleep, but
this could cause issues with other peripherals attached to the same power
supplies. Hence, this commit only holds the controller in reset during
system sleep and does not attempt to power it off.
Separate commit adds a new device tree property "hold-in-reset-in-suspend"
to control the hold-in-reset behavior of the controller during system
sleep.
Signed-off-by: Ondřej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
V1 -> V2:
- Rewrote the commit message and function name to better reflect it's
behavior.
v2 -> V3: Some patch style cleaning up, to confirm to submission standard.
drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
drivers/input/touchscreen/goodix.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..a35a203e04bf 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
static int goodix_ts_probe(struct i2c_client *client)
{
+ struct device_node *np = client->dev.of_node;
struct goodix_ts_data *ts;
const char *cfg_name;
int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
init_completion(&ts->firmware_loading_complete);
ts->contact_size = GOODIX_CONTACT_SIZE;
+ ts->hold_in_reset_in_suspend = of_property_read_bool(np, "hold-in-reset-in-suspend");
error = goodix_get_gpio_config(ts);
if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->hold_in_reset_in_suspend) {
+ goodix_free_irq(ts);
+ goodix_irq_direction_output(ts, 0);
+ gpiod_direction_output(ts->gpiod_rst, 0);
+ return 0;
+ }
+
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
u8 config_ver;
int error;
+ if (ts->hold_in_reset_in_suspend) {
+ error = goodix_reset(ts);
+ if (error) {
+ dev_err(dev, "Controller reset failed.\n");
+ return error;
+ }
+
+ error = goodix_request_irq(ts);
+ if (error)
+ return error;
+
+ return 0;
+ }
+
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
enable_irq(client->irq);
return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..c515d03b742c 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ bool hold_in_reset_in_suspend;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-12 18:40 ` [PATCH v3 2/2] " Jan Jasper de Kroon
@ 2023-03-12 18:59 ` Hans de Goede
0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2023-03-12 18:59 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: linux-input, megi, hadess, dmitry.torokhov, linux-kernel
Hi,
On 3/12/23 19:40, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
>
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
>
> Separate commit adds a new device tree property "hold-in-reset-in-suspend"
> to control the hold-in-reset behavior of the controller during system
> sleep.
>
> Signed-off-by: Ondřej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> V1 -> V2:
> - Rewrote the commit message and function name to better reflect it's
> behavior.
> v2 -> V3: Some patch style cleaning up, to confirm to submission standard.
>
> drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
> drivers/input/touchscreen/goodix.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b348172f19c3..a35a203e04bf 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
>
> static int goodix_ts_probe(struct i2c_client *client)
> {
> + struct device_node *np = client->dev.of_node;
> struct goodix_ts_data *ts;
> const char *cfg_name;
> int error;
> @@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
> i2c_set_clientdata(client, ts);
> init_completion(&ts->firmware_loading_complete);
> ts->contact_size = GOODIX_CONTACT_SIZE;
> + ts->hold_in_reset_in_suspend = of_property_read_bool(np, "hold-in-reset-in-suspend");
>
> error = goodix_get_gpio_config(ts);
> if (error)
> @@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
> if (ts->load_cfg_from_disk)
> wait_for_completion(&ts->firmware_loading_complete);
>
> + if (ts->hold_in_reset_in_suspend) {
> + goodix_free_irq(ts);
> + goodix_irq_direction_output(ts, 0);
> + gpiod_direction_output(ts->gpiod_rst, 0);
> + return 0;
> + }
> +
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> disable_irq(client->irq);
> @@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
> u8 config_ver;
> int error;
>
> + if (ts->hold_in_reset_in_suspend) {
> + error = goodix_reset(ts);
> + if (error) {
> + dev_err(dev, "Controller reset failed.\n");
> + return error;
> + }
> +
> + error = goodix_request_irq(ts);
> + if (error)
> + return error;
> +
> + return 0;
> + }
> +
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> enable_irq(client->irq);
> return 0;
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..c515d03b742c 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -104,6 +104,7 @@ struct goodix_ts_data {
> u8 main_clk[GOODIX_MAIN_CLK_LEN];
> int bak_ref_len;
> u8 *bak_ref;
> + bool hold_in_reset_in_suspend;
> };
>
> int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-11 13:15 ` [PATCH v2] input: touchscreen: goodix: Hold controller in reset " Jan Jasper de Kroon
2023-03-12 18:40 ` [PATCH v3 2/2] " Jan Jasper de Kroon
@ 2023-03-13 0:44 ` Jarrah
1 sibling, 0 replies; 8+ messages in thread
From: Jarrah @ 2023-03-13 0:44 UTC (permalink / raw)
To: Jan Jasper de Kroon; +Cc: linux-input, megi
On 3/12/23 00:15, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
>
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
>
> A separate commit adds a new device tree property "hold-in-reset-in-suspend"
> to control the hold-in-reset behavior of the controller during system sleep.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
Tested-by: Jarrah Gosbell <kernel@undef.tools>
Thank you for upstreaming these,
Jarrah.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-10 17:00 [PATCH] input: goodix: Add option to power off the controller during suspend Jan Jasper de Kroon
2023-03-10 21:19 ` Ondřej Jirman
2023-03-11 13:15 ` [PATCH v2] input: touchscreen: goodix: Hold controller in reset " Jan Jasper de Kroon
@ 2023-03-16 15:22 ` Jan Jasper de Kroon
2023-03-16 17:25 ` Hans de Goede
2 siblings, 1 reply; 8+ messages in thread
From: Jan Jasper de Kroon @ 2023-03-16 15:22 UTC (permalink / raw)
To: jajadekroon
Cc: dmitry.torokhov, hadess, hdegoede, linux-input, linux-kernel,
megi
From: Ondřej Jirman <megi@xff.cz>
The Goodix touchscreen controller used in PinePhone is not properly
suspended during system sleep, leading to high power consumption. This
commit modifies the driver to hold the controller in reset during
system sleep, reducing power consumption by around 40mW.
The original patch also disabled the regulators during system sleep, but
this could cause issues with other peripherals attached to the same power
supplies. Hence, this commit only holds the controller in reset during
system sleep and does not attempt to power it off.
A separate commit adds a new device tree property "goodix-hold-in-reset"
to control the hold-in-reset behavior of the controller during system sleep.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v3 to v4:
- Renamed the hold-in-reset-in-suspend function to
goodix-hold-in-reset to prevent potential naming conflicts with other
functions in the codebase. No functional changes were made.
Changes from v2 to v3:
- Some patch style cleaning up, to confirm to submission standard.
Changes from v1 to v2:
- Rewrote the commit message and function name to better reflect it's
behavior.
drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
drivers/input/touchscreen/goodix.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..9f86bdfbfc73 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
static int goodix_ts_probe(struct i2c_client *client)
{
+ struct device_node *np = client->dev.of_node;
struct goodix_ts_data *ts;
const char *cfg_name;
int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
init_completion(&ts->firmware_loading_complete);
ts->contact_size = GOODIX_CONTACT_SIZE;
+ ts->goodix_hold_in_reset = of_property_read_bool(np, "goodix-hold-in-reset");
error = goodix_get_gpio_config(ts);
if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->goodix_hold_in_reset) {
+ goodix_free_irq(ts);
+ goodix_irq_direction_output(ts, 0);
+ gpiod_direction_output(ts->gpiod_rst, 0);
+ return 0;
+ }
+
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
u8 config_ver;
int error;
+ if (ts->goodix_hold_in_reset) {
+ error = goodix_reset(ts);
+ if (error) {
+ dev_err(dev, "Controller reset failed.\n");
+ return error;
+ }
+
+ error = goodix_request_irq(ts);
+ if (error)
+ return error;
+
+ return 0;
+ }
+
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
enable_irq(client->irq);
return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..7be7eb1a8f27 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ bool goodix_hold_in_reset;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
2023-03-16 15:22 ` [PATCH v4 2/2] " Jan Jasper de Kroon
@ 2023-03-16 17:25 ` Hans de Goede
0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2023-03-16 17:25 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: dmitry.torokhov, hadess, linux-input, linux-kernel, megi
Hi,
On 3/16/23 16:22, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
>
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
>
> A separate commit adds a new device tree property "goodix-hold-in-reset"
> to control the hold-in-reset behavior of the controller during system sleep.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> ---
> Changes from v3 to v4:
> - Renamed the hold-in-reset-in-suspend function to
> goodix-hold-in-reset to prevent potential naming conflicts with other
> functions in the codebase. No functional changes were made.
>
> Changes from v2 to v3:
> - Some patch style cleaning up, to confirm to submission standard.
>
> Changes from v1 to v2:
> - Rewrote the commit message and function name to better reflect it's
> behavior.
> drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
> drivers/input/touchscreen/goodix.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b348172f19c3..9f86bdfbfc73 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
>
> static int goodix_ts_probe(struct i2c_client *client)
> {
> + struct device_node *np = client->dev.of_node;
> struct goodix_ts_data *ts;
> const char *cfg_name;
> int error;
> @@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
> i2c_set_clientdata(client, ts);
> init_completion(&ts->firmware_loading_complete);
> ts->contact_size = GOODIX_CONTACT_SIZE;
> + ts->goodix_hold_in_reset = of_property_read_bool(np, "goodix-hold-in-reset");
I only noticed this just now, sorry. Can you please use
"device_property_read_bool(&client-dev, "goodix-hold-in-reset");
instead, this way this will also work with non device-tree (aka OF)
platforms.
Also with devicetree properties if you prefix them by the vendor
name the vendor-prefix should end with a comma, so you get:
"goodix,hold-in-reset"
And I think this name is not very descriptive, this really should be something like:
"goodix,hold-in-reset-during-suspend"
to make what it does more clear.
I assume that this is going to get set inside a dts file, in that case this
new property should be documented in:
Documentation/devicetree/bindings/input/touchscree /goodix.yaml
and these devicetree-bindings changes must be in a separate patch
(and the dt-bindings maintainers should be Cc-ed on that patch).
Regards,
Hans
>
> error = goodix_get_gpio_config(ts);
> if (error)
> @@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
> if (ts->load_cfg_from_disk)
> wait_for_completion(&ts->firmware_loading_complete);
>
> + if (ts->goodix_hold_in_reset) {
> + goodix_free_irq(ts);
> + goodix_irq_direction_output(ts, 0);
> + gpiod_direction_output(ts->gpiod_rst, 0);
> + return 0;
> + }
> +
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> disable_irq(client->irq);
> @@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
> u8 config_ver;
> int error;
>
> + if (ts->goodix_hold_in_reset) {
> + error = goodix_reset(ts);
> + if (error) {
> + dev_err(dev, "Controller reset failed.\n");
> + return error;
> + }
> +
> + error = goodix_request_irq(ts);
> + if (error)
> + return error;
> +
> + return 0;
> + }
> +
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> enable_irq(client->irq);
> return 0;
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..7be7eb1a8f27 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -104,6 +104,7 @@ struct goodix_ts_data {
> u8 main_clk[GOODIX_MAIN_CLK_LEN];
> int bak_ref_len;
> u8 *bak_ref;
> + bool goodix_hold_in_reset;
> };
>
> int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
^ permalink raw reply [flat|nested] 8+ messages in thread