[PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Christophe Ricard]

Hi,

This serie only move from RFC to final. I am also adding gpio folks for potential
feedback.

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 2 patches propose a way to retrieve the correct interrupt polarity/type from a GpioInt
acpi declaration when using irq_get_trigger_type.

Best Regards
Christophe 

Christophe Ricard (2):
  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

 drivers/acpi/gsi.c          | 21 +--------------------
 drivers/acpi/utils.c        | 21 +++++++++++++++++++++
 drivers/gpio/gpiolib-acpi.c |  5 ++++-
 include/linux/acpi.h        |  2 ++
 4 files changed, 28 insertions(+), 21 deletions(-)

-- 
2.1.4

[PATCH v3 1/2] 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

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

[Christophe Ricard]

When a gpio is used as an interrupt, the irq_type was not available for
device driver. It is not align with devicetree probing.

Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
---
 drivers/gpio/gpiolib-acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index bbcac3a..cff8736 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;
+			irq_set_irq_type(gpiod_to_irq(lookup->desc),
+					 acpi_get_irq_type(agpio->triggering, agpio->polarity));
+		}
 	}
 
 	return 1;
-- 
2.1.4

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Wolfram Sang]

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


> This serie only move from RFC to final. I am also adding gpio folks for potential
> feedback.

Why is this relevant for I2C?


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

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Christophe Ricard]

Hi Wolfram,

This is correct that this serie does not impact i2c core at all.

Best Regards
Christophe

On 01/12/2015 02:03, Wolfram Sang wrote:
>> This serie only move from RFC to final. I am also adding gpio folks for potential
>> feedback.
> Why is this relevant for I2C?
>

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Mon, Nov 30, 2015 at 11:47:51PM +0100, Christophe Ricard wrote:
> 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.

Is this real problem you are solving here?

Also where does the device tree version set triggering flags for a GPIO
irq?

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Tue, Dec 01, 2015 at 01:25:50PM +0100, Christophe Ricard wrote:
>    For example during an i2c_device_probe where an i2c slave device
>    describe in devicetree has an interrupts property.
>    i2c_device_probe (drivers/i2c/i2c-core.c), retrieves irq property from
>    of_irq_get which will looks for an "interrupts" property in
>    of_irq_parse_of (drivers/of/irq.c).
>    of_irq_get will then call irq_create_mapping (kernel/irq/irqdomain.c)
>    which will set the irq_type retrieved during the interrupts node
>    parsing.

Found it now thanks.

>    This will allow from an i2c slave drivers to configure an interrupt
>    handler matching the exact devicetree data for the interrupts property
>    of the i2c slave node.

Makes sense.

>    Now for the same kind of i2c driver using acpi description, the GpioInt
>    polarity/type is at the moment never kept in the irq property.
>    It is possible to check that following about the same path...
>    i2c_device_probe (drivers/i2c/i2c-core.c), retrieves irq property from
>    acpi_dev_gpio_irq_get but does not save the irq_type.
>    This would allow not to have to use an additional gpio field and all
>    the configuration step to configure the gpio interrupt correctly in a
>    device driver and taking a real benefit of the GpioInt acpi keyword
>    compare to GpioIo keyword.
>    Most the of the drivers based on acpi description retrieve gpio number
>    to assign an interrupt and a fix polarity. I believe my patchset
>    proposal would improve this and allow to
>    be much closer with devicetree.
>    Do you see any issue with this ?

No, but I wonder if it would be better to do this in acpi_dev_gpio_irq_get()
instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Christophe Ricard]

Hi Mika,


On 01/12/2015 14:03, Mika Westerberg wrote:
[...]
>>     Now for the same kind of i2c driver using acpi description, the GpioInt
>>     polarity/type is at the moment never kept in the irq property.
>>     It is possible to check that following about the same path...
>>     i2c_device_probe (drivers/i2c/i2c-core.c), retrieves irq property from
>>     acpi_dev_gpio_irq_get but does not save the irq_type.
>>     This would allow not to have to use an additional gpio field and all
>>     the configuration step to configure the gpio interrupt correctly in a
>>     device driver and taking a real benefit of the GpioInt acpi keyword
>>     compare to GpioIo keyword.
>>     Most the of the drivers based on acpi description retrieve gpio number
>>     to assign an interrupt and a fix polarity. I believe my patchset
>>     proposal would improve this and allow to
>>     be much closer with devicetree.
>>     Do you see any issue with this ?
> No, but I wonder if it would be better to do this in acpi_dev_gpio_irq_get()
> instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?
I believe, setting the irq type requires triggering and polarity data 
stored into a struct acpi_resource_gpio.

acpi_dev_gpio_irq_get call acpi_get_gpiod_by_index which run acpi_find_gpio.

Actually acpi_dev_gpio_irq_get is called everytime an i2c device slave 
is probed, acpi_find_gpio will get called to "register" gpios.
If done correctly, i think this will be done only once...

The only improvement i may see would be to add an extra field in the 
acpi_gpio_info structure in drivers/gpio/gpiolib.h (for example int 
irq_type).
And call irq_set_irq_type in acpi_dev_gpio_irq_get if the gpio is an 
interrupt.

I guess the current proposal and this one are equivalent...

What do you think ?

Best Regards
Christophe

Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Mika Westerberg]

On Sat, Dec 05, 2015 at 12:36:19AM +0100, Christophe Ricard wrote:
> >No, but I wonder if it would be better to do this in acpi_dev_gpio_irq_get()
> >instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?
> I believe, setting the irq type requires triggering and polarity data stored
> into a struct acpi_resource_gpio.
> 
> acpi_dev_gpio_irq_get call acpi_get_gpiod_by_index which run acpi_find_gpio.
> 
> Actually acpi_dev_gpio_irq_get is called everytime an i2c device slave is
> probed, acpi_find_gpio will get called to "register" gpios.
> If done correctly, i think this will be done only once...
> 
> The only improvement i may see would be to add an extra field in the
> acpi_gpio_info structure in drivers/gpio/gpiolib.h (for example int
> irq_type).
> And call irq_set_irq_type in acpi_dev_gpio_irq_get if the gpio is an
> interrupt.
> 
> I guess the current proposal and this one are equivalent...
> 
> What do you think ?

Actually I think it may be useful to add triggering flags to
acpi_gpio_info.

RE: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

[Christophe Henri RICARD]

Hi Mika,

Ok, i will send a RFC serie tonight. I have also noticed the spi core set the irq field to -1 in acpi. This will be fixed in this serie.

Best Regards
Christophe

-----Original Message-----
From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
Sent: lundi 7 décembre 2015 11:52
To: Christophe Ricard
Cc: rjw@rjwysocki.net; Len Brown; Linus Walleij; Alexandre Courbot; andriy.shevchenko@linux.intel.com; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Christophe Henri RICARD
Subject: Re: [PATCH v3 0/2] ACPI: Add irq_type to gpio interrupt

On Sat, Dec 05, 2015 at 12:36:19AM +0100, Christophe Ricard wrote:
> >No, but I wonder if it would be better to do this in 
> >acpi_dev_gpio_irq_get() instead of acpi_find_gpio() which gets called everytime a GPIO is looked up?
> I believe, setting the irq type requires triggering and polarity data 
> stored into a struct acpi_resource_gpio.
> 
> acpi_dev_gpio_irq_get call acpi_get_gpiod_by_index which run acpi_find_gpio.
> 
> Actually acpi_dev_gpio_irq_get is called everytime an i2c device slave 
> is probed, acpi_find_gpio will get called to "register" gpios.
> If done correctly, i think this will be done only once...
> 
> The only improvement i may see would be to add an extra field in the 
> acpi_gpio_info structure in drivers/gpio/gpiolib.h (for example int 
> irq_type).
> And call irq_set_irq_type in acpi_dev_gpio_irq_get if the gpio is an 
> interrupt.
> 
> I guess the current proposal and this one are equivalent...
> 
> What do you think ?

Actually I think it may be useful to add triggering flags to acpi_gpio_info.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

On Mon, Nov 30, 2015 at 11:47 PM, Christophe Ricard
<christophe.ricard@gmail.com> wrote:

> When a gpio is used as an interrupt, the irq_type was not available for
> device driver. It is not align with devicetree probing.
>
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Rafael you can merge this into the ACPI tree.

Yours,
Linus Walleij