From: Doug Anderson <dianders@chromium.org>
To: Johan Hovold <johan+linaro@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Bjorn Andersson <andersson@kernel.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Linus Walleij <linus.walleij@linaro.org>,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org, Steev Klimaszewski <steev@kali.org>
Subject: Re: [PATCH v2 4/7] HID: i2c-hid: elan: fix reset suspend current leakage
Date: Fri, 10 May 2024 16:36:08 -0700 [thread overview]
Message-ID: <CAD=FV=V59t_tZ9Xk=uhbgOdTRYLKu+kZt8cpaksTkJo+D4yt8Q@mail.gmail.com> (raw)
In-Reply-To: <20240507144821.12275-5-johan+linaro@kernel.org>
Hi,
On Tue, May 7, 2024 at 7:48 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> @@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_elan, ops);
> int ret;
>
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
Could probably use a comment above it saying that this is important
when we have "no_reset_on_power_off" and doesn't do anything bad when
we don't so we can just do it unconditionally.
> +
> if (ihid_elan->vcc33) {
> ret = regulator_enable(ihid_elan->vcc33);
> if (ret)
> - return ret;
> + goto err_deassert_reset;
> }
>
> ret = regulator_enable(ihid_elan->vccio);
> - if (ret) {
> - regulator_disable(ihid_elan->vcc33);
> - return ret;
> - }
> + if (ret)
> + goto err_disable_vcc33;
>
> if (ihid_elan->chip_data->post_power_delay_ms)
> msleep(ihid_elan->chip_data->post_power_delay_ms);
> @@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
> msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
>
> return 0;
> +
> +err_disable_vcc33:
> + if (ihid_elan->vcc33)
> + regulator_disable(ihid_elan->vcc33);
> +err_deassert_reset:
> + if (ihid_elan->no_reset_on_power_off)
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
Small nit about the error label: it sounds as if when you go here you
_will_ deassert reset when in actuality you might or might not
(depending on no_reset_on_power_off). Personally I prefer to label
error jumps based on things I've done instead of things that the error
jump needs to do, so you could call them "err_enabled_vcc33" and
"err_asserted_reset"...
I don't feel that strongly about it, though, so if you love the label
you have then no need to change it.
> @@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_elan *ihid_elan =
> container_of(ops, struct i2c_hid_of_elan, ops);
>
> - gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> + /*
> + * Do not assert reset when the hardware allows for it to remain
> + * deasserted regardless of the state of the (shared) power supply to
> + * avoid wasting power when the supply is left on.
> + */
> + if (!ihid_elan->no_reset_on_power_off)
> + gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
> +
> if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
> msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
Shouldn't the above two lines be inside the "if
(!ihid_elan->no_reset_on_power_off)" test? If you're not setting the
reset GPIO then you don't need to do the delay, right?
> @@ -79,6 +96,7 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
> static int i2c_hid_of_elan_probe(struct i2c_client *client)
> {
> struct i2c_hid_of_elan *ihid_elan;
> + int ret;
>
> ihid_elan = devm_kzalloc(&client->dev, sizeof(*ihid_elan), GFP_KERNEL);
> if (!ihid_elan)
> @@ -93,21 +111,38 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> if (IS_ERR(ihid_elan->reset_gpio))
> return PTR_ERR(ihid_elan->reset_gpio);
>
> + ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node,
> + "no-reset-on-power-off");
Personally, I'd rather you query for the property before you request
the GPIO and then request the GPIO in the "powered off" state just to
keep everything in the most consistent state possible.
-Doug
next prev parent reply other threads:[~2024-05-10 23:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240507144821.12275-1-johan+linaro@kernel.org>
2024-05-07 14:48 ` [PATCH v2 4/7] HID: i2c-hid: elan: fix reset suspend current leakage Johan Hovold
2024-05-10 23:36 ` Doug Anderson [this message]
2024-05-20 11:44 ` Johan Hovold
2024-05-20 11:52 ` Johan Hovold
2024-05-20 15:38 ` Doug Anderson
2024-05-07 14:48 ` [PATCH v2 5/7] arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on Johan Hovold
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='CAD=FV=V59t_tZ9Xk=uhbgOdTRYLKu+kZt8cpaksTkJo+D4yt8Q@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=andersson@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=jikos@kernel.org \
--cc=johan+linaro@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=stable@vger.kernel.org \
--cc=steev@kali.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).