[patch 0/3] platform-drivers: x86: Cleanup pmic gpio interrupt

[patch 0/3] platform-drivers: x86: Cleanup pmic gpio interrupt

[Thomas Gleixner]

I stumbled over this gem when looking through the drivers which are
not yet converted to the new irq_chip functions.

Thanks,

	tglx

[patch 1/3] platform-drivers: x86: pmic: Fix up bogus irq hackery

[Thomas Gleixner]

[-- Attachment #1: drivers-x86-remove-bogus-irq-hack.patch --]
[-- Type: text/plain, Size: 1900 bytes --]

commit 456dc301([PATCH] intel_pmic_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.

Remove the sillyness and retrieve the interrupt data from irq_desc
directly. No need to got through circles to look it up.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Alek Du <alek.du@intel.com>
---
 drivers/platform/x86/intel_pmic_gpio.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
===================================================================
--- linux-2.6.orig/drivers/platform/x86/intel_pmic_gpio.c
+++ linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
@@ -244,11 +244,7 @@ static void pmic_irq_handler(unsigned ir
 			generic_handle_irq(pg->irq_base + gpio);
 		}
 	}
-
-	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);
+	desc->chip->irq_eoi(get_irq_desc_chip_data(desc));
 }
 
 static int __devinit platform_pmic_gpio_probe(struct platform_device *pdev)

[patch 2/3] platform-drivers: x86: Convert pmic to new irq_chip functions

[Thomas Gleixner]

[-- Attachment #1: platform-drivers-x86-convert-to-new-irq-chip-functions.patch --]
[-- Type: text/plain, Size: 1966 bytes --]

Old functions will go away soon. Remove the stray semicolons while at
it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Alek Du <alek.du@intel.com>
---
 drivers/platform/x86/intel_pmic_gpio.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Index: linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
===================================================================
--- linux-2.6.orig/drivers/platform/x86/intel_pmic_gpio.c
+++ linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
@@ -190,10 +190,10 @@ static void pmic_gpio_set(struct gpio_ch
 			1 << (offset - 16));
 }
 
-static int pmic_irq_type(unsigned irq, unsigned type)
+static int pmic_irq_type(struct irq_data *data, unsigned type)
 {
-	struct pmic_gpio *pg = get_irq_chip_data(irq);
-	u32 gpio = irq - pg->irq_base;
+	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
+	u32 gpio = data->irq - pg->irq_base;
 	unsigned long flags;
 
 	if (gpio >= pg->chip.ngpio)
@@ -207,8 +207,6 @@ static int pmic_irq_type(unsigned irq, u
 	return 0;
 }
 
-
-
 static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	struct pmic_gpio *pg = container_of(chip, struct pmic_gpio, chip);
@@ -217,19 +215,15 @@ static int pmic_gpio_to_irq(struct gpio_
 }
 
 /* the gpiointr register is read-clear, so just do nothing. */
-static void pmic_irq_unmask(unsigned irq)
-{
-};
+static void pmic_irq_unmask(struct irq_data *data) { }
 
-static void pmic_irq_mask(unsigned irq)
-{
-};
+static void pmic_irq_mask(struct irq_data *data) { }
 
 static struct irq_chip pmic_irqchip = {
 	.name		= "PMIC-GPIO",
-	.mask		= pmic_irq_mask,
-	.unmask		= pmic_irq_unmask,
-	.set_type	= pmic_irq_type,
+	.irq_mask	= pmic_irq_mask,
+	.irq_unmask	= pmic_irq_unmask,
+	.irq_set_type	= pmic_irq_type,
 };
 
 static void pmic_irq_handler(unsigned irq, struct irq_desc *desc)

[patch 3/3] platform-drivers: x86: pmic: Use irq_chip buslock mechanism

[Thomas Gleixner]

[-- Attachment #1: platform-drivers-x86-pmic-use-buslock.patch --]
[-- Type: text/plain, Size: 4348 bytes --]

The set_type function of the pmic irq chip is a horrible hack. It
schedules work because it cannot access the scu chip from the set_type
function. That breaks the assumption, that the type is set after
set_type has returned.

irq_chips provide buslock functions to avoid the above. Convert the
driver to use the proper model.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Feng Tang <feng.tang@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Alek Du <alek.du@intel.com>
---
 drivers/platform/x86/intel_pmic_gpio.c |   80 +++++++++++++--------------------
 1 file changed, 32 insertions(+), 48 deletions(-)

Index: linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
===================================================================
--- linux-2.6.orig/drivers/platform/x86/intel_pmic_gpio.c
+++ linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
@@ -60,23 +60,18 @@ enum pmic_gpio_register {
 #define GPOSW_DOU 0x08
 #define GPOSW_RDRV 0x30
 
+#define GPIO_UPDATE_TYPE	0x80000000
 
 #define NUM_GPIO 24
 
-struct pmic_gpio_irq {
-	spinlock_t lock;
-	u32 trigger[NUM_GPIO];
-	u32 dirty;
-	struct work_struct work;
-};
-
-
 struct pmic_gpio {
+	struct mutex		buslock;
 	struct gpio_chip	chip;
-	struct pmic_gpio_irq	irqtypes;
 	void			*gpiointr;
 	int			irq;
 	unsigned		irq_base;
+	unsigned int		update_type;
+	u32			trigger_type;
 };
 
 static void pmic_program_irqtype(int gpio, int type)
@@ -92,37 +87,6 @@ static void pmic_program_irqtype(int gpi
 		intel_scu_ipc_update_register(GPIO0 + gpio, 0x00, 0x10);
 };
 
-static void pmic_irqtype_work(struct work_struct *work)
-{
-	struct pmic_gpio_irq *t =
-		container_of(work, struct pmic_gpio_irq, work);
-	unsigned long flags;
-	int i;
-	u16 type;
-
-	spin_lock_irqsave(&t->lock, flags);
-	/* As we drop the lock, we may need multiple scans if we race the
-	   pmic_irq_type function */
-	while (t->dirty) {
-		/*
-		 *	For each pin that has the dirty bit set send an IPC
-		 *	message to configure the hardware via the PMIC
-		 */
-		for (i = 0; i < NUM_GPIO; i++) {
-			if (!(t->dirty & (1 << i)))
-				continue;
-			t->dirty &= ~(1 << i);
-			/* We can't trust the array entry or dirty
-			   once the lock is dropped */
-			type = t->trigger[i];
-			spin_unlock_irqrestore(&t->lock, flags);
-			pmic_program_irqtype(i, type);
-			spin_lock_irqsave(&t->lock, flags);
-		}
-	}
-	spin_unlock_irqrestore(&t->lock, flags);
-}
-
 static int pmic_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
 	if (offset > 8) {
@@ -190,20 +154,21 @@ static void pmic_gpio_set(struct gpio_ch
 			1 << (offset - 16));
 }
 
+/*
+ * This is called from genirq with pg->buslock locked and
+ * irq_desc->lock held. We can not access the scu bus here, so we
+ * store the change and update in the bus_sync_unlock() function below
+ */
 static int pmic_irq_type(struct irq_data *data, unsigned type)
 {
 	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
 	u32 gpio = data->irq - pg->irq_base;
-	unsigned long flags;
 
 	if (gpio >= pg->chip.ngpio)
 		return -EINVAL;
 
-	spin_lock_irqsave(&pg->irqtypes.lock, flags);
-	pg->irqtypes.trigger[gpio] = type;
-	pg->irqtypes.dirty |=  (1 << gpio);
-	spin_unlock_irqrestore(&pg->irqtypes.lock, flags);
-	schedule_work(&pg->irqtypes.work);
+	pg->trigger_type = type;
+	pg->update_type = gpio | GPIO_UPDATE_TYPE;
 	return 0;
 }
 
@@ -214,6 +179,26 @@ static int pmic_gpio_to_irq(struct gpio_
 	return pg->irq_base + offset;
 }
 
+static void pmic_bus_lock(struct irq_data *data)
+{
+	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&pg->buslock);
+}
+
+static void pmic_bus_sync_unlock(struct irq_data *data)
+{
+	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
+
+	if (pg->update_type) {
+		unsigned int gpio = pg->update_type & ~GPIO_UPDATE_TYPE;
+
+		pmic_program_irqtype(gpio, pg->trigger_type);
+		pg->update_type = 0;
+	}
+	mutex_unlock(&pg->buslock);
+}
+
 /* the gpiointr register is read-clear, so just do nothing. */
 static void pmic_irq_unmask(struct irq_data *data) { }
 
@@ -287,8 +272,7 @@ static int __devinit platform_pmic_gpio_
 	pg->chip.can_sleep = 1;
 	pg->chip.dev = dev;
 
-	INIT_WORK(&pg->irqtypes.work, pmic_irqtype_work);
-	spin_lock_init(&pg->irqtypes.lock);
+	mutex_init(&pg->buslock);
 
 	pg->chip.dev = dev;
 	retval = gpiochip_add(&pg->chip);

Re: [patch 0/3] platform-drivers: x86: Cleanup pmic gpio interrupt

[Matthew Garrett]

Applied, thanks.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 0/3] platform-drivers: x86: Cleanup pmic gpio interrupt

[Thomas Gleixner]

On Mon, 7 Feb 2011, Matthew Garrett wrote:

> Applied, thanks.

Btw, there is no real reason to have this as a chained handler. I
guess I need to find some time for educational documentation :)

See below.

Thanks,

	tglx

------->
Subject: platform-drivers: x86: pmic: Use request_irq instead of chained handler
From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 07 Feb 2011 21:24:29 +0100

There is no need to install a chained handler for this hardware. This
is a plain x86 IOAPIC interrupt which is handled by the core code
perfectly fine. There is nothing special about demultiplexing these
gpio interrupts which justifies a custom hack. Replace it by a plain
old interrupt handler installed with request_irq. That makes the code
agnostic about the underlying primary interrupt hardware. The overhead
for this is minimal, but it gives us the advantage of accounting,
balancing and to detect interrupt storms. gpio interrupts are not
really that performance critical.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/platform/x86/intel_pmic_gpio.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
===================================================================
--- linux-2.6.orig/drivers/platform/x86/intel_pmic_gpio.c
+++ linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
@@ -211,9 +211,9 @@ static struct irq_chip pmic_irqchip = {
 	.irq_set_type	= pmic_irq_type,
 };
 
-static void pmic_irq_handler(unsigned irq, struct irq_desc *desc)
+static irqreturn_t pmic_irq_handler(unsigned int irq, void *data)
 {
-	struct pmic_gpio *pg = (struct pmic_gpio *)get_irq_data(irq);
+	struct pmic_gpio *pg = data;
 	u8 intsts = *((u8 *)pg->gpiointr + 4);
 	int gpio;
 
@@ -223,7 +223,6 @@ static void pmic_irq_handler(unsigned ir
 			generic_handle_irq(pg->irq_base + gpio);
 		}
 	}
-	desc->chip->irq_eoi(get_irq_desc_chip_data(desc));
 }
 
 static int __devinit platform_pmic_gpio_probe(struct platform_device *pdev)
@@ -280,8 +279,13 @@ static int __devinit platform_pmic_gpio_
 		printk(KERN_ERR "%s: Can not add pmic gpio chip.\n", __func__);
 		goto err;
 	}
-	set_irq_data(pg->irq, pg);
-	set_irq_chained_handler(pg->irq, pmic_irq_handler);
+
+	retval = request_irq(pg->irq, pmic_irq_handler, 0, "pmic", pg);
+	if (retval) {
+		printk(KERN_WARN "pmic: Interrupt request failed\n");
+		goto err;
+	}
+
 	for (i = 0; i < 8; i++) {
 		set_irq_chip_and_handler_name(i + pg->irq_base, &pmic_irqchip,
 					handle_simple_irq, "demux");

Re: [patch 0/3] platform-drivers: x86: Cleanup pmic gpio interrupt

[Andrew Morton]

On Mon, 7 Feb 2011 21:41:30 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 7 Feb 2011, Matthew Garrett wrote:
> 
> > Applied, thanks.
> 
> Btw, there is no real reason to have this as a chained handler. I
> guess I need to find some time for educational documentation :)
> 
> See below.
> 
> Thanks,
> 
> 	tglx
> 
> ------->
> Subject: platform-drivers: x86: pmic: Use request_irq instead of chained handler
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Mon, 07 Feb 2011 21:24:29 +0100
> 
> There is no need to install a chained handler for this hardware. This
> is a plain x86 IOAPIC interrupt which is handled by the core code
> perfectly fine. There is nothing special about demultiplexing these
> gpio interrupts which justifies a custom hack. Replace it by a plain
> old interrupt handler installed with request_irq. That makes the code
> agnostic about the underlying primary interrupt hardware. The overhead
> for this is minimal, but it gives us the advantage of accounting,
> balancing and to detect interrupt storms. gpio interrupts are not
> really that performance critical.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/platform/x86/intel_pmic_gpio.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
> ===================================================================
> --- linux-2.6.orig/drivers/platform/x86/intel_pmic_gpio.c
> +++ linux-2.6/drivers/platform/x86/intel_pmic_gpio.c
> @@ -211,9 +211,9 @@ static struct irq_chip pmic_irqchip = {
>  	.irq_set_type	= pmic_irq_type,
>  };
>  
> -static void pmic_irq_handler(unsigned irq, struct irq_desc *desc)
> +static irqreturn_t pmic_irq_handler(unsigned int irq, void *data)
>  {
> -	struct pmic_gpio *pg = (struct pmic_gpio *)get_irq_data(irq);
> +	struct pmic_gpio *pg = data;
>  	u8 intsts = *((u8 *)pg->gpiointr + 4);
>  	int gpio;
>  
> @@ -223,7 +223,6 @@ static void pmic_irq_handler(unsigned ir
>  			generic_handle_irq(pg->irq_base + gpio);
>  		}
>  	}
> -	desc->chip->irq_eoi(get_irq_desc_chip_data(desc));
>  }
>  
>  static int __devinit platform_pmic_gpio_probe(struct platform_device *pdev)
> @@ -280,8 +279,13 @@ static int __devinit platform_pmic_gpio_
>  		printk(KERN_ERR "%s: Can not add pmic gpio chip.\n", __func__);
>  		goto err;
>  	}
> -	set_irq_data(pg->irq, pg);
> -	set_irq_chained_handler(pg->irq, pmic_irq_handler);
> +
> +	retval = request_irq(pg->irq, pmic_irq_handler, 0, "pmic", pg);
> +	if (retval) {
> +		printk(KERN_WARN "pmic: Interrupt request failed\n");
> +		goto err;
> +	}
> +
>  	for (i = 0; i < 8; i++) {
>  		set_irq_chip_and_handler_name(i + pg->irq_base, &pmic_irqchip,
>  					handle_simple_irq, "demux");

That didn't work out too well.


Subject: drivers/platform/x86/intel_pmic_gpio.c: fix big mess with i386 allmodconfig
From: Andrew Morton <akpm@linux-foundation.org>

Fix build error:

drivers/platform/x86/intel_pmic_gpio.c:285: error: 'KERN_WARN' undeclared (first use in this function)

And a bunch of warnings:

drivers/platform/x86/intel_pmic_gpio.c: In function 'pmic_irq_handler':
drivers/platform/x86/intel_pmic_gpio.c:226: warning: no return statement in function returning non-void
drivers/platform/x86/intel_pmic_gpio.c: In function 'platform_pmic_gpio_probe':
drivers/platform/x86/intel_pmic_gpio.c:283: warning: passing argument 2 of 'request_irq' from incompatible pointer type
drivers/platform/x86/intel_pmic_gpio.c: At top level:
drivers/platform/x86/intel_pmic_gpio.c:183: warning: 'pmic_bus_lock' defined but not used
drivers/platform/x86/intel_pmic_gpio.c:190: warning: 'pmic_bus_sync_unlock' defined but not used

This repairs linux-next's "platform-drivers: x86: pmic: Use request_irq
instead of chained handler".  Please don't introduce bisection holes into
mainline!

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/platform/x86/intel_pmic_gpio.c |   40 ++---------------------
 1 file changed, 5 insertions(+), 35 deletions(-)

diff -puN drivers/platform/x86/intel_pmic_gpio.c~drivers-platform-x86-intel_pmic_gpioc-fix-big-mess-with-i386-allmodconfig drivers/platform/x86/intel_pmic_gpio.c
--- a/drivers/platform/x86/intel_pmic_gpio.c~drivers-platform-x86-intel_pmic_gpioc-fix-big-mess-with-i386-allmodconfig
+++ a/drivers/platform/x86/intel_pmic_gpio.c
@@ -74,19 +74,6 @@ struct pmic_gpio {
 	u32			trigger_type;
 };
 
-static void pmic_program_irqtype(int gpio, int type)
-{
-	if (type & IRQ_TYPE_EDGE_RISING)
-		intel_scu_ipc_update_register(GPIO0 + gpio, 0x20, 0x20);
-	else
-		intel_scu_ipc_update_register(GPIO0 + gpio, 0x00, 0x20);
-
-	if (type & IRQ_TYPE_EDGE_FALLING)
-		intel_scu_ipc_update_register(GPIO0 + gpio, 0x10, 0x10);
-	else
-		intel_scu_ipc_update_register(GPIO0 + gpio, 0x00, 0x10);
-};
-
 static int pmic_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
 	if (offset > 8) {
@@ -179,26 +166,6 @@ static int pmic_gpio_to_irq(struct gpio_
 	return pg->irq_base + offset;
 }
 
-static void pmic_bus_lock(struct irq_data *data)
-{
-	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
-
-	mutex_lock(&pg->buslock);
-}
-
-static void pmic_bus_sync_unlock(struct irq_data *data)
-{
-	struct pmic_gpio *pg = irq_data_get_irq_chip_data(data);
-
-	if (pg->update_type) {
-		unsigned int gpio = pg->update_type & ~GPIO_UPDATE_TYPE;
-
-		pmic_program_irqtype(gpio, pg->trigger_type);
-		pg->update_type = 0;
-	}
-	mutex_unlock(&pg->buslock);
-}
-
 /* the gpiointr register is read-clear, so just do nothing. */
 static void pmic_irq_unmask(struct irq_data *data) { }
 
@@ -211,18 +178,21 @@ static struct irq_chip pmic_irqchip = {
 	.irq_set_type	= pmic_irq_type,
 };
 
-static irqreturn_t pmic_irq_handler(unsigned int irq, void *data)
+static irqreturn_t pmic_irq_handler(int irq, void *data)
 {
 	struct pmic_gpio *pg = data;
 	u8 intsts = *((u8 *)pg->gpiointr + 4);
 	int gpio;
+	irqreturn_t ret = IRQ_NONE;
 
 	for (gpio = 0; gpio < 8; gpio++) {
 		if (intsts & (1 << gpio)) {
 			pr_debug("pmic pin %d triggered\n", gpio);
 			generic_handle_irq(pg->irq_base + gpio);
+			ret = IRQ_HANDLED;
 		}
 	}
+	return ret;
 }
 
 static int __devinit platform_pmic_gpio_probe(struct platform_device *pdev)
@@ -282,7 +252,7 @@ static int __devinit platform_pmic_gpio_
 
 	retval = request_irq(pg->irq, pmic_irq_handler, 0, "pmic", pg);
 	if (retval) {
-		printk(KERN_WARN "pmic: Interrupt request failed\n");
+		printk(KERN_WARNING "pmic: Interrupt request failed\n");
 		goto err;
 	}
 
_