[PATCH 0/3] HID: Support for Lenovo Thinkpad 10 digitizer

[PATCH 0/3] HID: Support for Lenovo Thinkpad 10 digitizer

[Mika Westerberg]

Hi,

Previously I sent a patch that enables GPIO interrupts for i2c-hid. This
was not accepted by the community:

	https://lkml.org/lkml/2015/1/26/398

However, it turned out that there are actually device(s) out there that
need this support. One of them is Lenovo Thinkpad 10. It has touch screen
connected to IO-APIC but in addition it includes Wacom digitizer
that uses GPIO as an interrupt.

In this version of the patch, I limited the GPIO retrieving code path to be
taken only when the device is enumerated from ACPI. This allows DT to use
whatever is the appropriate mechanism to represent GPIO/interrupt
configuration and still makes current ACPI based devices to work.

The first two patches are needed to get the Wacom digitizer work properly
with i2c-hid.

Mika Westerberg (3):
  HID: wacom: Add support for I2C connected devices
  HID: i2c-hid: Read wMaxInputLength of input report instead of bufsize
  HID: i2c-hid: Add support for ACPI GPIO interrupts

 drivers/hid/i2c-hid/i2c-hid.c | 76 +++++++++++++++++++++++++++++++++----------
 drivers/hid/wacom_wac.c       |  5 +++
 2 files changed, 63 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH 1/3] HID: wacom: Add support for I2C connected devices

[Mika Westerberg]

Lenovo Thinkpad 10 has wacom digitizer connected as a I2C-HID device. Add
generic support for this.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/wacom_wac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 1a6507999a65..c8e73bf62468 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2969,6 +2969,10 @@ static const struct wacom_features wacom_features_HID_ANY_ID =
 	HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
 	.driver_data = (kernel_ulong_t)&wacom_features_##prod
 
+#define I2C_DEVICE_WACOM(prod)						\
+	HID_DEVICE(BUS_I2C, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
+	.driver_data = (kernel_ulong_t)&wacom_features_##prod
+
 #define USB_DEVICE_LENOVO(prod)					\
 	HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, prod),			\
 	.driver_data = (kernel_ulong_t)&wacom_features_##prod
@@ -3113,6 +3117,7 @@ const struct hid_device_id wacom_ids[] = {
 	{ USB_DEVICE_LENOVO(0x6004) },
 
 	{ USB_DEVICE_WACOM(HID_ANY_ID) },
+	{ I2C_DEVICE_WACOM(HID_ANY_ID) },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, wacom_ids);
-- 
2.1.4

Re: [PATCH 1/3] HID: wacom: Add support for I2C connected devices

[Benjamin Tissoires]

On Feb 23 2015 or thereabouts, Mika Westerberg wrote:
> Lenovo Thinkpad 10 has wacom digitizer connected as a I2C-HID device. Add
> generic support for this.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

This one is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

And can be applied right now I guess.

Cheers,
Benjamin

>  drivers/hid/wacom_wac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 1a6507999a65..c8e73bf62468 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -2969,6 +2969,10 @@ static const struct wacom_features wacom_features_HID_ANY_ID =
>  	HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
>  	.driver_data = (kernel_ulong_t)&wacom_features_##prod
>  
> +#define I2C_DEVICE_WACOM(prod)						\
> +	HID_DEVICE(BUS_I2C, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
> +	.driver_data = (kernel_ulong_t)&wacom_features_##prod
> +
>  #define USB_DEVICE_LENOVO(prod)					\
>  	HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, prod),			\
>  	.driver_data = (kernel_ulong_t)&wacom_features_##prod
> @@ -3113,6 +3117,7 @@ const struct hid_device_id wacom_ids[] = {
>  	{ USB_DEVICE_LENOVO(0x6004) },
>  
>  	{ USB_DEVICE_WACOM(HID_ANY_ID) },
> +	{ I2C_DEVICE_WACOM(HID_ANY_ID) },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(hid, wacom_ids);
> -- 
> 2.1.4
> 

Re: [PATCH 1/3] HID: wacom: Add support for I2C connected devices

[Jiri Kosina]

On Mon, 23 Feb 2015, Mika Westerberg wrote:

> Lenovo Thinkpad 10 has wacom digitizer connected as a I2C-HID device. Add
> generic support for this.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied, thanks.

> ---
>  drivers/hid/wacom_wac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 1a6507999a65..c8e73bf62468 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -2969,6 +2969,10 @@ static const struct wacom_features wacom_features_HID_ANY_ID =
>  	HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
>  	.driver_data = (kernel_ulong_t)&wacom_features_##prod
>  
> +#define I2C_DEVICE_WACOM(prod)						\
> +	HID_DEVICE(BUS_I2C, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
> +	.driver_data = (kernel_ulong_t)&wacom_features_##prod
> +
>  #define USB_DEVICE_LENOVO(prod)					\
>  	HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, prod),			\
>  	.driver_data = (kernel_ulong_t)&wacom_features_##prod
> @@ -3113,6 +3117,7 @@ const struct hid_device_id wacom_ids[] = {
>  	{ USB_DEVICE_LENOVO(0x6004) },
>  
>  	{ USB_DEVICE_WACOM(HID_ANY_ID) },
> +	{ I2C_DEVICE_WACOM(HID_ANY_ID) },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(hid, wacom_ids);
> -- 
> 2.1.4
> 

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/3] HID: i2c-hid: Read wMaxInputLength of input report instead of bufsize

[Mika Westerberg]

Wacom digitizer (WCOM0008) on Lenovo Thinkpad 10 seems to allow reading of
subsequent reports (or part of them) if we read more than wMaxInputLength
of data at the time. This data is not always aligned so that the next
report would start right after another.

For example we might get following sequence:

 i2c_hid i2c-WCOM0008:00: input: 0a 00 02 21 20 17 ad 22 11 03
 i2c_hid i2c-WCOM0008:00: input: ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03 0a 00 02 21 20 17
                                 ad 22 11 03
 i2c_hid i2c-WCOM0008:00: input: 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17 ad 22 11 03 0a 00
                                 02 21 20 17
 i2c_hid i2c-WCOM0008:00: i2c_hid_get_input: incomplete report (76/8450)

The bufsize is 76 and wMaxInputLength is 10. In above example the first
read gets right amount of data. The second and third reads get full bufsize
(76 bytes) but the report is missing a start already. This causes the
driver to reject the report because we got less than was expected by the
report length (0x2102 = 8450).

If we read only wMaxInputLength at the time this does not happen at all and
the digitizer works fine.

Based on this change the driver to read wMaxInputLength bytes instead of
bufsize if the value looks sane.

Reported-by: Sébastien Bourdeauducq <sb@m-labs.hk>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 8f1dfc5c5d9c..404ccde49acd 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -369,9 +369,17 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 
 static void i2c_hid_get_input(struct i2c_hid *ihid)
 {
+	unsigned max_input_length = le16_to_cpu(ihid->hdesc.wMaxInputLength);
 	int ret, ret_size;
 	int size = ihid->bufsize;
 
+	/*
+	 * Take the input length from HID descriptor if it is available and
+	 * looks reasonable.
+	 */
+	if (max_input_length > 0)
+		size = min_t(unsigned, ihid->bufsize, max_input_length);
+
 	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
 	if (ret != size) {
 		if (ret < 0)
-- 
2.1.4

Re: [PATCH 2/3] HID: i2c-hid: Read wMaxInputLength of input report instead of bufsize

[Benjamin Tissoires]

On Feb 23 2015 or thereabouts, Mika Westerberg wrote:
> Wacom digitizer (WCOM0008) on Lenovo Thinkpad 10 seems to allow reading of
> subsequent reports (or part of them) if we read more than wMaxInputLength
> of data at the time. This data is not always aligned so that the next
> report would start right after another.
> 
> For example we might get following sequence:
> 
>  i2c_hid i2c-WCOM0008:00: input: 0a 00 02 21 20 17 ad 22 11 03
>  i2c_hid i2c-WCOM0008:00: input: ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03 0a 00 02 21 20 17
>                                  ad 22 11 03
>  i2c_hid i2c-WCOM0008:00: input: 02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17 ad 22 11 03 0a 00
>                                  02 21 20 17
>  i2c_hid i2c-WCOM0008:00: i2c_hid_get_input: incomplete report (76/8450)
> 
> The bufsize is 76 and wMaxInputLength is 10. In above example the first
> read gets right amount of data. The second and third reads get full bufsize
> (76 bytes) but the report is missing a start already. This causes the
> driver to reject the report because we got less than was expected by the
> report length (0x2102 = 8450).
> 
> If we read only wMaxInputLength at the time this does not happen at all and
> the digitizer works fine.
> 
> Based on this change the driver to read wMaxInputLength bytes instead of
> bufsize if the value looks sane.
> 
> Reported-by: Sébastien Bourdeauducq <sb@m-labs.hk>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

We can actually drop this one. Jiri already applied a patch sent last
week which does the exact same thing:
https://patchwork.kernel.org/patch/5857521/ (should appear shortly in
Jiri's tree I guess).

But thanks for confirming that it was really needed for other devices.

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 8f1dfc5c5d9c..404ccde49acd 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -369,9 +369,17 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  
>  static void i2c_hid_get_input(struct i2c_hid *ihid)
>  {
> +	unsigned max_input_length = le16_to_cpu(ihid->hdesc.wMaxInputLength);
>  	int ret, ret_size;
>  	int size = ihid->bufsize;
>  
> +	/*
> +	 * Take the input length from HID descriptor if it is available and
> +	 * looks reasonable.
> +	 */
> +	if (max_input_length > 0)
> +		size = min_t(unsigned, ihid->bufsize, max_input_length);
> +
>  	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
>  	if (ret != size) {
>  		if (ret < 0)
> -- 
> 2.1.4
> 

Re: [PATCH 2/3] HID: i2c-hid: Read wMaxInputLength of input report instead of bufsize

[Mika Westerberg]

On Mon, Feb 23, 2015 at 10:26:35AM -0500, Benjamin Tissoires wrote:
> On Feb 23 2015 or thereabouts, Mika Westerberg wrote:
> > Wacom digitizer (WCOM0008) on Lenovo Thinkpad 10 seems to allow reading of
> > subsequent reports (or part of them) if we read more than wMaxInputLength
> > of data at the time. This data is not always aligned so that the next
> > report would start right after another.
> > 
> > For example we might get following sequence:
> > 
> >  i2c_hid i2c-WCOM0008:00: input: 0a 00 02 21 20 17 ad 22 11 03
> >  i2c_hid i2c-WCOM0008:00: input: ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03 0a 00 02 21 20 17
> >                                  ad 22 11 03
> >  i2c_hid i2c-WCOM0008:00: input: 02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17 ad 22 11 03 0a 00
> >                                  02 21 20 17
> >  i2c_hid i2c-WCOM0008:00: i2c_hid_get_input: incomplete report (76/8450)
> > 
> > The bufsize is 76 and wMaxInputLength is 10. In above example the first
> > read gets right amount of data. The second and third reads get full bufsize
> > (76 bytes) but the report is missing a start already. This causes the
> > driver to reject the report because we got less than was expected by the
> > report length (0x2102 = 8450).
> > 
> > If we read only wMaxInputLength at the time this does not happen at all and
> > the digitizer works fine.
> > 
> > Based on this change the driver to read wMaxInputLength bytes instead of
> > bufsize if the value looks sane.
> > 
> > Reported-by: Sébastien Bourdeauducq <sb@m-labs.hk>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> 
> We can actually drop this one. Jiri already applied a patch sent last
> week which does the exact same thing:
> https://patchwork.kernel.org/patch/5857521/ (should appear shortly in
> Jiri's tree I guess).

Indeed looks like it does the same. Thanks for letting me know.

> But thanks for confirming that it was really needed for other devices.

No problem :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Mika Westerberg]

The HID over I2C specification allows to have the interrupt for a HID
device to be GPIO instead of directly connected to the IO-APIC.

Add support for this so that when the driver does not find proper interrupt
number from the I2C client structure we check if it has ACPI GpioInt()
resource listed in _CRS. If it is found we convert it to an interrupt
number and use it instead.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 68 +++++++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 404ccde49acd..c6cad72f6296 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -37,6 +37,7 @@
 #include <linux/mutex.h>
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 
 #include <linux/i2c/i2c-hid.h>
 
@@ -144,6 +145,8 @@ struct i2c_hid {
 	unsigned long		flags;		/* device flags */
 
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
+	struct gpio_desc	*desc;
+	int			irq;
 
 	struct i2c_hid_platform_data pdata;
 };
@@ -790,16 +793,16 @@ static int i2c_hid_init_irq(struct i2c_client *client)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
 
-	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
+	dev_dbg(&client->dev, "Requesting IRQ: %d\n", ihid->irq);
 
-	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
+	ret = request_threaded_irq(ihid->irq, NULL, i2c_hid_irq,
 			IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 			client->name, ihid);
 	if (ret < 0) {
 		dev_warn(&client->dev,
 			"Could not register for %s interrupt, irq = %d,"
 			" ret = %d\n",
-			client->name, client->irq, ret);
+			client->name, ihid->irq, ret);
 
 		return ret;
 	}
@@ -846,6 +849,14 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 }
 
 #ifdef CONFIG_ACPI
+
+/* Default GPIO mapping */
+static const struct acpi_gpio_params i2c_hid_irq_gpio = { 0, 0, true };
+static const struct acpi_gpio_mapping i2c_hid_acpi_gpios[] = {
+	{ "gpios", &i2c_hid_irq_gpio, 1 },
+	{ },
+};
+
 static int i2c_hid_acpi_pdata(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
@@ -871,7 +882,7 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	pdata->hid_descriptor_address = obj->integer.value;
 	ACPI_FREE(obj);
 
-	return 0;
+	return acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
 }
 
 static const struct acpi_device_id i2c_hid_acpi_match[] = {
@@ -935,12 +946,6 @@ static int i2c_hid_probe(struct i2c_client *client,
 
 	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
 
-	if (!client->irq) {
-		dev_err(&client->dev,
-			"HID over i2c has not been provided an Int IRQ\n");
-		return -EINVAL;
-	}
-
 	ihid = kzalloc(sizeof(struct i2c_hid), GFP_KERNEL);
 	if (!ihid)
 		return -ENOMEM;
@@ -960,6 +965,23 @@ static int i2c_hid_probe(struct i2c_client *client,
 		ihid->pdata = *platform_data;
 	}
 
+	if (client->irq > 0) {
+		ihid->irq = client->irq;
+	} else if (ACPI_COMPANION(&client->dev)) {
+		ihid->desc = gpiod_get(&client->dev, NULL, GPIOD_IN);
+		if (IS_ERR(ihid->desc)) {
+			dev_err(&client->dev, "Failed to get GPIO interrupt\n");
+			return PTR_ERR(ihid->desc);
+		}
+
+		ihid->irq = gpiod_to_irq(ihid->desc);
+		if (ihid->irq < 0) {
+			gpiod_put(ihid->desc);
+			dev_err(&client->dev, "Failed to convert GPIO to IRQ\n");
+			return ihid->irq;
+		}
+	}
+
 	i2c_set_clientdata(client, ihid);
 
 	ihid->client = client;
@@ -1022,13 +1044,16 @@ err_mem_free:
 	hid_destroy_device(hid);
 
 err_irq:
-	free_irq(client->irq, ihid);
+	free_irq(ihid->irq, ihid);
 
 err_pm:
 	pm_runtime_put_noidle(&client->dev);
 	pm_runtime_disable(&client->dev);
 
 err:
+	if (ihid->desc)
+		gpiod_put(ihid->desc);
+
 	i2c_hid_free_buffers(ihid);
 	kfree(ihid);
 	return ret;
@@ -1047,13 +1072,18 @@ static int i2c_hid_remove(struct i2c_client *client)
 	hid = ihid->hid;
 	hid_destroy_device(hid);
 
-	free_irq(client->irq, ihid);
+	free_irq(ihid->irq, ihid);
 
 	if (ihid->bufsize)
 		i2c_hid_free_buffers(ihid);
 
+	if (ihid->desc)
+		gpiod_put(ihid->desc);
+
 	kfree(ihid);
 
+	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
+
 	return 0;
 }
 
@@ -1065,9 +1095,9 @@ static int i2c_hid_suspend(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int ret = 0;
 
-	disable_irq(client->irq);
+	disable_irq(ihid->irq);
 	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(client->irq);
+		enable_irq_wake(ihid->irq);
 
 	if (hid->driver && hid->driver->suspend)
 		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
@@ -1085,13 +1115,13 @@ static int i2c_hid_resume(struct device *dev)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid = ihid->hid;
 
-	enable_irq(client->irq);
+	enable_irq(ihid->irq);
 	ret = i2c_hid_hwreset(client);
 	if (ret)
 		return ret;
 
 	if (device_may_wakeup(&client->dev))
-		disable_irq_wake(client->irq);
+		disable_irq_wake(ihid->irq);
 
 	if (hid->driver && hid->driver->reset_resume) {
 		ret = hid->driver->reset_resume(hid);
@@ -1106,17 +1136,19 @@ static int i2c_hid_resume(struct device *dev)
 static int i2c_hid_runtime_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
-	disable_irq(client->irq);
+	disable_irq(ihid->irq);
 	return 0;
 }
 
 static int i2c_hid_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
-	enable_irq(client->irq);
+	enable_irq(ihid->irq);
 	i2c_hid_set_power(client, I2C_HID_PWR_ON);
 	return 0;
 }
-- 
2.1.4

Re: [PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Benjamin Tissoires]

On Feb 23 2015 or thereabouts, Mika Westerberg wrote:
> The HID over I2C specification allows to have the interrupt for a HID
> device to be GPIO instead of directly connected to the IO-APIC.
> 
> Add support for this so that when the driver does not find proper interrupt
> number from the I2C client structure we check if it has ACPI GpioInt()
> resource listed in _CRS. If it is found we convert it to an interrupt
> number and use it instead.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

I know this has been already discussed, so don't take this as a request
for a change. I however hoped that the acpi subsystem would provide the
irq directly in the i2c client without having to deal with it in
i2c_hid. For me, declaring the GPIO as an interrupt means that the irq
should be created in the first place. However, if we also have access to
the gpio, that means that we can read the value of it, which is better in
our case. So I am fine with all the solutions - as long as it works :)

I don't know enough of the use of gpios in acpi to properly review the
code. So I'll just add my:
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 68 +++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 404ccde49acd..c6cad72f6296 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -37,6 +37,7 @@
>  #include <linux/mutex.h>
>  #include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> @@ -144,6 +145,8 @@ struct i2c_hid {
>  	unsigned long		flags;		/* device flags */
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
> +	struct gpio_desc	*desc;
> +	int			irq;
>  
>  	struct i2c_hid_platform_data pdata;
>  };
> @@ -790,16 +793,16 @@ static int i2c_hid_init_irq(struct i2c_client *client)
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	int ret;
>  
> -	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
> +	dev_dbg(&client->dev, "Requesting IRQ: %d\n", ihid->irq);
>  
> -	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> +	ret = request_threaded_irq(ihid->irq, NULL, i2c_hid_irq,
>  			IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>  			client->name, ihid);
>  	if (ret < 0) {
>  		dev_warn(&client->dev,
>  			"Could not register for %s interrupt, irq = %d,"
>  			" ret = %d\n",
> -			client->name, client->irq, ret);
> +			client->name, ihid->irq, ret);
>  
>  		return ret;
>  	}
> @@ -846,6 +849,14 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>  }
>  
>  #ifdef CONFIG_ACPI
> +
> +/* Default GPIO mapping */
> +static const struct acpi_gpio_params i2c_hid_irq_gpio = { 0, 0, true };
> +static const struct acpi_gpio_mapping i2c_hid_acpi_gpios[] = {
> +	{ "gpios", &i2c_hid_irq_gpio, 1 },
> +	{ },
> +};
> +
>  static int i2c_hid_acpi_pdata(struct i2c_client *client,
>  		struct i2c_hid_platform_data *pdata)
>  {
> @@ -871,7 +882,7 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
>  	pdata->hid_descriptor_address = obj->integer.value;
>  	ACPI_FREE(obj);
>  
> -	return 0;
> +	return acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
>  }
>  
>  static const struct acpi_device_id i2c_hid_acpi_match[] = {
> @@ -935,12 +946,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>  
>  	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
>  
> -	if (!client->irq) {
> -		dev_err(&client->dev,
> -			"HID over i2c has not been provided an Int IRQ\n");
> -		return -EINVAL;
> -	}
> -
>  	ihid = kzalloc(sizeof(struct i2c_hid), GFP_KERNEL);
>  	if (!ihid)
>  		return -ENOMEM;
> @@ -960,6 +965,23 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		ihid->pdata = *platform_data;
>  	}
>  
> +	if (client->irq > 0) {
> +		ihid->irq = client->irq;
> +	} else if (ACPI_COMPANION(&client->dev)) {
> +		ihid->desc = gpiod_get(&client->dev, NULL, GPIOD_IN);
> +		if (IS_ERR(ihid->desc)) {
> +			dev_err(&client->dev, "Failed to get GPIO interrupt\n");
> +			return PTR_ERR(ihid->desc);
> +		}
> +
> +		ihid->irq = gpiod_to_irq(ihid->desc);
> +		if (ihid->irq < 0) {
> +			gpiod_put(ihid->desc);
> +			dev_err(&client->dev, "Failed to convert GPIO to IRQ\n");
> +			return ihid->irq;
> +		}
> +	}
> +
>  	i2c_set_clientdata(client, ihid);
>  
>  	ihid->client = client;
> @@ -1022,13 +1044,16 @@ err_mem_free:
>  	hid_destroy_device(hid);
>  
>  err_irq:
> -	free_irq(client->irq, ihid);
> +	free_irq(ihid->irq, ihid);
>  
>  err_pm:
>  	pm_runtime_put_noidle(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  err:
> +	if (ihid->desc)
> +		gpiod_put(ihid->desc);
> +
>  	i2c_hid_free_buffers(ihid);
>  	kfree(ihid);
>  	return ret;
> @@ -1047,13 +1072,18 @@ static int i2c_hid_remove(struct i2c_client *client)
>  	hid = ihid->hid;
>  	hid_destroy_device(hid);
>  
> -	free_irq(client->irq, ihid);
> +	free_irq(ihid->irq, ihid);
>  
>  	if (ihid->bufsize)
>  		i2c_hid_free_buffers(ihid);
>  
> +	if (ihid->desc)
> +		gpiod_put(ihid->desc);
> +
>  	kfree(ihid);
>  
> +	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
> +
>  	return 0;
>  }
>  
> @@ -1065,9 +1095,9 @@ static int i2c_hid_suspend(struct device *dev)
>  	struct hid_device *hid = ihid->hid;
>  	int ret = 0;
>  
> -	disable_irq(client->irq);
> +	disable_irq(ihid->irq);
>  	if (device_may_wakeup(&client->dev))
> -		enable_irq_wake(client->irq);
> +		enable_irq_wake(ihid->irq);
>  
>  	if (hid->driver && hid->driver->suspend)
>  		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
> @@ -1085,13 +1115,13 @@ static int i2c_hid_resume(struct device *dev)
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	struct hid_device *hid = ihid->hid;
>  
> -	enable_irq(client->irq);
> +	enable_irq(ihid->irq);
>  	ret = i2c_hid_hwreset(client);
>  	if (ret)
>  		return ret;
>  
>  	if (device_may_wakeup(&client->dev))
> -		disable_irq_wake(client->irq);
> +		disable_irq_wake(ihid->irq);
>  
>  	if (hid->driver && hid->driver->reset_resume) {
>  		ret = hid->driver->reset_resume(hid);
> @@ -1106,17 +1136,19 @@ static int i2c_hid_resume(struct device *dev)
>  static int i2c_hid_runtime_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  
>  	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> -	disable_irq(client->irq);
> +	disable_irq(ihid->irq);
>  	return 0;
>  }
>  
>  static int i2c_hid_runtime_resume(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  
> -	enable_irq(client->irq);
> +	enable_irq(ihid->irq);
>  	i2c_hid_set_power(client, I2C_HID_PWR_ON);
>  	return 0;
>  }
> -- 
> 2.1.4
> 

Re: [PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Mika Westerberg]

On Mon, Feb 23, 2015 at 10:35:02AM -0500, Benjamin Tissoires wrote:
> On Feb 23 2015 or thereabouts, Mika Westerberg wrote:
> > The HID over I2C specification allows to have the interrupt for a HID
> > device to be GPIO instead of directly connected to the IO-APIC.
> > 
> > Add support for this so that when the driver does not find proper interrupt
> > number from the I2C client structure we check if it has ACPI GpioInt()
> > resource listed in _CRS. If it is found we convert it to an interrupt
> > number and use it instead.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> 
> I know this has been already discussed, so don't take this as a request
> for a change. I however hoped that the acpi subsystem would provide the
> irq directly in the i2c client without having to deal with it in
> i2c_hid. For me, declaring the GPIO as an interrupt means that the irq
> should be created in the first place. However, if we also have access to
> the gpio, that means that we can read the value of it, which is better in
> our case. So I am fine with all the solutions - as long as it works :)

I understand your point.

It would be nice to do it that way but it is not so simple.

For example if we have some button array that has several GpioInts
listed, we convert the first to an interrupt but the driver needs to
handle all the rest itself (taking the one converted interrupt into
account).

Another problem that I can think of is to determine who is responsible
in releasing the GPIO and at which point. We cannot just use devm_* APIs
because no driver has been probed yet.

Also if the driver needs to use the interrupt as GPIO for reading its
value we would first need to release it (because it has been requested
by the core code), then re-request it, read the value and then somehow
release it back so that the core code can release it...

That's why I think it is better to let drivers handle these themselves
as they like.

Of course if someone has good idea how to do this properly in the core
code, I'm all ears.

> I don't know enough of the use of gpios in acpi to properly review the
> code. So I'll just add my:
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks :-)

Re: [PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Jiri Kosina]

On Mon, 23 Feb 2015, Mika Westerberg wrote:

> The HID over I2C specification allows to have the interrupt for a HID
> device to be GPIO instead of directly connected to the IO-APIC.
> 
> Add support for this so that when the driver does not find proper interrupt
> number from the I2C client structure we check if it has ACPI GpioInt()
> resource listed in _CRS. If it is found we convert it to an interrupt
> number and use it instead.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Antonio Ospite]

On Mon, 23 Feb 2015 15:52:45 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> The HID over I2C specification allows to have the interrupt for a HID
> device to be GPIO instead of directly connected to the IO-APIC.
> 
> Add support for this so that when the driver does not find proper interrupt
> number from the I2C client structure we check if it has ACPI GpioInt()
> resource listed in _CRS. If it is found we convert it to an interrupt
> number and use it instead.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 68 +++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 404ccde49acd..c6cad72f6296 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -37,6 +37,7 @@
>  #include <linux/mutex.h>
>  #include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> @@ -144,6 +145,8 @@ struct i2c_hid {
>  	unsigned long		flags;		/* device flags */
>  
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
> +	struct gpio_desc	*desc;

Just curious: can't the gpio_desc be local in the probe function?
I take that the gpio descriptor needs to be alive even after the irq
number has been derived from it in order for the gpio chip to remember
that the gpio has been requested by someone. Is that correct?

Thanks,
   Antonio

[...]

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 3/3] HID: i2c-hid: Add support for ACPI GPIO interrupts

[Mika Westerberg]

On Tue, Feb 24, 2015 at 10:27:05PM +0100, Antonio Ospite wrote:
> On Mon, 23 Feb 2015 15:52:45 +0200
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > The HID over I2C specification allows to have the interrupt for a HID
> > device to be GPIO instead of directly connected to the IO-APIC.
> > 
> > Add support for this so that when the driver does not find proper interrupt
> > number from the I2C client structure we check if it has ACPI GpioInt()
> > resource listed in _CRS. If it is found we convert it to an interrupt
> > number and use it instead.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 68 +++++++++++++++++++++++++++++++------------
> >  1 file changed, 50 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 404ccde49acd..c6cad72f6296 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/acpi.h>
> >  #include <linux/of.h>
> > +#include <linux/gpio/consumer.h>
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > @@ -144,6 +145,8 @@ struct i2c_hid {
> >  	unsigned long		flags;		/* device flags */
> >  
> >  	wait_queue_head_t	wait;		/* For waiting the interrupt */
> > +	struct gpio_desc	*desc;
> 
> Just curious: can't the gpio_desc be local in the probe function?
> I take that the gpio descriptor needs to be alive even after the irq
> number has been derived from it in order for the gpio chip to remember
> that the gpio has been requested by someone. Is that correct?

That's right and we need to release it in remove() as well.

Someone should convert this driver to use devm_* APIs...