[patch 0/5] gpio: langwell: Cleanup the interrupt mess

[patch 0/5] gpio: langwell: Cleanup the interrupt mess

[Thomas Gleixner]

The langwell interrupt handler is interesting and breaks when an
architecture enables GENERIC_HARDIRQS_NO_DEPRECATED.

Also this driver along with the other intel ones should depend on x86.

The series cleans that up. The last patch 5/5 is RFC and needs a
close look and testing by folks at Intel.

Thanks,

	tglx

[patch 1/5] gpio; Make Intel chipset gpio drivers depend on x86

[Thomas Gleixner]

[-- Attachment #1: gpio-langwell-kconfig.patch --]
[-- Type: text/plain, Size: 1084 bytes --]

Nothing outside of x86 can use that code.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/gpio/Kconfig |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6-tip/drivers/gpio/Kconfig
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/Kconfig
+++ linux-2.6-tip/drivers/gpio/Kconfig
@@ -101,7 +101,7 @@ config GPIO_VR41XX
 
 config GPIO_SCH
 	tristate "Intel SCH GPIO"
-	depends on GPIOLIB && PCI
+	depends on GPIOLIB && PCI && X86
 	select MFD_CORE
 	select LPC_SCH
 	help
@@ -321,13 +321,13 @@ config GPIO_BT8XX
 
 config GPIO_LANGWELL
 	bool "Intel Langwell/Penwell GPIO support"
-	depends on PCI
+	depends on PCI && X86
 	help
 	  Say Y here to support Intel Langwell/Penwell GPIO.
 
 config GPIO_PCH
 	tristate "PCH GPIO of Intel Topcliff"
-	depends on PCI
+	depends on PCI && X86
 	help
 	  This driver is for PCH(Platform controller Hub) GPIO of Intel Topcliff
 	  which is an IOH(Input/Output Hub) for x86 embedded processor.

[patch 2/5] gpio-langwell-fix-crap.patch

[Thomas Gleixner]

[-- Attachment #1: gpio-langwell-fix-crap.patch --]
[-- Type: text/plain, Size: 2574 bytes --]

commit 0766d20fd (langwell_gpio: modify EOI handling following change
of kernel irq subsystem)  changes

 -   desc->chip->eoi(irq);
 +
 +   if (desc->chip->irq_eoi)
 +           desc->chip->irq_eoi(irq_get_irq_data(irq));
 +   else
 +           dev_warn(pg->chip.dev, "missing EOI handler for irq %d\n", irq);

With the following explanation:

 "Latest kernel has many changes in IRQ subsystem and its interfaces,
  like adding irq_eoi" for struct irq_chip, this patch will make it
  support both the new and old interface."

This is completely bogus.

 #1) The changelog does not match the patch at all

 #2) This driver relies on the assumption that it sits behind an eoi
     capable interrupt line. If the implementation of the underlying
     chip changes from eoi to irq_eoi then this driver has to follow
     that change and not add a total bogosity.

 #3) Just mechanically changing eoi to irq_eoi without checking the
     background of that change is sloppy at best.

Remove the sillyness and retrieve the interrupt data from irq_desc
directly. No need to go through a sparse irq lookup.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Alek Du <alek.du@intel.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/gpio/langwell_gpio.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
+++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
@@ -187,10 +187,11 @@ MODULE_DEVICE_TABLE(pci, lnw_gpio_ids);
 
 static void lnw_irq_handler(unsigned irq, struct irq_desc *desc)
 {
-	struct lnw_gpio *lnw = get_irq_data(irq);
-	u32 base, gpio;
+	struct irq_data *data = irq_desc_get_irq_data(desc);
+	struct lnw_gpio *lnw = irq_data_get_irq_handler_data(data);
+	struct irq_chip *chip = irq_data_get_irq_chip(data);
+	u32 base, gpio, gedr_v;
 	void __iomem *gedr;
-	u32 gedr_v;
 
 	/* check GPIO controller to check which pin triggered the interrupt */
 	for (base = 0; base < lnw->chip.ngpio; base += 32) {
@@ -207,11 +208,7 @@ static void lnw_irq_handler(unsigned irq
 		writel(gedr_v, gedr);
 	}
 
-	if (desc->chip->irq_eoi)
-		desc->chip->irq_eoi(irq_get_irq_data(irq));
-	else
-		dev_warn(lnw->chip.dev, "missing EOI handler for irq %d\n", irq);
-
+	chip->irq_eoi(data);
 }
 
 static int __devinit lnw_gpio_probe(struct pci_dev *pdev,

[patch 3/5] gpio: langwell: Convert irq name space

[Thomas Gleixner]

[-- Attachment #1: gpio-langwell-convert-irq-namespace.patch --]
[-- Type: text/plain, Size: 1231 bytes --]

Convert to the new irq function names.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Alek Du <alek.du@intel.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

---
 drivers/gpio/langwell_gpio.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
+++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
@@ -276,12 +276,12 @@ static int __devinit lnw_gpio_probe(stru
 		dev_err(&pdev->dev, "langwell gpiochip_add error %d\n", retval);
 		goto err5;
 	}
-	set_irq_data(pdev->irq, lnw);
-	set_irq_chained_handler(pdev->irq, lnw_irq_handler);
+	irq_set_handler_data(pdev->irq, lnw);
+	irq_set_chained_handler(pdev->irq, lnw_irq_handler);
 	for (i = 0; i < lnw->chip.ngpio; i++) {
-		set_irq_chip_and_handler_name(i + lnw->irq_base, &lnw_irqchip,
-					handle_simple_irq, "demux");
-		set_irq_chip_data(i + lnw->irq_base, lnw);
+		irq_set_chip_and_handler_name(i + lnw->irq_base, &lnw_irqchip,
+					      handle_simple_irq, "demux");
+		irq_set_chip_data(i + lnw->irq_base, lnw);
 	}
 
 	spin_lock_init(&lnw->lock);

[patch 4/5] gpio: langwell: Simplify demux loop

[Thomas Gleixner]

[-- Attachment #1: gpio-langwell-simplify-loop.patch --]
[-- Type: text/plain, Size: 1467 bytes --]

Use __ffs() to find the pending interrupt source instead of looping 32
times.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Alek Du <alek.du@intel.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

---
 drivers/gpio/langwell_gpio.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
+++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
@@ -191,19 +191,20 @@ static void lnw_irq_handler(unsigned irq
 	struct lnw_gpio *lnw = irq_data_get_irq_handler_data(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	u32 base, gpio, gedr_v;
+	unsigned long pending;
 	void __iomem *gedr;
 
 	/* check GPIO controller to check which pin triggered the interrupt */
 	for (base = 0; base < lnw->chip.ngpio; base += 32) {
 		gedr = gpio_reg(&lnw->chip, base, GEDR);
-		gedr_v = readl(gedr);
+		gedr_v = pending = readl(gedr);
 		if (!gedr_v)
 			continue;
-		for (gpio = base; gpio < base + 32; gpio++)
-			if (gedr_v & BIT(gpio % 32)) {
-				pr_debug("pin %d triggered\n", gpio);
-				generic_handle_irq(lnw->irq_base + gpio);
-			}
+		while (pending) {
+			gpio = __ffs(pending) - 1;
+			pending &= ~BIT(gpio);
+			generic_handle_irq(lnw->irq_base + base + gpio);
+		}
 		/* clear the edge detect status bit */
 		writel(gedr_v, gedr);
 	}

[patch 5/5] [RFC] gpio: langwell: Clear edge bit before handling

[Thomas Gleixner]

[-- Attachment #1: gpio-langwell-clear-edge-bit-before-handler.patch --]
[-- Type: text/plain, Size: 1766 bytes --]

I don't have the specs for this beast, but it looks a lot like the PXA
GPIO block. Though I bet it's the same IP and the driver should have
reused the PXA code.

Acknowleding the edge detect status after handling one or more gpio
interrupts looks wrong. We might lose an edge which came in while we
handled the previous one.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Alek Du <alek.du@intel.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

---
 drivers/gpio/langwell_gpio.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
+++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
@@ -190,23 +190,22 @@ static void lnw_irq_handler(unsigned irq
 	struct irq_data *data = irq_desc_get_irq_data(desc);
 	struct lnw_gpio *lnw = irq_data_get_irq_handler_data(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
-	u32 base, gpio, gedr_v;
+	u32 base, gpio, mask;
 	unsigned long pending;
 	void __iomem *gedr;
 
 	/* check GPIO controller to check which pin triggered the interrupt */
 	for (base = 0; base < lnw->chip.ngpio; base += 32) {
 		gedr = gpio_reg(&lnw->chip, base, GEDR);
-		gedr_v = pending = readl(gedr);
-		if (!gedr_v)
-			continue;
+		pending = readl(gedr);
 		while (pending) {
 			gpio = __ffs(pending) - 1;
-			pending &= ~BIT(gpio);
+			mask = BIT(gpio);
+			pending &= ~mask;
+			/* Clear before handling so we can't lose an edge */
+			writel(mask, gedr);
 			generic_handle_irq(lnw->irq_base + base + gpio);
 		}
-		/* clear the edge detect status bit */
-		writel(gedr_v, gedr);
 	}
 
 	chip->irq_eoi(data);

RE: [patch 5/5] [RFC] gpio: langwell: Clear edge bit before handling

[Du, Alek]

On Fri, 18 Mar 2011 03:32:58 +0800
Thomas Gleixner <tglx@linutronix.de> wrote:

> I don't have the specs for this beast, but it looks a lot like the PXA
> GPIO block. Though I bet it's the same IP and the driver should have
> reused the PXA code.
> 

I think so.

> Acknowleding the edge detect status after handling one or more gpio
> interrupts looks wrong. We might lose an edge which came in while we
> handled the previous one.
> 

Thanks for it. Although losing interrupt can always happen if the pending
IRQs are more than 1, but your patch will reduce the possibility.

Thanks,
Alek

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Feng Tang <feng.tang@intel.com>
> Cc: Alek Du <alek.du@intel.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 
> ---
>  drivers/gpio/langwell_gpio.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
> +++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
> @@ -190,23 +190,22 @@ static void lnw_irq_handler(unsigned irq
>  	struct irq_data *data = irq_desc_get_irq_data(desc);
>  	struct lnw_gpio *lnw = irq_data_get_irq_handler_data(data);
>  	struct irq_chip *chip = irq_data_get_irq_chip(data);
> -	u32 base, gpio, gedr_v;
> +	u32 base, gpio, mask;
>  	unsigned long pending;
>  	void __iomem *gedr;
>  
>  	/* check GPIO controller to check which pin triggered the interrupt */
>  	for (base = 0; base < lnw->chip.ngpio; base += 32) {
>  		gedr = gpio_reg(&lnw->chip, base, GEDR);
> -		gedr_v = pending = readl(gedr);
> -		if (!gedr_v)
> -			continue;
> +		pending = readl(gedr);
>  		while (pending) {
>  			gpio = __ffs(pending) - 1;
> -			pending &= ~BIT(gpio);
> +			mask = BIT(gpio);
> +			pending &= ~mask;
> +			/* Clear before handling so we can't lose an edge */
> +			writel(mask, gedr);
>  			generic_handle_irq(lnw->irq_base + base + gpio);
>  		}
> -		/* clear the edge detect status bit */
> -		writel(gedr_v, gedr);
>  	}
>  
>  	chip->irq_eoi(data);
> 
> 

Re: [patch 5/5] [RFC] gpio: langwell: Clear edge bit before handling

[Grant Likely]

On Fri, Mar 18, 2011 at 10:33:56AM +0800, Du, Alek wrote:
> On Fri, 18 Mar 2011 03:32:58 +0800
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > I don't have the specs for this beast, but it looks a lot like the PXA
> > GPIO block. Though I bet it's the same IP and the driver should have
> > reused the PXA code.
> > 
> 
> I think so.
> 
> > Acknowleding the edge detect status after handling one or more gpio
> > interrupts looks wrong. We might lose an edge which came in while we
> > handled the previous one.
> > 
> 
> Thanks for it. Although losing interrupt can always happen if the pending
> IRQs are more than 1, but your patch will reduce the possibility.

I'll take that as an ack.  Unless you tell me otherwise, I'll pick it
up and ask Linus to pull it shortly.

g.

> 
> Thanks,
> Alek
> 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Feng Tang <feng.tang@intel.com>
> > Cc: Alek Du <alek.du@intel.com>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > 
> > ---
> >  drivers/gpio/langwell_gpio.c |   13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > Index: linux-2.6-tip/drivers/gpio/langwell_gpio.c
> > ===================================================================
> > --- linux-2.6-tip.orig/drivers/gpio/langwell_gpio.c
> > +++ linux-2.6-tip/drivers/gpio/langwell_gpio.c
> > @@ -190,23 +190,22 @@ static void lnw_irq_handler(unsigned irq
> >  	struct irq_data *data = irq_desc_get_irq_data(desc);
> >  	struct lnw_gpio *lnw = irq_data_get_irq_handler_data(data);
> >  	struct irq_chip *chip = irq_data_get_irq_chip(data);
> > -	u32 base, gpio, gedr_v;
> > +	u32 base, gpio, mask;
> >  	unsigned long pending;
> >  	void __iomem *gedr;
> >  
> >  	/* check GPIO controller to check which pin triggered the interrupt */
> >  	for (base = 0; base < lnw->chip.ngpio; base += 32) {
> >  		gedr = gpio_reg(&lnw->chip, base, GEDR);
> > -		gedr_v = pending = readl(gedr);
> > -		if (!gedr_v)
> > -			continue;
> > +		pending = readl(gedr);
> >  		while (pending) {
> >  			gpio = __ffs(pending) - 1;
> > -			pending &= ~BIT(gpio);
> > +			mask = BIT(gpio);
> > +			pending &= ~mask;
> > +			/* Clear before handling so we can't lose an edge */
> > +			writel(mask, gedr);
> >  			generic_handle_irq(lnw->irq_base + base + gpio);
> >  		}
> > -		/* clear the edge detect status bit */
> > -		writel(gedr_v, gedr);
> >  	}
> >  
> >  	chip->irq_eoi(data);
> > 
> > 
> 

Re: [patch 0/5] gpio: langwell: Cleanup the interrupt mess

[Grant Likely]

On Thu, Mar 17, 2011 at 07:32:43PM -0000, Thomas Gleixner wrote:
> The langwell interrupt handler is interesting and breaks when an
> architecture enables GENERIC_HARDIRQS_NO_DEPRECATED.
> 
> Also this driver along with the other intel ones should depend on x86.
> 
> The series cleans that up. The last patch 5/5 is RFC and needs a
> close look and testing by folks at Intel.

Applied first 4, thanks.  Will apply last one when I get acks from the
Intel folks.

g.

Re: [patch 0/5] gpio: langwell: Cleanup the interrupt mess

[Thomas Gleixner]

On Thu, 17 Mar 2011, Grant Likely wrote:

> On Thu, Mar 17, 2011 at 07:32:43PM -0000, Thomas Gleixner wrote:
> > The langwell interrupt handler is interesting and breaks when an
> > architecture enables GENERIC_HARDIRQS_NO_DEPRECATED.
> > 
> > Also this driver along with the other intel ones should depend on x86.
> > 
> > The series cleans that up. The last patch 5/5 is RFC and needs a
> > close look and testing by folks at Intel.
> 
> Applied first 4, thanks.  Will apply last one when I get acks from the
> Intel folks.

Fair enough.

You should also poke those folks why they did write another driver for
the same gpio IP block. Though I wouldn't be surprised if we have more
than one copy of this in the tree. :(

Thanks,

	tglx