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@xxxxxxxxxxxx>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>
> 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
