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

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

[Mika Westerberg]

Hi,

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 [1].

Following two patches does 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.

This requires that the boot firmware (BIOS/coreboot) configures these pins
correctly (input, etc) before handing over to OS. I've tested this on Intel
Baytrail, Braswell and Skylake based machines where this is true.

[1] https://lkml.org/lkml/2015/3/25/103

Mika Westerberg (2):
  gpio / ACPI: Add support for retrieving GpioInt resources from a device
  i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

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

-- 
2.1.4

[PATCH 1/2] 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>
---
 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 1/2] gpio / ACPI: Add support for retrieving GpioInt resources from a device

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 06:05:06 PM Mika Westerberg 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>

> ---
>  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 */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Mika Westerberg]

On Tue, May 05, 2015 at 01:47:36AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, April 28, 2015 06:05:06 PM Mika Westerberg 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>

Thanks Rafael.

I'm going to change the other patch a bit to address conserns from
Wolfram so that we will use irq == 0 to indicate an invalid interrupt
instead of -1.

[PATCH 2/2] 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 987c124432c5..01ef731281e2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -632,8 +632,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 <= 0) {
+		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 2/2] i2c / ACPI: Assign IRQ for devices that have GpioInt automatically

[Wolfram Sang]

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


> -	if (!client->irq && dev->of_node) {
> -		int irq = of_irq_get(dev->of_node, 0);
> +	if (client->irq <= 0) {
> +		int irq = -ENOENT;

Why the move from !client->irq to <= 0? If I didn't miss something,
interrupt numbers are still a sleeping dog with all the unsigned vs
signed fuzz. If this change is needed, this needs proper description and
ideally a seperate patch.


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

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

[Mika Westerberg]

On Wed, Apr 29, 2015 at 11:56:06AM +0200, Wolfram Sang wrote:
> 
> > -	if (!client->irq && dev->of_node) {
> > -		int irq = of_irq_get(dev->of_node, 0);
> > +	if (client->irq <= 0) {
> > +		int irq = -ENOENT;
> 
> Why the move from !client->irq to <= 0? If I didn't miss something,
> interrupt numbers are still a sleeping dog with all the unsigned vs
> signed fuzz. If this change is needed, this needs proper description and
> ideally a seperate patch.

It is there because ACPI parts of I2C client enumeration code
initializes client->irq with -1. Alternatively we can change that code
to use 0 for missing IRQ.

Re: [PATCH 0/2] ACPI: Translate Linux IRQ number directly from GpioInt

[sathyanarayanan kuppuswamy]

On 04/28/2015 08:05 AM, Mika Westerberg wrote:
> Hi,
>
> 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 [1].
>
> Following two patches does 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.
>
> This requires that the boot firmware (BIOS/coreboot) configures these pins
> correctly (input, etc) before handing over to OS. I've tested this on Intel
> Baytrail, Braswell and Skylake based machines where this is true.
Why not configure the GPIO pin as input in the API itself ?
>
> [1] https://lkml.org/lkml/2015/3/25/103
>
> Mika Westerberg (2):
>    gpio / ACPI: Add support for retrieving GpioInt resources from a device
>    i2c / ACPI: Assign IRQ for devices that have GpioInt automatically
>
>   drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++++++++
>   drivers/i2c/i2c-core.c      |  9 +++++++--
>   include/linux/acpi.h        |  7 +++++++
>   3 files changed, 43 insertions(+), 2 deletions(-)
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH 0/2] ACPI: Translate Linux IRQ number directly from GpioInt

[Mika Westerberg]

On Tue, Apr 28, 2015 at 10:36:48AM -0700, sathyanarayanan kuppuswamy wrote:
> >This requires that the boot firmware (BIOS/coreboot) configures these pins
> >correctly (input, etc) before handing over to OS. I've tested this on Intel
> >Baytrail, Braswell and Skylake based machines where this is true.
>
> Why not configure the GPIO pin as input in the API itself ?

IIRC Octavian tried to do just that but it turned out that some of the
irqchip functions are called with spinlock held, which means you can't
call all gpiod_* functions there (as they might sleep).

Re: [PATCH 0/2] ACPI: Translate Linux IRQ number directly from GpioInt

[sathyanarayanan kuppuswamy]

On 04/29/2015 02:26 AM, Mika Westerberg wrote:
> On Tue, Apr 28, 2015 at 10:36:48AM -0700, sathyanarayanan kuppuswamy wrote:
>>> This requires that the boot firmware (BIOS/coreboot) configures these pins
>>> correctly (input, etc) before handing over to OS. I've tested this on Intel
>>> Baytrail, Braswell and Skylake based machines where this is true.
>> Why not configure the GPIO pin as input in the API itself ?
> IIRC Octavian tried to do just that but it turned out that some of the
> irqchip functions are called with spinlock held, which means you can't
> call all gpiod_* functions there (as they might sleep).
Got it. Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer