| Message ID | 20201125141022.321643-1-coiby.xu@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v4] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status | expand |
On Wed, Nov 25, 2020 at 10:10:22PM +0800, Coiby Xu wrote: > For a broken touchpad, it may take several months or longer to be fixed. > Polling mode could be a fallback solution for enthusiastic Linux users > when they have a new laptop. It also acts like a debugging feature. If > polling mode works for a broken touchpad, we can almost be certain > the root cause is related to the interrupt or power setting. > > This patch could fix touchpads of Lenovo AMD gaming laptops including > Legion-5 15ARH05 (R7000), Legion-5P (R7000P) and IdeaPad Gaming 3 > 15ARH05. > > When polling mode is enabled, an I2C device can't wake up the suspended > system since enable/disable_irq_wake is invalid for polling mode. > > Three module parameters are added to i2c-hid, > - polling_mode: by default set to 0, i.e., polling is disabled > - polling_interval_idle_ms: the polling internal when the touchpad > is idle, default to 10ms > - polling_interval_active_us: the polling internal when the touchpad > is active, default to 4000us > > User can change the last two runtime polling parameter by writing to > /sys/module/i2c_hid/parameters/polling_interval_{idle_ms,active_us}. > > Note xf86-input-synaptics doesn't work well with this polling mode > for the Synaptics touchpad. The Synaptics touchpad would often locks > into scroll mode when using multitouch gestures [1]. One remedy is to > decrease the polling interval. > > Thanks to Barnabás's thorough review of this patch and the useful > feedback! > > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190/comments/235 > > Cc: <stable@vger.kernel.org> > Cc: Barnabás Pőcze <pobrn@protonmail.com> > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 > Signed-off-by: Coiby Xu <coiby.xu@gmail.com> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 152 +++++++++++++++++++++++++++-- > 1 file changed, 142 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index aeff1ffb0c8b..f25503f31ccf 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -36,6 +36,8 @@ > #include <linux/hid.h> > #include <linux/mutex.h> > #include <linux/acpi.h> > +#include <linux/kthread.h> > +#include <linux/gpio/driver.h> > #include <linux/of.h> > #include <linux/regulator/consumer.h> > > @@ -60,6 +62,25 @@ > #define I2C_HID_PWR_ON 0x00 > #define I2C_HID_PWR_SLEEP 0x01 > > +/* polling mode */ > +#define I2C_HID_POLLING_DISABLED 0 > +#define I2C_HID_POLLING_GPIO_PIN 1 > +#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000 > +#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10 > + > +static u8 polling_mode; > +module_param(polling_mode, byte, 0444); > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); Module parameters are for the 1990's, they are global and horrible to try to work with. You should provide something on a per-device basis, as what happens if your system requires different things here for different devices? You set this for all devices :( thanks, greg k-h
2020. november 25., szerda 16:07 keltezéssel, Greg KH írta: > [...] > > +static u8 polling_mode; > > +module_param(polling_mode, byte, 0444); > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); > > Module parameters are for the 1990's, they are global and horrible to > try to work with. You should provide something on a per-device basis, > as what happens if your system requires different things here for > different devices? You set this for all devices :( > [...] Hi do you think something like what the usbcore has would be better? A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"? Regards, Barnabás Pőcze
On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote: > 2020. november 25., szerda 16:07 keltezéssel, Greg KH írta: > > > [...] > > > +static u8 polling_mode; > > > +module_param(polling_mode, byte, 0444); > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); > > > > Module parameters are for the 1990's, they are global and horrible to > > try to work with. You should provide something on a per-device basis, > > as what happens if your system requires different things here for > > different devices? You set this for all devices :( > > [...] > > Hi > > do you think something like what the usbcore has would be better? > A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"? Not really, that's just for debugging, and asking users to test something, not for a final solution to anything. thanks, greg k-h
2020. december 9., szerda 8:00 keltezéssel, Greg KH írta: > On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote: > > > 2020. november 25., szerda 16:07 keltezéssel, Greg KH írta: > > > > > [...] > > > > > > > +static u8 polling_mode; > > > > +module_param(polling_mode, byte, 0444); > > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); > > > > > > Module parameters are for the 1990's, they are global and horrible to > > > try to work with. You should provide something on a per-device basis, > > > as what happens if your system requires different things here for > > > different devices? You set this for all devices :( > > > [...] > > > > Hi > > do you think something like what the usbcore has would be better? > > A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"? > > Not really, that's just for debugging, and asking users to test > something, not for a final solution to anything. My understanding is that this polling mode option is by no means intended as a final solution, it's purely for debugging/fallback: "Polling mode could be a fallback solution for enthusiastic Linux users when they have a new laptop. It also acts like a debugging feature. If polling mode works for a broken touchpad, we can almost be certain the root cause is related to the interrupt or power setting." What would you suggest instead of the module parameter? Regards, Barnabás Pőcze
On Wed, Dec 09, 2020 at 03:38:11PM +0000, Barnabás Pőcze wrote: > 2020. december 9., szerda 8:00 keltezéssel, Greg KH írta: > > > On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote: > > > > > 2020. november 25., szerda 16:07 keltezéssel, Greg KH írta: > > > > > > > [...] > > > > > > > > > +static u8 polling_mode; > > > > > +module_param(polling_mode, byte, 0444); > > > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); > > > > > > > > Module parameters are for the 1990's, they are global and horrible to > > > > try to work with. You should provide something on a per-device basis, > > > > as what happens if your system requires different things here for > > > > different devices? You set this for all devices :( > > > > [...] > > > > > > Hi > > > do you think something like what the usbcore has would be better? > > > A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"? > > > > Not really, that's just for debugging, and asking users to test > > something, not for a final solution to anything. > > My understanding is that this polling mode option is by no means intended > as a final solution, it's purely for debugging/fallback: > > "Polling mode could be a fallback solution for enthusiastic Linux users > when they have a new laptop. It also acts like a debugging feature. If > polling mode works for a broken touchpad, we can almost be certain > the root cause is related to the interrupt or power setting." > > What would you suggest instead of the module parameter? a debugfs file? That means it's only for debugging and you have to be root to change the value. thanks, greg k-h
Hi Greg and Barnabás, On Wed, Dec 09, 2020 at 04:44:35PM +0100, Greg KH wrote: >On Wed, Dec 09, 2020 at 03:38:11PM +0000, Barnabás Pőcze wrote: >> 2020. december 9., szerda 8:00 keltezéssel, Greg KH írta: >> >> > On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote: >> > >> > > 2020. november 25., szerda 16:07 keltezéssel, Greg KH írta: >> > > >> > > > [...] >> > > > >> > > > > +static u8 polling_mode; >> > > > > +module_param(polling_mode, byte, 0444); >> > > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); >> > > > >> > > > Module parameters are for the 1990's, they are global and horrible to >> > > > try to work with. You should provide something on a per-device basis, >> > > > as what happens if your system requires different things here for >> > > > different devices? You set this for all devices :( >> > > > [...] Thank you for pointing out that. >> > > >> > > Hi >> > > do you think something like what the usbcore has would be better? >> > > A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"? >> > >> > Not really, that's just for debugging, and asking users to test >> > something, not for a final solution to anything. >> This patch is not only for debugging. The primary reason is as a fallback solution to save the touchpad. The mentioned touchpads will be fixed by Linux 5.11 which means the enthusiastic Linux users have to wait for ~10 months to get their touchpads fixed. >> My understanding is that this polling mode option is by no means intended >> as a final solution, it's purely for debugging/fallback: >> >> "Polling mode could be a fallback solution for enthusiastic Linux users >> when they have a new laptop. It also acts like a debugging feature. If >> polling mode works for a broken touchpad, we can almost be certain >> the root cause is related to the interrupt or power setting." >> >> What would you suggest instead of the module parameter? > >a debugfs file? That means it's only for debugging and you have to be >root to change the value. > Thank you for the helpful discussion and the suggestions! If we can answer the following two questions, it could help decide what's the next move. 1. How many machines have two or more I2C HID devices? For the laptops this patch try to fix, they only have one I2C HID device, i.e., the touchpad. If it's the case with most machines running Linux, then we don't need to support per-HID-I2C-device setting and module parameter is the most user-friendly since the user doesn't even need to know the <vendor-id>/<product-id> pair. 2. How many I2C HID devices like touchpads could be saved by this patch in the future? If polling could potentially save lots of I2C hid devices, we are more motivated to make it easier to use. -- Best regards, Coiby
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index aeff1ffb0c8b..f25503f31ccf 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -36,6 +36,8 @@ #include <linux/hid.h> #include <linux/mutex.h> #include <linux/acpi.h> +#include <linux/kthread.h> +#include <linux/gpio/driver.h> #include <linux/of.h> #include <linux/regulator/consumer.h> @@ -60,6 +62,25 @@ #define I2C_HID_PWR_ON 0x00 #define I2C_HID_PWR_SLEEP 0x01 +/* polling mode */ +#define I2C_HID_POLLING_DISABLED 0 +#define I2C_HID_POLLING_GPIO_PIN 1 +#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000 +#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10 + +static u8 polling_mode; +module_param(polling_mode, byte, 0444); +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status"); + +static unsigned int polling_interval_active_us __read_mostly = I2C_HID_POLLING_INTERVAL_ACTIVE_US; +module_param(polling_interval_active_us, uint, 0644); +MODULE_PARM_DESC(polling_interval_active_us, + "Poll every {polling_interval_active_us} us when the touchpad is active (default=" __stringify(I2C_HID_POLLING_INTERVAL_ACTIVE_US) " us)"); + +static unsigned int polling_interval_idle_ms __read_mostly = I2C_HID_POLLING_INTERVAL_IDLE_MS; +module_param(polling_interval_idle_ms, uint, 0644); +MODULE_PARM_DESC(polling_interval_idle_ms, + "Poll every {polling_interval_idle_ms} ms when the touchpad is idle (default=" __stringify(I2C_HID_POLLING_INTERVAL_IDLE_MS) "ms)"); /* debug option */ static bool debug; module_param(debug, bool, 0444); @@ -158,6 +179,10 @@ struct i2c_hid { struct i2c_hid_platform_data pdata; + struct task_struct *polling_thread; + unsigned long irq_trigger_type; + struct irq_desc *irq_desc; + bool irq_wake_enabled; struct mutex reset_lock; }; @@ -772,7 +797,9 @@ static int i2c_hid_start(struct hid_device *hid) i2c_hid_free_buffers(ihid); ret = i2c_hid_alloc_buffers(ihid, bufsize); - enable_irq(client->irq); + + if (polling_mode == I2C_HID_POLLING_DISABLED) + enable_irq(client->irq); if (ret) return ret; @@ -814,6 +841,96 @@ struct hid_ll_driver i2c_hid_ll_driver = { }; EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); +static int get_gpio_pin_state(struct irq_desc *irq_desc) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(&irq_desc->irq_data); + int value; + + /* + * This part of code is borrowed from gpiod_get_raw_value_commit in + * drivers/gpio/gpiolib.c. Ideally, gpiod_get_value_cansleep can be re-used + * instead but there is no API of converting (struct irq_desc *) to + * (struct gpio_desc*). + */ + value = gc->get ? gc->get(gc, irq_desc->irq_data.hwirq) : -EIO; + value = value < 0 ? value : !!value; + return value; +} + +static bool interrupt_line_active(struct i2c_hid *ihid) +{ + int status = get_gpio_pin_state(ihid->irq_desc); + struct i2c_client *client = ihid->client; + + if (status < 0) { + dev_dbg_ratelimited(&client->dev, + "Failed to get GPIO Interrupt line status for %s", + client->name); + return false; + } + /* + * According to Windows Precsiontion Touchpad's specs + * https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity, + * GPIO Interrupt Assertion Leve could be either ActiveLow or + * ActiveHigh. + */ + if (ihid->irq_trigger_type & IRQF_TRIGGER_LOW) + return !status; + + return status; +} + +static int i2c_hid_polling_thread(void *i2c_hid) +{ + struct i2c_hid *ihid = i2c_hid; + unsigned int polling_interval_idle; + + while (!kthread_should_stop()) { + while (interrupt_line_active(i2c_hid) && + !test_bit(I2C_HID_READ_PENDING, &ihid->flags) && + !kthread_should_stop()) { + i2c_hid_get_input(ihid); + usleep_range(polling_interval_active_us, + polling_interval_active_us + 100); + } + /* + * re-calculate polling_interval_idle + * so the module parameters polling_interval_idle_ms can be + * changed dynamically through sysfs as polling_interval_active_us + */ + polling_interval_idle = polling_interval_idle_ms * 1000; + usleep_range(polling_interval_idle, + polling_interval_idle + 1000); + } + + return 0; +} + +static int i2c_hid_init_polling(struct i2c_hid *ihid) +{ + struct i2c_client *client = ihid->client; + + ihid->irq_trigger_type = irq_get_trigger_type(client->irq); + if (!ihid->irq_trigger_type) { + dev_dbg(&client->dev, + "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s", + client->name); + return -EINVAL; + } + + ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid, + "I2C HID polling thread"); + + if (IS_ERR(ihid->polling_thread)) { + dev_err(&client->dev, "Failed to create I2C HID polling thread"); + return PTR_ERR(ihid->polling_thread); + } + + ihid->irq_desc = irq_to_desc(client->irq); + wake_up_process(ihid->polling_thread); + return 0; +} + static int i2c_hid_init_irq(struct i2c_client *client) { struct i2c_hid *ihid = i2c_get_clientdata(client); @@ -1014,6 +1131,15 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client, pdata->post_power_delay_ms = val; } +static void free_irq_or_stop_polling(struct i2c_client *client, + struct i2c_hid *ihid) +{ + if (polling_mode != I2C_HID_POLLING_DISABLED) + kthread_stop(ihid->polling_thread); + else + free_irq(client->irq, ihid); +} + static int i2c_hid_probe(struct i2c_client *client, const struct i2c_device_id *dev_id) { @@ -1109,7 +1235,11 @@ static int i2c_hid_probe(struct i2c_client *client, if (ret < 0) goto err_regulator; - ret = i2c_hid_init_irq(client); + if (polling_mode != I2C_HID_POLLING_DISABLED) + ret = i2c_hid_init_polling(ihid); + else + ret = i2c_hid_init_irq(client); + if (ret < 0) goto err_regulator; @@ -1148,7 +1278,7 @@ static int i2c_hid_probe(struct i2c_client *client, hid_destroy_device(hid); err_irq: - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); err_regulator: regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies), @@ -1165,7 +1295,7 @@ static int i2c_hid_remove(struct i2c_client *client) hid = ihid->hid; hid_destroy_device(hid); - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); if (ihid->bufsize) i2c_hid_free_buffers(ihid); @@ -1181,7 +1311,7 @@ static void i2c_hid_shutdown(struct i2c_client *client) struct i2c_hid *ihid = i2c_get_clientdata(client); i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); i2c_hid_acpi_shutdown(&client->dev); } @@ -1204,15 +1334,16 @@ static int i2c_hid_suspend(struct device *dev) /* Save some power */ i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); - disable_irq(client->irq); + if (polling_mode == I2C_HID_POLLING_DISABLED) + disable_irq(client->irq); - if (device_may_wakeup(&client->dev)) { + if (device_may_wakeup(&client->dev) && polling_mode == I2C_HID_POLLING_DISABLED) { wake_status = enable_irq_wake(client->irq); if (!wake_status) ihid->irq_wake_enabled = true; else hid_warn(hid, "Failed to enable irq wake: %d\n", - wake_status); + wake_status); } else { regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies), ihid->pdata.supplies); @@ -1229,7 +1360,7 @@ static int i2c_hid_resume(struct device *dev) struct hid_device *hid = ihid->hid; int wake_status; - if (!device_may_wakeup(&client->dev)) { + if (!device_may_wakeup(&client->dev) || polling_mode != I2C_HID_POLLING_DISABLED) { ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies), ihid->pdata.supplies); if (ret) @@ -1246,7 +1377,8 @@ static int i2c_hid_resume(struct device *dev) wake_status); } - enable_irq(client->irq); + if (polling_mode == I2C_HID_POLLING_DISABLED) + enable_irq(client->irq); /* Instead of resetting device, simply powers the device on. This * solves "incomplete reports" on Raydium devices 2386:3118 and
For a broken touchpad, it may take several months or longer to be fixed. Polling mode could be a fallback solution for enthusiastic Linux users when they have a new laptop. It also acts like a debugging feature. If polling mode works for a broken touchpad, we can almost be certain the root cause is related to the interrupt or power setting. This patch could fix touchpads of Lenovo AMD gaming laptops including Legion-5 15ARH05 (R7000), Legion-5P (R7000P) and IdeaPad Gaming 3 15ARH05. When polling mode is enabled, an I2C device can't wake up the suspended system since enable/disable_irq_wake is invalid for polling mode. Three module parameters are added to i2c-hid, - polling_mode: by default set to 0, i.e., polling is disabled - polling_interval_idle_ms: the polling internal when the touchpad is idle, default to 10ms - polling_interval_active_us: the polling internal when the touchpad is active, default to 4000us User can change the last two runtime polling parameter by writing to /sys/module/i2c_hid/parameters/polling_interval_{idle_ms,active_us}. Note xf86-input-synaptics doesn't work well with this polling mode for the Synaptics touchpad. The Synaptics touchpad would often locks into scroll mode when using multitouch gestures [1]. One remedy is to decrease the polling interval. Thanks to Barnabás's thorough review of this patch and the useful feedback! [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190/comments/235 Cc: <stable@vger.kernel.org> Cc: Barnabás Pőcze <pobrn@protonmail.com> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Signed-off-by: Coiby Xu <coiby.xu@gmail.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 152 +++++++++++++++++++++++++++-- 1 file changed, 142 insertions(+), 10 deletions(-)