From: Fei Shao <fshao@chromium.org> To: Benjamin Tissoires <benjamin.tissoires@redhat.com>, Douglas Anderson <dianders@chromium.org>, Rob Herring <robh+dt@kernel.org> Cc: linux-mediatek <linux-mediatek@lists.infradead.org>, Fei Shao <fshao@chromium.org>, Dmitry Torokhov <dmitry.torokhov@gmail.com>, Jiri Kosina <jikos@kernel.org>, Matthias Kaehlcke <mka@chromium.org>, Stephen Kitt <steve@sk2.org>, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property Date: Tue, 18 Apr 2023 20:49:52 +0800 [thread overview] Message-ID: <20230418124953.3170028-3-fshao@chromium.org> (raw) In-Reply-To: <20230418124953.3170028-1-fshao@chromium.org> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") introduced a change to tie the reset line of the Goodix touchscreen to the state of the regulator to fix a power leakage issue in suspend. After some time, the change was deemed unnecessary and was reverted in commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the regulator") due to difficulties in managing regulator notifiers for designs like Evoker, which provides a second power rail to touchscreen. However, the revert caused a power regression on another Chromebook device Steelix in the field, which has a dedicated always-on regulator for touchscreen and was covered by the workaround in the first commit. To address both cases, this patch adds the support for the `powered-in-suspend` property in the driver that allows the driver to determine whether the touchscreen is still powered in suspend, and handle the reset GPIO accordingly as below: - When set to true, the driver does not assert the reset GPIO in power down. To ensure a clean start and the consistent behavior, it does the assertion in power up instead. This is for designs with a dedicated always-on regulator. - When set to false, the driver uses the original control flow and asserts GPIO and disable regulators normally. This is for the two-regulator and shared-regulator designs. Signed-off-by: Fei Shao <fshao@chromium.org> --- drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c index 0060e3dcd775..b438db8ca6f4 100644 --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix { struct regulator *vdd; struct regulator *vddio; struct gpio_desc *reset_gpio; + bool powered_in_suspend; const struct goodix_i2c_hid_timing_data *timings; }; @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops) container_of(ops, struct i2c_hid_of_goodix, ops); int ret; - ret = regulator_enable(ihid_goodix->vdd); - if (ret) - return ret; - - ret = regulator_enable(ihid_goodix->vddio); - if (ret) - return ret; + /* + * This is to ensure that the reset GPIO will be asserted and the + * regulators will be enabled for all cases. + */ + if (ihid_goodix->powered_in_suspend) { + /* + * This is not mandatory, but we assert reset here (instead of + * in power-down) to ensure that the device will have a clean + * state later on just like the normal scenarios would have. + * + * Also, since the regulators were not disabled in power-down, + * we don't need to enable them here. + */ + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1); + } else { + /* + * In this case, the reset is already asserted (either in + * probe or power-down). + * All we need is to enable the regulators. + */ + ret = regulator_enable(ihid_goodix->vdd); + if (ret) + return ret; + + ret = regulator_enable(ihid_goodix->vddio); + if (ret) + return ret; + } if (ihid_goodix->timings->post_power_delay_ms) msleep(ihid_goodix->timings->post_power_delay_ms); @@ -60,6 +82,13 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops) struct i2c_hid_of_goodix *ihid_goodix = container_of(ops, struct i2c_hid_of_goodix, ops); + /* + * Don't assert reset GPIO or disable regulators if we're keeping the + * device powered in suspend. + */ + if (ihid_goodix->powered_in_suspend) + return; + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1); regulator_disable(ihid_goodix->vddio); regulator_disable(ihid_goodix->vdd); @@ -91,6 +120,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client) if (IS_ERR(ihid_goodix->vddio)) return PTR_ERR(ihid_goodix->vddio); + ihid_goodix->powered_in_suspend = + of_property_read_bool(client->dev.of_node, "powered-in-suspend"); + ihid_goodix->timings = device_get_match_data(&client->dev); return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0); -- 2.40.0.634.g4ca3ef3211-goog
WARNING: multiple messages have this Message-ID (diff)
From: Fei Shao <fshao@chromium.org> To: Benjamin Tissoires <benjamin.tissoires@redhat.com>, Douglas Anderson <dianders@chromium.org>, Rob Herring <robh+dt@kernel.org> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>, Stephen Kitt <steve@sk2.org>, Jiri Kosina <jikos@kernel.org>, linux-kernel@vger.kernel.org, Matthias Kaehlcke <mka@chromium.org>, linux-mediatek <linux-mediatek@lists.infradead.org>, linux-input@vger.kernel.org Subject: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property Date: Tue, 18 Apr 2023 20:49:52 +0800 [thread overview] Message-ID: <20230418124953.3170028-3-fshao@chromium.org> (raw) In-Reply-To: <20230418124953.3170028-1-fshao@chromium.org> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") introduced a change to tie the reset line of the Goodix touchscreen to the state of the regulator to fix a power leakage issue in suspend. After some time, the change was deemed unnecessary and was reverted in commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the regulator") due to difficulties in managing regulator notifiers for designs like Evoker, which provides a second power rail to touchscreen. However, the revert caused a power regression on another Chromebook device Steelix in the field, which has a dedicated always-on regulator for touchscreen and was covered by the workaround in the first commit. To address both cases, this patch adds the support for the `powered-in-suspend` property in the driver that allows the driver to determine whether the touchscreen is still powered in suspend, and handle the reset GPIO accordingly as below: - When set to true, the driver does not assert the reset GPIO in power down. To ensure a clean start and the consistent behavior, it does the assertion in power up instead. This is for designs with a dedicated always-on regulator. - When set to false, the driver uses the original control flow and asserts GPIO and disable regulators normally. This is for the two-regulator and shared-regulator designs. Signed-off-by: Fei Shao <fshao@chromium.org> --- drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c index 0060e3dcd775..b438db8ca6f4 100644 --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix { struct regulator *vdd; struct regulator *vddio; struct gpio_desc *reset_gpio; + bool powered_in_suspend; const struct goodix_i2c_hid_timing_data *timings; }; @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops) container_of(ops, struct i2c_hid_of_goodix, ops); int ret; - ret = regulator_enable(ihid_goodix->vdd); - if (ret) - return ret; - - ret = regulator_enable(ihid_goodix->vddio); - if (ret) - return ret; + /* + * This is to ensure that the reset GPIO will be asserted and the + * regulators will be enabled for all cases. + */ + if (ihid_goodix->powered_in_suspend) { + /* + * This is not mandatory, but we assert reset here (instead of + * in power-down) to ensure that the device will have a clean + * state later on just like the normal scenarios would have. + * + * Also, since the regulators were not disabled in power-down, + * we don't need to enable them here. + */ + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1); + } else { + /* + * In this case, the reset is already asserted (either in + * probe or power-down). + * All we need is to enable the regulators. + */ + ret = regulator_enable(ihid_goodix->vdd); + if (ret) + return ret; + + ret = regulator_enable(ihid_goodix->vddio); + if (ret) + return ret; + } if (ihid_goodix->timings->post_power_delay_ms) msleep(ihid_goodix->timings->post_power_delay_ms); @@ -60,6 +82,13 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops) struct i2c_hid_of_goodix *ihid_goodix = container_of(ops, struct i2c_hid_of_goodix, ops); + /* + * Don't assert reset GPIO or disable regulators if we're keeping the + * device powered in suspend. + */ + if (ihid_goodix->powered_in_suspend) + return; + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1); regulator_disable(ihid_goodix->vddio); regulator_disable(ihid_goodix->vdd); @@ -91,6 +120,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client) if (IS_ERR(ihid_goodix->vddio)) return PTR_ERR(ihid_goodix->vddio); + ihid_goodix->powered_in_suspend = + of_property_read_bool(client->dev.of_node, "powered-in-suspend"); + ihid_goodix->timings = device_get_match_data(&client->dev); return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0); -- 2.40.0.634.g4ca3ef3211-goog
next prev parent reply other threads:[~2023-04-18 12:51 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-18 12:49 [PATCH 0/2] Fix Goodix touchscreen power leakage for MT8186 boards Fei Shao 2023-04-18 12:49 ` Fei Shao 2023-04-18 12:49 ` [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property Fei Shao 2023-04-18 12:49 ` Fei Shao 2023-04-18 14:18 ` Doug Anderson 2023-04-18 18:02 ` Matthias Brugger 2023-04-19 0:20 ` Jeff LaBundy 2023-04-19 10:44 ` Fei Shao 2023-04-19 14:38 ` Doug Anderson 2023-04-19 15:18 ` Jeff LaBundy 2023-04-19 15:18 ` Jeff LaBundy 2023-04-19 15:41 ` Doug Anderson 2023-04-19 15:41 ` Doug Anderson 2023-04-20 8:13 ` Fei Shao 2023-04-24 3:31 ` Jeff LaBundy 2023-04-24 18:14 ` Doug Anderson 2023-04-24 18:14 ` Doug Anderson 2023-04-25 8:34 ` Fei Shao 2023-04-18 12:49 ` Fei Shao [this message] 2023-04-18 12:49 ` [PATCH 2/2] HID: i2c-hid: goodix: Add support for " Fei Shao 2023-04-18 14:22 ` Doug Anderson 2023-04-24 3:38 ` Jeff LaBundy 2023-04-24 18:16 ` Doug Anderson 2023-04-24 18:16 ` Doug Anderson 2023-04-25 8:36 ` Fei Shao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230418124953.3170028-3-fshao@chromium.org \ --to=fshao@chromium.org \ --cc=benjamin.tissoires@redhat.com \ --cc=dianders@chromium.org \ --cc=dmitry.torokhov@gmail.com \ --cc=jikos@kernel.org \ --cc=linux-input@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=mka@chromium.org \ --cc=robh+dt@kernel.org \ --cc=steve@sk2.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.