Message-ID: <Y+HMejpI7DoKwWYm@google.com>
Date:   Mon, 6 Feb 2023 19:58:50 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, Stephen Kitt <steve@....org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] HID: i2c-hid: goodix: Stop tying the reset line to
 the regulator

On Mon, Feb 06, 2023 at 06:48:13PM -0800, Douglas Anderson wrote:
> In commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to
> true state of the regulator"), we started tying the reset line of
> Goodix touchscreens to the regulator.
> 
> The primary motivation for that patch was some pre-production hardware
> (specifically sc7180-trogdor-homestar) where it was proposed to hook
> the touchscreen's main 3.3V power rail to an always-on supply. In such
> a case, when we turned "off" the touchscreen in Linux it was bad to
> assert the "reset" GPIO because that was causing a power drain. The
> patch accomplished that goal and did it in a general sort of way that
> didn't require special properties to be added in the device tree for
> homestar.
> 
> It turns out that the design of using an always-on power rail for the
> touchscreen was rejected soon after the patch was written and long
> before sc7180-trogdor-homestar went into production. The final design
> of homestar actually fully separates the rail for the touchscreen and
> the display panel and both can be powered off and on. That means that
> the original motivation for the feature is gone.
> 
> There are 3 other users of the goodix i2c-hid driver in mainline.
> 
> I'll first talk about 2 of the other users in mainline: coachz and
> mrbland. On both coachz and mrbland the touchscreen power and panel
> power _are_ shared. That means that the patch to tie the reset line to
> the true state of the regulator _is_ doing something on those
> boards. Specifically, the patch reduced power consumption by tens of
> mA in the case where we turned the touchscreen off but left the panel
> on. Other than saving a small bit of power, the patch wasn't truly
> necessary. That being said, even though a small bit of power was saved
> in the state of "panel on + touchscreen off", that's not actually a
> state we ever expect to be in, except perhaps for very short periods
> of time at boot or during suspend/resume. Thus, the patch is truly not
> necessary. It should be further noted that, as documented in the
> original patch, the current code still didn't optimize power for every
> corner case of the "shared rail" situation.
> 
> The last user in mainline was very recently added: evoker. Evoker is
> actually the motivation for me removing this bit of code. It turns out
> that for evoker we need to manage a second power rail for IO to the
> touchscreen. Trying to fit the management of this IO rail into the
> regulator notifiers turns out to be extremely hard. To avoid lockdep
> splats you shouldn't enable/disable other regulators in regulator
> notifiers and trying to find a way around this was going to be fairly
> difficult.
> 
> Given the lack of any true motivation to tie the reset line to the
> regulator, lets go back to the simpler days and remove the code. This
> is, effectively, a revert of commit bdbc65eb77ee ("HID: i2c-hid:
> goodix: Fix a lockdep splat"), commit 25ddd7cfc582 ("HID: i2c-hid:
> goodix: Use the devm variant of regulator_register_notifier()"), and
> commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
> state of the regulator").
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.com>

> ---
> 
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 88 ++++---------------------
>  1 file changed, 13 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 29c6cb174032..584d833dc0aa 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -26,28 +26,28 @@ struct i2c_hid_of_goodix {
>  	struct i2chid_ops ops;
>  
>  	struct regulator *vdd;
> -	struct notifier_block nb;
>  	struct gpio_desc *reset_gpio;
>  	const struct goodix_i2c_hid_timing_data *timings;
>  };
>  
> -static void goodix_i2c_hid_deassert_reset(struct i2c_hid_of_goodix *ihid_goodix,
> -					  bool regulator_just_turned_on)
> +static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
>  {
> -	if (regulator_just_turned_on && ihid_goodix->timings->post_power_delay_ms)
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(ops, struct i2c_hid_of_goodix, ops);
> +	int ret;
> +
> +	ret = regulator_enable(ihid_goodix->vdd);
> +	if (ret)
> +		return ret;
> +
> +	if (ihid_goodix->timings->post_power_delay_ms)
>  		msleep(ihid_goodix->timings->post_power_delay_ms);
>  
>  	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
>  	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
>  		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
> -}
> -
> -static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> -{
> -	struct i2c_hid_of_goodix *ihid_goodix =
> -		container_of(ops, struct i2c_hid_of_goodix, ops);
>  
> -	return regulator_enable(ihid_goodix->vdd);
> +	return 0;
>  }
>  
>  static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> @@ -55,42 +55,14 @@ 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);
>  
> +	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
>  	regulator_disable(ihid_goodix->vdd);
>  }
>  
> -static int ihid_goodix_vdd_notify(struct notifier_block *nb,
> -				    unsigned long event,
> -				    void *ignored)
> -{
> -	struct i2c_hid_of_goodix *ihid_goodix =
> -		container_of(nb, struct i2c_hid_of_goodix, nb);
> -	int ret = NOTIFY_OK;
> -
> -	switch (event) {
> -	case REGULATOR_EVENT_PRE_DISABLE:
> -		gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> -		break;
> -
> -	case REGULATOR_EVENT_ENABLE:
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
> -		break;
> -
> -	case REGULATOR_EVENT_ABORT_DISABLE:
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, false);
> -		break;
> -
> -	default:
> -		ret = NOTIFY_DONE;
> -		break;
> -	}
> -
> -	return ret;
> -}
> -
>  static int i2c_hid_of_goodix_probe(struct i2c_client *client)
>  {
>  	struct i2c_hid_of_goodix *ihid_goodix;
> -	int ret;
> +
>  	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
>  				   GFP_KERNEL);
>  	if (!ihid_goodix)
> @@ -111,40 +83,6 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
>  
>  	ihid_goodix->timings = device_get_match_data(&client->dev);
>  
> -	/*
> -	 * We need to control the "reset" line in lockstep with the regulator
> -	 * actually turning on an off instead of just when we make the request.
> -	 * This matters if the regulator is shared with another consumer.
> -	 * - If the regulator is off then we must assert reset. The reset
> -	 *   line is active low and on some boards it could cause a current
> -	 *   leak if left high.
> -	 * - If the regulator is on then we don't want reset asserted for very
> -	 *   long. Holding the controller in reset apparently draws extra
> -	 *   power.
> -	 */
> -	ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
> -	ret = devm_regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb);
> -	if (ret)
> -		return dev_err_probe(&client->dev, ret,
> -			"regulator notifier request failed\n");
> -
> -	/*
> -	 * If someone else is holding the regulator on (or the regulator is
> -	 * an always-on one) we might never be told to deassert reset. Do it
> -	 * now... and temporarily bump the regulator reference count just to
> -	 * make sure it is impossible for this to race with our own notifier!
> -	 * We also assume that someone else might have _just barely_ turned
> -	 * the regulator on so we'll do the full "post_power_delay" just in
> -	 * case.
> -	 */
> -	if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) {
> -		ret = regulator_enable(ihid_goodix->vdd);
> -		if (ret)
> -			return ret;
> -		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
> -		regulator_disable(ihid_goodix->vdd);
> -	}
> -
>  	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
>  }
>  
> -- 
> 2.39.1.519.gcb327c4b5f-goog
> 

-- 
Dmitry

Powered by blists - more mailing lists