[PATCH 1/2] gpio add interrupt support on pca953x

[PATCH 1/2] gpio add interrupt support on pca953x

[Haojian Zhuang]

[PATCH 1/2] gpio add interrupt support on pca953x

[Trilok Soni]

Hi,

> +static void pca953x_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + ? ? ? struct pca953x_chip *chip = desc->handler_data;
> +
> + ? ? ? desc->chip->ack(irq);
> + ? ? ? schedule_work(&chip->irq_work);
> +}

Please use threaded IRQ support. Check request_threaded_irq API and I
think wm8350 driver under drivers/mfd/ for example in latest kernel.




-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

[PATCH 1/2] gpio add interrupt support on pca953x

[Mark Brown]

On Tue, Dec 22, 2009 at 07:29:03AM -0500, Haojian Zhuang wrote:

> +static void pca953x_irq_work(struct work_struct *work)
> +{
> +	DECLARE_BITMAP(rising, PCA953X_NUM_IRQ);
> +	DECLARE_BITMAP(falling, PCA953X_NUM_IRQ);

...

> +static void pca953x_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct pca953x_chip *chip = desc->handler_data;
> +
> +	desc->chip->ack(irq);
> +	schedule_work(&chip->irq_work);
> +}

genirq ought to be able to handle this in-line with the assistance of
the lock() and sync_unlock() operations that are available to irq_chip,
you shouldn't need a workqueue.  The wm831x driver is as far as I know
the only driver actually doing this at the minute.

> +static struct irq_chip pca953x_irqchip = {
> +	.name		= "GPIO",

Might be nice to call this "pxa953x" or something else including that
name - GPIO is a bit too generic, it's not going to immediately obvious
which chip this GPIO is for.

[PATCH 1/2] gpio add interrupt support on pca953x

[Eric Miao]

Haojian,

Marc Zyngier was doing the similar thing, and submitted a patch to
Andrew Morton already, but I prefer a full-functional IRQ chip based
solution, so you may want to work with him together on this.

Marc,

Do you have the link to your patch?

- eric

On Tue, Dec 22, 2009 at 8:29 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> From b8cc5f82ae9a1883ee4d6fa453bed63d4431b9e4 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date: Tue, 22 Dec 2009 15:08:41 -0500
> Subject: [PATCH] gpio: add interrupt support on pca953x
>
> Support interrupt on pca953x gpio expander.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
> ?drivers/gpio/pca953x.c | ?121 +++++++++++++++++++++++++++++++++++++++++++++++-
> ?1 files changed, 119 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index 6a2fb3f..2d6b9e8 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -16,6 +16,7 @@
> ?#include <linux/gpio.h>
> ?#include <linux/i2c.h>
> ?#include <linux/i2c/pca953x.h>
> +#include <linux/irq.h>
> ?#ifdef CONFIG_OF_GPIO
> ?#include <linux/of_platform.h>
> ?#include <linux/of_gpio.h>
> @@ -50,12 +51,19 @@ MODULE_DEVICE_TABLE(i2c, pca953x_id);
>
> ?struct pca953x_chip {
> ? ? ? ?unsigned gpio_start;
> + ? ? ? unsigned irq_start;
> ? ? ? ?uint16_t reg_output;
> ? ? ? ?uint16_t reg_direction;
> + ? ? ? uint16_t irq_mask;
> + ? ? ? uint16_t irq_falling_edge;
> + ? ? ? uint16_t irq_rising_edge;
> + ? ? ? uint16_t last_input;
>
> - ? ? ? struct i2c_client *client;
> ? ? ? ?struct pca953x_platform_data *dyn_pdata;
> - ? ? ? struct gpio_chip gpio_chip;
> + ? ? ? struct i2c_client ? ? ? *client;
> + ? ? ? struct gpio_chip ? ? ? ?gpio_chip;
> + ? ? ? struct mutex ? ? ? ? ? ?irq_lock;
> + ? ? ? struct work_struct ? ? ?irq_work;
> ? ? ? ?char **names;
> ?};
>
> @@ -250,11 +258,113 @@ pca953x_get_alt_pdata(struct i2c_client *client)
> ?}
> ?#endif
>
> +#define PCA953X_NUM_IRQ ? ? ? ? ? ? ? ? ? ? ? ?16
> +
> +static void pca953x_irq_work(struct work_struct *work)
> +{
> + ? ? ? DECLARE_BITMAP(rising, PCA953X_NUM_IRQ);
> + ? ? ? DECLARE_BITMAP(falling, PCA953X_NUM_IRQ);
> + ? ? ? struct pca953x_chip *chip;
> + ? ? ? unsigned short input, mask;
> + ? ? ? int ret, irq;
> +
> + ? ? ? chip = container_of(work, struct pca953x_chip, irq_work);
> + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &input);
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? return;
> +
> + ? ? ? mutex_lock(&chip->irq_lock);
> + ? ? ? mask = (input ^ chip->last_input) & chip->irq_mask;
> + ? ? ? rising[0] = (input & mask) & chip->irq_rising_edge;
> + ? ? ? falling[0] = (~input & mask) & chip->irq_falling_edge;
> +
> + ? ? ? while (!bitmap_empty(rising, PCA953X_NUM_IRQ)) {
> + ? ? ? ? ? ? ? irq = find_first_bit(rising, PCA953X_NUM_IRQ);
> + ? ? ? ? ? ? ? clear_bit(irq, rising);
> + ? ? ? ? ? ? ? generic_handle_irq(chip->irq_start + irq);
> + ? ? ? }
> + ? ? ? while (!bitmap_empty(falling, PCA953X_NUM_IRQ)) {
> + ? ? ? ? ? ? ? irq = find_first_bit(falling, PCA953X_NUM_IRQ);
> + ? ? ? ? ? ? ? clear_bit(irq, falling);
> + ? ? ? ? ? ? ? generic_handle_irq(chip->irq_start + irq);
> + ? ? ? }
> + ? ? ? chip->last_input = input;
> + ? ? ? mutex_unlock(&chip->irq_lock);
> +}
> +
> +static void pca953x_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + ? ? ? struct pca953x_chip *chip = desc->handler_data;
> +
> + ? ? ? desc->chip->ack(irq);
> + ? ? ? schedule_work(&chip->irq_work);
> +}
> +
> +static void pca953x_irq_enable(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? chip->irq_mask |= 1u << (irq - chip->irq_start);
> +}
> +
> +static void pca953x_irq_disable(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? chip->irq_mask &= ~(1u << (irq - chip->irq_start));
> +}
> +
> +static int pca953x_irq_type(unsigned int irq, unsigned int trigger)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> + ? ? ? unsigned short mask = 1u << (irq - chip->irq_start);
> +
> + ? ? ? if (trigger & IRQ_TYPE_EDGE_RISING)
> + ? ? ? ? ? ? ? chip->irq_rising_edge |= mask;
> + ? ? ? else
> + ? ? ? ? ? ? ? chip->irq_rising_edge &= ~mask;
> + ? ? ? if (trigger & IRQ_TYPE_EDGE_FALLING)
> + ? ? ? ? ? ? ? chip->irq_falling_edge |= mask;
> + ? ? ? else
> + ? ? ? ? ? ? ? chip->irq_falling_edge &= ~mask;
> + ? ? ? return 0;
> +}
> +
> +static struct irq_chip pca953x_irqchip = {
> + ? ? ? .name ? ? ? ? ? = "GPIO",
> + ? ? ? .enable ? ? ? ? = pca953x_irq_enable,
> + ? ? ? .disable ? ? ? ?= pca953x_irq_disable,
> + ? ? ? .set_type ? ? ? = pca953x_irq_type,
> +};
> +
> +static void pca953x_setup_irq(struct i2c_client *client)
> +{
> + ? ? ? struct pca953x_chip *chip = i2c_get_clientdata(client);
> + ? ? ? int i;
> +
> + ? ? ? if (!client->irq)
> + ? ? ? ? ? ? ? return;
> +
> + ? ? ? mutex_init(&chip->irq_lock);
> + ? ? ? chip->irq_start = gpio_to_irq(chip->gpio_start);
> + ? ? ? for (i = 0; i < chip->gpio_chip.ngpio; i++) {
> + ? ? ? ? ? ? ? set_irq_chip(chip->irq_start + i, &pca953x_irqchip);
> + ? ? ? ? ? ? ? set_irq_chip_data(chip->irq_start + i, chip);
> + ? ? ? ? ? ? ? set_irq_handler(chip->irq_start + i, handle_edge_irq);
> + ? ? ? ? ? ? ? set_irq_flags(chip->irq_start + i, IRQF_VALID);
> + ? ? ? }
> + ? ? ? set_irq_type(client->irq, IRQ_TYPE_EDGE_FALLING);
> + ? ? ? set_irq_data(client->irq, chip);
> + ? ? ? set_irq_chained_handler(client->irq, pca953x_irq_handler);
> + ? ? ? INIT_WORK(&chip->irq_work, pca953x_irq_work);
> +}
> +
> ?static int __devinit pca953x_probe(struct i2c_client *client,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct i2c_device_id *id)
> ?{
> ? ? ? ?struct pca953x_platform_data *pdata;
> ? ? ? ?struct pca953x_chip *chip;
> + ? ? ? unsigned short input;
> ? ? ? ?int ret;
>
> ? ? ? ?chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
> @@ -282,12 +392,18 @@ static int __devinit pca953x_probe(struct
> i2c_client *client,
> ? ? ? ?chip->gpio_start = pdata->gpio_base;
>
> ? ? ? ?chip->names = pdata->names;
> + ? ? ? chip->irq_mask = -1UL;
>
> ? ? ? ?/* initialize cached registers from their original values.
> ? ? ? ? * we can't share this chip with another i2c master.
> ? ? ? ? */
> ? ? ? ?pca953x_setup_gpio(chip, id->driver_data);
>
> + ? ? ? /* clear interrupt status */
> + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &input);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? goto out_failed;
> +
> ? ? ? ?ret = pca953x_read_reg(chip, PCA953X_OUTPUT, &chip->reg_output);
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?goto out_failed;
> @@ -314,6 +430,7 @@ static int __devinit pca953x_probe(struct
> i2c_client *client,
> ? ? ? ?}
>
> ? ? ? ?i2c_set_clientdata(client, chip);
> + ? ? ? pca953x_setup_irq(client);
> ? ? ? ?return 0;
>
> ?out_failed:
> --
> 1.5.6.5
>

[PATCH 1/2] gpio add interrupt support on pca953x

[Marc Zyngier]

Eric, Haojian,

I indeed have something very similar, except it uses a threaded
interrupt handler, and is slightly more configurable.

I'll post the patch in a few hours, as I get back home.

Thanks,

	M.

On Tue, 22 Dec 2009 21:41:51 +0800
Eric Miao <eric.y.miao@gmail.com> wrote:

> Haojian,
> 
> Marc Zyngier was doing the similar thing, and submitted a patch to
> Andrew Morton already, but I prefer a full-functional IRQ chip based
> solution, so you may want to work with him together on this.
> 
> Marc,
> 
> Do you have the link to your patch?
> 
> - eric
> 
> On Tue, Dec 22, 2009 at 8:29 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
> > From b8cc5f82ae9a1883ee4d6fa453bed63d4431b9e4 Mon Sep 17 00:00:00 2001
> > From: Haojian Zhuang <haojian.zhuang@marvell.com>
> > Date: Tue, 22 Dec 2009 15:08:41 -0500
> > Subject: [PATCH] gpio: add interrupt support on pca953x
> >
> > Support interrupt on pca953x gpio expander.
> >
> > Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> > ---
> > ?drivers/gpio/pca953x.c | ?121 +++++++++++++++++++++++++++++++++++++++++++++++-
> > ?1 files changed, 119 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> > index 6a2fb3f..2d6b9e8 100644
> > --- a/drivers/gpio/pca953x.c
> > +++ b/drivers/gpio/pca953x.c
> > @@ -16,6 +16,7 @@
> > ?#include <linux/gpio.h>
> > ?#include <linux/i2c.h>
> > ?#include <linux/i2c/pca953x.h>
> > +#include <linux/irq.h>
> > ?#ifdef CONFIG_OF_GPIO
> > ?#include <linux/of_platform.h>
> > ?#include <linux/of_gpio.h>
> > @@ -50,12 +51,19 @@ MODULE_DEVICE_TABLE(i2c, pca953x_id);
> >
> > ?struct pca953x_chip {
> > ? ? ? ?unsigned gpio_start;
> > + ? ? ? unsigned irq_start;
> > ? ? ? ?uint16_t reg_output;
> > ? ? ? ?uint16_t reg_direction;
> > + ? ? ? uint16_t irq_mask;
> > + ? ? ? uint16_t irq_falling_edge;
> > + ? ? ? uint16_t irq_rising_edge;
> > + ? ? ? uint16_t last_input;
> >
> > - ? ? ? struct i2c_client *client;
> > ? ? ? ?struct pca953x_platform_data *dyn_pdata;
> > - ? ? ? struct gpio_chip gpio_chip;
> > + ? ? ? struct i2c_client ? ? ? *client;
> > + ? ? ? struct gpio_chip ? ? ? ?gpio_chip;
> > + ? ? ? struct mutex ? ? ? ? ? ?irq_lock;
> > + ? ? ? struct work_struct ? ? ?irq_work;
> > ? ? ? ?char **names;
> > ?};
> >
> > @@ -250,11 +258,113 @@ pca953x_get_alt_pdata(struct i2c_client *client)
> > ?}
> > ?#endif
> >
> > +#define PCA953X_NUM_IRQ ? ? ? ? ? ? ? ? ? ? ? ?16
> > +
> > +static void pca953x_irq_work(struct work_struct *work)
> > +{
> > + ? ? ? DECLARE_BITMAP(rising, PCA953X_NUM_IRQ);
> > + ? ? ? DECLARE_BITMAP(falling, PCA953X_NUM_IRQ);
> > + ? ? ? struct pca953x_chip *chip;
> > + ? ? ? unsigned short input, mask;
> > + ? ? ? int ret, irq;
> > +
> > + ? ? ? chip = container_of(work, struct pca953x_chip, irq_work);
> > + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &input);
> > + ? ? ? if (ret < 0)
> > + ? ? ? ? ? ? ? return;
> > +
> > + ? ? ? mutex_lock(&chip->irq_lock);
> > + ? ? ? mask = (input ^ chip->last_input) & chip->irq_mask;
> > + ? ? ? rising[0] = (input & mask) & chip->irq_rising_edge;
> > + ? ? ? falling[0] = (~input & mask) & chip->irq_falling_edge;
> > +
> > + ? ? ? while (!bitmap_empty(rising, PCA953X_NUM_IRQ)) {
> > + ? ? ? ? ? ? ? irq = find_first_bit(rising, PCA953X_NUM_IRQ);
> > + ? ? ? ? ? ? ? clear_bit(irq, rising);
> > + ? ? ? ? ? ? ? generic_handle_irq(chip->irq_start + irq);
> > + ? ? ? }
> > + ? ? ? while (!bitmap_empty(falling, PCA953X_NUM_IRQ)) {
> > + ? ? ? ? ? ? ? irq = find_first_bit(falling, PCA953X_NUM_IRQ);
> > + ? ? ? ? ? ? ? clear_bit(irq, falling);
> > + ? ? ? ? ? ? ? generic_handle_irq(chip->irq_start + irq);
> > + ? ? ? }
> > + ? ? ? chip->last_input = input;
> > + ? ? ? mutex_unlock(&chip->irq_lock);
> > +}
> > +
> > +static void pca953x_irq_handler(unsigned int irq, struct irq_desc *desc)
> > +{
> > + ? ? ? struct pca953x_chip *chip = desc->handler_data;
> > +
> > + ? ? ? desc->chip->ack(irq);
> > + ? ? ? schedule_work(&chip->irq_work);
> > +}
> > +
> > +static void pca953x_irq_enable(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? chip->irq_mask |= 1u << (irq - chip->irq_start);
> > +}
> > +
> > +static void pca953x_irq_disable(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? chip->irq_mask &= ~(1u << (irq - chip->irq_start));
> > +}
> > +
> > +static int pca953x_irq_type(unsigned int irq, unsigned int trigger)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > + ? ? ? unsigned short mask = 1u << (irq - chip->irq_start);
> > +
> > + ? ? ? if (trigger & IRQ_TYPE_EDGE_RISING)
> > + ? ? ? ? ? ? ? chip->irq_rising_edge |= mask;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? chip->irq_rising_edge &= ~mask;
> > + ? ? ? if (trigger & IRQ_TYPE_EDGE_FALLING)
> > + ? ? ? ? ? ? ? chip->irq_falling_edge |= mask;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? chip->irq_falling_edge &= ~mask;
> > + ? ? ? return 0;
> > +}
> > +
> > +static struct irq_chip pca953x_irqchip = {
> > + ? ? ? .name ? ? ? ? ? = "GPIO",
> > + ? ? ? .enable ? ? ? ? = pca953x_irq_enable,
> > + ? ? ? .disable ? ? ? ?= pca953x_irq_disable,
> > + ? ? ? .set_type ? ? ? = pca953x_irq_type,
> > +};
> > +
> > +static void pca953x_setup_irq(struct i2c_client *client)
> > +{
> > + ? ? ? struct pca953x_chip *chip = i2c_get_clientdata(client);
> > + ? ? ? int i;
> > +
> > + ? ? ? if (!client->irq)
> > + ? ? ? ? ? ? ? return;
> > +
> > + ? ? ? mutex_init(&chip->irq_lock);
> > + ? ? ? chip->irq_start = gpio_to_irq(chip->gpio_start);
> > + ? ? ? for (i = 0; i < chip->gpio_chip.ngpio; i++) {
> > + ? ? ? ? ? ? ? set_irq_chip(chip->irq_start + i, &pca953x_irqchip);
> > + ? ? ? ? ? ? ? set_irq_chip_data(chip->irq_start + i, chip);
> > + ? ? ? ? ? ? ? set_irq_handler(chip->irq_start + i, handle_edge_irq);
> > + ? ? ? ? ? ? ? set_irq_flags(chip->irq_start + i, IRQF_VALID);
> > + ? ? ? }
> > + ? ? ? set_irq_type(client->irq, IRQ_TYPE_EDGE_FALLING);
> > + ? ? ? set_irq_data(client->irq, chip);
> > + ? ? ? set_irq_chained_handler(client->irq, pca953x_irq_handler);
> > + ? ? ? INIT_WORK(&chip->irq_work, pca953x_irq_work);
> > +}
> > +
> > ?static int __devinit pca953x_probe(struct i2c_client *client,
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct i2c_device_id *id)
> > ?{
> > ? ? ? ?struct pca953x_platform_data *pdata;
> > ? ? ? ?struct pca953x_chip *chip;
> > + ? ? ? unsigned short input;
> > ? ? ? ?int ret;
> >
> > ? ? ? ?chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
> > @@ -282,12 +392,18 @@ static int __devinit pca953x_probe(struct
> > i2c_client *client,
> > ? ? ? ?chip->gpio_start = pdata->gpio_base;
> >
> > ? ? ? ?chip->names = pdata->names;
> > + ? ? ? chip->irq_mask = -1UL;
> >
> > ? ? ? ?/* initialize cached registers from their original values.
> > ? ? ? ? * we can't share this chip with another i2c master.
> > ? ? ? ? */
> > ? ? ? ?pca953x_setup_gpio(chip, id->driver_data);
> >
> > + ? ? ? /* clear interrupt status */
> > + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &input);
> > + ? ? ? if (ret)
> > + ? ? ? ? ? ? ? goto out_failed;
> > +
> > ? ? ? ?ret = pca953x_read_reg(chip, PCA953X_OUTPUT, &chip->reg_output);
> > ? ? ? ?if (ret)
> > ? ? ? ? ? ? ? ?goto out_failed;
> > @@ -314,6 +430,7 @@ static int __devinit pca953x_probe(struct
> > i2c_client *client,
> > ? ? ? ?}
> >
> > ? ? ? ?i2c_set_clientdata(client, chip);
> > + ? ? ? pca953x_setup_irq(client);
> > ? ? ? ?return 0;
> >
> > ?out_failed:
> > --
> > 1.5.6.5
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Fast. Cheap. Reliable. Pick two.

[PATCH 1/2] gpio add interrupt support on pca953x

[Marc Zyngier]

On Tue, 22 Dec 2009 21:41:51 +0800
Eric Miao <eric.y.miao@gmail.com> wrote:

Eric, Haojian,

> Marc Zyngier was doing the similar thing, and submitted a patch to
> Andrew Morton already, but I prefer a full-functional IRQ chip based
> solution, so you may want to work with him together on this.
> 
> Marc,
> 
> Do you have the link to your patch?

Here's the current state of my tree, quickly rebased to 2.6.33-rc1.
To summarize, the main differences with Haojian's patch:
- uses threaded irqs/nested irqs
- setup is slightly more complicated (interrupt source mask and base
  interrupt number), but more correct (no dependency on gpio_to_irq()
  being a linear mapping, which is not always the case)

>From d8dc00d1ebedcc889c41add70cceb8f15f106261 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@misterjones.org>
Date: Tue, 22 Dec 2009 17:38:29 +0100
Subject: [PATCH] Add interrupt handling capability to pca953x.

Most of the GPIO expanders controlled by the pca953x driver are
able to report changes on the input pins through an *INT pin.

This patch implements the irq_chip functionality (edge detection
only).

The driver has been tested on an Arcom Zeus.

Signed-off-by: Marc Zyngier <maz@misterjones.org>
---
 drivers/gpio/pca953x.c      |  207 ++++++++++++++++++++++++++++++++++++++++---
 include/linux/i2c/pca953x.h |    5 +
 2 files changed, 199 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 6a2fb3f..06c34a0 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -14,6 +14,8 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pca953x.h>
 #ifdef CONFIG_OF_GPIO
@@ -26,23 +28,28 @@
 #define PCA953X_INVERT         2
 #define PCA953X_DIRECTION      3
 
+#define PCA953X_GPIOS	       0x00FF
+#define PCA953X_INT	       0x0100
+
 static const struct i2c_device_id pca953x_id[] = {
-	{ "pca9534", 8, },
-	{ "pca9535", 16, },
+	{ "pca9534", 8  | PCA953X_INT, },
+	{ "pca9535", 16 | PCA953X_INT, },
 	{ "pca9536", 4, },
-	{ "pca9537", 4, },
-	{ "pca9538", 8, },
-	{ "pca9539", 16, },
-	{ "pca9554", 8, },
-	{ "pca9555", 16, },
+	{ "pca9537", 4  | PCA953X_INT, },
+	{ "pca9538", 8  | PCA953X_INT, },
+	{ "pca9539", 16 | PCA953X_INT, },
+	{ "pca9554", 8  | PCA953X_INT, },
+	{ "pca9555", 16 | PCA953X_INT, },
 	{ "pca9556", 8, },
 	{ "pca9557", 8, },
 
 	{ "max7310", 8, },
-	{ "max7315", 8, },
-	{ "pca6107", 8, },
-	{ "tca6408", 8, },
-	{ "tca6416", 16, },
+	{ "max7312", 16 | PCA953X_INT, },
+	{ "max7313", 16 | PCA953X_INT, },
+	{ "max7315", 8  | PCA953X_INT, },
+	{ "pca6107", 8  | PCA953X_INT, },
+	{ "tca6408", 8  | PCA953X_INT, },
+	{ "tca6416", 16 | PCA953X_INT, },
 	/* NYET:  { "tca6424", 24, }, */
 	{ }
 };
@@ -53,6 +60,15 @@ struct pca953x_chip {
 	uint16_t reg_output;
 	uint16_t reg_direction;
 
+	struct mutex irq_lock;
+	uint16_t irq_interrupt;
+	uint16_t irq_mask;
+	uint16_t irq_stat;
+	uint16_t irq_trig_raise;
+	uint16_t irq_trig_fall;
+
+	int	 irq_base;
+
 	struct i2c_client *client;
 	struct pca953x_platform_data *dyn_pdata;
 	struct gpio_chip gpio_chip;
@@ -202,6 +218,121 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 	gc->names = chip->names;
 }
 
+static int pca953x_gpio_to_irq(struct gpio_chip *gc, unsigned off)
+{
+	struct pca953x_chip *chip;
+
+	chip = container_of(gc, struct pca953x_chip, gpio_chip);
+	if (chip->irq_interrupt & (1 << off))
+		return chip->irq_base + off;
+
+	return -ENXIO;
+}
+
+static void pca953x_irq_mask(unsigned int irq)
+{
+	struct pca953x_chip *chip = get_irq_chip_data(irq);
+
+	chip->irq_mask &= ~(1 << (irq - chip->irq_base));
+}
+
+static void pca953x_irq_unmask(unsigned int irq)
+{
+	struct pca953x_chip *chip = get_irq_chip_data(irq);
+
+	chip->irq_mask |= 1 << (irq - chip->irq_base);
+}
+
+static void pca953x_irq_bus_lock(unsigned int irq)
+{
+	struct pca953x_chip *chip = get_irq_chip_data(irq);
+
+	mutex_lock(&chip->irq_lock);
+}
+
+static void pca953x_irq_bus_sync_unlock(unsigned int irq)
+{
+	struct pca953x_chip *chip = get_irq_chip_data(irq);
+
+	mutex_unlock(&chip->irq_lock);
+}
+
+static int pca953x_irq_set_type(unsigned int irq, unsigned int type)
+{
+	struct pca953x_chip *chip = get_irq_chip_data(irq);
+	uint16_t mask = 1 << (irq - chip->irq_base);
+
+	if (!(type & IRQ_TYPE_EDGE_BOTH)) {
+		dev_err(&chip->client->dev, "unsupported irq type %d\n", type);
+		return -EINVAL;
+	}
+
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		chip->irq_trig_fall |= mask;
+	else
+		chip->irq_trig_fall &= ~mask;
+
+	if (type & IRQ_TYPE_EDGE_RISING)
+		chip->irq_trig_raise |= mask;
+	else
+		chip->irq_trig_raise &= ~mask;
+
+	return 0;
+}
+
+static struct irq_chip pca953x_irq_chip = {
+	.name			= "pca953x",
+	.mask			= pca953x_irq_mask,
+	.unmask			= pca953x_irq_unmask,
+	.bus_lock		= pca953x_irq_bus_lock,
+	.bus_sync_unlock	= pca953x_irq_bus_sync_unlock,
+	.set_type		= pca953x_irq_set_type,
+};
+
+static irqreturn_t pca953x_irq_handler(int irq, void *devid)
+{
+	struct pca953x_chip *chip = devid;
+	uint16_t cur_stat;
+	uint16_t old_stat;
+	uint16_t pending;
+	int ret;
+
+	ret = pca953x_read_reg(chip, PCA953X_INPUT, &cur_stat);
+	if (ret)
+		return IRQ_HANDLED;
+
+	cur_stat &= (chip->irq_interrupt & chip->reg_direction);
+	old_stat = chip->irq_stat;
+	pending = (cur_stat ^ old_stat) & chip->irq_mask;
+
+	if (!pending)
+		return IRQ_HANDLED;
+
+	chip->irq_stat = cur_stat;
+
+	do {
+		uint16_t level;
+		uint16_t level_mask;
+		int trigger = 0;
+
+		level = __ffs(pending);
+		level_mask = 1 << level;
+
+		if (old_stat & level_mask & chip->irq_trig_fall)
+			trigger = 1;
+
+		if (cur_stat & level_mask & chip->irq_trig_raise)
+			trigger = 1;
+
+		if (trigger)
+			handle_nested_irq(level + chip->irq_base);
+
+		pending &= ~level_mask;
+	} while (pending);
+
+	return IRQ_HANDLED;
+}
+
 /*
  * Handlers for alternative sources of platform_data
  */
@@ -286,7 +417,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 	/* initialize cached registers from their original values.
 	 * we can't share this chip with another i2c master.
 	 */
-	pca953x_setup_gpio(chip, id->driver_data);
+	pca953x_setup_gpio(chip, id->driver_data & PCA953X_GPIOS);
 
 	ret = pca953x_read_reg(chip, PCA953X_OUTPUT, &chip->reg_output);
 	if (ret)
@@ -301,10 +432,57 @@ static int __devinit pca953x_probe(struct i2c_client *client,
 	if (ret)
 		goto out_failed;
 
+	if (pdata->interrupt && (id->driver_data & PCA953X_INT)) {
+		int lvl;
+
+		ret = pca953x_read_reg(chip, PCA953X_INPUT,
+				       &chip->irq_stat);
+		if (ret)
+			goto out_failed;
+
+		/*
+		 * There is no way to know which GPIO line generated the
+		 * interrupt.  We have to rely on the previous read for
+		 * this purpose.
+		 */
+		chip->irq_stat &= chip->irq_interrupt & chip->reg_direction;
+		chip->irq_interrupt = pdata->interrupt;
+		chip->irq_base = pdata->irq_base;
+		mutex_init(&chip->irq_lock);
+
+		/* FIXME: Error handling? */
+		for (lvl = 0; lvl < chip->gpio_chip.ngpio; lvl++) {
+			if (chip->irq_interrupt & (1 << lvl)) {
+				int irq = lvl + chip->irq_base;
+
+				set_irq_chip_data(irq, chip);
+				set_irq_chip_and_handler(irq, &pca953x_irq_chip,
+							 handle_edge_irq);
+				set_irq_nested_thread(irq, 1);
+				set_irq_flags(irq, IRQF_VALID);
+			}
+		}
+
+		ret = request_threaded_irq(client->irq,
+					   NULL,
+					   pca953x_irq_handler,
+					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					   dev_name(&client->dev), chip);
+		if (ret) {
+			dev_err(&client->dev, "failed to request irq %d\n",
+				client->irq);
+			goto out_failed;
+		}
+
+		chip->gpio_chip.to_irq = pca953x_gpio_to_irq;
+	}
 
 	ret = gpiochip_add(&chip->gpio_chip);
-	if (ret)
+	if (ret) {
+		if (pdata->interrupt)
+			free_irq(client->irq, chip);
 		goto out_failed;
+	}
 
 	if (pdata->setup) {
 		ret = pdata->setup(client, chip->gpio_chip.base,
@@ -345,6 +523,9 @@ static int pca953x_remove(struct i2c_client *client)
 		return ret;
 	}
 
+	if (chip->irq_interrupt)
+		free_irq(client->irq, chip);
+
 	kfree(chip->dyn_pdata);
 	kfree(chip);
 	return 0;
diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
index 81736d6..6a2c8a6 100644
--- a/include/linux/i2c/pca953x.h
+++ b/include/linux/i2c/pca953x.h
@@ -7,6 +7,11 @@ struct pca953x_platform_data {
 	/* initial polarity inversion setting */
 	uint16_t	invert;
 
+	/* input bitmask to trigger the interrupt */
+	uint16_t	interrupt;
+
+	int		irq_base;
+
 	void		*context;	/* param to setup/teardown */
 
 	int		(*setup)(struct i2c_client *client,
-- 
1.6.0.4


-- 
I'm the slime oozin' out from your TV set...

[PATCH 1/2] gpio add interrupt support on pca953x

[Eric Miao]

On Wed, Dec 23, 2009 at 3:59 AM, Marc Zyngier <maz@misterjones.org> wrote:
> On Tue, 22 Dec 2009 21:41:51 +0800
> Eric Miao <eric.y.miao@gmail.com> wrote:
>
> Eric, Haojian,
>
>> Marc Zyngier was doing the similar thing, and submitted a patch to
>> Andrew Morton already, but I prefer a full-functional IRQ chip based
>> solution, so you may want to work with him together on this.
>>
>> Marc,
>>
>> Do you have the link to your patch?
>
> Here's the current state of my tree, quickly rebased to 2.6.33-rc1.
> To summarize, the main differences with Haojian's patch:
> - uses threaded irqs/nested irqs
> - setup is slightly more complicated (interrupt source mask and base
> ?interrupt number), but more correct (no dependency on gpio_to_irq()
> ?being a linear mapping, which is not always the case)
>

Marc,

This looks good work to me.

Haojian,

Could you help review this patch and see if there's any gap that needs
to be filled to support the platforms on your side?

[PATCH 1/2] gpio add interrupt support on pca953x

[Haojian Zhuang]

On Tue, Dec 22, 2009 at 2:59 PM, Marc Zyngier <maz@misterjones.org> wrote:
> On Tue, 22 Dec 2009 21:41:51 +0800
> Eric Miao <eric.y.miao@gmail.com> wrote:
>
> Eric, Haojian,
>
>> Marc Zyngier was doing the similar thing, and submitted a patch to
>> Andrew Morton already, but I prefer a full-functional IRQ chip based
>> solution, so you may want to work with him together on this.
>>
>> Marc,
>>
>> Do you have the link to your patch?
>
> Here's the current state of my tree, quickly rebased to 2.6.33-rc1.
> To summarize, the main differences with Haojian's patch:
> - uses threaded irqs/nested irqs
> - setup is slightly more complicated (interrupt source mask and base
> ?interrupt number), but more correct (no dependency on gpio_to_irq()
> ?being a linear mapping, which is not always the case)
>
> From d8dc00d1ebedcc889c41add70cceb8f15f106261 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@misterjones.org>
> Date: Tue, 22 Dec 2009 17:38:29 +0100
> Subject: [PATCH] Add interrupt handling capability to pca953x.
>
> Most of the GPIO expanders controlled by the pca953x driver are
> able to report changes on the input pins through an *INT pin.
>
> This patch implements the irq_chip functionality (edge detection
> only).
>
> The driver has been tested on an Arcom Zeus.
>
> Signed-off-by: Marc Zyngier <maz@misterjones.org>
> ---
> ?drivers/gpio/pca953x.c ? ? ?| ?207 ++++++++++++++++++++++++++++++++++++++++---
> ?include/linux/i2c/pca953x.h | ? ?5 +
> ?2 files changed, 199 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index 6a2fb3f..06c34a0 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -14,6 +14,8 @@
> ?#include <linux/module.h>
> ?#include <linux/init.h>
> ?#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> ?#include <linux/i2c.h>
> ?#include <linux/i2c/pca953x.h>
> ?#ifdef CONFIG_OF_GPIO
> @@ -26,23 +28,28 @@
> ?#define PCA953X_INVERT ? ? ? ? 2
> ?#define PCA953X_DIRECTION ? ? ?3
>
> +#define PCA953X_GPIOS ? ? ? ? 0x00FF
> +#define PCA953X_INT ? ? ? ? ? 0x0100
> +
> ?static const struct i2c_device_id pca953x_id[] = {
> - ? ? ? { "pca9534", 8, },
> - ? ? ? { "pca9535", 16, },
> + ? ? ? { "pca9534", 8 ?| PCA953X_INT, },
> + ? ? ? { "pca9535", 16 | PCA953X_INT, },
> ? ? ? ?{ "pca9536", 4, },
> - ? ? ? { "pca9537", 4, },
> - ? ? ? { "pca9538", 8, },
> - ? ? ? { "pca9539", 16, },
> - ? ? ? { "pca9554", 8, },
> - ? ? ? { "pca9555", 16, },
> + ? ? ? { "pca9537", 4 ?| PCA953X_INT, },
> + ? ? ? { "pca9538", 8 ?| PCA953X_INT, },
> + ? ? ? { "pca9539", 16 | PCA953X_INT, },
> + ? ? ? { "pca9554", 8 ?| PCA953X_INT, },
> + ? ? ? { "pca9555", 16 | PCA953X_INT, },
> ? ? ? ?{ "pca9556", 8, },
> ? ? ? ?{ "pca9557", 8, },
>
> ? ? ? ?{ "max7310", 8, },
> - ? ? ? { "max7315", 8, },
> - ? ? ? { "pca6107", 8, },
> - ? ? ? { "tca6408", 8, },
> - ? ? ? { "tca6416", 16, },
> + ? ? ? { "max7312", 16 | PCA953X_INT, },
> + ? ? ? { "max7313", 16 | PCA953X_INT, },
> + ? ? ? { "max7315", 8 ?| PCA953X_INT, },
> + ? ? ? { "pca6107", 8 ?| PCA953X_INT, },
> + ? ? ? { "tca6408", 8 ?| PCA953X_INT, },
> + ? ? ? { "tca6416", 16 | PCA953X_INT, },
> ? ? ? ?/* NYET: ?{ "tca6424", 24, }, */
> ? ? ? ?{ }
> ?};
> @@ -53,6 +60,15 @@ struct pca953x_chip {
> ? ? ? ?uint16_t reg_output;
> ? ? ? ?uint16_t reg_direction;
>
> + ? ? ? struct mutex irq_lock;
> + ? ? ? uint16_t irq_interrupt;
> + ? ? ? uint16_t irq_mask;
> + ? ? ? uint16_t irq_stat;
> + ? ? ? uint16_t irq_trig_raise;
> + ? ? ? uint16_t irq_trig_fall;
> +
> + ? ? ? int ? ? ?irq_base;
> +
> ? ? ? ?struct i2c_client *client;
> ? ? ? ?struct pca953x_platform_data *dyn_pdata;
> ? ? ? ?struct gpio_chip gpio_chip;
> @@ -202,6 +218,121 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
> ? ? ? ?gc->names = chip->names;
> ?}
>
> +static int pca953x_gpio_to_irq(struct gpio_chip *gc, unsigned off)
> +{
> + ? ? ? struct pca953x_chip *chip;
> +
> + ? ? ? chip = container_of(gc, struct pca953x_chip, gpio_chip);
> + ? ? ? if (chip->irq_interrupt & (1 << off))
> + ? ? ? ? ? ? ? return chip->irq_base + off;
> +
> + ? ? ? return -ENXIO;
> +}
> +
> +static void pca953x_irq_mask(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? chip->irq_mask &= ~(1 << (irq - chip->irq_base));
> +}
> +
> +static void pca953x_irq_unmask(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? chip->irq_mask |= 1 << (irq - chip->irq_base);
> +}
> +
> +static void pca953x_irq_bus_lock(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? mutex_lock(&chip->irq_lock);
> +}
> +
> +static void pca953x_irq_bus_sync_unlock(unsigned int irq)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> +
> + ? ? ? mutex_unlock(&chip->irq_lock);
> +}
> +
> +static int pca953x_irq_set_type(unsigned int irq, unsigned int type)
> +{
> + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> + ? ? ? uint16_t mask = 1 << (irq - chip->irq_base);
> +
> + ? ? ? if (!(type & IRQ_TYPE_EDGE_BOTH)) {
> + ? ? ? ? ? ? ? dev_err(&chip->client->dev, "unsupported irq type %d\n", type);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? if (type & IRQ_TYPE_EDGE_FALLING)
> + ? ? ? ? ? ? ? chip->irq_trig_fall |= mask;
> + ? ? ? else
> + ? ? ? ? ? ? ? chip->irq_trig_fall &= ~mask;
> +
> + ? ? ? if (type & IRQ_TYPE_EDGE_RISING)
> + ? ? ? ? ? ? ? chip->irq_trig_raise |= mask;
> + ? ? ? else
> + ? ? ? ? ? ? ? chip->irq_trig_raise &= ~mask;
> +
> + ? ? ? return 0;
> +}
> +
> +static struct irq_chip pca953x_irq_chip = {
> + ? ? ? .name ? ? ? ? ? ? ? ? ? = "pca953x",
> + ? ? ? .mask ? ? ? ? ? ? ? ? ? = pca953x_irq_mask,
> + ? ? ? .unmask ? ? ? ? ? ? ? ? = pca953x_irq_unmask,
> + ? ? ? .bus_lock ? ? ? ? ? ? ? = pca953x_irq_bus_lock,
> + ? ? ? .bus_sync_unlock ? ? ? ?= pca953x_irq_bus_sync_unlock,
> + ? ? ? .set_type ? ? ? ? ? ? ? = pca953x_irq_set_type,
> +};
> +
> +static irqreturn_t pca953x_irq_handler(int irq, void *devid)
> +{
> + ? ? ? struct pca953x_chip *chip = devid;
> + ? ? ? uint16_t cur_stat;
> + ? ? ? uint16_t old_stat;
> + ? ? ? uint16_t pending;
> + ? ? ? int ret;
> +
> + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &cur_stat);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return IRQ_HANDLED;
> +
> + ? ? ? cur_stat &= (chip->irq_interrupt & chip->reg_direction);
> + ? ? ? old_stat = chip->irq_stat;
> + ? ? ? pending = (cur_stat ^ old_stat) & chip->irq_mask;
> +
> + ? ? ? if (!pending)
> + ? ? ? ? ? ? ? return IRQ_HANDLED;
> +
> + ? ? ? chip->irq_stat = cur_stat;
> +
> + ? ? ? do {
> + ? ? ? ? ? ? ? uint16_t level;
> + ? ? ? ? ? ? ? uint16_t level_mask;
> + ? ? ? ? ? ? ? int trigger = 0;
> +
> + ? ? ? ? ? ? ? level = __ffs(pending);
> + ? ? ? ? ? ? ? level_mask = 1 << level;
> +
> + ? ? ? ? ? ? ? if (old_stat & level_mask & chip->irq_trig_fall)
> + ? ? ? ? ? ? ? ? ? ? ? trigger = 1;
> +
> + ? ? ? ? ? ? ? if (cur_stat & level_mask & chip->irq_trig_raise)
> + ? ? ? ? ? ? ? ? ? ? ? trigger = 1;
> +
> + ? ? ? ? ? ? ? if (trigger)
> + ? ? ? ? ? ? ? ? ? ? ? handle_nested_irq(level + chip->irq_base);

I suggest not to use handle_nested_irq(). I think that we can use
generic_handle_irq() instead. The illustration is in below.

> +
> + ? ? ? ? ? ? ? pending &= ~level_mask;
> + ? ? ? } while (pending);
> +
> + ? ? ? return IRQ_HANDLED;
> +}
> +
> ?/*
> ?* Handlers for alternative sources of platform_data
> ?*/
> @@ -286,7 +417,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> ? ? ? ?/* initialize cached registers from their original values.
> ? ? ? ? * we can't share this chip with another i2c master.
> ? ? ? ? */
> - ? ? ? pca953x_setup_gpio(chip, id->driver_data);
> + ? ? ? pca953x_setup_gpio(chip, id->driver_data & PCA953X_GPIOS);
>
> ? ? ? ?ret = pca953x_read_reg(chip, PCA953X_OUTPUT, &chip->reg_output);
> ? ? ? ?if (ret)
> @@ -301,10 +432,57 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?goto out_failed;
>
> + ? ? ? if (pdata->interrupt && (id->driver_data & PCA953X_INT)) {
> + ? ? ? ? ? ? ? int lvl;
> +
> + ? ? ? ? ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&chip->irq_stat);
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? goto out_failed;
> +
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* There is no way to know which GPIO line generated the
> + ? ? ? ? ? ? ? ?* interrupt. ?We have to rely on the previous read for
> + ? ? ? ? ? ? ? ?* this purpose.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? chip->irq_stat &= chip->irq_interrupt & chip->reg_direction;

chip->irq_interrupt isn't assigned in probe function. It only results
chip->irq_stat is always zero. The IRQ event is got from comparing
between previous state and current state of input value. So it would
trigger unpected IRQ event on another input pin if one input pin
asserts IRQ.

> + ? ? ? ? ? ? ? chip->irq_interrupt = pdata->interrupt;

I think that pdata->interrupt or chip->irq_interrupt is unnecessary.
chip->irq_trigger_fall and chip->irq_trigger_rise is already enough.

> + ? ? ? ? ? ? ? chip->irq_base = pdata->irq_base;
> + ? ? ? ? ? ? ? mutex_init(&chip->irq_lock);
> +
> + ? ? ? ? ? ? ? /* FIXME: Error handling? */
> + ? ? ? ? ? ? ? for (lvl = 0; lvl < chip->gpio_chip.ngpio; lvl++) {
> + ? ? ? ? ? ? ? ? ? ? ? if (chip->irq_interrupt & (1 << lvl)) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int irq = lvl + chip->irq_base;
> +
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_chip_data(irq, chip);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_chip_and_handler(irq, &pca953x_irq_chip,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?handle_edge_irq);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_nested_thread(irq, 1);

set_irq_nested_thread(irq, 1) will only mark IRQ_NESTED_THREAD on irq
descriptor. It results that driver have to invoke request_thread_irq()
with thread_fn parameter to declare IRQ. Device could be linked to
GPIO directly or GPIO expander. We should keep to use request_irq()
for compatible. set_irq_nested_thread() shouldn't use at here.

> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_flags(irq, IRQF_VALID);
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? ret = request_threaded_irq(client->irq,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pca953x_irq_handler,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dev_name(&client->dev), chip);
> + ? ? ? ? ? ? ? if (ret) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&client->dev, "failed to request irq %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? client->irq);
> + ? ? ? ? ? ? ? ? ? ? ? goto out_failed;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? chip->gpio_chip.to_irq = pca953x_gpio_to_irq;
> + ? ? ? }
>
> ? ? ? ?ret = gpiochip_add(&chip->gpio_chip);
> - ? ? ? if (ret)
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? if (pdata->interrupt)
> + ? ? ? ? ? ? ? ? ? ? ? free_irq(client->irq, chip);
> ? ? ? ? ? ? ? ?goto out_failed;
> + ? ? ? }
>
> ? ? ? ?if (pdata->setup) {
> ? ? ? ? ? ? ? ?ret = pdata->setup(client, chip->gpio_chip.base,
> @@ -345,6 +523,9 @@ static int pca953x_remove(struct i2c_client *client)
> ? ? ? ? ? ? ? ?return ret;
> ? ? ? ?}
>
> + ? ? ? if (chip->irq_interrupt)
> + ? ? ? ? ? ? ? free_irq(client->irq, chip);
> +
> ? ? ? ?kfree(chip->dyn_pdata);
> ? ? ? ?kfree(chip);
> ? ? ? ?return 0;
> diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
> index 81736d6..6a2c8a6 100644
> --- a/include/linux/i2c/pca953x.h
> +++ b/include/linux/i2c/pca953x.h
> @@ -7,6 +7,11 @@ struct pca953x_platform_data {
> ? ? ? ?/* initial polarity inversion setting */
> ? ? ? ?uint16_t ? ? ? ?invert;
>
> + ? ? ? /* input bitmask to trigger the interrupt */
> + ? ? ? uint16_t ? ? ? ?interrupt;

interrupt field is unnecessary to the driver.
> +
> + ? ? ? int ? ? ? ? ? ? irq_base;
> +
> ? ? ? ?void ? ? ? ? ? ?*context; ? ? ? /* param to setup/teardown */
>
> ? ? ? ?int ? ? ? ? ? ? (*setup)(struct i2c_client *client,
> --
> 1.6.0.4
>
>
> --
> I'm the slime oozin' out from your TV set...
>

Hi Marc,

I appended my comments.

Thanks
Haojian

[PATCH 1/2] gpio add interrupt support on pca953x

[Marc Zyngier]

On Wed, 23 Dec 2009 03:35:57 -0500
Haojian Zhuang <haojian.zhuang@gmail.com> wrote:

[Putting Mark Brown in the loop, as wm831x is the de-facto reference]

> On Tue, Dec 22, 2009 at 2:59 PM, Marc Zyngier <maz@misterjones.org> wrote:
> > On Tue, 22 Dec 2009 21:41:51 +0800
> > Eric Miao <eric.y.miao@gmail.com> wrote:
> >
> > Eric, Haojian,
> >
> >> Marc Zyngier was doing the similar thing, and submitted a patch to
> >> Andrew Morton already, but I prefer a full-functional IRQ chip based
> >> solution, so you may want to work with him together on this.
> >>
> >> Marc,
> >>
> >> Do you have the link to your patch?
> >
> > Here's the current state of my tree, quickly rebased to 2.6.33-rc1.
> > To summarize, the main differences with Haojian's patch:
> > - uses threaded irqs/nested irqs
> > - setup is slightly more complicated (interrupt source mask and base
> > ?interrupt number), but more correct (no dependency on gpio_to_irq()
> > ?being a linear mapping, which is not always the case)
> >
> > From d8dc00d1ebedcc889c41add70cceb8f15f106261 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <maz@misterjones.org>
> > Date: Tue, 22 Dec 2009 17:38:29 +0100
> > Subject: [PATCH] Add interrupt handling capability to pca953x.
> >
> > Most of the GPIO expanders controlled by the pca953x driver are
> > able to report changes on the input pins through an *INT pin.
> >
> > This patch implements the irq_chip functionality (edge detection
> > only).
> >
> > The driver has been tested on an Arcom Zeus.
> >
> > Signed-off-by: Marc Zyngier <maz@misterjones.org>
> > ---
> > ?drivers/gpio/pca953x.c ? ? ?| ?207 ++++++++++++++++++++++++++++++++++++++++---
> > ?include/linux/i2c/pca953x.h | ? ?5 +
> > ?2 files changed, 199 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> > index 6a2fb3f..06c34a0 100644
> > --- a/drivers/gpio/pca953x.c
> > +++ b/drivers/gpio/pca953x.c
> > @@ -14,6 +14,8 @@
> > ?#include <linux/module.h>
> > ?#include <linux/init.h>
> > ?#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > ?#include <linux/i2c.h>
> > ?#include <linux/i2c/pca953x.h>
> > ?#ifdef CONFIG_OF_GPIO
> > @@ -26,23 +28,28 @@
> > ?#define PCA953X_INVERT ? ? ? ? 2
> > ?#define PCA953X_DIRECTION ? ? ?3
> >
> > +#define PCA953X_GPIOS ? ? ? ? 0x00FF
> > +#define PCA953X_INT ? ? ? ? ? 0x0100
> > +
> > ?static const struct i2c_device_id pca953x_id[] = {
> > - ? ? ? { "pca9534", 8, },
> > - ? ? ? { "pca9535", 16, },
> > + ? ? ? { "pca9534", 8 ?| PCA953X_INT, },
> > + ? ? ? { "pca9535", 16 | PCA953X_INT, },
> > ? ? ? ?{ "pca9536", 4, },
> > - ? ? ? { "pca9537", 4, },
> > - ? ? ? { "pca9538", 8, },
> > - ? ? ? { "pca9539", 16, },
> > - ? ? ? { "pca9554", 8, },
> > - ? ? ? { "pca9555", 16, },
> > + ? ? ? { "pca9537", 4 ?| PCA953X_INT, },
> > + ? ? ? { "pca9538", 8 ?| PCA953X_INT, },
> > + ? ? ? { "pca9539", 16 | PCA953X_INT, },
> > + ? ? ? { "pca9554", 8 ?| PCA953X_INT, },
> > + ? ? ? { "pca9555", 16 | PCA953X_INT, },
> > ? ? ? ?{ "pca9556", 8, },
> > ? ? ? ?{ "pca9557", 8, },
> >
> > ? ? ? ?{ "max7310", 8, },
> > - ? ? ? { "max7315", 8, },
> > - ? ? ? { "pca6107", 8, },
> > - ? ? ? { "tca6408", 8, },
> > - ? ? ? { "tca6416", 16, },
> > + ? ? ? { "max7312", 16 | PCA953X_INT, },
> > + ? ? ? { "max7313", 16 | PCA953X_INT, },
> > + ? ? ? { "max7315", 8 ?| PCA953X_INT, },
> > + ? ? ? { "pca6107", 8 ?| PCA953X_INT, },
> > + ? ? ? { "tca6408", 8 ?| PCA953X_INT, },
> > + ? ? ? { "tca6416", 16 | PCA953X_INT, },
> > ? ? ? ?/* NYET: ?{ "tca6424", 24, }, */
> > ? ? ? ?{ }
> > ?};
> > @@ -53,6 +60,15 @@ struct pca953x_chip {
> > ? ? ? ?uint16_t reg_output;
> > ? ? ? ?uint16_t reg_direction;
> >
> > + ? ? ? struct mutex irq_lock;
> > + ? ? ? uint16_t irq_interrupt;
> > + ? ? ? uint16_t irq_mask;
> > + ? ? ? uint16_t irq_stat;
> > + ? ? ? uint16_t irq_trig_raise;
> > + ? ? ? uint16_t irq_trig_fall;
> > +
> > + ? ? ? int ? ? ?irq_base;
> > +
> > ? ? ? ?struct i2c_client *client;
> > ? ? ? ?struct pca953x_platform_data *dyn_pdata;
> > ? ? ? ?struct gpio_chip gpio_chip;
> > @@ -202,6 +218,121 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
> > ? ? ? ?gc->names = chip->names;
> > ?}
> >
> > +static int pca953x_gpio_to_irq(struct gpio_chip *gc, unsigned off)
> > +{
> > + ? ? ? struct pca953x_chip *chip;
> > +
> > + ? ? ? chip = container_of(gc, struct pca953x_chip, gpio_chip);
> > + ? ? ? if (chip->irq_interrupt & (1 << off))
> > + ? ? ? ? ? ? ? return chip->irq_base + off;
> > +
> > + ? ? ? return -ENXIO;
> > +}
> > +
> > +static void pca953x_irq_mask(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? chip->irq_mask &= ~(1 << (irq - chip->irq_base));
> > +}
> > +
> > +static void pca953x_irq_unmask(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? chip->irq_mask |= 1 << (irq - chip->irq_base);
> > +}
> > +
> > +static void pca953x_irq_bus_lock(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? mutex_lock(&chip->irq_lock);
> > +}
> > +
> > +static void pca953x_irq_bus_sync_unlock(unsigned int irq)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > +
> > + ? ? ? mutex_unlock(&chip->irq_lock);
> > +}
> > +
> > +static int pca953x_irq_set_type(unsigned int irq, unsigned int type)
> > +{
> > + ? ? ? struct pca953x_chip *chip = get_irq_chip_data(irq);
> > + ? ? ? uint16_t mask = 1 << (irq - chip->irq_base);
> > +
> > + ? ? ? if (!(type & IRQ_TYPE_EDGE_BOTH)) {
> > + ? ? ? ? ? ? ? dev_err(&chip->client->dev, "unsupported irq type %d\n", type);
> > + ? ? ? ? ? ? ? return -EINVAL;
> > + ? ? ? }
> > +
> > + ? ? ? if (type & IRQ_TYPE_EDGE_FALLING)
> > + ? ? ? ? ? ? ? chip->irq_trig_fall |= mask;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? chip->irq_trig_fall &= ~mask;
> > +
> > + ? ? ? if (type & IRQ_TYPE_EDGE_RISING)
> > + ? ? ? ? ? ? ? chip->irq_trig_raise |= mask;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? chip->irq_trig_raise &= ~mask;
> > +
> > + ? ? ? return 0;
> > +}
> > +
> > +static struct irq_chip pca953x_irq_chip = {
> > + ? ? ? .name ? ? ? ? ? ? ? ? ? = "pca953x",
> > + ? ? ? .mask ? ? ? ? ? ? ? ? ? = pca953x_irq_mask,
> > + ? ? ? .unmask ? ? ? ? ? ? ? ? = pca953x_irq_unmask,
> > + ? ? ? .bus_lock ? ? ? ? ? ? ? = pca953x_irq_bus_lock,
> > + ? ? ? .bus_sync_unlock ? ? ? ?= pca953x_irq_bus_sync_unlock,
> > + ? ? ? .set_type ? ? ? ? ? ? ? = pca953x_irq_set_type,
> > +};
> > +
> > +static irqreturn_t pca953x_irq_handler(int irq, void *devid)
> > +{
> > + ? ? ? struct pca953x_chip *chip = devid;
> > + ? ? ? uint16_t cur_stat;
> > + ? ? ? uint16_t old_stat;
> > + ? ? ? uint16_t pending;
> > + ? ? ? int ret;
> > +
> > + ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT, &cur_stat);
> > + ? ? ? if (ret)
> > + ? ? ? ? ? ? ? return IRQ_HANDLED;
> > +
> > + ? ? ? cur_stat &= (chip->irq_interrupt & chip->reg_direction);
> > + ? ? ? old_stat = chip->irq_stat;
> > + ? ? ? pending = (cur_stat ^ old_stat) & chip->irq_mask;
> > +
> > + ? ? ? if (!pending)
> > + ? ? ? ? ? ? ? return IRQ_HANDLED;
> > +
> > + ? ? ? chip->irq_stat = cur_stat;
> > +
> > + ? ? ? do {
> > + ? ? ? ? ? ? ? uint16_t level;
> > + ? ? ? ? ? ? ? uint16_t level_mask;
> > + ? ? ? ? ? ? ? int trigger = 0;
> > +
> > + ? ? ? ? ? ? ? level = __ffs(pending);
> > + ? ? ? ? ? ? ? level_mask = 1 << level;
> > +
> > + ? ? ? ? ? ? ? if (old_stat & level_mask & chip->irq_trig_fall)
> > + ? ? ? ? ? ? ? ? ? ? ? trigger = 1;
> > +
> > + ? ? ? ? ? ? ? if (cur_stat & level_mask & chip->irq_trig_raise)
> > + ? ? ? ? ? ? ? ? ? ? ? trigger = 1;
> > +
> > + ? ? ? ? ? ? ? if (trigger)
> > + ? ? ? ? ? ? ? ? ? ? ? handle_nested_irq(level + chip->irq_base);
> 
> I suggest not to use handle_nested_irq(). I think that we can use
> generic_handle_irq() instead. The illustration is in below.
> 
> > +
> > + ? ? ? ? ? ? ? pending &= ~level_mask;
> > + ? ? ? } while (pending);
> > +
> > + ? ? ? return IRQ_HANDLED;
> > +}
> > +
> > ?/*
> > ?* Handlers for alternative sources of platform_data
> > ?*/
> > @@ -286,7 +417,7 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> > ? ? ? ?/* initialize cached registers from their original values.
> > ? ? ? ? * we can't share this chip with another i2c master.
> > ? ? ? ? */
> > - ? ? ? pca953x_setup_gpio(chip, id->driver_data);
> > + ? ? ? pca953x_setup_gpio(chip, id->driver_data & PCA953X_GPIOS);
> >
> > ? ? ? ?ret = pca953x_read_reg(chip, PCA953X_OUTPUT, &chip->reg_output);
> > ? ? ? ?if (ret)
> > @@ -301,10 +432,57 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> > ? ? ? ?if (ret)
> > ? ? ? ? ? ? ? ?goto out_failed;
> >
> > + ? ? ? if (pdata->interrupt && (id->driver_data & PCA953X_INT)) {
> > + ? ? ? ? ? ? ? int lvl;
> > +
> > + ? ? ? ? ? ? ? ret = pca953x_read_reg(chip, PCA953X_INPUT,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&chip->irq_stat);
> > + ? ? ? ? ? ? ? if (ret)
> > + ? ? ? ? ? ? ? ? ? ? ? goto out_failed;
> > +
> > + ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ?* There is no way to know which GPIO line generated the
> > + ? ? ? ? ? ? ? ?* interrupt. ?We have to rely on the previous read for
> > + ? ? ? ? ? ? ? ?* this purpose.
> > + ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? chip->irq_stat &= chip->irq_interrupt & chip->reg_direction;
> 
> chip->irq_interrupt isn't assigned in probe function. It only results
> chip->irq_stat is always zero. The IRQ event is got from comparing
> between previous state and current state of input value. So it would
> trigger unpected IRQ event on another input pin if one input pin
> asserts IRQ.

Nice catch, will fix. Actually, as you mentioned, we can probably drop
the feature altogether.

> > + ? ? ? ? ? ? ? chip->irq_interrupt = pdata->interrupt;
> 
> I think that pdata->interrupt or chip->irq_interrupt is unnecessary.
> chip->irq_trigger_fall and chip->irq_trigger_rise is already enough.

Yes, this is a leftover from the previous version, where you could
selectively chose which input pin would we used as an interrupt.
 
> > + ? ? ? ? ? ? ? chip->irq_base = pdata->irq_base;
> > + ? ? ? ? ? ? ? mutex_init(&chip->irq_lock);
> > +
> > + ? ? ? ? ? ? ? /* FIXME: Error handling? */
> > + ? ? ? ? ? ? ? for (lvl = 0; lvl < chip->gpio_chip.ngpio; lvl++) {
> > + ? ? ? ? ? ? ? ? ? ? ? if (chip->irq_interrupt & (1 << lvl)) {
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int irq = lvl + chip->irq_base;
> > +
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_chip_data(irq, chip);
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_chip_and_handler(irq, &pca953x_irq_chip,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?handle_edge_irq);
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_nested_thread(irq, 1);
> 
> set_irq_nested_thread(irq, 1) will only mark IRQ_NESTED_THREAD on irq
> descriptor. It results that driver have to invoke request_thread_irq()
> with thread_fn parameter to declare IRQ. Device could be linked to
> GPIO directly or GPIO expander. We should keep to use request_irq()
> for compatible. set_irq_nested_thread() shouldn't use at here.

While I fully understand your point, I'm more worried about the
correctness of the approach. I'm under the impression that the use of
IRQ_NESTED_THREAD is mandatory if we're using a threaded handler.

@Mark Brown: what is your take on this? Is it possible to use
generic_handle_irq() from within a threaded handler?

> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_irq_flags(irq, IRQF_VALID);
> > + ? ? ? ? ? ? ? ? ? ? ? }
> > + ? ? ? ? ? ? ? }
> > +
> > + ? ? ? ? ? ? ? ret = request_threaded_irq(client->irq,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pca953x_irq_handler,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dev_name(&client->dev), chip);
> > + ? ? ? ? ? ? ? if (ret) {
> > + ? ? ? ? ? ? ? ? ? ? ? dev_err(&client->dev, "failed to request irq %d\n",
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? client->irq);
> > + ? ? ? ? ? ? ? ? ? ? ? goto out_failed;
> > + ? ? ? ? ? ? ? }
> > +
> > + ? ? ? ? ? ? ? chip->gpio_chip.to_irq = pca953x_gpio_to_irq;
> > + ? ? ? }
> >
> > ? ? ? ?ret = gpiochip_add(&chip->gpio_chip);
> > - ? ? ? if (ret)
> > + ? ? ? if (ret) {
> > + ? ? ? ? ? ? ? if (pdata->interrupt)
> > + ? ? ? ? ? ? ? ? ? ? ? free_irq(client->irq, chip);
> > ? ? ? ? ? ? ? ?goto out_failed;
> > + ? ? ? }
> >
> > ? ? ? ?if (pdata->setup) {
> > ? ? ? ? ? ? ? ?ret = pdata->setup(client, chip->gpio_chip.base,
> > @@ -345,6 +523,9 @@ static int pca953x_remove(struct i2c_client *client)
> > ? ? ? ? ? ? ? ?return ret;
> > ? ? ? ?}
> >
> > + ? ? ? if (chip->irq_interrupt)
> > + ? ? ? ? ? ? ? free_irq(client->irq, chip);
> > +
> > ? ? ? ?kfree(chip->dyn_pdata);
> > ? ? ? ?kfree(chip);
> > ? ? ? ?return 0;
> > diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
> > index 81736d6..6a2c8a6 100644
> > --- a/include/linux/i2c/pca953x.h
> > +++ b/include/linux/i2c/pca953x.h
> > @@ -7,6 +7,11 @@ struct pca953x_platform_data {
> > ? ? ? ?/* initial polarity inversion setting */
> > ? ? ? ?uint16_t ? ? ? ?invert;
> >
> > + ? ? ? /* input bitmask to trigger the interrupt */
> > + ? ? ? uint16_t ? ? ? ?interrupt;
> 
> interrupt field is unnecessary to the driver.
> > +
> > + ? ? ? int ? ? ? ? ? ? irq_base;
> > +
> > ? ? ? ?void ? ? ? ? ? ?*context; ? ? ? /* param to setup/teardown */
> >
> > ? ? ? ?int ? ? ? ? ? ? (*setup)(struct i2c_client *client,
> > --
> > 1.6.0.4
> >
> >
> > --
> > I'm the slime oozin' out from your TV set...
> >
> 
> Hi Marc,
> 
> I appended my comments.
> 
> Thanks
> Haojian
> 


-- 
Fast. Cheap. Reliable. Pick two.

[PATCH 1/2] gpio add interrupt support on pca953x

[Mark Brown]

On Wed, Dec 23, 2009 at 12:18:25PM +0100, Marc Zyngier wrote:
> On Wed, 23 Dec 2009 03:35:57 -0500
> Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
> > On Tue, Dec 22, 2009 at 2:59 PM, Marc Zyngier <maz@misterjones.org> wrote:

> [Putting Mark Brown in the loop, as wm831x is the de-facto reference]

Guys, could you please remember to cut unneeded context from replies?

> > set_irq_nested_thread(irq, 1) will only mark IRQ_NESTED_THREAD on irq
> > descriptor. It results that driver have to invoke request_thread_irq()
> > with thread_fn parameter to declare IRQ. Device could be linked to
> > GPIO directly or GPIO expander. We should keep to use request_irq()
> > for compatible. set_irq_nested_thread() shouldn't use at here.

> While I fully understand your point, I'm more worried about the
> correctness of the approach. I'm under the impression that the use of
> IRQ_NESTED_THREAD is mandatory if we're using a threaded handler.

> @Mark Brown: what is your take on this? Is it possible to use
> generic_handle_irq() from within a threaded handler?

ICBW but I believe you'll find that you get into trouble with lockdep if
try to do that - the TWL4030 drivers in current mainline jump through
some hoops in the client drivers as a result of this if you want to look
at some examples.  Once you're in a threaded IRQ you're in a slightly
different context and need to take account of that.

To the extent that this proves to be a problem in practical systems it's
something that needs to be addressed in the genirq core rather than
trying to jump through hoops in the individual drivers.  Trying to work
around the genirq core is likely to be fragile for both the interrupt
controller driver and the client drivers trying to use it.

[PATCH 1/2] gpio add interrupt support on pca953x

[Haojian Zhuang]

On Wed, Dec 23, 2009 at 9:29 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 23, 2009 at 12:18:25PM +0100, Marc Zyngier wrote:
>> On Wed, 23 Dec 2009 03:35:57 -0500
>> Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
>> > On Tue, Dec 22, 2009 at 2:59 PM, Marc Zyngier <maz@misterjones.org> wrote:
>
>> [Putting Mark Brown in the loop, as wm831x is the de-facto reference]
>
> Guys, could you please remember to cut unneeded context from replies?
>
>> > set_irq_nested_thread(irq, 1) will only mark IRQ_NESTED_THREAD on irq
>> > descriptor. It results that driver have to invoke request_thread_irq()
>> > with thread_fn parameter to declare IRQ. Device could be linked to
>> > GPIO directly or GPIO expander. We should keep to use request_irq()
>> > for compatible. set_irq_nested_thread() shouldn't use at here.
>
>> While I fully understand your point, I'm more worried about the
>> correctness of the approach. I'm under the impression that the use of
>> IRQ_NESTED_THREAD is mandatory if we're using a threaded handler.
>
>> @Mark Brown: what is your take on this? Is it possible to use
>> generic_handle_irq() from within a threaded handler?
>
> ICBW but I believe you'll find that you get into trouble with lockdep if
> try to do that - the TWL4030 drivers in current mainline jump through
> some hoops in the client drivers as a result of this if you want to look
> at some examples. ?Once you're in a threaded IRQ you're in a slightly
> different context and need to take account of that.
>

So do you mean that we should disable IRQ in the threaded IRQ context?

> To the extent that this proves to be a problem in practical systems it's
> something that needs to be addressed in the genirq core rather than
> trying to jump through hoops in the individual drivers. ?Trying to work
> around the genirq core is likely to be fragile for both the interrupt
> controller driver and the client drivers trying to use it.
>

[PATCH 1/2] gpio add interrupt support on pca953x

[Mark Brown]

On Wed, Dec 23, 2009 at 09:48:07PM +0800, Haojian Zhuang wrote:
> On Wed, Dec 23, 2009 at 9:29 PM, Mark Brown

> >> While I fully understand your point, I'm more worried about the
> >> correctness of the approach. I'm under the impression that the use of
> >> IRQ_NESTED_THREAD is mandatory if we're using a threaded handler.

> >> @Mark Brown: what is your take on this? Is it possible to use
> >> generic_handle_irq() from within a threaded handler?

> > ICBW but I believe you'll find that you get into trouble with lockdep if
> > try to do that - the TWL4030 drivers in current mainline jump through
> > some hoops in the client drivers as a result of this if you want to look
> > at some examples. ?Once you're in a threaded IRQ you're in a slightly
> > different context and need to take account of that.

> So do you mean that we should disable IRQ in the threaded IRQ context?

I'm not sure exactly what makes you say that but if you're using the
genirq infrastructure then it should do the right thing for you here.
TWL4030 wasn't doing that (it predates genirq supporting I2C devices)
which was why it was running into trouble.