[PATCH v2 0/3] ACPI: Translate Linux IRQ number directly from GpioInt

[PATCH v2 0/3] ACPI: Translate Linux IRQ number directly from GpioInt

[Mika Westerberg]

Hi,

This is second iteration of the series. The previous version can be found
from [1].

Currently drivers for ACPI enumerated devices that have their interrupt
line connected to a GPIO controller instead of IO-APIC are required to do
complete gpiod_get()/gpiod_to_irq() etc. dance themselves. This adds
unnecessary lines of code to these drivers.

It turned out that DT solved the problem already with introduction of
of_irq_get() which is able to handle GPIO based interrupts as well through
irqchip API [2].

The following three patches achieve the same for ACPI by introducing new
function acpi_dev_gpio_irq_get() that is then used in I2C core to automatically
translate ACPI GpioInt resource to Linux IRQ number.

Changes to the previous version:
 - Added Rafael's ack to the first patch
 - Introduce new patch that makes I2C ACPI slave enumeration code use 0 to mean
   no interrupt.

If no objections, I would like this to merged via either I2C or GPIO trees.

Thanks.

[1] https://lkml.org/lkml/2015/4/28/455
[2] https://lkml.org/lkml/2015/3/25/103

Mika Westerberg (3):
  gpio / ACPI: Add support for retrieving GpioInt resources from a device
  i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned
  i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

 drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c      | 12 ++++++++----
 include/linux/acpi.h        |  7 +++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] gpio / ACPI: Add support for retrieving GpioInt resources from a device

[Mika Westerberg]

ACPI specification knows two types of GPIOs: GpioIo and GpioInt. The latter
is used to describe that a given device interrupt line is connected to a
specific GPIO pin. Typical ACPI _CRS entry for such device looks like
below:

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

Currently drivers need to request a GPIO corresponding to the right GpioInt
and then translate that to Linux IRQ number. This adds unnecessary lines of
boiler-plate code.

We can ease this a bit by introducing acpi_dev_gpio_irq_get() analogous to
of_irq_get(). This function translates given GpioInt resource under the
device in question to the suitable Linux IRQ number.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++++++++
 include/linux/acpi.h        |  7 +++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d2303d50f561..bff29bb0a3fe 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -514,6 +514,35 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
 	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
 }
 
+/**
+ * acpi_dev_gpio_irq_get() - Find GpioInt and translate it to Linux IRQ number
+ * @adev: pointer to a ACPI device to get IRQ from
+ * @index: index of GpioInt resource (starting from %0)
+ *
+ * If the device has one or more GpioInt resources, this function can be
+ * used to translate from the GPIO offset in the resource to the Linux IRQ
+ * number.
+ *
+ * Return: Linux IRQ number (>%0) on success, negative errno on failure.
+ */
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
+{
+	int idx, i;
+
+	for (i = 0, idx = 0; idx <= index; i++) {
+		struct acpi_gpio_info info;
+		struct gpio_desc *desc;
+
+		desc = acpi_get_gpiod_by_index(adev, NULL, i, &info);
+		if (IS_ERR(desc))
+			break;
+		if (info.gpioint && idx++ == index)
+			return gpiod_to_irq(desc);
+	}
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_get);
+
 static acpi_status
 acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 			    u32 bits, u64 *value, void *handler_context,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index e4da5e35e29c..f57c440642cd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -721,6 +721,8 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
 	if (adev)
 		adev->driver_gpios = NULL;
 }
+
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
 #else
 static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
 			      const struct acpi_gpio_mapping *gpios)
@@ -728,6 +730,11 @@ static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
 	return -ENXIO;
 }
 static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
+
+static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
+{
+	return -ENXIO;
+}
 #endif
 
 /* Device properties */
-- 
2.1.4

Re: [PATCH v2 1/3] gpio / ACPI: Add support for retrieving GpioInt resources from a device

[Linus Walleij]

On Wed, May 6, 2015 at 12:29 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> ACPI specification knows two types of GPIOs: GpioIo and GpioInt. The latter
> is used to describe that a given device interrupt line is connected to a
> specific GPIO pin. Typical ACPI _CRS entry for such device looks like
> below:
>
>     Name (_CRS, ResourceTemplate ()
>     {
>         I2cSerialBus (0x004A, ControllerInitiated, 0x00061A80,
>                       AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>                       0x00, ResourceConsumer)
>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>                 IoRestrictionOutputOnly, "\\_SB.GPO0",
>                 0x00, ResourceConsumer)
>         {
>             0x004B
>         }
>         GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
>                  "\\_SB.GPO0", 0x00, ResourceConsumer)
>         {
>             0x004C
>         }
>     })
>
> Currently drivers need to request a GPIO corresponding to the right GpioInt
> and then translate that to Linux IRQ number. This adds unnecessary lines of
> boiler-plate code.
>
> We can ease this a bit by introducing acpi_dev_gpio_irq_get() analogous to
> of_irq_get(). This function translates given GpioInt resource under the
> device in question to the suitable Linux IRQ number.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] gpio / ACPI: Add support for retrieving GpioInt resources from a device

[Antonio Ospite]

On Wed,  6 May 2015 13:29:06 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> ACPI specification knows two types of GPIOs: GpioIo and GpioInt. The latter
> is used to describe that a given device interrupt line is connected to a
> specific GPIO pin. Typical ACPI _CRS entry for such device looks like
> below:
> 
>     Name (_CRS, ResourceTemplate ()
>     {
>         I2cSerialBus (0x004A, ControllerInitiated, 0x00061A80,
>                       AddressingMode7Bit, "\\_SB.PCI0.I2C6",
>                       0x00, ResourceConsumer)
>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>                 IoRestrictionOutputOnly, "\\_SB.GPO0",
>                 0x00, ResourceConsumer)
>         {
>             0x004B
>         }
>         GpioInt (Level, ActiveLow, Shared, PullDefault, 0x0000,
>                  "\\_SB.GPO0", 0x00, ResourceConsumer)
>         {
>             0x004C
>         }
>     })
> 
> Currently drivers need to request a GPIO corresponding to the right GpioInt
> and then translate that to Linux IRQ number. This adds unnecessary lines of
> boiler-plate code.
> 
> We can ease this a bit by introducing acpi_dev_gpio_irq_get() analogous to
> of_irq_get(). This function translates given GpioInt resource under the
> device in question to the suitable Linux IRQ number.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Antonio Ospite <ao2@ao2.it>

Thanks a lot Mika, this is very useful, my touchscreen now works without
adding a struct acpi_gpio_mapping and the related boilerplate code to
the goodix driver.

Thanks again,
   Antonio

> ---
>  drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++++++++
>  include/linux/acpi.h        |  7 +++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index d2303d50f561..bff29bb0a3fe 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -514,6 +514,35 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
>  	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
>  }
>  
> +/**
> + * acpi_dev_gpio_irq_get() - Find GpioInt and translate it to Linux IRQ number
> + * @adev: pointer to a ACPI device to get IRQ from
> + * @index: index of GpioInt resource (starting from %0)
> + *
> + * If the device has one or more GpioInt resources, this function can be
> + * used to translate from the GPIO offset in the resource to the Linux IRQ
> + * number.
> + *
> + * Return: Linux IRQ number (>%0) on success, negative errno on failure.

The percent in the comment here is supposed to be and equal sign, isn't
it?

> + */
> +int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
> +{
> +	int idx, i;
> +
> +	for (i = 0, idx = 0; idx <= index; i++) {
> +		struct acpi_gpio_info info;
> +		struct gpio_desc *desc;
> +
> +		desc = acpi_get_gpiod_by_index(adev, NULL, i, &info);
> +		if (IS_ERR(desc))
> +			break;
> +		if (info.gpioint && idx++ == index)
> +			return gpiod_to_irq(desc);
> +	}
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_get);
> +
>  static acpi_status
>  acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  			    u32 bits, u64 *value, void *handler_context,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index e4da5e35e29c..f57c440642cd 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -721,6 +721,8 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
>  	if (adev)
>  		adev->driver_gpios = NULL;
>  }
> +
> +int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
>  #else
>  static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
>  			      const struct acpi_gpio_mapping *gpios)
> @@ -728,6 +730,11 @@ static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
>  	return -ENXIO;
>  }
>  static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
> +
> +static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
> +{
> +	return -ENXIO;
> +}
>  #endif
>  
>  /* Device properties */
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
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?

[PATCH v2 2/3] i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned

[Mika Westerberg]

This is the convention used in most parts of the kernel including DT
counterpart of I2C slave enumeration. To make things consistent do the same
for ACPI I2C slave enumeration path as well.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 987c124432c5..c21b3de70234 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -107,7 +107,7 @@ static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
 			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
 				info->flags |= I2C_CLIENT_TEN;
 		}
-	} else if (info->irq < 0) {
+	} else if (!info->irq) {
 		struct resource r;
 
 		if (acpi_dev_resource_interrupt(ares, 0, &r))
@@ -134,7 +134,6 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 
 	memset(&info, 0, sizeof(info));
 	info.fwnode = acpi_fwnode_handle(adev);
-	info.irq = -1;
 
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list,
-- 
2.1.4

Re: [PATCH v2 2/3] i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned

[Linus Walleij]

On Wed, May 6, 2015 at 12:29 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> This is the convention used in most parts of the kernel including DT
> counterpart of I2C slave enumeration. To make things consistent do the same
> for ACPI I2C slave enumeration path as well.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

This needs to go through the GPIO tree along with 3/3.
Wolfram, can I have your ACK on this patch?

Yours,
Linus Walleij

Re: [PATCH v2 2/3] i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned

[wsa]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Wed, May 06, 2015 at 01:29:07PM +0300, Mika Westerberg wrote:
> This is the convention used in most parts of the kernel including DT
> counterpart of I2C slave enumeration. To make things consistent do the same
> for ACPI I2C slave enumeration path as well.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/3] i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned

[Linus Walleij]

On Wed, May 6, 2015 at 12:29 PM, Mika Westerberg
<mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:

> This is the convention used in most parts of the kernel including DT
> counterpart of I2C slave enumeration. To make things consistent do the same
> for ACPI I2C slave enumeration path as well.
>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Patch applied to the GPIO tree with Wolfram's ACK.

Yours,
Linus Walleij

[PATCH v2 3/3] i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

[Mika Westerberg]

Following what DT already does. If the device does not have ACPI Interrupt
resource but instead it has one or more GpioInt resources listed below it,
we take the first GpioInt resource, convert it to suitable Linux IRQ number
and pass it to the driver instead.

This makes drivers simpler because the don't need to care about GPIOs at
all if only thing they need is interrupt.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/i2c-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c21b3de70234..fc2ee8213fb6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -631,8 +631,13 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
-	if (!client->irq && dev->of_node) {
-		int irq = of_irq_get(dev->of_node, 0);
+	if (!client->irq) {
+		int irq = -ENOENT;
+
+		if (dev->of_node)
+			irq = of_irq_get(dev->of_node, 0);
+		else if (ACPI_COMPANION(dev))
+			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
 
 		if (irq == -EPROBE_DEFER)
 			return irq;
-- 
2.1.4

Re: [PATCH v2 3/3] i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

[Linus Walleij]

On Wed, May 6, 2015 at 12:29 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Following what DT already does. If the device does not have ACPI Interrupt
> resource but instead it has one or more GpioInt resources listed below it,
> we take the first GpioInt resource, convert it to suitable Linux IRQ number
> and pass it to the driver instead.
>
> This makes drivers simpler because the don't need to care about GPIOs at
> all if only thing they need is interrupt.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Need Wolfram's ACK on this too, for funneling through the GPIO tree.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

[wsa]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Wed, May 06, 2015 at 01:29:08PM +0300, Mika Westerberg wrote:
> Following what DT already does. If the device does not have ACPI Interrupt
> resource but instead it has one or more GpioInt resources listed below it,
> we take the first GpioInt resource, convert it to suitable Linux IRQ number
> and pass it to the driver instead.
> 
> This makes drivers simpler because the don't need to care about GPIOs at
> all if only thing they need is interrupt.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/3] i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

[Linus Walleij]

On Wed, May 6, 2015 at 12:29 PM, Mika Westerberg
<mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:

> Following what DT already does. If the device does not have ACPI Interrupt
> resource but instead it has one or more GpioInt resources listed below it,
> we take the first GpioInt resource, convert it to suitable Linux IRQ number
> and pass it to the driver instead.
>
> This makes drivers simpler because the don't need to care about GPIOs at
> all if only thing they need is interrupt.
>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Patch applied to the GPIO tree with Wolfram's ACK.

Yours,
Linus Walleij