diff mbox series

HID: i2c-hid: goodix: Tie the reset line to true state of the regulator

Message ID 20210611092847.1.I358cae5e33f742765fd38485d6ddf1a4a978644d@changeid (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series HID: i2c-hid: goodix: Tie the reset line to true state of the regulator | expand

Commit Message

Doug Anderson June 11, 2021, 4:28 p.m. UTC
The regulator for the touchscreen could be:
* A dedicated regulator just for the touchscreen.
* A regulator shared with something else in the system.
* An always-on regulator.

How we want the "reset" line to behave depends a bit on which of those
three cases we're in. Currently the code is written with the
assumption that it has a dedicated regulator, but that's not really
guaranteed to be the case.

The problem we run into is that if we leave the touchscreen powered on
(because someone else is requesting the regulator or it's an always-on
regulator) and we assert reset then we apparently burn an extra 67 mW
of power. That's not great.

Let's instead tie the control of the reset line to the true state of
the regulator as reported by regulator notifiers. If we have an
always-on regulator our notifier will never be called. If we have a
shared regulator then our notifier will be called when the touchscreen
is truly turned on or truly turned off.

Using notifiers like this nicely handles all the cases without
resorting to hacks like pretending that there is no "reset" GPIO if we
have an always-on regulator.

NOTE: if the regulator is on a shared line it's still possible that
things could be a little off. Specifically, this case is not handled
even after this patch:
1. Suspend goodix (send "sleep", goodix stops requesting regulator on)
2. Other regulator user turns off (regulator fully turns off).
3. Goodix driver gets notified and asserts reset.
4. Other regulator user turns on.
5. Goodix driver gets notified and deasserts reset.
6. Nobody resumes goodix.

With that set of steps we'll have reset deasserted but we will have
lost the results of the I2C_HID_PWR_SLEEP from the suspend path. That
means we might be in higher power than we could be even if the goodix
driver thinks things are suspended. Presumably, however, we're still
in better shape than if we were asserting "reset" the whole time. If
somehow the above situation is actually affecting someone and we want
to do better we can deal with it when we have a real use case.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 90 +++++++++++++++++++++----
 1 file changed, 77 insertions(+), 13 deletions(-)

Comments

Dan Carpenter June 22, 2021, 8:49 a.m. UTC | #1
Hi Douglas,

url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/HID-i2c-hid-goodix-Tie-the-reset-line-to-true-state-of-the-regulator/20210617-102450
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: i386-randconfig-m021-20210622 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hid/i2c-hid/i2c-hid-of-goodix.c:151 i2c_hid_of_goodix_probe() warn: inconsistent returns '&ihid_goodix->regulator_mutex'.

vim +151 drivers/hid/i2c-hid/i2c-hid-of-goodix.c

c1ed18c11bdb80 Douglas Anderson 2021-01-15   95  static int i2c_hid_of_goodix_probe(struct i2c_client *client,
c1ed18c11bdb80 Douglas Anderson 2021-01-15   96  				   const struct i2c_device_id *id)
c1ed18c11bdb80 Douglas Anderson 2021-01-15   97  {
c1ed18c11bdb80 Douglas Anderson 2021-01-15   98  	struct i2c_hid_of_goodix *ihid_goodix;
2b87ff72c696bc Douglas Anderson 2021-06-11   99  	int ret;
c1ed18c11bdb80 Douglas Anderson 2021-01-15  100  	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
c1ed18c11bdb80 Douglas Anderson 2021-01-15  101  				   GFP_KERNEL);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  102  	if (!ihid_goodix)
c1ed18c11bdb80 Douglas Anderson 2021-01-15  103  		return -ENOMEM;
c1ed18c11bdb80 Douglas Anderson 2021-01-15  104  
2b87ff72c696bc Douglas Anderson 2021-06-11  105  	mutex_init(&ihid_goodix->regulator_mutex);
2b87ff72c696bc Douglas Anderson 2021-06-11  106  
c1ed18c11bdb80 Douglas Anderson 2021-01-15  107  	ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
c1ed18c11bdb80 Douglas Anderson 2021-01-15  108  	ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
c1ed18c11bdb80 Douglas Anderson 2021-01-15  109  
c1ed18c11bdb80 Douglas Anderson 2021-01-15  110  	/* Start out with reset asserted */
c1ed18c11bdb80 Douglas Anderson 2021-01-15  111  	ihid_goodix->reset_gpio =
c1ed18c11bdb80 Douglas Anderson 2021-01-15  112  		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  113  	if (IS_ERR(ihid_goodix->reset_gpio))
c1ed18c11bdb80 Douglas Anderson 2021-01-15  114  		return PTR_ERR(ihid_goodix->reset_gpio);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  115  
c1ed18c11bdb80 Douglas Anderson 2021-01-15  116  	ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
c1ed18c11bdb80 Douglas Anderson 2021-01-15  117  	if (IS_ERR(ihid_goodix->vdd))
c1ed18c11bdb80 Douglas Anderson 2021-01-15  118  		return PTR_ERR(ihid_goodix->vdd);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  119  
c1ed18c11bdb80 Douglas Anderson 2021-01-15  120  	ihid_goodix->timings = device_get_match_data(&client->dev);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  121  
2b87ff72c696bc Douglas Anderson 2021-06-11  122  	/*
2b87ff72c696bc Douglas Anderson 2021-06-11  123  	 * We need to control the "reset" line in lockstep with the regulator
2b87ff72c696bc Douglas Anderson 2021-06-11  124  	 * actually turning on an off instead of just when we make the request.
2b87ff72c696bc Douglas Anderson 2021-06-11  125  	 * This matters if the regulator is shared with another consumer.
2b87ff72c696bc Douglas Anderson 2021-06-11  126  	 * - If the regulator is off then we must assert reset. The reset
2b87ff72c696bc Douglas Anderson 2021-06-11  127  	 *   line is active low and on some boards it could cause a current
2b87ff72c696bc Douglas Anderson 2021-06-11  128  	 *   leak if left high.
2b87ff72c696bc Douglas Anderson 2021-06-11  129  	 * - If the regulator is on then we don't want reset asserted for very
2b87ff72c696bc Douglas Anderson 2021-06-11  130  	 *   long. Holding the controller in reset apparently draws extra
2b87ff72c696bc Douglas Anderson 2021-06-11  131  	 *   power.
2b87ff72c696bc Douglas Anderson 2021-06-11  132  	 */
2b87ff72c696bc Douglas Anderson 2021-06-11  133  	mutex_lock(&ihid_goodix->regulator_mutex);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2b87ff72c696bc Douglas Anderson 2021-06-11  134  	ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
2b87ff72c696bc Douglas Anderson 2021-06-11  135  	ret = regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb);
2b87ff72c696bc Douglas Anderson 2021-06-11  136  	if (ret)
2b87ff72c696bc Douglas Anderson 2021-06-11  137  		return dev_err_probe(&client->dev, ret,
2b87ff72c696bc Douglas Anderson 2021-06-11  138  			"regulator notifier request failed\n");
                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Drop the lock before returning?

2b87ff72c696bc Douglas Anderson 2021-06-11  139  
2b87ff72c696bc Douglas Anderson 2021-06-11  140  	/*
2b87ff72c696bc Douglas Anderson 2021-06-11  141  	 * If someone else is holding the regulator on (or the regulator is
2b87ff72c696bc Douglas Anderson 2021-06-11  142  	 * an always-on one) we might never be told to deassert reset. Do it
2b87ff72c696bc Douglas Anderson 2021-06-11  143  	 * now. Here we'll assume that someone else might have _just
2b87ff72c696bc Douglas Anderson 2021-06-11  144  	 * barely_ turned the regulator on so we'll do the full
2b87ff72c696bc Douglas Anderson 2021-06-11  145  	 * "post_power_delay" just in case.
2b87ff72c696bc Douglas Anderson 2021-06-11  146  	 */
2b87ff72c696bc Douglas Anderson 2021-06-11  147  	if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd))
2b87ff72c696bc Douglas Anderson 2021-06-11  148  		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
2b87ff72c696bc Douglas Anderson 2021-06-11  149  	mutex_unlock(&ihid_goodix->regulator_mutex);
2b87ff72c696bc Douglas Anderson 2021-06-11  150  
c1ed18c11bdb80 Douglas Anderson 2021-01-15 @151  	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001);
c1ed18c11bdb80 Douglas Anderson 2021-01-15  152  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index ee0225982a82..c13ea29c7911 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -26,28 +26,29 @@  struct i2c_hid_of_goodix {
 	struct i2chid_ops ops;
 
 	struct regulator *vdd;
+	struct notifier_block nb;
+	struct mutex regulator_mutex;
 	struct gpio_desc *reset_gpio;
 	const struct goodix_i2c_hid_timing_data *timings;
 };
 
-static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
+static void goodix_i2c_hid_deassert_reset(struct i2c_hid_of_goodix *ihid_goodix,
+					  bool regulator_just_turned_on)
 {
-	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)
+	if (regulator_just_turned_on && 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);
+}
 
-	return 0;
+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);
 }
 
 static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
@@ -55,20 +56,54 @@  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;
+
+	mutex_lock(&ihid_goodix->regulator_mutex);
+
+	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;
+	}
+
+	mutex_unlock(&ihid_goodix->regulator_mutex);
+
+	return ret;
+}
+
 static int i2c_hid_of_goodix_probe(struct i2c_client *client,
 				   const struct i2c_device_id *id)
 {
 	struct i2c_hid_of_goodix *ihid_goodix;
-
+	int ret;
 	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
 				   GFP_KERNEL);
 	if (!ihid_goodix)
 		return -ENOMEM;
 
+	mutex_init(&ihid_goodix->regulator_mutex);
+
 	ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
 	ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
 
@@ -84,6 +119,35 @@  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.
+	 */
+	mutex_lock(&ihid_goodix->regulator_mutex);
+	ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
+	ret = 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. Here we'll 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))
+		goodix_i2c_hid_deassert_reset(ihid_goodix, true);
+	mutex_unlock(&ihid_goodix->regulator_mutex);
+
 	return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001);
 }