Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
@ 2020-10-09  8:11 Coiby Xu
  2020-10-15 10:33 ` Barnabás Pőcze
  0 siblings, 1 reply; 3+ messages in thread
From: Coiby Xu @ 2020-10-09  8:11 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, open list

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.

Two module parameters are added to i2c-hid,
    - polling_mode: by default set to 0, i.e., polling is disabled
    - polling_interval (ms): user can change this runtime parameter by
      writing to /sys/module/i2c_hid/parameters/polling_interval

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 127 +++++++++++++++++++++++++++--
 1 file changed, 119 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index dbd04492825d..c7af127959f6 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,18 @@
 #define I2C_HID_PWR_ON		0x00
 #define I2C_HID_PWR_SLEEP	0x01

+/* polling mode */
+#define I2C_POLLING_DISABLED 0
+#define I2C_POLLING_GPIO_PIN 1
+#define POLLING_INTERVAL 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO pin's status");
+
+static unsigned int polling_interval = 10;
+module_param(polling_interval, uint, 0644);
+MODULE_PARM_DESC(polling_interval, "Poll every {polling_interval} ms. Default to 10 ms");
 /* debug option */
 static bool debug;
 module_param(debug, bool, 0444);
@@ -158,6 +172,8 @@ struct i2c_hid {

 	struct i2c_hid_platform_data pdata;

+	struct task_struct *polling_thread;
+
 	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
 };
@@ -772,7 +788,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_POLLING_DISABLED)
+			enable_irq(client->irq);

 		if (ret)
 			return ret;
@@ -814,6 +832,83 @@ 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);
+
+	return gc->get(gc, irq_desc->irq_data.hwirq);
+}
+
+static bool interrupt_line_active(struct i2c_client *client)
+{
+	unsigned long trigger_type = irq_get_trigger_type(client->irq);
+	struct irq_desc *irq_desc = irq_to_desc(client->irq);
+
+	/*
+	 * 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 (trigger_type & IRQF_TRIGGER_LOW)
+		return !get_gpio_pin_state(irq_desc);
+
+	return get_gpio_pin_state(irq_desc);
+}
+
+static int i2c_hid_polling_thread(void *i2c_hid)
+{
+	struct i2c_hid *ihid = i2c_hid;
+	struct i2c_client *client = ihid->client;
+	unsigned int polling_interval_ms;
+
+	while (1) {
+		polling_interval_ms = polling_interval*1000;
+		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
+			usleep_range(50000, 100000);
+
+		if (kthread_should_stop())
+			break;
+
+		while (interrupt_line_active(client)) {
+			i2c_hid_get_input(ihid);
+			/*
+			 * keeping polling for new data at a rate of ~200Hz
+			 * until the interrupt line becomes inactive
+			 */
+			usleep_range(4000, 6000);
+		}
+
+		usleep_range(polling_interval_ms, polling_interval_ms);
+	}
+
+	do_exit(0);
+	return 0;
+}
+
+static int i2c_hid_init_polling(struct i2c_hid *ihid)
+{
+	struct i2c_client *client = ihid->client;
+
+	if (!irq_get_trigger_type(client->irq)) {
+		dev_warn(&client->dev,
+			 "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s",
+			 client->name);
+		return -1;
+	}
+
+	ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid,
+					      "I2C HID polling thread");
+
+	if (ihid->polling_thread) {
+		pr_info("I2C HID polling thread");
+		wake_up_process(ihid->polling_thread);
+		return 0;
+	}
+
+	return -1;
+}
+
 static int i2c_hid_init_irq(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -997,6 +1092,16 @@ 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_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)
 {
@@ -1090,7 +1195,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_POLLING_DISABLED)
+		ret = i2c_hid_init_polling(ihid);
+	else
+		ret = i2c_hid_init_irq(client);
+
 	if (ret < 0)
 		goto err_regulator;

@@ -1129,7 +1238,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),
@@ -1146,7 +1255,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);
@@ -1162,7 +1271,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);
 }

 #ifdef CONFIG_PM_SLEEP
@@ -1183,7 +1292,8 @@ 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_POLLING_DISABLED)
+		disable_irq(client->irq);

 	if (device_may_wakeup(&client->dev)) {
 		wake_status = enable_irq_wake(client->irq);
@@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)

 		if (ihid->pdata.post_power_delay_ms)
 			msleep(ihid->pdata.post_power_delay_ms);
-	} else if (ihid->irq_wake_enabled) {
+	} else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
 			ihid->irq_wake_enabled = false;
@@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
 				wake_status);
 	}

-	enable_irq(client->irq);
+	if (polling_mode != I2C_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
--
2.28.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
  2020-10-09  8:11 [PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status Coiby Xu
@ 2020-10-15 10:33 ` Barnabás Pőcze
  2020-10-16 14:32   ` Coiby Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Barnabás Pőcze @ 2020-10-15 10:33 UTC (permalink / raw)
  To: Coiby Xu; +Cc: linux-input, Jiri Kosina, Benjamin Tissoires, linux-kernel

Hi,

I believe this patch causes I2C HID devices not to work with IRQs after resuming
from suspend.


> [...]
>  #ifdef CONFIG_PM_SLEEP
> @@ -1183,7 +1292,8 @@ 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_POLLING_DISABLED)
> +		disable_irq(client->irq);
>

The IRQ is disabled when suspending if polling is *off*.


>  	if (device_may_wakeup(&client->dev)) {
>  		wake_status = enable_irq_wake(client->irq);
> @@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>
>  		if (ihid->pdata.post_power_delay_ms)
>  			msleep(ihid->pdata.post_power_delay_ms);
> -	} else if (ihid->irq_wake_enabled) {
> +	} else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {

As a side note, I'm not sure if the 'polling_mode != I2C_POLLING_DISABLED' part
is necessary (or that it's necessary *here*). It causes 'i2c_hid_resume' and
'i2c_hid_suspend' to be "asymmetric" which - I believe - may cause problems.


>  		wake_status = disable_irq_wake(client->irq);
>  		if (!wake_status)
>  			ihid->irq_wake_enabled = false;
> @@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
>  				wake_status);
>  	}
>
> -	enable_irq(client->irq);
> +	if (polling_mode != I2C_POLLING_DISABLED)
> +		enable_irq(client->irq);
>

The IRQ is enabled when resuming if polling is *on*. It should be enabled if polling is *off*
in my opinion.


> [...]


Regards,
Barnabás Pőcze

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
  2020-10-15 10:33 ` Barnabás Pőcze
@ 2020-10-16 14:32   ` Coiby Xu
  0 siblings, 0 replies; 3+ messages in thread
From: Coiby Xu @ 2020-10-16 14:32 UTC (permalink / raw)
  To: Barnabás Pőcze
  Cc: linux-input, Jiri Kosina, Benjamin Tissoires, linux-kernel

Hi Barnabás,

Thank you for reviewing theis patch! I've Cced a new version to you.

On Thu, Oct 15, 2020 at 10:33:50AM +0000, Barnabás Pőcze wrote:
>Hi,
>
>I believe this patch causes I2C HID devices not to work with IRQs after resuming
>from suspend.
>
>
>> [...]
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -1183,7 +1292,8 @@ 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_POLLING_DISABLED)
>> +		disable_irq(client->irq);
>>
>
>The IRQ is disabled when suspending if polling is *off*.
>
>
>>  	if (device_may_wakeup(&client->dev)) {
>>  		wake_status = enable_irq_wake(client->irq);
>> @@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>>
>>  		if (ihid->pdata.post_power_delay_ms)
>>  			msleep(ihid->pdata.post_power_delay_ms);
>> -	} else if (ihid->irq_wake_enabled) {
>> +	} else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {
>
>As a side note, I'm not sure if the 'polling_mode != I2C_POLLING_DISABLED' part
>is necessary (or that it's necessary *here*). It causes 'i2c_hid_resume' and
>'i2c_hid_suspend' to be "asymmetric" which - I believe - may cause problems.
>
>
>>  		wake_status = disable_irq_wake(client->irq);
>>  		if (!wake_status)
>>  			ihid->irq_wake_enabled = false;
>> @@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
>>  				wake_status);
>>  	}
>>
>> -	enable_irq(client->irq);
>> +	if (polling_mode != I2C_POLLING_DISABLED)
>> +		enable_irq(client->irq);
>>
>
>The IRQ is enabled when resuming if polling is *on*. It should be enabled if polling is *off*
>in my opinion.
>
>
>> [...]
>
>
>Regards,
>Barnabás Pőcze

--
Best regards,
Coiby

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09  8:11 [PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status Coiby Xu
2020-10-15 10:33 ` Barnabás Pőcze
2020-10-16 14:32   ` Coiby Xu

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git