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

[PATCH v4 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 v3:
- A new fied irq_type in acpi_gpio_info is added and the way the irq_type is attached to the irq is improved.
- The irq described in the acpi spi slave node is now set 

Best Regards
Christophe 

Christophe Ricard (3):
  acpi: Rename acpi_gsi_get_irq_type to acpi_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/utils.c        | 21 +++++++++++++++++++++
 drivers/gpio/gpiolib-acpi.c | 22 ++++++++++++++++++----
 drivers/gpio/gpiolib.h      |  1 +
 drivers/spi/spi.c           |  2 +-
 include/linux/acpi.h        |  2 ++
 6 files changed, 44 insertions(+), 25 deletions(-)

-- 
2.1.4

[PATCH v4 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_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 utility function.

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

diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
index fa4585a..2828351 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_get_irq_type(trigger, polarity);
 	fwspec.param_count = 2;
 
 	return irq_create_fwspec_mapping(&fwspec);
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 475c907..715b24b 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -33,6 +33,27 @@
 #define _COMPONENT		ACPI_BUS_COMPONENT
 ACPI_MODULE_NAME("utils");
 
+unsigned int acpi_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;
+	}
+}
+EXPORT_SYMBOL_GPL(acpi_get_irq_type);
+
+
 /* --------------------------------------------------------------------------
                             Object Evaluation Helpers
    -------------------------------------------------------------------------- */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d863e12..b0c5a11 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -204,6 +204,8 @@ int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);
 void acpi_set_irq_model(enum acpi_irq_model_id model,
 			struct fwnode_handle *fwnode);
 
+unsigned int acpi_get_irq_type(int trigger, int polarity);
+
 #ifdef CONFIG_X86_IO_APIC
 extern int acpi_get_override_irq(u32 gsi, int *trigger, int *polarity);
 #else
-- 
2.1.4

Re: [PATCH v4 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_get_irq_type and export symbol

[Mika Westerberg]

On Mon, Dec 07, 2015 at 11:39:10PM +0100, Christophe Ricard wrote:
> acpi_gsi_get_irq_type could be use out of gsi purpose.
> 
> Rename and make it available as a utility function.
> 
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>

There is already a function acpi_dev_irq_flags() converts ACPI flags to
Linux resource IRQ flags. Any reason you are not using that?

Re: [PATCH v4 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_get_irq_type and export symbol

[Christophe Ricard]

Hi Mika,

gpio irq type are usually denoted using IRQ_TYPE_xxx.
Lukily acpi_dev_irq_flags would work for  because IORESOURCE_IRQ and 
IRQ_TYPE numbers match.
In include/linux/ioport.h, IORESOURCE_IRQ looks to be more related with 
PnP stuff:
"PnP IRQ specific bits (IORESOURCE_BITS)"

I believe changes done in this patch take benefit of another existing 
function used in acpi gsi context that is doing the
job as expected.

Do you still believe acpi_dev_irq_flags is more appropriate ?

Best Regards
Christophe

On 08/12/2015 12:28, Mika Westerberg wrote:
> On Mon, Dec 07, 2015 at 11:39:10PM +0100, Christophe Ricard wrote:
>> acpi_gsi_get_irq_type could be use out of gsi purpose.
>>
>> Rename and make it available as a utility function.
>>
>> Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
> There is already a function acpi_dev_irq_flags() converts ACPI flags to
> Linux resource IRQ flags. Any reason you are not using that?

--
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 v4 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_get_irq_type and export symbol

[Mika Westerberg]

On Tue, Dec 08, 2015 at 10:57:19PM +0100, Christophe Ricard wrote:
> I believe changes done in this patch take benefit of another existing
> function used in acpi gsi context that is doing the
> job as expected.
> 
> Do you still believe acpi_dev_irq_flags is more appropriate ?

I don't have strong feelings about that. I just wanted to point out
there already exists such function.

However, if you do use acpi_gsi_get_irq_type() I think it should be
placed to the same file as acpi_dev_irq_flags().

Re: [PATCH v4 1/3] acpi: Rename acpi_gsi_get_irq_type to acpi_get_irq_type and export symbol

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 3:36 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Dec 08, 2015 at 10:57:19PM +0100, Christophe Ricard wrote:
>> I believe changes done in this patch take benefit of another existing
>> function used in acpi gsi context that is doing the
>> job as expected.
>>
>> Do you still believe acpi_dev_irq_flags is more appropriate ?
>
> I don't have strong feelings about that. I just wanted to point out
> there already exists such function.
>
> However, if you do use acpi_gsi_get_irq_type() I think it should be
> placed to the same file as acpi_dev_irq_flags().

And perhaps renamed accordingly

acpi_dev_get_irq_type() I suppose.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 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 it available in acpi_find_gpio with a new acpi_gpio_info field
(irq_type) and setthe 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 | 22 ++++++++++++++++++----
 drivers/gpio/gpiolib.h      |  1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index bbcac3a..4b893c8 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -416,9 +416,12 @@ 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)
+		if (lookup->info.gpioint) {
 			lookup->info.active_low =
 				agpio->polarity == ACPI_ACTIVE_LOW;
+			lookup->info.irq_type = acpi_get_irq_type(agpio->triggering,
+								  agpio->polarity);
+		}
 	}
 
 	return 1;
@@ -529,7 +532,7 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
  */
 int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 {
-	int idx, i;
+	int idx, i, irq;
 
 	for (i = 0, idx = 0; idx <= index; i++) {
 		struct acpi_gpio_info info;
@@ -538,8 +541,19 @@ 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) {
+			irq = gpiod_to_irq(desc);
+			if (irq < 0) {
+				dev_err(&adev->dev,
+					"Failed to translate GPIO to IRQ\n");
+				return irq;
+			}
+			/* Set type if specified and different than the current one */
+			if (info.irq_type != IRQ_TYPE_NONE &&
+			    info.irq_type != irq_get_trigger_type(irq))
+				irq_set_irq_type(irq, info.irq_type);
+			return irq;
+		}
 	}
 	return -ENOENT;
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 78e634d..624fbc4 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -27,6 +27,7 @@ struct acpi_device;
 struct acpi_gpio_info {
 	bool gpioint;
 	bool active_low;
+	int irq_type;
 };
 
 /* 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 v4 2/3] ACPI / gpio: Add irq_type when a gpio is used as an interrupt

[Mika Westerberg]

On Mon, Dec 07, 2015 at 11:39:11PM +0100, Christophe Ricard wrote:
> When a gpio is used as an interrupt in acpi, the irq_type was not
> available for device driver.
> 
> Make it available in acpi_find_gpio with a new acpi_gpio_info field
> (irq_type) and setthe irq_type if necessary in acpi_dev_gpio_irq_get.
> 
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 22 ++++++++++++++++++----
>  drivers/gpio/gpiolib.h      |  1 +
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index bbcac3a..4b893c8 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -416,9 +416,12 @@ 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)
> +		if (lookup->info.gpioint) {
>  			lookup->info.active_low =
>  				agpio->polarity == ACPI_ACTIVE_LOW;

Since we have irq_type (I prefer irq_flags) we can get rid of active_low
above. That information is already in the irq_type field.

> +			lookup->info.irq_type = acpi_get_irq_type(agpio->triggering,
> +								  agpio->polarity);
> +		}
>  	}
>  
>  	return 1;
> @@ -529,7 +532,7 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
>   */
>  int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
>  {
> -	int idx, i;
> +	int idx, i, irq;
>  
>  	for (i = 0, idx = 0; idx <= index; i++) {
>  		struct acpi_gpio_info info;
> @@ -538,8 +541,19 @@ 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) {

Why not declare irq here?

			int irq = gpiod_to_irq(desc);

> +			irq = gpiod_to_irq(desc);
> +			if (irq < 0) {
> +				dev_err(&adev->dev,
> +					"Failed to translate GPIO to IRQ\n");

Is it really necessary to print this error?

> +				return irq;
> +			}
> +			/* Set type if specified and different than the current one */
> +			if (info.irq_type != IRQ_TYPE_NONE &&
> +			    info.irq_type != irq_get_trigger_type(irq))
> +				irq_set_irq_type(irq, info.irq_type);
> +			return irq;
> +		}
>  	}
>  	return -ENOENT;
>  }
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 78e634d..624fbc4 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -27,6 +27,7 @@ struct acpi_device;
>  struct acpi_gpio_info {
>  	bool gpioint;
>  	bool active_low;
> +	int irq_type;

Use unsigned long here.

>  };
>  
>  /* gpio suffixes used for ACPI and device tree lookup */
> -- 
> 2.1.4

[PATCH v4 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-qxv4g6HH51o@public.gmane.org>
---
 drivers/spi/spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a5f53de..5c10815 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1534,7 +1534,7 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 	}
 
 	ACPI_COMPANION_SET(&spi->dev, adev);
-	spi->irq = -1;
+	spi->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&spi->dev), 0);
 
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list,
-- 
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 v4 3/3] ACPI / spi: attach gpio irq from acpi description to spi device

[Mika Westerberg]

On Mon, Dec 07, 2015 at 11:39:12PM +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, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index a5f53de..5c10815 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1534,7 +1534,7 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
>  	}
>  
>  	ACPI_COMPANION_SET(&spi->dev, adev);
> -	spi->irq = -1;
> +	spi->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&spi->dev), 0);

You can use "adev" here instead of ACPI_COMPANION(&spi->dev).

>  
>  	INIT_LIST_HEAD(&resource_list);
>  	ret = acpi_dev_get_resources(adev, &resource_list,
> -- 
> 2.1.4

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

[Mika Westerberg]

On Mon, Dec 07, 2015 at 11:39:12PM +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, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index a5f53de..5c10815 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1534,7 +1534,7 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
>  	}
>  
>  	ACPI_COMPANION_SET(&spi->dev, adev);
> -	spi->irq = -1;
> +	spi->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&spi->dev), 0);

Also I think we should first check for the normal Interrupt() resource
(like what we are already doing in acpi_spi_add_resource()) and only
then resort back to use GPIO

>  
>  	INIT_LIST_HEAD(&resource_list);
>  	ret = acpi_dev_get_resources(adev, &resource_list,
> -- 
> 2.1.4