[PATCH v7 0/3] ACPI: Add irq_type to gpio interrupt

[PATCH v7 0/3] ACPI: Add irq_type to gpio interrupt

[Christophe Ricard]

Hi,

ACPI probing method does not retrieve irq_type from a gpio interrupt declared
with GpioInt as it is done with devicetree probing. In other terms, irq_get_trigger_type
will always send back 0.

Those 3 patches propose a way to retrieve the correct interrupt polarity/type from a GpioInt
acpi declaration when using irq_get_trigger_type.

Since v4 after Mika and Andy's feedbacks:
- acpi_gpio_info active_low field got renamed polarity. Note that polarity/active_low is having sense
also when a gpio is not mapped as an interrupt but as an output (For example: check gpiod_set_value
in drivers/gpio/gpiolib.c).
- acpi_gsi_get_irq_type is now renamed acpi_dev_get_irq_type and moved to drivers/acpi/resource.c
- The irq described in the acpi spi slave node is now set once we fall back to retrieve normal interrupt.

Since v5 after Andy's feedbacks:
- Make sure we are not using u* type but pure C type for non hardware value.

Since v6:
- Patches now applies on top of Raphael tree tag linux-next

Best Regards
Christophe 

Christophe Ricard (3):
  acpi: Rename acpi_gsi_get_irq_type to acpi_dev_get_irq_type and export
    symbol
  ACPI / gpio: Add irq_type when a gpio is used as an interrupt
  ACPI / spi: attach gpio irq from acpi description to spi device

 drivers/acpi/gsi.c          | 21 +--------------------
 drivers/acpi/resource.c     | 26 ++++++++++++++++++++++++++
 drivers/gpio/gpiolib-acpi.c | 30 ++++++++++++++++++++++++------
 drivers/gpio/gpiolib.c      |  4 ++--
 drivers/gpio/gpiolib.h      |  3 ++-
 drivers/spi/spi.c           |  2 ++
 include/linux/acpi.h        |  1 +
 7 files changed, 58 insertions(+), 29 deletions(-)

-- 
2.1.4

[PATCH v7 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_dev_get_irq_type and export symbol

[Christophe Ricard]

acpi_gsi_get_irq_type could be use out of gsi purpose.

Rename and make it available as a resource function.

Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
---
 drivers/acpi/gsi.c      | 21 +--------------------
 drivers/acpi/resource.c | 26 ++++++++++++++++++++++++++
 include/linux/acpi.h    |  1 +
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
index fa4585a..ee9e0f2 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/gsi.c
@@ -17,25 +17,6 @@ enum acpi_irq_model_id acpi_irq_model;
 
 static struct fwnode_handle *acpi_gsi_domain_id;
 
-static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity)
-{
-	switch (polarity) {
-	case ACPI_ACTIVE_LOW:
-		return trigger == ACPI_EDGE_SENSITIVE ?
-		       IRQ_TYPE_EDGE_FALLING :
-		       IRQ_TYPE_LEVEL_LOW;
-	case ACPI_ACTIVE_HIGH:
-		return trigger == ACPI_EDGE_SENSITIVE ?
-		       IRQ_TYPE_EDGE_RISING :
-		       IRQ_TYPE_LEVEL_HIGH;
-	case ACPI_ACTIVE_BOTH:
-		if (trigger == ACPI_EDGE_SENSITIVE)
-			return IRQ_TYPE_EDGE_BOTH;
-	default:
-		return IRQ_TYPE_NONE;
-	}
-}
-
 /**
  * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
  * @gsi: GSI IRQ number to map
@@ -82,7 +63,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
 
 	fwspec.fwnode = acpi_gsi_domain_id;
 	fwspec.param[0] = gsi;
-	fwspec.param[1] = acpi_gsi_get_irq_type(trigger, polarity);
+	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
 	fwspec.param_count = 2;
 
 	return irq_create_fwspec_mapping(&fwspec);
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index cdc5c25..d02fd53 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -23,6 +23,7 @@
 #include <linux/export.h>
 #include <linux/ioport.h>
 #include <linux/slab.h>
+#include <linux/irq.h>
 
 #ifdef CONFIG_X86
 #define valid_IRQ(i) (((i) != 0) && ((i) != 2))
@@ -336,6 +337,31 @@ unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_irq_flags);
 
+/**
+ * acpi_dev_get_irq_type - Determine irq type.
+ * @triggering: Triggering type as provided by ACPI.
+ * @polarity: Interrupt polarity as provided by ACPI.
+ */
+unsigned int acpi_dev_get_irq_type(int triggering, int polarity)
+{
+	switch (polarity) {
+	case ACPI_ACTIVE_LOW:
+		return triggering == ACPI_EDGE_SENSITIVE ?
+		       IRQ_TYPE_EDGE_FALLING :
+		       IRQ_TYPE_LEVEL_LOW;
+	case ACPI_ACTIVE_HIGH:
+		return triggering == ACPI_EDGE_SENSITIVE ?
+		       IRQ_TYPE_EDGE_RISING :
+		       IRQ_TYPE_LEVEL_HIGH;
+	case ACPI_ACTIVE_BOTH:
+		if (triggering == ACPI_EDGE_SENSITIVE)
+			return IRQ_TYPE_EDGE_BOTH;
+	default:
+		return IRQ_TYPE_NONE;
+	}
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type);
+
 static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
 {
 	res->start = gsi;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1991aea..42adb80 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -318,6 +318,7 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
 					 struct resource_win *win);
 unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);
+unsigned int acpi_dev_get_irq_type(int triggering, int polarity);
 bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 				 struct resource *res);
 
-- 
2.1.4

Re: [PATCH v7 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_dev_get_irq_type and export symbol

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:11:31AM +0100, Christophe Ricard wrote:
> acpi_gsi_get_irq_type could be use out of gsi purpose.
> 
> Rename and make it available as a resource function.
> 
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v7 2/3] ACPI / gpio: Add irq_type when a gpio is used as an interrupt

[Christophe Ricard]

When a gpio is used as an interrupt in acpi, the irq_type was not
available for device driver.

Make available polarity and triggering information in acpi_find_gpio by
renaming acpi_gpio_info field active_low  to polarity and adding triggering
field (edge/level).
For sanity, in gpiolib.c replace info.active_low by
"info.polarity == GPIO_ACTIVE_LOW".

Set the irq_type if necessary in acpi_dev_gpio_irq_get.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/gpio/gpiolib-acpi.c | 30 ++++++++++++++++++++++++------
 drivers/gpio/gpiolib.c      |  4 ++--
 drivers/gpio/gpiolib.h      |  3 ++-
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 16a7b68..9f83fc6 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -418,9 +418,11 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 		 * GpioIo is used then the only way to set the flag is
 		 * to use _DSD "gpios" property.
 		 */
-		if (lookup->info.gpioint)
-			lookup->info.active_low =
-				agpio->polarity == ACPI_ACTIVE_LOW;
+		if (lookup->info.gpioint) {
+			lookup->info.polarity = agpio->polarity;
+			lookup->info.triggering = agpio->triggering;
+		}
+
 	}
 
 	return 1;
@@ -447,7 +449,7 @@ static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup,
 	if (info) {
 		*info = lookup->info;
 		if (lookup->active_low)
-			info->active_low = lookup->active_low;
+			info->polarity = lookup->active_low;
 	}
 	return 0;
 }
@@ -595,6 +597,7 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
 int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 {
 	int idx, i;
+	unsigned int irq_flags;
 
 	for (i = 0, idx = 0; idx <= index; i++) {
 		struct acpi_gpio_info info;
@@ -603,8 +606,23 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 		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);
+		if (info.gpioint && idx++ == index) {
+			int irq = gpiod_to_irq(desc);
+
+			if (irq < 0)
+				return irq;
+
+			irq_flags = acpi_dev_get_irq_type(info.triggering,
+							  info.polarity);
+
+			/* Set type if specified and different than the current one */
+			if (irq_flags != IRQ_TYPE_NONE &&
+			    irq_flags != irq_get_trigger_type(irq))
+				irq_set_irq_type(irq, irq_flags);
+
+			return irq;
+		}
+
 	}
 	return -ENOENT;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2a91f32..5b67177 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1873,7 +1873,7 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 			return desc;
 	}
 
-	if (info.active_low)
+	if (info.polarity == GPIO_ACTIVE_LOW)
 		*flags |= GPIO_ACTIVE_LOW;
 
 	return desc;
@@ -2211,7 +2211,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 		desc = acpi_node_get_gpiod(fwnode, propname, 0, &info);
 		if (!IS_ERR(desc))
-			active_low = info.active_low;
+			active_low = info.polarity == GPIO_ACTIVE_LOW;
 	}
 
 	if (IS_ERR(desc))
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 98ab08c..5ac3b88 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -26,7 +26,8 @@ struct acpi_device;
  */
 struct acpi_gpio_info {
 	bool gpioint;
-	bool active_low;
+	int polarity;
+	int triggering;
 };
 
 /* gpio suffixes used for ACPI and device tree lookup */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/3] ACPI / gpio: Add irq_type when a gpio is used as an interrupt

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:11:32AM +0100, Christophe Ricard wrote:
> When a gpio is used as an interrupt in acpi, the irq_type was not
> available for device driver.
> 
> Make available polarity and triggering information in acpi_find_gpio by
> renaming acpi_gpio_info field active_low  to polarity and adding triggering
> field (edge/level).
> For sanity, in gpiolib.c replace info.active_low by
> "info.polarity == GPIO_ACTIVE_LOW".
> 
> Set the irq_type if necessary in acpi_dev_gpio_irq_get.
> 
> Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/gpio/gpiolib-acpi.c | 30 ++++++++++++++++++++++++------
>  drivers/gpio/gpiolib.c      |  4 ++--
>  drivers/gpio/gpiolib.h      |  3 ++-
>  3 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 16a7b68..9f83fc6 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -418,9 +418,11 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
>  		 * GpioIo is used then the only way to set the flag is
>  		 * to use _DSD "gpios" property.
>  		 */
> -		if (lookup->info.gpioint)
> -			lookup->info.active_low =
> -				agpio->polarity == ACPI_ACTIVE_LOW;
> +		if (lookup->info.gpioint) {
> +			lookup->info.polarity = agpio->polarity;

Since you are passing ACPI_ACTIVE_LOW/HIGH/BOTH here and then later on
compare that to GPIO_ACTIVE_LOW you should either put comment here
explaining that we expect the two to be equal, or alternatively convert
ACPI flags here to the corresponding Linux ones.

> +			lookup->info.triggering = agpio->triggering;
> +		}
> +
>  	}
>  
>  	return 1;
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 3/3] ACPI / spi: attach gpio irq from acpi description to spi device

[Christophe Ricard]

spi->irq was ignoring GpioInt property setting it to -1.
acpi_dev_gpio_irq_get returns and configure the slave irq according to
the acpi slave node description.
It is now inline with devicetree behavior.

Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
---
 drivers/spi/spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2b0a8ec..6e5d1f7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1583,6 +1583,8 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 
 		if (acpi_dev_resource_interrupt(ares, 0, &r))
 			spi->irq = r.start;
+		else
+			spi->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&spi->dev), 0);
 	}
 
 	/* Always tell the ACPI core to skip this resource */
-- 
2.1.4

Re: [PATCH v7 3/3] ACPI / spi: attach gpio irq from acpi description to spi device

[Mark Brown]

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

On Tue, Dec 15, 2015 at 08:11:33AM +0100, Christophe Ricard wrote:
> spi->irq was ignoring GpioInt property setting it to -1.
> acpi_dev_gpio_irq_get returns and configure the slave irq according to
> the acpi slave node description.
> It is now inline with devicetree behavior.

Acked-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

though it seems like we might want to have a common "get the interrupt
for this ACPI device" function here.  OTOH there's only a few users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v7 3/3] ACPI / spi: attach gpio irq from acpi description to spi device

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:11:33AM +0100, Christophe Ricard wrote:
> spi->irq was ignoring GpioInt property setting it to -1.
> acpi_dev_gpio_irq_get returns and configure the slave irq according to
> the acpi slave node description.
> It is now inline with devicetree behavior.
> 
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
> ---
>  drivers/spi/spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 2b0a8ec..6e5d1f7 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1583,6 +1583,8 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
>  
>  		if (acpi_dev_resource_interrupt(ares, 0, &r))
>  			spi->irq = r.start;
> +		else
> +			spi->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&spi->dev), 0);

This still does not work as expected if the device has other resources
in _CRS.

Instead of this, you should do something like below which only uses GPIO
when we are sure no Interrupt() resource was found.

acpi_spi_add_device()
{
	...
	acpi_dev_get_resources()
	...

	if (spi->irq < 0)
		spi->irq = acpi_dev_gpio_irq_get(adev, 0);