| Message ID | 20230316152159.66922-1-jajadekroon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | input: goodix: Add option to power off the controller during suspend | expand |
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);
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);