[PATCH] gpio: mpc8xxx: Correct irq handler function

[PATCH] gpio: mpc8xxx: Correct irq handler function

[Liu Gang]

>From the beginning of the gpio-mpc8xxx.c, the "handle_level_irq"
has being used to handle GPIO interrupts in the PowerPC/Layerscape
platforms. But actually, almost all PowerPC/Layerscape platforms
assert an interrupt request upon either a high-to-low change or
any change on the state of the signal.

So the "handle_level_irq" is not reasonable for PowerPC/Layerscape
GPIO interrupt, it should be "handle_edge_irq". Otherwise the system
may lost some interrupts from the PIN's state changes.

Signed-off-by: Liu Gang <Gang.Liu@nxp.com>
---
 drivers/gpio/gpio-mpc8xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 425501c..793518a 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -239,7 +239,7 @@ static int mpc8xxx_gpio_irq_map(struct irq_domain *h, unsigned int irq,
 				irq_hw_number_t hwirq)
 {
 	irq_set_chip_data(irq, h->host_data);
-	irq_set_chip_and_handler(irq, &mpc8xxx_irq_chip, handle_level_irq);
+	irq_set_chip_and_handler(irq, &mpc8xxx_irq_chip, handle_edge_irq);
 
 	return 0;
 }
-- 
2.1.0.27.g96db324

[PATCH] gpio: mpc8xxx: Correct irq handler function

[Liu Gang]

>From the beginning of the gpio-mpc8xxx.c, the "handle_level_irq"
has being used to handle GPIO interrupts in the PowerPC/Layerscape
platforms. But actually, almost all PowerPC/Layerscape platforms
assert an interrupt request upon either a high-to-low change or
any change on the state of the signal.

So the "handle_level_irq" is not reasonable for PowerPC/Layerscape
GPIO interrupt, it should be "handle_edge_irq". Otherwise the system
may lost some interrupts from the PIN's state changes.

Signed-off-by: Liu Gang <Gang.Liu@nxp.com>
---
 drivers/gpio/gpio-mpc8xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 425501c..793518a 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -239,7 +239,7 @@ static int mpc8xxx_gpio_irq_map(struct irq_domain *h, unsigned int irq,
 				irq_hw_number_t hwirq)
 {
 	irq_set_chip_data(irq, h->host_data);
-	irq_set_chip_and_handler(irq, &mpc8xxx_irq_chip, handle_level_irq);
+	irq_set_chip_and_handler(irq, &mpc8xxx_irq_chip, handle_edge_irq);
 
 	return 0;
 }
-- 
2.1.0.27.g96db324

Re: [PATCH] gpio: mpc8xxx: Correct irq handler function

[Linus Walleij]

On Fri, Oct 21, 2016 at 9:31 AM, Liu Gang <Gang.Liu@nxp.com> wrote:

> From the beginning of the gpio-mpc8xxx.c, the "handle_level_irq"
> has being used to handle GPIO interrupts in the PowerPC/Layerscape
> platforms. But actually, almost all PowerPC/Layerscape platforms
> assert an interrupt request upon either a high-to-low change or
> any change on the state of the signal.
>
> So the "handle_level_irq" is not reasonable for PowerPC/Layerscape
> GPIO interrupt, it should be "handle_edge_irq". Otherwise the system
> may lost some interrupts from the PIN's state changes.
>
> Signed-off-by: Liu Gang <Gang.Liu@nxp.com>

Yes and especially so since this irqchip implements .irq_ack() and
unless you use the edge handler it will never be called.

Patch applied for fixes, tell me if it also needs to go into stable.

If you want to look closer at this driver, I think it is possible to
simplify it quite a bit by using GPIOLIB_IRQCHIP like a few
other drivers, for example gpio-pl061.c. Patches accepted :)

Yours,
Linus Walleij

RE: [PATCH] gpio: mpc8xxx: Correct irq handler function

[Gang Liu]

 
> On Fri, Oct 21, 2016 at 9:31 AM, Liu Gang <Gang.Liu@nxp.com> wrote:
> 
> > From the beginning of the gpio-mpc8xxx.c, the "handle_level_irq"
> > has being used to handle GPIO interrupts in the PowerPC/Layerscape
> > platforms. But actually, almost all PowerPC/Layerscape platforms
> > assert an interrupt request upon either a high-to-low change or any
> > change on the state of the signal.
> >
> > So the "handle_level_irq" is not reasonable for PowerPC/Layerscape
> > GPIO interrupt, it should be "handle_edge_irq". Otherwise the system
> > may lost some interrupts from the PIN's state changes.
> >
> > Signed-off-by: Liu Gang <Gang.Liu@nxp.com>
> 
> Yes and especially so since this irqchip implements .irq_ack() and unless you
> use the edge handler it will never be called.
> 
> Patch applied for fixes, tell me if it also needs to go into stable.
> 
[Liu Gang] yes, please also put it into stable. Thanks!

> If you want to look closer at this driver, I think it is possible to simplify it quite a
> bit by using GPIOLIB_IRQCHIP like a few other drivers, for example gpio-pl061.c.
> Patches accepted :)
> 
> Yours,
> Linus Walleij

Re: [PATCH] gpio: mpc8xxx: Correct irq handler function

[Linus Walleij]

On Mon, Oct 24, 2016 at 5:46 AM, Gang Liu <gang.liu@nxp.com> wrote:
>> On Fri, Oct 21, 2016 at 9:31 AM, Liu Gang <Gang.Liu@nxp.com> wrote:
>>
>> > From the beginning of the gpio-mpc8xxx.c, the "handle_level_irq"
>> > has being used to handle GPIO interrupts in the PowerPC/Layerscape
>> > platforms. But actually, almost all PowerPC/Layerscape platforms
>> > assert an interrupt request upon either a high-to-low change or any
>> > change on the state of the signal.
>> >
>> > So the "handle_level_irq" is not reasonable for PowerPC/Layerscape
>> > GPIO interrupt, it should be "handle_edge_irq". Otherwise the system
>> > may lost some interrupts from the PIN's state changes.
>> >
>> > Signed-off-by: Liu Gang <Gang.Liu@nxp.com>
>>
>> Yes and especially so since this irqchip implements .irq_ack() and unless you
>> use the edge handler it will never be called.
>>
>> Patch applied for fixes, tell me if it also needs to go into stable.
>>
> [Liu Gang] yes, please also put it into stable. Thanks!

Ooops too late. Will tell Greg to pick this separately.

Yours,
Linus Walleij