linux/drivers/hid/i2c-hid/i2c-hid-of-goodix.c

178 lines
5.1 KiB
C
Raw Normal View History

// SPDX-License-Identifier: GPL-2.0
/*
* Driver for Goodix touchscreens that use the i2c-hid protocol.
*
* Copyright 2020 Google LLC
*/
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/pm.h>
#include <linux/regulator/consumer.h>
#include "i2c-hid.h"
struct goodix_i2c_hid_timing_data {
unsigned int post_gpio_reset_delay_ms;
unsigned int post_power_delay_ms;
};
struct i2c_hid_of_goodix {
struct i2chid_ops ops;
struct regulator *vdd;
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
struct notifier_block nb;
struct gpio_desc *reset_gpio;
const struct goodix_i2c_hid_timing_data *timings;
};
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
static void goodix_i2c_hid_deassert_reset(struct i2c_hid_of_goodix *ihid_goodix,
bool regulator_just_turned_on)
{
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
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);
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
}
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
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)
{
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
regulator_disable(ihid_goodix->vdd);
}
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
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;
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
int ret;
ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
GFP_KERNEL);
if (!ihid_goodix)
return -ENOMEM;
ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
/* Start out with reset asserted */
ihid_goodix->reset_gpio =
devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(ihid_goodix->reset_gpio))
return PTR_ERR(ihid_goodix->reset_gpio);
ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
if (IS_ERR(ihid_goodix->vdd))
return PTR_ERR(ihid_goodix->vdd);
ihid_goodix->timings = device_get_match_data(&client->dev);
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
/*
* 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);
HID: i2c-hid: goodix: Fix a lockdep splat I'm was on the receiving end of a lockdep splat from this driver and after scratching my head I couldn't be entirely sure it was a false positive given we would also have to think about whether the regulator locking is safe (since the notifier is called whilst holding regulator locks which are also needed for regulator_is_enabled() ). Regardless of whether it is a real bug or not, the mutex isn't needed. We can use reference counting tricks instead to avoid races with the notifier calls. The observed splat follows: ------------------------------------------------------ kworker/u16:3/127 is trying to acquire lock: ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94 but task is already holding lock: ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}: down_write+0x68/0x8c blocking_notifier_chain_register+0x54/0x70 regulator_register_notifier+0x1c/0x24 devm_regulator_register_notifier+0x58/0x98 i2c_hid_of_goodix_probe+0xdc/0x158 i2c_device_probe+0x25d/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}: __lock_acquire+0xd24/0xfe8 lock_acquire+0x288/0x2f4 __mutex_lock+0xa0/0x338 mutex_lock_nested+0x3c/0x5c ihid_goodix_vdd_notify+0x30/0x94 notifier_call_chain+0x6c/0x8c blocking_notifier_call_chain+0x48/0x70 _notifier_call_chain.isra.0+0x18/0x20 _regulator_enable+0xc0/0x178 regulator_enable+0x40/0x7c goodix_i2c_hid_power_up+0x18/0x20 i2c_hid_core_power_up.isra.0+0x1c/0x2c i2c_hid_core_probe+0xd8/0x3d4 i2c_hid_of_goodix_probe+0x14c/0x158 i2c_device_probe+0x25c/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); *** DEADLOCK *** Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2022-01-28 17:46:25 +00:00
if (ret)
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
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
HID: i2c-hid: goodix: Fix a lockdep splat I'm was on the receiving end of a lockdep splat from this driver and after scratching my head I couldn't be entirely sure it was a false positive given we would also have to think about whether the regulator locking is safe (since the notifier is called whilst holding regulator locks which are also needed for regulator_is_enabled() ). Regardless of whether it is a real bug or not, the mutex isn't needed. We can use reference counting tricks instead to avoid races with the notifier calls. The observed splat follows: ------------------------------------------------------ kworker/u16:3/127 is trying to acquire lock: ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94 but task is already holding lock: ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}: down_write+0x68/0x8c blocking_notifier_chain_register+0x54/0x70 regulator_register_notifier+0x1c/0x24 devm_regulator_register_notifier+0x58/0x98 i2c_hid_of_goodix_probe+0xdc/0x158 i2c_device_probe+0x25d/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}: __lock_acquire+0xd24/0xfe8 lock_acquire+0x288/0x2f4 __mutex_lock+0xa0/0x338 mutex_lock_nested+0x3c/0x5c ihid_goodix_vdd_notify+0x30/0x94 notifier_call_chain+0x6c/0x8c blocking_notifier_call_chain+0x48/0x70 _notifier_call_chain.isra.0+0x18/0x20 _regulator_enable+0xc0/0x178 regulator_enable+0x40/0x7c goodix_i2c_hid_power_up+0x18/0x20 i2c_hid_core_power_up.isra.0+0x1c/0x2c i2c_hid_core_probe+0xd8/0x3d4 i2c_hid_of_goodix_probe+0x14c/0x158 i2c_device_probe+0x25c/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); *** DEADLOCK *** Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2022-01-28 17:46:25 +00:00
* 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.
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
*/
HID: i2c-hid: goodix: Fix a lockdep splat I'm was on the receiving end of a lockdep splat from this driver and after scratching my head I couldn't be entirely sure it was a false positive given we would also have to think about whether the regulator locking is safe (since the notifier is called whilst holding regulator locks which are also needed for regulator_is_enabled() ). Regardless of whether it is a real bug or not, the mutex isn't needed. We can use reference counting tricks instead to avoid races with the notifier calls. The observed splat follows: ------------------------------------------------------ kworker/u16:3/127 is trying to acquire lock: ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94 but task is already holding lock: ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}: down_write+0x68/0x8c blocking_notifier_chain_register+0x54/0x70 regulator_register_notifier+0x1c/0x24 devm_regulator_register_notifier+0x58/0x98 i2c_hid_of_goodix_probe+0xdc/0x158 i2c_device_probe+0x25d/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}: __lock_acquire+0xd24/0xfe8 lock_acquire+0x288/0x2f4 __mutex_lock+0xa0/0x338 mutex_lock_nested+0x3c/0x5c ihid_goodix_vdd_notify+0x30/0x94 notifier_call_chain+0x6c/0x8c blocking_notifier_call_chain+0x48/0x70 _notifier_call_chain.isra.0+0x18/0x20 _regulator_enable+0xc0/0x178 regulator_enable+0x40/0x7c goodix_i2c_hid_power_up+0x18/0x20 i2c_hid_core_power_up.isra.0+0x1c/0x2c i2c_hid_core_probe+0xd8/0x3d4 i2c_hid_of_goodix_probe+0x14c/0x158 i2c_device_probe+0x25c/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); *** DEADLOCK *** Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2022-01-28 17:46:25 +00:00
if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) {
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
goodix_i2c_hid_deassert_reset(ihid_goodix, true);
HID: i2c-hid: goodix: Fix a lockdep splat I'm was on the receiving end of a lockdep splat from this driver and after scratching my head I couldn't be entirely sure it was a false positive given we would also have to think about whether the regulator locking is safe (since the notifier is called whilst holding regulator locks which are also needed for regulator_is_enabled() ). Regardless of whether it is a real bug or not, the mutex isn't needed. We can use reference counting tricks instead to avoid races with the notifier calls. The observed splat follows: ------------------------------------------------------ kworker/u16:3/127 is trying to acquire lock: ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94 but task is already holding lock: ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}: down_write+0x68/0x8c blocking_notifier_chain_register+0x54/0x70 regulator_register_notifier+0x1c/0x24 devm_regulator_register_notifier+0x58/0x98 i2c_hid_of_goodix_probe+0xdc/0x158 i2c_device_probe+0x25d/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}: __lock_acquire+0xd24/0xfe8 lock_acquire+0x288/0x2f4 __mutex_lock+0xa0/0x338 mutex_lock_nested+0x3c/0x5c ihid_goodix_vdd_notify+0x30/0x94 notifier_call_chain+0x6c/0x8c blocking_notifier_call_chain+0x48/0x70 _notifier_call_chain.isra.0+0x18/0x20 _regulator_enable+0xc0/0x178 regulator_enable+0x40/0x7c goodix_i2c_hid_power_up+0x18/0x20 i2c_hid_core_power_up.isra.0+0x1c/0x2c i2c_hid_core_probe+0xd8/0x3d4 i2c_hid_of_goodix_probe+0x14c/0x158 i2c_device_probe+0x25c/0x270 really_probe+0x174/0x2cc __driver_probe_device+0xc0/0xd8 driver_probe_device+0x50/0xe4 __device_attach_driver+0xa8/0xc0 bus_for_each_drv+0x9c/0xc0 __device_attach_async_helper+0x6c/0xbc async_run_entry_fn+0x38/0x100 process_one_work+0x294/0x438 worker_thread+0x180/0x258 kthread+0x120/0x130 ret_from_fork+0x10/0x20 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); lock(&(&rdev->notifier)->rwsem); lock(&ihid_goodix->regulator_mutex); *** DEADLOCK *** Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2022-01-28 17:46:25 +00:00
regulator_disable(ihid_goodix->vdd);
}
HID: i2c-hid: goodix: Tie the reset line to true state of the regulator 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> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
2021-06-25 08:18:36 -07:00
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
}
static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
.post_power_delay_ms = 10,
.post_gpio_reset_delay_ms = 180,
};
static const struct of_device_id goodix_i2c_hid_of_match[] = {
{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
{ }
};
MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
static struct i2c_driver goodix_i2c_hid_ts_driver = {
.driver = {
.name = "i2c_hid_of_goodix",
.pm = &i2c_hid_core_pm,
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
.of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
},
.probe_new = i2c_hid_of_goodix_probe,
.remove = i2c_hid_core_remove,
.shutdown = i2c_hid_core_shutdown,
};
module_i2c_driver(goodix_i2c_hid_ts_driver);
MODULE_AUTHOR("Douglas Anderson <dianders@chromium.org>");
MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
MODULE_LICENSE("GPL v2");