[PATCH 1/2] HID: i2c-hid: The interrupt should be level sensitive

[PATCH 1/2] HID: i2c-hid: The interrupt should be level sensitive

[Mika Westerberg]

The Microsoft HID over I2C specification says two things regarding the
interrupt:

 1) The interrupt should be level sensitive
 2) The device keeps the interrupt asserted as long as it has more reports
    available.

We've seen that at least some Atmel and N-Trig panels keep the line low as
long as they have something to send. The current version of the driver only
detects the first edge but then fails to read rest of the reports (as the
line is still asserted).

Make the driver follow the specification and configure the HID interrupt to
be level sensitive.

The Windows HID over I2C driver also seems to do the same.

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

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d43e967e7533..8f1dfc5c5d9c 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -785,7 +785,7 @@ static int i2c_hid_init_irq(struct i2c_client *client)
 	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
 
 	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
-			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 			client->name, ihid);
 	if (ret < 0) {
 		dev_warn(&client->dev,
-- 
2.1.4

[PATCH 2/2] HID: i2c-hid: Add support for 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 the device has property
named "gpios". This is then assumed to be the GPIO that serves as an
interrupt for the device.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
 drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
 2 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
index 488edcb264c4..8f4a99dad3b9 100644
--- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
@@ -15,7 +15,10 @@ Required properties:
 - reg: i2c slave address
 - hid-descr-addr: HID descriptor address
 - interrupt-parent: the phandle for the interrupt controller
-- interrupts: interrupt line
+- interrupts: interrupt line if the device uses IO-APIC interrupts
+
+Optional properties:
+- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
 
 Example:
 
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 8f1dfc5c5d9c..d3c49ff4888e 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;
 };
@@ -782,16 +785,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;
 	}
@@ -838,6 +841,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)
 {
@@ -863,7 +874,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[] = {
@@ -927,12 +938,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;
@@ -952,6 +957,25 @@ static int i2c_hid_probe(struct i2c_client *client,
 		ihid->pdata = *platform_data;
 	}
 
+	if (client->irq > 0) {
+		ihid->irq = client->irq;
+	} else {
+		ihid->desc = gpiod_get(&client->dev, NULL);
+		if (IS_ERR(ihid->desc)) {
+			dev_err(&client->dev, "Failed to get GPIO interrupt\n");
+			return PTR_ERR(ihid->desc);
+		}
+
+		gpiod_direction_input(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;
@@ -1014,13 +1038,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;
@@ -1039,13 +1066,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;
 }
 
@@ -1057,9 +1089,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);
@@ -1077,13 +1109,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);
@@ -1098,17 +1130,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 2/2] HID: i2c-hid: Add support for GPIO interrupts

[Mark Rutland]

On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> named "gpios". This is then assumed to be the GPIO that serves as an
> interrupt for the device.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
>  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
>  2 files changed, 56 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> index 488edcb264c4..8f4a99dad3b9 100644
> --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> @@ -15,7 +15,10 @@ Required properties:
>  - reg: i2c slave address
>  - hid-descr-addr: HID descriptor address
>  - interrupt-parent: the phandle for the interrupt controller
> -- interrupts: interrupt line
> +- interrupts: interrupt line if the device uses IO-APIC interrupts
> +
> +Optional properties:
> +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts

Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
controller should be marked as an interrupt-controller, and the GPIO
described as an interrupt line. That also gets you the appropriate
configuration for the GPIO as an interrupt.

Does this GPIO serve any other purpose than an ersatz interrupt line?
If not, it should probably be described as an interrupt. From the PoV of
this device, it's just an interrupt controller hooked up to the
interrupt pin.

Thanks,
Mark.

>  
>  Example:
>  
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 8f1dfc5c5d9c..d3c49ff4888e 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;
>  };
> @@ -782,16 +785,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;
>  	}
> @@ -838,6 +841,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)
>  {
> @@ -863,7 +874,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[] = {
> @@ -927,12 +938,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;
> @@ -952,6 +957,25 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		ihid->pdata = *platform_data;
>  	}
>  
> +	if (client->irq > 0) {
> +		ihid->irq = client->irq;
> +	} else {
> +		ihid->desc = gpiod_get(&client->dev, NULL);
> +		if (IS_ERR(ihid->desc)) {
> +			dev_err(&client->dev, "Failed to get GPIO interrupt\n");
> +			return PTR_ERR(ihid->desc);
> +		}
> +
> +		gpiod_direction_input(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;
> @@ -1014,13 +1038,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;
> @@ -1039,13 +1066,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;
>  }
>  
> @@ -1057,9 +1089,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);
> @@ -1077,13 +1109,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);
> @@ -1098,17 +1130,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 2/2] HID: i2c-hid: Add support for GPIO interrupts

[Mika Westerberg]

On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > named "gpios". This is then assumed to be the GPIO that serves as an
> > interrupt for the device.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> >  2 files changed, 56 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > index 488edcb264c4..8f4a99dad3b9 100644
> > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > @@ -15,7 +15,10 @@ Required properties:
> >  - reg: i2c slave address
> >  - hid-descr-addr: HID descriptor address
> >  - interrupt-parent: the phandle for the interrupt controller
> > -- interrupts: interrupt line
> > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > +
> > +Optional properties:
> > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> 
> Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> controller should be marked as an interrupt-controller, and the GPIO
> described as an interrupt line. That also gets you the appropriate
> configuration for the GPIO as an interrupt.
> 
> Does this GPIO serve any other purpose than an ersatz interrupt line?

It is just an interrupt.

> If not, it should probably be described as an interrupt. From the PoV of
> this device, it's just an interrupt controller hooked up to the
> interrupt pin.

What I'm trying to do is to get a GPIO that is described in ACPI (as
GpioInt() in _CRS) to be supported in this driver using gpiolib like
this:

	desc = gpiod_get(&client->dev, NULL);

This calls to find "gpios" property which ends up finding the GpioInt()
in _CRS.

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

[Mark Rutland]

On Mon, Jan 26, 2015 at 02:47:29PM +0000, Mika Westerberg wrote:
> On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> > On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > > named "gpios". This is then assumed to be the GPIO that serves as an
> > > interrupt for the device.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> > >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > index 488edcb264c4..8f4a99dad3b9 100644
> > > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > @@ -15,7 +15,10 @@ Required properties:
> > >  - reg: i2c slave address
> > >  - hid-descr-addr: HID descriptor address
> > >  - interrupt-parent: the phandle for the interrupt controller
> > > -- interrupts: interrupt line
> > > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > > +
> > > +Optional properties:
> > > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> > 
> > Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> > controller should be marked as an interrupt-controller, and the GPIO
> > described as an interrupt line. That also gets you the appropriate
> > configuration for the GPIO as an interrupt.
> > 
> > Does this GPIO serve any other purpose than an ersatz interrupt line?
> 
> It is just an interrupt.
> 
> > If not, it should probably be described as an interrupt. From the PoV of
> > this device, it's just an interrupt controller hooked up to the
> > interrupt pin.
> 
> What I'm trying to do is to get a GPIO that is described in ACPI (as
> GpioInt() in _CRS) to be supported in this driver using gpiolib like
> this:
> 
> 	desc = gpiod_get(&client->dev, NULL);
> 
> This calls to find "gpios" property which ends up finding the GpioInt()
> in _CRS.

I understand what you are trying to do, but I disagree on the principle.
If it's logically an interrupt, it should be described as an interrupt.

If ACPI lacks the ability to describe things in that fashion, it's yet
another reason that we shouldn't be pretending that DT and ACPI are the
same thing.

Mark.

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

[Mika Westerberg]

On Mon, Jan 26, 2015 at 02:50:01PM +0000, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 02:47:29PM +0000, Mika Westerberg wrote:
> > On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> > > On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > > > named "gpios". This is then assumed to be the GPIO that serves as an
> > > > interrupt for the device.
> > > > 
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > ---
> > > >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> > > >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> > > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > index 488edcb264c4..8f4a99dad3b9 100644
> > > > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > @@ -15,7 +15,10 @@ Required properties:
> > > >  - reg: i2c slave address
> > > >  - hid-descr-addr: HID descriptor address
> > > >  - interrupt-parent: the phandle for the interrupt controller
> > > > -- interrupts: interrupt line
> > > > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > > > +
> > > > +Optional properties:
> > > > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> > > 
> > > Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> > > controller should be marked as an interrupt-controller, and the GPIO
> > > described as an interrupt line. That also gets you the appropriate
> > > configuration for the GPIO as an interrupt.
> > > 
> > > Does this GPIO serve any other purpose than an ersatz interrupt line?
> > 
> > It is just an interrupt.
> > 
> > > If not, it should probably be described as an interrupt. From the PoV of
> > > this device, it's just an interrupt controller hooked up to the
> > > interrupt pin.
> > 
> > What I'm trying to do is to get a GPIO that is described in ACPI (as
> > GpioInt() in _CRS) to be supported in this driver using gpiolib like
> > this:
> > 
> > 	desc = gpiod_get(&client->dev, NULL);
> > 
> > This calls to find "gpios" property which ends up finding the GpioInt()
> > in _CRS.
> 
> I understand what you are trying to do, but I disagree on the principle.
> If it's logically an interrupt, it should be described as an interrupt.

It is a GPIO line that is used as interrupt. It is not IO-APIC interrupt
or anything like that. It will be handled through a GPIO driver.

> If ACPI lacks the ability to describe things in that fashion, it's yet
> another reason that we shouldn't be pretending that DT and ACPI are the
> same thing.

I'm not saying they are the same thing (they're not). I'm trying to get
a GPIO from ACPI translated to an interrupt so that the driver can use
it. Preferably so that the DT description does not prevent people from
using the same on non-ACPI platforms.

ACPI can desribe Interrupt(), GpioInt() and GpioIo() just fine. It is
the Microsoft HID over I2C specification that says it should be
GpioInt() even though we have seen Interrupt() used there as well.

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

[Mark Rutland]

On Mon, Jan 26, 2015 at 03:16:37PM +0000, Mika Westerberg wrote:
> On Mon, Jan 26, 2015 at 02:50:01PM +0000, Mark Rutland wrote:
> > On Mon, Jan 26, 2015 at 02:47:29PM +0000, Mika Westerberg wrote:
> > > On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> > > > On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > > > > named "gpios". This is then assumed to be the GPIO that serves as an
> > > > > interrupt for the device.
> > > > > 
> > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > ---
> > > > >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> > > > >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> > > > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > index 488edcb264c4..8f4a99dad3b9 100644
> > > > > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > @@ -15,7 +15,10 @@ Required properties:
> > > > >  - reg: i2c slave address
> > > > >  - hid-descr-addr: HID descriptor address
> > > > >  - interrupt-parent: the phandle for the interrupt controller
> > > > > -- interrupts: interrupt line
> > > > > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > > > > +
> > > > > +Optional properties:
> > > > > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> > > > 
> > > > Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> > > > controller should be marked as an interrupt-controller, and the GPIO
> > > > described as an interrupt line. That also gets you the appropriate
> > > > configuration for the GPIO as an interrupt.
> > > > 
> > > > Does this GPIO serve any other purpose than an ersatz interrupt line?
> > > 
> > > It is just an interrupt.
> > > 
> > > > If not, it should probably be described as an interrupt. From the PoV of
> > > > this device, it's just an interrupt controller hooked up to the
> > > > interrupt pin.
> > > 
> > > What I'm trying to do is to get a GPIO that is described in ACPI (as
> > > GpioInt() in _CRS) to be supported in this driver using gpiolib like
> > > this:
> > > 
> > > 	desc = gpiod_get(&client->dev, NULL);
> > > 
> > > This calls to find "gpios" property which ends up finding the GpioInt()
> > > in _CRS.
> > 
> > I understand what you are trying to do, but I disagree on the principle.
> > If it's logically an interrupt, it should be described as an interrupt.
> 
> It is a GPIO line that is used as interrupt. It is not IO-APIC interrupt
> or anything like that. It will be handled through a GPIO driver.

I understand that the interrupt line is wired up to a GPIO controller,
where the GPIO controller is able to raise an interrupt as required.

However, from the PoV of the I2C device, this doesn't matter. From it's
PoV it raises an interrupt, and that's all. It has no idea what happens
to be wired up to its IRQ pin, and nor should it.

> > If ACPI lacks the ability to describe things in that fashion, it's yet
> > another reason that we shouldn't be pretending that DT and ACPI are the
> > same thing.
> 
> I'm not saying they are the same thing (they're not). I'm trying to get
> a GPIO from ACPI translated to an interrupt so that the driver can use
> it. Preferably so that the DT description does not prevent people from
> using the same on non-ACPI platforms.

If you're following the Microsoft HID over I2C ACPI spec, why is this DT
binding document relevant. Assuming you're following the spec, you won't
be using _DSD with "hid-over-i2c". If you're not following the spec then
you aren't following the spec, so the spec is irrelevant.

> ACPI can desribe Interrupt(), GpioInt() and GpioIo() just fine. It is
> the Microsoft HID over I2C specification that says it should be
> GpioInt() even though we have seen Interrupt() used there as well.

Ok, so if the HID over I2C spec says that for ACPI, do that for ACPI.

I don't follow why the DT binding should do this.

Mark.

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

[Mika Westerberg]

On Mon, Jan 26, 2015 at 04:01:20PM +0000, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 03:16:37PM +0000, Mika Westerberg wrote:
> > On Mon, Jan 26, 2015 at 02:50:01PM +0000, Mark Rutland wrote:
> > > On Mon, Jan 26, 2015 at 02:47:29PM +0000, Mika Westerberg wrote:
> > > > On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> > > > > On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > > > > > named "gpios". This is then assumed to be the GPIO that serves as an
> > > > > > interrupt for the device.
> > > > > > 
> > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> > > > > >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> > > > > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > index 488edcb264c4..8f4a99dad3b9 100644
> > > > > > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > @@ -15,7 +15,10 @@ Required properties:
> > > > > >  - reg: i2c slave address
> > > > > >  - hid-descr-addr: HID descriptor address
> > > > > >  - interrupt-parent: the phandle for the interrupt controller
> > > > > > -- interrupts: interrupt line
> > > > > > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > > > > > +
> > > > > > +Optional properties:
> > > > > > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> > > > > 
> > > > > Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> > > > > controller should be marked as an interrupt-controller, and the GPIO
> > > > > described as an interrupt line. That also gets you the appropriate
> > > > > configuration for the GPIO as an interrupt.
> > > > > 
> > > > > Does this GPIO serve any other purpose than an ersatz interrupt line?
> > > > 
> > > > It is just an interrupt.
> > > > 
> > > > > If not, it should probably be described as an interrupt. From the PoV of
> > > > > this device, it's just an interrupt controller hooked up to the
> > > > > interrupt pin.
> > > > 
> > > > What I'm trying to do is to get a GPIO that is described in ACPI (as
> > > > GpioInt() in _CRS) to be supported in this driver using gpiolib like
> > > > this:
> > > > 
> > > > 	desc = gpiod_get(&client->dev, NULL);
> > > > 
> > > > This calls to find "gpios" property which ends up finding the GpioInt()
> > > > in _CRS.
> > > 
> > > I understand what you are trying to do, but I disagree on the principle.
> > > If it's logically an interrupt, it should be described as an interrupt.
> > 
> > It is a GPIO line that is used as interrupt. It is not IO-APIC interrupt
> > or anything like that. It will be handled through a GPIO driver.
> 
> I understand that the interrupt line is wired up to a GPIO controller,
> where the GPIO controller is able to raise an interrupt as required.
> 
> However, from the PoV of the I2C device, this doesn't matter. From it's
> PoV it raises an interrupt, and that's all. It has no idea what happens
> to be wired up to its IRQ pin, and nor should it.
> 
> > > If ACPI lacks the ability to describe things in that fashion, it's yet
> > > another reason that we shouldn't be pretending that DT and ACPI are the
> > > same thing.
> > 
> > I'm not saying they are the same thing (they're not). I'm trying to get
> > a GPIO from ACPI translated to an interrupt so that the driver can use
> > it. Preferably so that the DT description does not prevent people from
> > using the same on non-ACPI platforms.
> 
> If you're following the Microsoft HID over I2C ACPI spec, why is this DT
> binding document relevant. Assuming you're following the spec, you won't
> be using _DSD with "hid-over-i2c". If you're not following the spec then
> you aren't following the spec, so the spec is irrelevant.
> 
> > ACPI can desribe Interrupt(), GpioInt() and GpioIo() just fine. It is
> > the Microsoft HID over I2C specification that says it should be
> > GpioInt() even though we have seen Interrupt() used there as well.
> 
> Ok, so if the HID over I2C spec says that for ACPI, do that for ACPI.
> 
> I don't follow why the DT binding should do this.

I added it there because I thought it's the right thing to do. After all
it is assumed that there exists property "gpios" now for both ACPI and
DT if I pass NULL in gpiod_get().

Obviously I was wrong.

I'll remove the binding documentation from the next revision and call it
"ACPI only" if it makes this work for you.

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

[Mark Rutland]

On Mon, Jan 26, 2015 at 04:13:56PM +0000, Mika Westerberg wrote:
> On Mon, Jan 26, 2015 at 04:01:20PM +0000, Mark Rutland wrote:
> > On Mon, Jan 26, 2015 at 03:16:37PM +0000, Mika Westerberg wrote:
> > > On Mon, Jan 26, 2015 at 02:50:01PM +0000, Mark Rutland wrote:
> > > > On Mon, Jan 26, 2015 at 02:47:29PM +0000, Mika Westerberg wrote:
> > > > > On Mon, Jan 26, 2015 at 02:37:24PM +0000, Mark Rutland wrote:
> > > > > > On Mon, Jan 26, 2015 at 02:29:33PM +0000, 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 the device has property
> > > > > > > named "gpios". This is then assumed to be the GPIO that serves as an
> > > > > > > interrupt for the device.
> > > > > > > 
> > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/hid/hid-over-i2c.txt       |  5 +-
> > > > > > >  drivers/hid/i2c-hid/i2c-hid.c                      | 70 ++++++++++++++++------
> > > > > > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > > index 488edcb264c4..8f4a99dad3b9 100644
> > > > > > > --- a/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/hid/hid-over-i2c.txt
> > > > > > > @@ -15,7 +15,10 @@ Required properties:
> > > > > > >  - reg: i2c slave address
> > > > > > >  - hid-descr-addr: HID descriptor address
> > > > > > >  - interrupt-parent: the phandle for the interrupt controller
> > > > > > > -- interrupts: interrupt line
> > > > > > > +- interrupts: interrupt line if the device uses IO-APIC interrupts
> > > > > > > +
> > > > > > > +Optional properties:
> > > > > > > +- gpios: GPIO used as an interrupt if the device uses GPIO interrupts
> > > > > > 
> > > > > > Elsewhere we've said that for a GPIO acting as an interrupt line, GPIO
> > > > > > controller should be marked as an interrupt-controller, and the GPIO
> > > > > > described as an interrupt line. That also gets you the appropriate
> > > > > > configuration for the GPIO as an interrupt.
> > > > > > 
> > > > > > Does this GPIO serve any other purpose than an ersatz interrupt line?
> > > > > 
> > > > > It is just an interrupt.
> > > > > 
> > > > > > If not, it should probably be described as an interrupt. From the PoV of
> > > > > > this device, it's just an interrupt controller hooked up to the
> > > > > > interrupt pin.
> > > > > 
> > > > > What I'm trying to do is to get a GPIO that is described in ACPI (as
> > > > > GpioInt() in _CRS) to be supported in this driver using gpiolib like
> > > > > this:
> > > > > 
> > > > > 	desc = gpiod_get(&client->dev, NULL);
> > > > > 
> > > > > This calls to find "gpios" property which ends up finding the GpioInt()
> > > > > in _CRS.
> > > > 
> > > > I understand what you are trying to do, but I disagree on the principle.
> > > > If it's logically an interrupt, it should be described as an interrupt.
> > > 
> > > It is a GPIO line that is used as interrupt. It is not IO-APIC interrupt
> > > or anything like that. It will be handled through a GPIO driver.
> > 
> > I understand that the interrupt line is wired up to a GPIO controller,
> > where the GPIO controller is able to raise an interrupt as required.
> > 
> > However, from the PoV of the I2C device, this doesn't matter. From it's
> > PoV it raises an interrupt, and that's all. It has no idea what happens
> > to be wired up to its IRQ pin, and nor should it.
> > 
> > > > If ACPI lacks the ability to describe things in that fashion, it's yet
> > > > another reason that we shouldn't be pretending that DT and ACPI are the
> > > > same thing.
> > > 
> > > I'm not saying they are the same thing (they're not). I'm trying to get
> > > a GPIO from ACPI translated to an interrupt so that the driver can use
> > > it. Preferably so that the DT description does not prevent people from
> > > using the same on non-ACPI platforms.
> > 
> > If you're following the Microsoft HID over I2C ACPI spec, why is this DT
> > binding document relevant. Assuming you're following the spec, you won't
> > be using _DSD with "hid-over-i2c". If you're not following the spec then
> > you aren't following the spec, so the spec is irrelevant.
> > 
> > > ACPI can desribe Interrupt(), GpioInt() and GpioIo() just fine. It is
> > > the Microsoft HID over I2C specification that says it should be
> > > GpioInt() even though we have seen Interrupt() used there as well.
> > 
> > Ok, so if the HID over I2C spec says that for ACPI, do that for ACPI.
> > 
> > I don't follow why the DT binding should do this.
> 
> I added it there because I thought it's the right thing to do. After all
> it is assumed that there exists property "gpios" now for both ACPI and
> DT if I pass NULL in gpiod_get().

That's not great. Now we have ACPI and DT concerns leaking into each
other when they shouldn't necessarily.

> Obviously I was wrong.

Well, not quite. We shouldn't accept non-documented properties in DT,
and documenting the property is certainly appreciated. The issue here
seems to be an inconsistency between how we handle GPIO-backed
interrupts described in DT vs ACPI.

As far as I can tell from the specs I've looked at, GpioInt describes an
interrupt that is backed by a GPIO. That sounds similar to what we would
do in DT in that we are describing the resource logically as an
interrupt.

What I don't follow is why GpioInt seems to be translated as a GPIO
rather than as an interrupt which happens to be backed by a GPIO. Were
it not for that, the DT and ACPI cases would align better.

> I'll remove the binding documentation from the next revision and call it
> "ACPI only" if it makes this work for you.

I would prefer not if that still leaves the property possible in the DT
case. We should figure out how we cater for these differences more
generally; I'm sure more are going to appear.

Mark.

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

[Mika Westerberg]

On Mon, Jan 26, 2015 at 04:39:30PM +0000, Mark Rutland wrote:
> What I don't follow is why GpioInt seems to be translated as a GPIO
> rather than as an interrupt which happens to be backed by a GPIO. Were
> it not for that, the DT and ACPI cases would align better.

Because it *is* a GPIO.

In my experience we have had two kinds of interrupts that the devices
are connected to: pins connected directly to the interrupt controller
(IO-APIC or whatever), and pins connected to the GPIO controller. This
is the later.

Now, the device in question has following resources:

	Name (_CRS, ResourceTemplate () {
		I2cSerialBus (0x004C, ControllerInitiated, 0x00061A80,
		    AddressingMode7Bit, "\\_SB.PCI0.I2C6",
		    0x00, ResourceConsumer,,)
		GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
		    "\\_SB.GPO0", 0x00, ResourceConsumer,,)
		    {
			0x004C
		    }
	})

The GpioInt() above refers to GPIO controller ("\_SB.GPO0") and its pin
number 0x4c.

Normally I would do this in the driver regardless of where it is
described (DT, ACPI, whatnot):

	desc = gpiod_get(dev, NULL);
	gpiod_direction_input(desc);
	irq = gpiod_to_irq(desc);

Then the "irq" can be used for request_irq() and friends.

Note how both DT and ACPI cases align just fine.

If the above is not the right way to use GPIOs as interrupt, can you
please tell me how it is done then?

BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
(which is why I added it to the documentation).

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

[Mark Rutland]

On Tue, Jan 27, 2015 at 10:16:10AM +0000, Mika Westerberg wrote:
> On Mon, Jan 26, 2015 at 04:39:30PM +0000, Mark Rutland wrote:
> > What I don't follow is why GpioInt seems to be translated as a GPIO
> > rather than as an interrupt which happens to be backed by a GPIO. Were
> > it not for that, the DT and ACPI cases would align better.
> 
> Because it *is* a GPIO.

I don't disagree on this point.

However, this is irrelevant from the PoV of the device in question. It
doesn't care what its interrupt line is wired up to so long as it
behaves like an interrupt. Hopefully the example below will clarify what
I'm getting at here.

> In my experience we have had two kinds of interrupts that the devices
> are connected to: pins connected directly to the interrupt controller
> (IO-APIC or whatever), and pins connected to the GPIO controller. This
> is the later.
> 
> Now, the device in question has following resources:
> 
> 	Name (_CRS, ResourceTemplate () {
> 		I2cSerialBus (0x004C, ControllerInitiated, 0x00061A80,
> 		    AddressingMode7Bit, "\\_SB.PCI0.I2C6",
> 		    0x00, ResourceConsumer,,)
> 		GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
> 		    "\\_SB.GPO0", 0x00, ResourceConsumer,,)
> 		    {
> 			0x004C
> 		    }
> 	})
> 
> The GpioInt() above refers to GPIO controller ("\_SB.GPO0") and its pin
> number 0x4c.
> 
> Normally I would do this in the driver regardless of where it is
> described (DT, ACPI, whatnot):
> 
> 	desc = gpiod_get(dev, NULL);
> 	gpiod_direction_input(desc);
> 	irq = gpiod_to_irq(desc);
> 
> Then the "irq" can be used for request_irq() and friends.
> 
> Note how both DT and ACPI cases align just fine.
> 
> If the above is not the right way to use GPIOs as interrupt, can you
> please tell me how it is done then?


So lets say we have a device which generates an interrupt:

	device@f00 {
		compatible = "some-interrupting-device";
		reg = <0xf00 0x100>;
		interrupts = < ... >;
	};

It's intended that this is connected to an interrupt controller:

	ic: interrupt-controller@b00 {
		compatible = "some-interrupt-controller";
		reg = <0xb00 0x100>;
		#interrupt-cells = <1>;
	};

	device@f00 {
		compatible = "some-interrupting-device";
		reg = <0xf00 0x100>;
		interrupt-parent = <&ic>;
		interrupts = <0x3>;
	};

But in some cases, this gets connected to a GPIO controller. In these
cases, the device is still logically generating an interrupt, and the
fact that the endpoint is an interrupt controller is irrelevant from the
PoV of the device. So we acknowledge that the GPIO controller is also
capable of acting as an interrupt controller, and mark it as such:

	gc: gpio-controller@000 {
		compatible = "some-gpio-controller";
		reg = <0x000 0x100>;
		#gpio-cells = <1>;
		#interrupt-cells = <1>;
	};

	device@f00 {
		compatible = "some-interrupting-device";
		reg = <0xf00 0x100>;
		interrupt-parent = <&gc>;
		interrupts = <0x1>;
	};

Thus the device binding only describes the logical interrupt, and the
driver only needs to handle interrupts.

In cases where the binding/driver actually care about the GPIO being a
GPIO (e.g. for card detect in an MMC controller), describing the GPIO as
a GPIO makes sense, and we can try gpio_to_irq as an optimisation over
polling the state of the GPIO.
 
> BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
> (which is why I added it to the documentation).

Sure. My concern is that we should not need to deal with GPIOs in this
case were the GPIO is only there to function as an interrupt.

Given that GpioInt seems to describe an interrupt which happens to be
backed by a GPIO, I don't understand what it is necessary to translate
this as a GPIO rather than an interrupt. If it were going to be used as
a GPIO, then it would be a GpioIO object, no?

Thanks,
Mark.

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

[Mika Westerberg]

On Tue, Jan 27, 2015 at 10:39:25AM +0000, Mark Rutland wrote:
> > If the above is not the right way to use GPIOs as interrupt, can you
> > please tell me how it is done then?
> 
> 
> So lets say we have a device which generates an interrupt:
> 
> 	device@f00 {
> 		compatible = "some-interrupting-device";
> 		reg = <0xf00 0x100>;
> 		interrupts = < ... >;
> 	};
> 
> It's intended that this is connected to an interrupt controller:
> 
> 	ic: interrupt-controller@b00 {
> 		compatible = "some-interrupt-controller";
> 		reg = <0xb00 0x100>;
> 		#interrupt-cells = <1>;
> 	};
> 
> 	device@f00 {
> 		compatible = "some-interrupting-device";
> 		reg = <0xf00 0x100>;
> 		interrupt-parent = <&ic>;
> 		interrupts = <0x3>;
> 	};
> 
> But in some cases, this gets connected to a GPIO controller. In these
> cases, the device is still logically generating an interrupt, and the
> fact that the endpoint is an interrupt controller is irrelevant from the
> PoV of the device. So we acknowledge that the GPIO controller is also
> capable of acting as an interrupt controller, and mark it as such:
> 
> 	gc: gpio-controller@000 {
> 		compatible = "some-gpio-controller";
> 		reg = <0x000 0x100>;
> 		#gpio-cells = <1>;
> 		#interrupt-cells = <1>;
> 	};
> 
> 	device@f00 {
> 		compatible = "some-interrupting-device";
> 		reg = <0xf00 0x100>;
> 		interrupt-parent = <&gc>;
> 		interrupts = <0x1>;
> 	};
> 
> Thus the device binding only describes the logical interrupt, and the
> driver only needs to handle interrupts.

OK.

> In cases where the binding/driver actually care about the GPIO being a
> GPIO (e.g. for card detect in an MMC controller), describing the GPIO as
> a GPIO makes sense, and we can try gpio_to_irq as an optimisation over
> polling the state of the GPIO.

Well, I've seen touch panels where you actually need to switch the GPIO
to be output and do some magic before you can use the same GPIO as an
interrupt.

> > BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
> > (which is why I added it to the documentation).
> 
> Sure. My concern is that we should not need to deal with GPIOs in this
> case were the GPIO is only there to function as an interrupt.
> 
> Given that GpioInt seems to describe an interrupt which happens to be
> backed by a GPIO, I don't understand what it is necessary to translate
> this as a GPIO rather than an interrupt. If it were going to be used as
> a GPIO, then it would be a GpioIO object, no?

OK, so where do you propose we handle the translation if not in the
driver? Also keep in mind that some of the devices may have multiple
GpioInt()s.

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

[Mark Rutland]

On Tue, Jan 27, 2015 at 10:59:31AM +0000, Mika Westerberg wrote:
> On Tue, Jan 27, 2015 at 10:39:25AM +0000, Mark Rutland wrote:
> > > If the above is not the right way to use GPIOs as interrupt, can you
> > > please tell me how it is done then?
> > 
> > 
> > So lets say we have a device which generates an interrupt:
> > 
> > 	device@f00 {
> > 		compatible = "some-interrupting-device";
> > 		reg = <0xf00 0x100>;
> > 		interrupts = < ... >;
> > 	};
> > 
> > It's intended that this is connected to an interrupt controller:
> > 
> > 	ic: interrupt-controller@b00 {
> > 		compatible = "some-interrupt-controller";
> > 		reg = <0xb00 0x100>;
> > 		#interrupt-cells = <1>;
> > 	};
> > 
> > 	device@f00 {
> > 		compatible = "some-interrupting-device";
> > 		reg = <0xf00 0x100>;
> > 		interrupt-parent = <&ic>;
> > 		interrupts = <0x3>;
> > 	};
> > 
> > But in some cases, this gets connected to a GPIO controller. In these
> > cases, the device is still logically generating an interrupt, and the
> > fact that the endpoint is an interrupt controller is irrelevant from the
> > PoV of the device. So we acknowledge that the GPIO controller is also
> > capable of acting as an interrupt controller, and mark it as such:
> > 
> > 	gc: gpio-controller@000 {
> > 		compatible = "some-gpio-controller";
> > 		reg = <0x000 0x100>;
> > 		#gpio-cells = <1>;
> > 		#interrupt-cells = <1>;
> > 	};
> > 
> > 	device@f00 {
> > 		compatible = "some-interrupting-device";
> > 		reg = <0xf00 0x100>;
> > 		interrupt-parent = <&gc>;
> > 		interrupts = <0x1>;
> > 	};
> > 
> > Thus the device binding only describes the logical interrupt, and the
> > driver only needs to handle interrupts.
> 
> OK.
> 
> > In cases where the binding/driver actually care about the GPIO being a
> > GPIO (e.g. for card detect in an MMC controller), describing the GPIO as
> > a GPIO makes sense, and we can try gpio_to_irq as an optimisation over
> > polling the state of the GPIO.
> 
> Well, I've seen touch panels where you actually need to switch the GPIO
> to be output and do some magic before you can use the same GPIO as an
> interrupt.

Ok. That's a nasty case, but surely in that case the relevant GPIO
shoiuld be a GpioIO object for output?

> > > BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
> > > (which is why I added it to the documentation).
> > 
> > Sure. My concern is that we should not need to deal with GPIOs in this
> > case were the GPIO is only there to function as an interrupt.
> > 
> > Given that GpioInt seems to describe an interrupt which happens to be
> > backed by a GPIO, I don't understand what it is necessary to translate
> > this as a GPIO rather than an interrupt. If it were going to be used as
> > a GPIO, then it would be a GpioIO object, no?
> 
> OK, so where do you propose we handle the translation if not in the
> driver? Also keep in mind that some of the devices may have multiple
> GpioInt()s.

To me it seems that GpioInt objects should be translated to interrupts
by some core code. How are interrupts described and handed in ACPI? Are
they resource along the lines of GpioInts, or are they a completely
separate class of device property?

Thanks,
Mark

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

[Mika Westerberg]

On Tue, Jan 27, 2015 at 11:14:58AM +0000, Mark Rutland wrote:
> On Tue, Jan 27, 2015 at 10:59:31AM +0000, Mika Westerberg wrote:
> > On Tue, Jan 27, 2015 at 10:39:25AM +0000, Mark Rutland wrote:
> > > > If the above is not the right way to use GPIOs as interrupt, can you
> > > > please tell me how it is done then?
> > > 
> > > 
> > > So lets say we have a device which generates an interrupt:
> > > 
> > > 	device@f00 {
> > > 		compatible = "some-interrupting-device";
> > > 		reg = <0xf00 0x100>;
> > > 		interrupts = < ... >;
> > > 	};
> > > 
> > > It's intended that this is connected to an interrupt controller:
> > > 
> > > 	ic: interrupt-controller@b00 {
> > > 		compatible = "some-interrupt-controller";
> > > 		reg = <0xb00 0x100>;
> > > 		#interrupt-cells = <1>;
> > > 	};
> > > 
> > > 	device@f00 {
> > > 		compatible = "some-interrupting-device";
> > > 		reg = <0xf00 0x100>;
> > > 		interrupt-parent = <&ic>;
> > > 		interrupts = <0x3>;
> > > 	};
> > > 
> > > But in some cases, this gets connected to a GPIO controller. In these
> > > cases, the device is still logically generating an interrupt, and the
> > > fact that the endpoint is an interrupt controller is irrelevant from the
> > > PoV of the device. So we acknowledge that the GPIO controller is also
> > > capable of acting as an interrupt controller, and mark it as such:
> > > 
> > > 	gc: gpio-controller@000 {
> > > 		compatible = "some-gpio-controller";
> > > 		reg = <0x000 0x100>;
> > > 		#gpio-cells = <1>;
> > > 		#interrupt-cells = <1>;
> > > 	};
> > > 
> > > 	device@f00 {
> > > 		compatible = "some-interrupting-device";
> > > 		reg = <0xf00 0x100>;
> > > 		interrupt-parent = <&gc>;
> > > 		interrupts = <0x1>;
> > > 	};
> > > 
> > > Thus the device binding only describes the logical interrupt, and the
> > > driver only needs to handle interrupts.
> > 
> > OK.
> > 
> > > In cases where the binding/driver actually care about the GPIO being a
> > > GPIO (e.g. for card detect in an MMC controller), describing the GPIO as
> > > a GPIO makes sense, and we can try gpio_to_irq as an optimisation over
> > > polling the state of the GPIO.
> > 
> > Well, I've seen touch panels where you actually need to switch the GPIO
> > to be output and do some magic before you can use the same GPIO as an
> > interrupt.
> 
> Ok. That's a nasty case, but surely in that case the relevant GPIO
> shoiuld be a GpioIO object for output?

I can't remember the details anymore, possibly it was GpioIo().

Nothing prevents you from using GpioIo() as an interrupt.

> 
> > > > BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
> > > > (which is why I added it to the documentation).
> > > 
> > > Sure. My concern is that we should not need to deal with GPIOs in this
> > > case were the GPIO is only there to function as an interrupt.
> > > 
> > > Given that GpioInt seems to describe an interrupt which happens to be
> > > backed by a GPIO, I don't understand what it is necessary to translate
> > > this as a GPIO rather than an interrupt. If it were going to be used as
> > > a GPIO, then it would be a GpioIO object, no?
> > 
> > OK, so where do you propose we handle the translation if not in the
> > driver? Also keep in mind that some of the devices may have multiple
> > GpioInt()s.
> 
> To me it seems that GpioInt objects should be translated to interrupts
> by some core code. How are interrupts described and handed in ACPI? Are
> they resource along the lines of GpioInts, or are they a completely
> separate class of device property?

They are similar resources in _CRS, like GpioIo/GpioInt etc. Below is
from another touch panel:

	Name (_CRS, ResourceTemplate () {
		I2cSerialBus (0x004C, ControllerInitiated, 0x00061A80,
			AddressingMode7Bit, "\\_SB.PCI0.I2C1", 0x00, ResourceConsumer,,)
		Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
		{
		    0x00000022,
		}
	})

If we see one of the above we automatically add it to client->irq in
case of I2C device.

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

[Mark Rutland]

On Tue, Jan 27, 2015 at 11:30:41AM +0000, Mika Westerberg wrote:
> On Tue, Jan 27, 2015 at 11:14:58AM +0000, Mark Rutland wrote:
> > On Tue, Jan 27, 2015 at 10:59:31AM +0000, Mika Westerberg wrote:
> > > On Tue, Jan 27, 2015 at 10:39:25AM +0000, Mark Rutland wrote:
> > > > > If the above is not the right way to use GPIOs as interrupt, can you
> > > > > please tell me how it is done then?
> > > > 
> > > > 
> > > > So lets say we have a device which generates an interrupt:
> > > > 
> > > > 	device@f00 {
> > > > 		compatible = "some-interrupting-device";
> > > > 		reg = <0xf00 0x100>;
> > > > 		interrupts = < ... >;
> > > > 	};
> > > > 
> > > > It's intended that this is connected to an interrupt controller:
> > > > 
> > > > 	ic: interrupt-controller@b00 {
> > > > 		compatible = "some-interrupt-controller";
> > > > 		reg = <0xb00 0x100>;
> > > > 		#interrupt-cells = <1>;
> > > > 	};
> > > > 
> > > > 	device@f00 {
> > > > 		compatible = "some-interrupting-device";
> > > > 		reg = <0xf00 0x100>;
> > > > 		interrupt-parent = <&ic>;
> > > > 		interrupts = <0x3>;
> > > > 	};
> > > > 
> > > > But in some cases, this gets connected to a GPIO controller. In these
> > > > cases, the device is still logically generating an interrupt, and the
> > > > fact that the endpoint is an interrupt controller is irrelevant from the
> > > > PoV of the device. So we acknowledge that the GPIO controller is also
> > > > capable of acting as an interrupt controller, and mark it as such:
> > > > 
> > > > 	gc: gpio-controller@000 {
> > > > 		compatible = "some-gpio-controller";
> > > > 		reg = <0x000 0x100>;
> > > > 		#gpio-cells = <1>;
> > > > 		#interrupt-cells = <1>;
> > > > 	};
> > > > 
> > > > 	device@f00 {
> > > > 		compatible = "some-interrupting-device";
> > > > 		reg = <0xf00 0x100>;
> > > > 		interrupt-parent = <&gc>;
> > > > 		interrupts = <0x1>;
> > > > 	};
> > > > 
> > > > Thus the device binding only describes the logical interrupt, and the
> > > > driver only needs to handle interrupts.
> > > 
> > > OK.
> > > 
> > > > In cases where the binding/driver actually care about the GPIO being a
> > > > GPIO (e.g. for card detect in an MMC controller), describing the GPIO as
> > > > a GPIO makes sense, and we can try gpio_to_irq as an optimisation over
> > > > polling the state of the GPIO.
> > > 
> > > Well, I've seen touch panels where you actually need to switch the GPIO
> > > to be output and do some magic before you can use the same GPIO as an
> > > interrupt.
> > 
> > Ok. That's a nasty case, but surely in that case the relevant GPIO
> > shoiuld be a GpioIO object for output?
> 
> I can't remember the details anymore, possibly it was GpioIo().
> 
> Nothing prevents you from using GpioIo() as an interrupt.

Certainly. As I mention above, in the case of something like a card
detect pin, it makes sense to be able to acquire an interrupt for the
GPIO.

> > > > > BTW, passing NULL to gpiod_get() implies property named "gpios" in DT
> > > > > (which is why I added it to the documentation).
> > > > 
> > > > Sure. My concern is that we should not need to deal with GPIOs in this
> > > > case were the GPIO is only there to function as an interrupt.
> > > > 
> > > > Given that GpioInt seems to describe an interrupt which happens to be
> > > > backed by a GPIO, I don't understand what it is necessary to translate
> > > > this as a GPIO rather than an interrupt. If it were going to be used as
> > > > a GPIO, then it would be a GpioIO object, no?
> > > 
> > > OK, so where do you propose we handle the translation if not in the
> > > driver? Also keep in mind that some of the devices may have multiple
> > > GpioInt()s.
> > 
> > To me it seems that GpioInt objects should be translated to interrupts
> > by some core code. How are interrupts described and handed in ACPI? Are
> > they resource along the lines of GpioInts, or are they a completely
> > separate class of device property?
> 
> They are similar resources in _CRS, like GpioIo/GpioInt etc. Below is
> from another touch panel:
> 
> 	Name (_CRS, ResourceTemplate () {
> 		I2cSerialBus (0x004C, ControllerInitiated, 0x00061A80,
> 			AddressingMode7Bit, "\\_SB.PCI0.I2C1", 0x00, ResourceConsumer,,)
> 		Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
> 		{
> 		    0x00000022,
> 		}
> 	})
> 
> If we see one of the above we automatically add it to client->irq in
> case of I2C device.

Ok, that allays my fear w.r.t. ordering of the resources.

As I see it, the fact that we convert GpioInt entries to GPIOs rather
than irqs when parsing _CRS is the issue here, and to me it makes no
sense that we do so. Were we to treat them as interrupts, the binding is
fine as-is, and we'd do the same thing in DT and ACPI.

The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
interrupt (which happens to be backed by a GPIO), and is not something
that necessarily makes sense as a GPIO.

So why do we currently ignore the GpioInt/GpioIo distinction and treat
GpioInts as GPIOs rather than interrupts?

Thanks,
Mark.

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

[Mika Westerberg]

On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> Ok, that allays my fear w.r.t. ordering of the resources.
> 
> As I see it, the fact that we convert GpioInt entries to GPIOs rather
> than irqs when parsing _CRS is the issue here, and to me it makes no
> sense that we do so. Were we to treat them as interrupts, the binding is
> fine as-is, and we'd do the same thing in DT and ACPI.
> 
> The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> interrupt (which happens to be backed by a GPIO), and is not something
> that necessarily makes sense as a GPIO.

I would rather say that GpioInt *is* a GPIO. That can then used as an
interrupt but it should not prevent you from using it as GPIO instead.
For example if you just want to poll that something is 0 or 1. That
should be possible as well and nothing say that you cannot do that for
GpioInt().

> So why do we currently ignore the GpioInt/GpioIo distinction and treat
> GpioInts as GPIOs rather than interrupts?

See above.

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

[Mark Rutland]

On Tue, Jan 27, 2015 at 02:41:27PM +0000, Mika Westerberg wrote:
> On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> > Ok, that allays my fear w.r.t. ordering of the resources.
> > 
> > As I see it, the fact that we convert GpioInt entries to GPIOs rather
> > than irqs when parsing _CRS is the issue here, and to me it makes no
> > sense that we do so. Were we to treat them as interrupts, the binding is
> > fine as-is, and we'd do the same thing in DT and ACPI.
> > 
> > The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> > interrupt (which happens to be backed by a GPIO), and is not something
> > that necessarily makes sense as a GPIO.
> 
> I would rather say that GpioInt *is* a GPIO. That can then used as an
> interrupt but it should not prevent you from using it as GPIO instead.
> For example if you just want to poll that something is 0 or 1. That
> should be possible as well and nothing say that you cannot do that for
> GpioInt().

>From my POV a GpioInt is logically an interrupt, or it would be a
GpioIo. That doesn't necessarily mean it's invalid to try to query its
state as a GPIO, but I do not think that it makes sense to handle it by
default as a GPIO given that it was handed to us as a GpioInt so that it
can be used as an interrupt.

If it's just the case that ACPI and DT differ w.r.t. how this case (an
interrupt line wired to a GPIO) is described, that in itself is fine;
different standards have different models.

However, I do not think we must change the DT binding and violate
established DT practice simply becuase on the ACPI side things are
different (nor would it make sense to do things the other way around).
If the two have different rules, then we should handle those rules
separately rather than trying to force the two together when they
clearly don't fit.

In the driver that can easily be achieved with separate probe paths.
W.R.T. the DT binding, I do not think it makes sense to pretend this
is also an ACPI binding.

Thanks,
Mark.

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

[Mika Westerberg]

On Tue, Jan 27, 2015 at 03:06:06PM +0000, Mark Rutland wrote:
> On Tue, Jan 27, 2015 at 02:41:27PM +0000, Mika Westerberg wrote:
> > On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> > > Ok, that allays my fear w.r.t. ordering of the resources.
> > > 
> > > As I see it, the fact that we convert GpioInt entries to GPIOs rather
> > > than irqs when parsing _CRS is the issue here, and to me it makes no
> > > sense that we do so. Were we to treat them as interrupts, the binding is
> > > fine as-is, and we'd do the same thing in DT and ACPI.
> > > 
> > > The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> > > interrupt (which happens to be backed by a GPIO), and is not something
> > > that necessarily makes sense as a GPIO.
> > 
> > I would rather say that GpioInt *is* a GPIO. That can then used as an
> > interrupt but it should not prevent you from using it as GPIO instead.
> > For example if you just want to poll that something is 0 or 1. That
> > should be possible as well and nothing say that you cannot do that for
> > GpioInt().
> 
> >From my POV a GpioInt is logically an interrupt, or it would be a
> GpioIo. That doesn't necessarily mean it's invalid to try to query its
> state as a GPIO, but I do not think that it makes sense to handle it by
> default as a GPIO given that it was handed to us as a GpioInt so that it
> can be used as an interrupt.
> 
> If it's just the case that ACPI and DT differ w.r.t. how this case (an
> interrupt line wired to a GPIO) is described, that in itself is fine;
> different standards have different models.
> 
> However, I do not think we must change the DT binding and violate
> established DT practice simply becuase on the ACPI side things are
> different (nor would it make sense to do things the other way around).
> If the two have different rules, then we should handle those rules
> separately rather than trying to force the two together when they
> clearly don't fit.

I agree.

> In the driver that can easily be achieved with separate probe paths.

Yes, that's what this patch is doing. It has different paths for
interrupt and GPIO cases. If we find that there is an interrupt number
already, then we use that directly.

If not we try to look for a GPIO which we could use as an interrupt.

The reason why this is not done only in the ACPI probe path is that
there is nothing ACPI specific in gpiod_get() and friends.

> W.R.T. the DT binding, I do not think it makes sense to pretend this
> is also an ACPI binding.

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

[Mark Rutland]

On Tue, Jan 27, 2015 at 03:21:27PM +0000, Mika Westerberg wrote:
> On Tue, Jan 27, 2015 at 03:06:06PM +0000, Mark Rutland wrote:
> > On Tue, Jan 27, 2015 at 02:41:27PM +0000, Mika Westerberg wrote:
> > > On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> > > > Ok, that allays my fear w.r.t. ordering of the resources.
> > > > 
> > > > As I see it, the fact that we convert GpioInt entries to GPIOs rather
> > > > than irqs when parsing _CRS is the issue here, and to me it makes no
> > > > sense that we do so. Were we to treat them as interrupts, the binding is
> > > > fine as-is, and we'd do the same thing in DT and ACPI.
> > > > 
> > > > The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> > > > interrupt (which happens to be backed by a GPIO), and is not something
> > > > that necessarily makes sense as a GPIO.
> > > 
> > > I would rather say that GpioInt *is* a GPIO. That can then used as an
> > > interrupt but it should not prevent you from using it as GPIO instead.
> > > For example if you just want to poll that something is 0 or 1. That
> > > should be possible as well and nothing say that you cannot do that for
> > > GpioInt().
> > 
> > >From my POV a GpioInt is logically an interrupt, or it would be a
> > GpioIo. That doesn't necessarily mean it's invalid to try to query its
> > state as a GPIO, but I do not think that it makes sense to handle it by
> > default as a GPIO given that it was handed to us as a GpioInt so that it
> > can be used as an interrupt.
> > 
> > If it's just the case that ACPI and DT differ w.r.t. how this case (an
> > interrupt line wired to a GPIO) is described, that in itself is fine;
> > different standards have different models.
> > 
> > However, I do not think we must change the DT binding and violate
> > established DT practice simply becuase on the ACPI side things are
> > different (nor would it make sense to do things the other way around).
> > If the two have different rules, then we should handle those rules
> > separately rather than trying to force the two together when they
> > clearly don't fit.
> 
> I agree.
> 
> > In the driver that can easily be achieved with separate probe paths.
> 
> Yes, that's what this patch is doing. It has different paths for
> interrupt and GPIO cases. If we find that there is an interrupt number
> already, then we use that directly.
> 
> If not we try to look for a GPIO which we could use as an interrupt.

My comment about separate probe paths was w.r.t. ACPI and DT. In DT,
acquiring a GPIO here shouldn't be necessary (and ideally shouldn't be
allowed) because the GPIO should be described as an interrupt (we need
this to have the correct flags).

> The reason why this is not done only in the ACPI probe path is that
> there is nothing ACPI specific in gpiod_get() and friends.

While the high-level APIs are not specific to ACPI or DT, the behaviour
we want for DT is subtly different top the behaviour you want for ACPI.

In DT we don't necessarily have the relevant flags (e.g. edge vs level,
active high vs active low) unless the GPIO is described as an interrupt,
so permitting it to be described as a GPIO permits situations which we
cannot handle correctly. In the DT case therefore we must not acquire
the interrupt GPIO as a GPIO; it must be described as an interrupt.

So if we need to acquire a GPIO in the ACPI case, this should be limited
to the ACPI case.

Looking further at ACPI, the flags issue also seems to be a problem
there. If the interrupt GPIO were described as a GpioIo, then you could
acquire it as a GPIO with gpiod_get(), but wouldn't have the EdgeLevel
and ActiveLevel properties, and therefore cannot necessarily correctly
convert the GPIO to an irq, no?

Thanks,
Mark.

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

[Mika Westerberg]

On Tue, Jan 27, 2015 at 03:57:47PM +0000, Mark Rutland wrote:
> On Tue, Jan 27, 2015 at 03:21:27PM +0000, Mika Westerberg wrote:
> > On Tue, Jan 27, 2015 at 03:06:06PM +0000, Mark Rutland wrote:
> > > On Tue, Jan 27, 2015 at 02:41:27PM +0000, Mika Westerberg wrote:
> > > > On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> > > > > Ok, that allays my fear w.r.t. ordering of the resources.
> > > > > 
> > > > > As I see it, the fact that we convert GpioInt entries to GPIOs rather
> > > > > than irqs when parsing _CRS is the issue here, and to me it makes no
> > > > > sense that we do so. Were we to treat them as interrupts, the binding is
> > > > > fine as-is, and we'd do the same thing in DT and ACPI.
> > > > > 
> > > > > The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> > > > > interrupt (which happens to be backed by a GPIO), and is not something
> > > > > that necessarily makes sense as a GPIO.
> > > > 
> > > > I would rather say that GpioInt *is* a GPIO. That can then used as an
> > > > interrupt but it should not prevent you from using it as GPIO instead.
> > > > For example if you just want to poll that something is 0 or 1. That
> > > > should be possible as well and nothing say that you cannot do that for
> > > > GpioInt().
> > > 
> > > >From my POV a GpioInt is logically an interrupt, or it would be a
> > > GpioIo. That doesn't necessarily mean it's invalid to try to query its
> > > state as a GPIO, but I do not think that it makes sense to handle it by
> > > default as a GPIO given that it was handed to us as a GpioInt so that it
> > > can be used as an interrupt.
> > > 
> > > If it's just the case that ACPI and DT differ w.r.t. how this case (an
> > > interrupt line wired to a GPIO) is described, that in itself is fine;
> > > different standards have different models.
> > > 
> > > However, I do not think we must change the DT binding and violate
> > > established DT practice simply becuase on the ACPI side things are
> > > different (nor would it make sense to do things the other way around).
> > > If the two have different rules, then we should handle those rules
> > > separately rather than trying to force the two together when they
> > > clearly don't fit.
> > 
> > I agree.
> > 
> > > In the driver that can easily be achieved with separate probe paths.
> > 
> > Yes, that's what this patch is doing. It has different paths for
> > interrupt and GPIO cases. If we find that there is an interrupt number
> > already, then we use that directly.
> > 
> > If not we try to look for a GPIO which we could use as an interrupt.
> 
> My comment about separate probe paths was w.r.t. ACPI and DT. In DT,
> acquiring a GPIO here shouldn't be necessary (and ideally shouldn't be
> allowed) because the GPIO should be described as an interrupt (we need
> this to have the correct flags).
> 
> > The reason why this is not done only in the ACPI probe path is that
> > there is nothing ACPI specific in gpiod_get() and friends.
> 
> While the high-level APIs are not specific to ACPI or DT, the behaviour
> we want for DT is subtly different top the behaviour you want for ACPI.
> 
> In DT we don't necessarily have the relevant flags (e.g. edge vs level,
> active high vs active low) unless the GPIO is described as an interrupt,
> so permitting it to be described as a GPIO permits situations which we
> cannot handle correctly. In the DT case therefore we must not acquire
> the interrupt GPIO as a GPIO; it must be described as an interrupt.
> 
> So if we need to acquire a GPIO in the ACPI case, this should be limited
> to the ACPI case.

How about cases where platform data (no ACPI or DT) is used? You can
also provide lookup table to the GPIOs from platform data.

> Looking further at ACPI, the flags issue also seems to be a problem
> there. If the interrupt GPIO were described as a GpioIo, then you could
> acquire it as a GPIO with gpiod_get(), but wouldn't have the EdgeLevel
> and ActiveLevel properties, and therefore cannot necessarily correctly
> convert the GPIO to an irq, no?

That's right but typically what I've seen the drivers configure the GPIO
as they think is necessary.

Anyway, I think I'll give up now and leave the i2c-hid.c driver to
support only interrupts for now.

Re: [PATCH 1/2] HID: i2c-hid: The interrupt should be level sensitive

[Benjamin Tissoires]

On Jan 26 2015 or thereabouts, Mika Westerberg wrote:
> The Microsoft HID over I2C specification says two things regarding the
> interrupt:
> 
>  1) The interrupt should be level sensitive
>  2) The device keeps the interrupt asserted as long as it has more reports
>     available.
> 
> We've seen that at least some Atmel and N-Trig panels keep the line low as
> long as they have something to send. The current version of the driver only
> detects the first edge but then fails to read rest of the reports (as the
> line is still asserted).
> 
> Make the driver follow the specification and configure the HID interrupt to
> be level sensitive.
> 
> The Windows HID over I2C driver also seems to do the same.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

FWIW, this one is
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

looks like we are going to drop 2/2, but this one is still valid.

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index d43e967e7533..8f1dfc5c5d9c 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -785,7 +785,7 @@ static int i2c_hid_init_irq(struct i2c_client *client)
>  	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
>  
>  	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> -			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>  			client->name, ihid);
>  	if (ret < 0) {
>  		dev_warn(&client->dev,
> -- 
> 2.1.4
> 

Re: [PATCH 1/2] HID: i2c-hid: The interrupt should be level sensitive

[Jiri Kosina]

On Thu, 29 Jan 2015, Benjamin Tissoires wrote:

> > The Microsoft HID over I2C specification says two things regarding the
> > interrupt:
> > 
> >  1) The interrupt should be level sensitive
> >  2) The device keeps the interrupt asserted as long as it has more reports
> >     available.
> > 
> > We've seen that at least some Atmel and N-Trig panels keep the line low as
> > long as they have something to send. The current version of the driver only
> > detects the first edge but then fails to read rest of the reports (as the
> > line is still asserted).
> > 
> > Make the driver follow the specification and configure the HID interrupt to
> > be level sensitive.
> > 
> > The Windows HID over I2C driver also seems to do the same.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> 
> FWIW, this one is
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> looks like we are going to drop 2/2, but this one is still valid.

Applied to for-3.20/upstream-fixes, thanks for your friendly ping in form 
of Ack :)

-- 
Jiri Kosina
SUSE Labs