[PATCH 2.6.24-rc5-mm 2/3] gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander

[PATCH 2.6.24-rc5-mm 2/3] gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander

[eric miao]

>From b45be77acbf592b9c2085ed03ab5f16d780fa8c7 Mon Sep 17 00:00:00 2001
From: eric miao <eric.miao@marvell.com>
Date: Mon, 10 Dec 2007 17:24:36 +0800
Subject: [PATCH] gpiolib: add Generic IRQ support for 16-bit PCA9539
GPIO expander

This patch adds the generic IRQ support for the PCA9539 on-chip GPIOs.

Note: due to the inaccessibility of the generic IRQ code within modules,
this support is only available if the driver is built-in.

Signed-off-by: eric miao <eric.miao@marvell.com>
---
 drivers/gpio/Kconfig   |   11 +++-
 drivers/gpio/pca9539.c |  183 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 193 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4b54f60..a4f89a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -17,7 +17,16 @@ config GPIO_PCA9539
 	  parts are made by NXP and TI.

 	  This driver can also be built as a module.  If so, the module
-	  will be called pca9539.
+	  will be called pca9539.  Note: the Generic IRQ support for the
+	  chip will only be available if the driver is built-in
+
+config GPIO_PCA9539_GENERIC_IRQ
+	bool "Generic IRQ support for PCA9539"
+	depends on GPIO_PCA9539=y && GENERIC_HARDIRQS
+	help
+	  Say yes here to support the Generic IRQ for the PCA9539 on-chip
+	  GPIO lines. Only pin-changed IRQs (IRQ_TYPE_EDGE_BOTH) are
+	  supported in hardware.

 config GPIO_PCF857X
 	tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca9539.c
index 955d891..ad3267b 100644
--- a/drivers/gpio/pca9539.c
+++ b/drivers/gpio/pca9539.c
@@ -14,6 +14,9 @@

 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pca9539.h>

@@ -33,6 +36,22 @@ struct pca9539_chip {

 	struct i2c_client *client;
 	struct gpio_chip gpio_chip;
+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+	/*
+	 * Note: Generic IRQ is not accessible within module code, the IRQ
+	 * support will thus _only_ be available if the driver is built-in
+	 */
+	int irq;	/* IRQ for the chip itself */
+	int irq_start;	/* starting IRQ for the on-chip GPIO lines */
+
+	uint16_t irq_mask;
+	uint16_t irq_falling_edge;
+	uint16_t irq_rising_edge;
+	uint16_t last_input;
+
+	struct irq_chip irq_chip;
+	struct work_struct irq_work;
+#endif
 };

 static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
@@ -155,6 +174,156 @@ static int pca9539_init_gpio(struct pca9539_chip *chip)
 	return gpiochip_add(gc);
 }

+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+/* FIXME: change to schedule_delayed_work() here if reading out of
+ * registers does not reflect the actual pin levels
+ */
+
+static void pca9539_irq_work(struct work_struct *work)
+{
+	struct pca9539_chip *chip;
+	uint16_t input, mask, rising, falling;
+	int ret, i;
+
+	chip = container_of(work, struct pca9539_chip, irq_work);
+
+	ret = pca9539_read_reg(chip, PCA9539_INPUT, &input);
+	if (ret < 0)
+		return;
+
+	mask = (input ^ chip->last_input) & chip->irq_mask;
+	rising = (input & mask) & chip->irq_rising_edge;
+	falling = (~input & mask) & chip->irq_falling_edge;
+
+	irq_enter();
+
+	for (i = 0; i < NR_PCA9539_GPIOS; i++) {
+		if ((rising | falling) & (1u << i)) {
+			int irq = chip->irq_start + i;
+			struct irq_desc *desc;
+
+			desc = irq_desc + irq;
+			desc_handle_irq(irq, desc);
+		}
+	}
+
+	irq_exit();
+
+	chip->last_input = input;
+}
+
+static void fastcall
+pca9539_irq_demux(unsigned int irq, struct irq_desc *desc)
+{
+	struct pca9539_chip *chip = desc->handler_data;
+
+	desc->chip->mask(chip->irq);
+	desc->chip->ack(chip->irq);
+	schedule_work(&chip->irq_work);
+	desc->chip->unmask(chip->irq);
+}
+
+static void pca9539_irq_mask(unsigned int irq)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+
+	chip->irq_mask &= ~(1u << (irq - chip->irq_start));
+}
+
+static void pca9539_irq_unmask(unsigned int irq)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+
+	chip->irq_mask |= 1u << (irq - chip->irq_start);
+}
+
+static void pca9539_irq_ack(unsigned int irq)
+{
+	/* unfortunately, we have to provide an empty irq_chip.ack even
+	 * if we do nothing here, Generic IRQ will complain otherwise
+	 */
+}
+
+static int pca9539_irq_set_type(unsigned int irq, unsigned int type)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+	uint16_t mask = 1u << (irq - chip->irq_start);
+
+	if (type == IRQT_PROBE) {
+		if ((mask & chip->irq_rising_edge) ||
+		    (mask & chip->irq_falling_edge) ||
+		    (mask & ~chip->reg_direction))
+			return 0;
+
+		type = __IRQT_RISEDGE | __IRQT_FALEDGE;
+	}
+
+	gpio_direction_input(irq_to_gpio(irq));
+
+	if (type & __IRQT_RISEDGE)
+		chip->irq_rising_edge |= mask;
+	else
+		chip->irq_rising_edge &= ~mask;
+
+	if (type & __IRQT_FALEDGE)
+		chip->irq_falling_edge |= mask;
+	else
+		chip->irq_falling_edge &= ~mask;
+
+	return 0;
+}
+
+static int pca9539_init_irq(struct pca9539_chip *chip)
+{
+	struct irq_chip *ic = &chip->irq_chip;
+	int irq, irq_start;
+
+	/* initial input register value for IRQ level change detection */
+	pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);
+
+	chip->irq = chip->client->irq;
+	chip->irq_start = irq_start = gpio_to_irq(chip->gpio_start);
+
+	/* do not install GPIO interrupts for the chip if
+	 * 1. the PCA9539 interrupt line is not used
+	 * 2. or the GPIO interrupt number exceeds NR_IRQS
+	 */
+	if (chip->irq <= 0 || irq_start + NR_PCA9539_GPIOS >= NR_IRQS)
+		return -EINVAL;
+
+	chip->irq_mask	= 0;
+	chip->irq_rising_edge  = 0;
+	chip->irq_falling_edge = 0;
+
+	ic->ack = pca9539_irq_ack;
+	ic->mask = pca9539_irq_mask;
+	ic->unmask = pca9539_irq_unmask;
+	ic->set_type = pca9539_irq_set_type;
+
+	for (irq = irq_start; irq < irq_start + NR_PCA9539_GPIOS; irq++) {
+		set_irq_chip(irq, ic);
+		set_irq_chip_data(irq, chip);
+		set_irq_handler(irq, handle_edge_irq);
+		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+	}
+
+	set_irq_type(chip->irq, IRQT_FALLING);
+	set_irq_data(chip->irq, chip);
+	set_irq_chained_handler(chip->irq, pca9539_irq_demux);
+
+	INIT_WORK(&chip->irq_work, pca9539_irq_work);
+	return 0;
+}
+#else
+static inline int pca9539_init_irq(struct pca9539_chip *chip)
+{
+	return 0;
+}
+#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */
+
 static int __devinit pca9539_probe(struct i2c_client *client)
 {
 	struct pca9539_platform_data *pdata;
@@ -191,6 +360,12 @@ static int __devinit pca9539_probe(struct
i2c_client *client)
 			dev_dbg(&client->dev, "setup failed, %d\n", ret);
 	}

+	ret = pca9539_init_irq(chip);
+	if (ret) {
+		ret = gpiochip_remove(&chip->gpio_chip);
+		goto out_failed;
+	}
+
 	i2c_set_clientdata(client, chip);
 	return 0;

@@ -199,6 +374,13 @@ out_failed:
 	return ret;
 }

+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+static int pca9539_remove(struct i2c_client *client)
+{
+	dev_err(&client->dev, "failed to unload driver with IRQ support\n");
+	return -EINVAL;
+}
+#else
 static int pca9539_remove(struct i2c_client *client)
 {
 	struct pca9539_platform_data *pdata = client->dev.platform_data;
@@ -221,6 +403,7 @@ static int pca9539_remove(struct i2c_client *client)
 	kfree(chip);
 	return 0;
 }
+#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */

 static struct i2c_driver pca9539_driver = {
 	.driver = {
-- 
1.5.2.5.GIT



-- 
Cheers
- eric

Re: [PATCH 2.6.24-rc5-mm 2/3] gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander

[eric miao]

[updated according to David's suggestion to handle the error
of I2C transfer]

>From c9b78718488dadc702f40789bd532d1f1765d76e Mon Sep 17 00:00:00 2001
From: eric miao <eric.miao@marvell.com>
Date: Mon, 10 Dec 2007 17:24:36 +0800
Subject: [PATCH] gpiolib: add Generic IRQ support for 16-bit PCA9539
GPIO expander

This patch adds the generic IRQ support for the PCA9539 on-chip GPIOs.

Note: due to the inaccessibility of the generic IRQ code within modules,
this support is only available if the driver is built-in.

Signed-off-by: eric miao <eric.miao@marvell.com>
---
 drivers/gpio/Kconfig   |   11 +++-
 drivers/gpio/pca9539.c |  185 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4b54f60..a4f89a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -17,7 +17,16 @@ config GPIO_PCA9539
 	  parts are made by NXP and TI.

 	  This driver can also be built as a module.  If so, the module
-	  will be called pca9539.
+	  will be called pca9539.  Note: the Generic IRQ support for the
+	  chip will only be available if the driver is built-in
+
+config GPIO_PCA9539_GENERIC_IRQ
+	bool "Generic IRQ support for PCA9539"
+	depends on GPIO_PCA9539=y && GENERIC_HARDIRQS
+	help
+	  Say yes here to support the Generic IRQ for the PCA9539 on-chip
+	  GPIO lines. Only pin-changed IRQs (IRQ_TYPE_EDGE_BOTH) are
+	  supported in hardware.

 config GPIO_PCF857X
 	tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders"
diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca9539.c
index fc8bee4..10f9549 100644
--- a/drivers/gpio/pca9539.c
+++ b/drivers/gpio/pca9539.c
@@ -14,6 +14,9 @@

 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pca9539.h>

@@ -33,6 +36,22 @@ struct pca9539_chip {

 	struct i2c_client *client;
 	struct gpio_chip gpio_chip;
+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+	/*
+	 * Note: Generic IRQ is not accessible within module code, the IRQ
+	 * support will thus _only_ be available if the driver is built-in
+	 */
+	int irq;	/* IRQ for the chip itself */
+	int irq_start;	/* starting IRQ for the on-chip GPIO lines */
+
+	uint16_t irq_mask;
+	uint16_t irq_falling_edge;
+	uint16_t irq_rising_edge;
+	uint16_t last_input;
+
+	struct irq_chip irq_chip;
+	struct work_struct irq_work;
+#endif
 };

 static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val)
@@ -155,6 +174,158 @@ static int pca9539_init_gpio(struct pca9539_chip *chip)
 	return gpiochip_add(gc);
 }

+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+/* FIXME: change to schedule_delayed_work() here if reading out of
+ * registers does not reflect the actual pin levels
+ */
+
+static void pca9539_irq_work(struct work_struct *work)
+{
+	struct pca9539_chip *chip;
+	uint16_t input, mask, rising, falling;
+	int ret, i;
+
+	chip = container_of(work, struct pca9539_chip, irq_work);
+
+	ret = pca9539_read_reg(chip, PCA9539_INPUT, &input);
+	if (ret < 0)
+		return;
+
+	mask = (input ^ chip->last_input) & chip->irq_mask;
+	rising = (input & mask) & chip->irq_rising_edge;
+	falling = (~input & mask) & chip->irq_falling_edge;
+
+	irq_enter();
+
+	for (i = 0; i < NR_PCA9539_GPIOS; i++) {
+		if ((rising | falling) & (1u << i)) {
+			int irq = chip->irq_start + i;
+			struct irq_desc *desc;
+
+			desc = irq_desc + irq;
+			desc_handle_irq(irq, desc);
+		}
+	}
+
+	irq_exit();
+
+	chip->last_input = input;
+}
+
+static void fastcall
+pca9539_irq_demux(unsigned int irq, struct irq_desc *desc)
+{
+	struct pca9539_chip *chip = desc->handler_data;
+
+	desc->chip->mask(chip->irq);
+	desc->chip->ack(chip->irq);
+	schedule_work(&chip->irq_work);
+	desc->chip->unmask(chip->irq);
+}
+
+static void pca9539_irq_mask(unsigned int irq)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+
+	chip->irq_mask &= ~(1u << (irq - chip->irq_start));
+}
+
+static void pca9539_irq_unmask(unsigned int irq)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+
+	chip->irq_mask |= 1u << (irq - chip->irq_start);
+}
+
+static void pca9539_irq_ack(unsigned int irq)
+{
+	/* unfortunately, we have to provide an empty irq_chip.ack even
+	 * if we do nothing here, Generic IRQ will complain otherwise
+	 */
+}
+
+static int pca9539_irq_set_type(unsigned int irq, unsigned int type)
+{
+	struct irq_desc *desc = irq_desc + irq;
+	struct pca9539_chip *chip = desc->chip_data;
+	uint16_t mask = 1u << (irq - chip->irq_start);
+
+	if (type == IRQT_PROBE) {
+		if ((mask & chip->irq_rising_edge) ||
+		    (mask & chip->irq_falling_edge) ||
+		    (mask & ~chip->reg_direction))
+			return 0;
+
+		type = __IRQT_RISEDGE | __IRQT_FALEDGE;
+	}
+
+	gpio_direction_input(irq_to_gpio(irq));
+
+	if (type & __IRQT_RISEDGE)
+		chip->irq_rising_edge |= mask;
+	else
+		chip->irq_rising_edge &= ~mask;
+
+	if (type & __IRQT_FALEDGE)
+		chip->irq_falling_edge |= mask;
+	else
+		chip->irq_falling_edge &= ~mask;
+
+	return 0;
+}
+
+static int pca9539_init_irq(struct pca9539_chip *chip)
+{
+	struct irq_chip *ic = &chip->irq_chip;
+	int ret, irq, irq_start;
+
+	/* initial input register value for IRQ level change detection */
+	ret = pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);
+	if (ret)
+		return -EIO;
+
+	chip->irq = chip->client->irq;
+	chip->irq_start = irq_start = gpio_to_irq(chip->gpio_start);
+
+	/* do not install GPIO interrupts for the chip if
+	 * 1. the PCA9539 interrupt line is not used
+	 * 2. or the GPIO interrupt number exceeds NR_IRQS
+	 */
+	if (chip->irq <= 0 || irq_start + NR_PCA9539_GPIOS >= NR_IRQS)
+		return -EINVAL;
+
+	chip->irq_mask	= 0;
+	chip->irq_rising_edge  = 0;
+	chip->irq_falling_edge = 0;
+
+	ic->ack = pca9539_irq_ack;
+	ic->mask = pca9539_irq_mask;
+	ic->unmask = pca9539_irq_unmask;
+	ic->set_type = pca9539_irq_set_type;
+
+	for (irq = irq_start; irq < irq_start + NR_PCA9539_GPIOS; irq++) {
+		set_irq_chip(irq, ic);
+		set_irq_chip_data(irq, chip);
+		set_irq_handler(irq, handle_edge_irq);
+		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+	}
+
+	set_irq_type(chip->irq, IRQT_FALLING);
+	set_irq_data(chip->irq, chip);
+	set_irq_chained_handler(chip->irq, pca9539_irq_demux);
+
+	INIT_WORK(&chip->irq_work, pca9539_irq_work);
+	return 0;
+}
+#else
+static inline int pca9539_init_irq(struct pca9539_chip *chip)
+{
+	return 0;
+}
+#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */
+
 static int __devinit pca9539_probe(struct i2c_client *client)
 {
 	struct pca9539_platform_data *pdata;
@@ -204,6 +375,12 @@ static int __devinit pca9539_probe(struct
i2c_client *client)
 			dev_dbg(&client->dev, "setup failed, %d\n", ret);
 	}

+	ret = pca9539_init_irq(chip);
+	if (ret) {
+		ret = gpiochip_remove(&chip->gpio_chip);
+		goto out_failed;
+	}
+
 	i2c_set_clientdata(client, chip);
 	return 0;

@@ -212,6 +389,13 @@ out_failed:
 	return ret;
 }

+#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
+static int pca9539_remove(struct i2c_client *client)
+{
+	dev_err(&client->dev, "failed to unload driver with IRQ support\n");
+	return -EINVAL;
+}
+#else
 static int pca9539_remove(struct i2c_client *client)
 {
 	struct pca9539_platform_data *pdata = client->dev.platform_data;
@@ -234,6 +418,7 @@ static int pca9539_remove(struct i2c_client *client)
 	kfree(chip);
 	return 0;
 }
+#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */

 static struct i2c_driver pca9539_driver = {
 	.driver = {
-- 
1.5.2.5.GIT

---
Cheers
- eric

Re: [PATCH 2.6.24-rc5-mm 2/3] gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander

[David Brownell]

> From: eric miao <eric.miao@marvell.com>
> 
> This patch adds the generic IRQ support for the PCA9539 on-chip GPIOs.

This one bothers me a bit on some technical grounds.  One problem is
that these chips are not designed for reliable IRQ management, so no
matter what a driver does it can't deliver that.  The other is with
respect to what this code does.


As background, here's what the TI docs say about IRQ support on this
chip.  In this area the pca953x parts closely resemble pcf857x ones,
which I've looked at.  I can say that I haven't (yet?) happened
across boards that use the IRQ mechanism on those '857x parts ...

| Interrupt (INT) Output
|
| An interrupt is generated by any rising or falling edge of the port
| inputs in the input mode.

That's an issue.  Your code is trying to emulate all three edge trigger
modes instead of just "both edges".  I've come to believe such emulation
is not a good thing, since it necessarily loses in some cases.


|                           After time, T(iv), the signal INT is valid. 
| Resetting the interrupt circuit is achieved when data on the port is
| changed to the original setting,

Another issue.  The IRQ will effectively clear by itself, leaving
no record of exactly what triggered the IRQ. 

So IRQs on this chip are problematic at the hardware level, except
as a lossy "something changed" notification to help avoid polling
the chip for the current status of input pins.


|                                    data is read from the port that
| generated the interrupt, or in a Stop event. 

Another issue.  IRQ pulses can be arbitrarily short -- maybe too short
to register at the host, given glitch removal circuitry! -- when a
host is performing I/O to the chip while the signal is being raised.


|                                               Resetting occurs in the 
| read mode at the acknowledge (ACK) bit or not acknowledge (NACK) bit
| after the falling edge of the SCL signal. In a Stop event, INT is cleared
| after the rising edge of SDA. Interrupts that occur during the ACK or
| NACK clock pulse can be lost (or be very short) due to the resetting
| of the interrupt during this pulse. Each change of the I/Os after
| resetting is detected and is transmitted as INT.
|
| Reading from or writing to another device does not affect the interrupt
| circuit, and a pin configured as an output cannot cause an interrupt.
| Changing an I/O from an output to an input may cause a false interrupt
| to occur if the state of the pin does not match the contents of the
| Input Port register. Because each 8-bit port is read independently, the
| interrupt caused by port 0 is not cleared by a read of port 1, or vice versa.
|
| INT has an open-drain structure and requires a pullup resistor to VCC.

Now, there *are* I2C GPIO expanders that have non-lossy IRQ support,
but these '857x and '953x parts don't seem to be in that category.


> ---
>  drivers/gpio/Kconfig   |   11 +++-
>  drivers/gpio/pca9539.c |  185 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+), 1 deletions(-)
> 
> 	...
>
> --- a/drivers/gpio/pca9539.c
> +++ b/drivers/gpio/pca9539.c

As to what this code does:


> @@ -155,6 +174,158 @@ static int pca9539_init_gpio(struct pca9539_chip *chip)
>  	return gpiochip_add(gc);
>  }
> 
> +#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
> +/* FIXME: change to schedule_delayed_work() here if reading out of
> + * registers does not reflect the actual pin levels
> + */

Docs say reading the input registers does reflect current signal
levels -- no IRQ latches involved.  So that FIXME should go.


> +
> +static void pca9539_irq_work(struct work_struct *work)
> +{
> +	struct pca9539_chip *chip;
> +	uint16_t input, mask, rising, falling;
> +	int ret, i;
> +
> +	chip = container_of(work, struct pca9539_chip, irq_work);
> +
> +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &input);
> +	if (ret < 0)
> +		return;
> +
> +	mask = (input ^ chip->last_input) & chip->irq_mask;
> +	rising = (input & mask) & chip->irq_rising_edge;
> +	falling = (~input & mask) & chip->irq_falling_edge;

As noted above, this stuff is all lossy.  You won't be able to
issue some IRQs that should be issued.

> +
> +	irq_enter();

I thought irq_enter/irq_exit were intended to bracket hardirq
contexts?  This isn't a hardirq context... it's a task.


> +
> +	for (i = 0; i < NR_PCA9539_GPIOS; i++) {
> +		if ((rising | falling) & (1u << i)) {
> +			int irq = chip->irq_start + i;
> +			struct irq_desc *desc;
> +
> +			desc = irq_desc + irq;
> +			desc_handle_irq(irq, desc);

But desc_handle_irq() is an obsolete ARM-only call.  Instead,
generic_handle_irq() would be better. 

And I'm sure the loop itself can be streamlined ... calculate
the mask of which GPIOs you'll issue IRQs for, then ffs() to
quickly find the bits set in that mask.  Call that IRQ, clear
that bit in the mask ... loop "while pending" not "for each IRQ".


> +		}
> +	}
> +
> +	irq_exit();
> +
> +	chip->last_input = input;

There's no locking, so this could clobber a more-current value.
Plus, the "last input" value doesn't get updated outside of
this IRQ handling logic...


> +}
> +
> +static void fastcall
> +pca9539_irq_demux(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct pca9539_chip *chip = desc->handler_data;
> +
> +	desc->chip->mask(chip->irq);
> +	desc->chip->ack(chip->irq);
> +	schedule_work(&chip->irq_work);
> +	desc->chip->unmask(chip->irq);

I've never seen a chained IRQ handler that tries to work like that!
Or for that matter, an IRQ handler for an I2C chip that doing that.

Wouldn't it be simpler to not try using the "chained" mechanism, and
instead just have an IRQ handler that disables the (parameter) IRQ
and schedules the work?  The IRQ could be re-enabled as soon as the
work task reads the GPIO input values.


> +}
> +
> +static void pca9539_irq_mask(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_desc + irq;
> +	struct pca9539_chip *chip = desc->chip_data;
> +
> +	chip->irq_mask &= ~(1u << (irq - chip->irq_start));
> +}
> +
> +static void pca9539_irq_unmask(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_desc + irq;
> +	struct pca9539_chip *chip = desc->chip_data;
> +
> +	chip->irq_mask |= 1u << (irq - chip->irq_start);
> +}

So the IRQ masking is done purely on the host side ... which is fine,
except that I don't see a way to disable those pin-changed IRQs when
none of them are used (all are masked).


> +static void pca9539_irq_ack(unsigned int irq)
> +{
> +	/* unfortunately, we have to provide an empty irq_chip.ack even
> +	 * if we do nothing here, Generic IRQ will complain otherwise
> +	 */
> +}

I'm pretty sure I've seen paths through genirq that don't require
use of such NOPs.  :)


> +static int pca9539_irq_set_type(unsigned int irq, unsigned int type)
> +{
> +	struct irq_desc *desc = irq_desc + irq;
> +	struct pca9539_chip *chip = desc->chip_data;
> +	uint16_t mask = 1u << (irq - chip->irq_start);
> +
> +	if (type == IRQT_PROBE) {

Do you really need to support IRQT_PROBE?  I've never had occasion
to use that, and couldn't swear it's implemented right here.


> +		if ((mask & chip->irq_rising_edge) ||
> +		    (mask & chip->irq_falling_edge) ||
> +		    (mask & ~chip->reg_direction))
> +			return 0;
> +
> +		type = __IRQT_RISEDGE | __IRQT_FALEDGE;
> +	}
> +
> +	gpio_direction_input(irq_to_gpio(irq));

Hmm, I'd rather see this be "if it's not an input, fail".


> +
> +	if (type & __IRQT_RISEDGE)
> +		chip->irq_rising_edge |= mask;
> +	else
> +		chip->irq_rising_edge &= ~mask;
> +
> +	if (type & __IRQT_FALEDGE)
> +		chip->irq_falling_edge |= mask;
> +	else
> +		chip->irq_falling_edge &= ~mask;

And I'd rather see this be "if it's not BOTHEDGES or NONE, fail".

At least with BOTHEDGES, the irq handlers must be prepared to
cope with some cases of seemingly-lost IRQs, but with single
edge triggering they generally expect the hardware to latch
things as usual.


> +
> +	return 0;
> +}
> +
> +static int pca9539_init_irq(struct pca9539_chip *chip)
> +{
> +	struct irq_chip *ic = &chip->irq_chip;
> +	int ret, irq, irq_start;
> +
> +	/* initial input register value for IRQ level change detection */
> +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);

... which isn't updated on most read-input-pins codepaths ...

> +	if (ret)
> +		return -EIO;

"return ret"


> +
> +	chip->irq = chip->client->irq;
> +	chip->irq_start = irq_start = gpio_to_irq(chip->gpio_start);
> +
> +	/* do not install GPIO interrupts for the chip if
> +	 * 1. the PCA9539 interrupt line is not used
> +	 * 2. or the GPIO interrupt number exceeds NR_IRQS
> +	 */
> +	if (chip->irq <= 0 || irq_start + NR_PCA9539_GPIOS >= NR_IRQS)

Also, if irq_start < 0 ...


> +		return -EINVAL;
> +
> +	chip->irq_mask	= 0;
> +	chip->irq_rising_edge  = 0;
> +	chip->irq_falling_edge = 0;
> +
> +	ic->ack = pca9539_irq_ack;
> +	ic->mask = pca9539_irq_mask;
> +	ic->unmask = pca9539_irq_unmask;
> +	ic->set_type = pca9539_irq_set_type;
> +
> +	for (irq = irq_start; irq < irq_start + NR_PCA9539_GPIOS; irq++) {
> +		set_irq_chip(irq, ic);
> +		set_irq_chip_data(irq, chip);
> +		set_irq_handler(irq, handle_edge_irq);
> +		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> +	}
> +
> +	set_irq_type(chip->irq, IRQT_FALLING);
> +	set_irq_data(chip->irq, chip);
> +	set_irq_chained_handler(chip->irq, pca9539_irq_demux);
> +
> +	INIT_WORK(&chip->irq_work, pca9539_irq_work);
> +	return 0;
> +}
> +#else
> +static inline int pca9539_init_irq(struct pca9539_chip *chip)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */
> +
>  static int __devinit pca9539_probe(struct i2c_client *client)
>  {
>  	struct pca9539_platform_data *pdata;

Re: [PATCH 2.6.24-rc5-mm 2/3] gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander

[eric miao]

> > From: eric miao <eric.miao@marvell.com>
> >
> > This patch adds the generic IRQ support for the PCA9539 on-chip GPIOs.
>
> This one bothers me a bit on some technical grounds.  One problem is
> that these chips are not designed for reliable IRQ management, so no
> matter what a driver does it can't deliver that.  The other is with
> respect to what this code does.
>
>
> As background, here's what the TI docs say about IRQ support on this
> chip.  In this area the pca953x parts closely resemble pcf857x ones,
> which I've looked at.  I can say that I haven't (yet?) happened
> across boards that use the IRQ mechanism on those '857x parts ...
>
> | Interrupt (INT) Output
> |
> | An interrupt is generated by any rising or falling edge of the port
> | inputs in the input mode.
>
> That's an issue.  Your code is trying to emulate all three edge trigger
> modes instead of just "both edges".  I've come to believe such emulation
> is not a good thing, since it necessarily loses in some cases.
>
>
> |                           After time, T(iv), the signal INT is valid.
> | Resetting the interrupt circuit is achieved when data on the port is
> | changed to the original setting,
>
> Another issue.  The IRQ will effectively clear by itself, leaving
> no record of exactly what triggered the IRQ.
>
> So IRQs on this chip are problematic at the hardware level, except
> as a lossy "something changed" notification to help avoid polling
> the chip for the current status of input pins.
>
>
> |                                    data is read from the port that
> | generated the interrupt, or in a Stop event.
>
> Another issue.  IRQ pulses can be arbitrarily short -- maybe too short
> to register at the host, given glitch removal circuitry! -- when a
> host is performing I/O to the chip while the signal is being raised.
>
>
> |                                               Resetting occurs in the
> | read mode at the acknowledge (ACK) bit or not acknowledge (NACK) bit
> | after the falling edge of the SCL signal. In a Stop event, INT is cleared
> | after the rising edge of SDA. Interrupts that occur during the ACK or
> | NACK clock pulse can be lost (or be very short) due to the resetting
> | of the interrupt during this pulse. Each change of the I/Os after
> | resetting is detected and is transmitted as INT.
> |
> | Reading from or writing to another device does not affect the interrupt
> | circuit, and a pin configured as an output cannot cause an interrupt.
> | Changing an I/O from an output to an input may cause a false interrupt
> | to occur if the state of the pin does not match the contents of the
> | Input Port register. Because each 8-bit port is read independently, the
> | interrupt caused by port 0 is not cleared by a read of port 1, or vice versa.
> |
> | INT has an open-drain structure and requires a pullup resistor to VCC.
>
> Now, there *are* I2C GPIO expanders that have non-lossy IRQ support,
> but these '857x and '953x parts don't seem to be in that category.
>

Indeed, the chip does not provide reliable enough IRQ support, so it is
designed to cope with slow signals on our system, where the code below
works OK most of the time, but this is risky.

>
> > ---
> >  drivers/gpio/Kconfig   |   11 +++-
> >  drivers/gpio/pca9539.c |  185 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 195 insertions(+), 1 deletions(-)
> >
> > 	...
> >
> > --- a/drivers/gpio/pca9539.c
> > +++ b/drivers/gpio/pca9539.c
>
> As to what this code does:
>
>
> > @@ -155,6 +174,158 @@ static int pca9539_init_gpio(struct pca9539_chip *chip)
> >  	return gpiochip_add(gc);
> >  }
> >
> > +#ifdef CONFIG_GPIO_PCA9539_GENERIC_IRQ
> > +/* FIXME: change to schedule_delayed_work() here if reading out of
> > + * registers does not reflect the actual pin levels
> > + */
>
> Docs say reading the input registers does reflect current signal
> levels -- no IRQ latches involved.  So that FIXME should go.
>

Docs don't always tell the truth, especially when the chip is connected to
different possible signals, this issue is really observed several times,
though not very frequently.  I can remove this til really necessary.

>
> > +
> > +static void pca9539_irq_work(struct work_struct *work)
> > +{
> > +	struct pca9539_chip *chip;
> > +	uint16_t input, mask, rising, falling;
> > +	int ret, i;
> > +
> > +	chip = container_of(work, struct pca9539_chip, irq_work);
> > +
> > +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &input);
> > +	if (ret < 0)
> > +		return;
> > +
> > +	mask = (input ^ chip->last_input) & chip->irq_mask;
> > +	rising = (input & mask) & chip->irq_rising_edge;
> > +	falling = (~input & mask) & chip->irq_falling_edge;
>
> As noted above, this stuff is all lossy.  You won't be able to
> issue some IRQs that should be issued.
>

Yes, and the work_queue schedule latency will make this worse, a level
change may have already gone and INT is cleared, while the code here
won't detect this.

> > +
> > +	irq_enter();
>
> I thought irq_enter/irq_exit were intended to bracket hardirq
> contexts?  This isn't a hardirq context... it's a task.
>

However, I currently have no idea if the IRQ handler will be impact by
the incorrect context.

>
> > +
> > +	for (i = 0; i < NR_PCA9539_GPIOS; i++) {
> > +		if ((rising | falling) & (1u << i)) {
> > +			int irq = chip->irq_start + i;
> > +			struct irq_desc *desc;
> > +
> > +			desc = irq_desc + irq;
> > +			desc_handle_irq(irq, desc);
>
> But desc_handle_irq() is an obsolete ARM-only call.  Instead,
> generic_handle_irq() would be better.
>

OK

> And I'm sure the loop itself can be streamlined ... calculate
> the mask of which GPIOs you'll issue IRQs for, then ffs() to
> quickly find the bits set in that mask.  Call that IRQ, clear
> that bit in the mask ... loop "while pending" not "for each IRQ".
>

OK

>
> > +		}
> > +	}
> > +
> > +	irq_exit();
> > +
> > +	chip->last_input = input;
>
> There's no locking, so this could clobber a more-current value.
> Plus, the "last input" value doesn't get updated outside of
> this IRQ handling logic...
>

well, it is assumed that any change of the input port will issue an
INT, and this block of code being invoked to record "last input"

>
> > +}
> > +
> > +static void fastcall
> > +pca9539_irq_demux(unsigned int irq, struct irq_desc *desc)
> > +{
> > +	struct pca9539_chip *chip = desc->handler_data;
> > +
> > +	desc->chip->mask(chip->irq);
> > +	desc->chip->ack(chip->irq);
> > +	schedule_work(&chip->irq_work);
> > +	desc->chip->unmask(chip->irq);
>
> I've never seen a chained IRQ handler that tries to work like that!
> Or for that matter, an IRQ handler for an I2C chip that doing that.
>
> Wouldn't it be simpler to not try using the "chained" mechanism, and
> instead just have an IRQ handler that disables the (parameter) IRQ
> and schedules the work?  The IRQ could be re-enabled as soon as the
> work task reads the GPIO input values.
>

OK.

>
> > +}
> > +
> > +static void pca9539_irq_mask(unsigned int irq)
> > +{
> > +	struct irq_desc *desc = irq_desc + irq;
> > +	struct pca9539_chip *chip = desc->chip_data;
> > +
> > +	chip->irq_mask &= ~(1u << (irq - chip->irq_start));
> > +}
> > +
> > +static void pca9539_irq_unmask(unsigned int irq)
> > +{
> > +	struct irq_desc *desc = irq_desc + irq;
> > +	struct pca9539_chip *chip = desc->chip_data;
> > +
> > +	chip->irq_mask |= 1u << (irq - chip->irq_start);
> > +}
>
> So the IRQ masking is done purely on the host side ... which is fine,
> except that I don't see a way to disable those pin-changed IRQs when
> none of them are used (all are masked).
>

even if none are used, I still have to keep the IRQ enabled to record
the "last input"...

>
> > +static void pca9539_irq_ack(unsigned int irq)
> > +{
> > +	/* unfortunately, we have to provide an empty irq_chip.ack even
> > +	 * if we do nothing here, Generic IRQ will complain otherwise
> > +	 */
> > +}
>
> I'm pretty sure I've seen paths through genirq that don't require
> use of such NOPs.  :)
>

You mean dummy_irq_chip.ack ?

>
> > +static int pca9539_irq_set_type(unsigned int irq, unsigned int type)
> > +{
> > +	struct irq_desc *desc = irq_desc + irq;
> > +	struct pca9539_chip *chip = desc->chip_data;
> > +	uint16_t mask = 1u << (irq - chip->irq_start);
> > +
> > +	if (type == IRQT_PROBE) {
>
> Do you really need to support IRQT_PROBE?  I've never had occasion
> to use that, and couldn't swear it's implemented right here.
>

I would like to remove this either :-)

>
> > +		if ((mask & chip->irq_rising_edge) ||
> > +		    (mask & chip->irq_falling_edge) ||
> > +		    (mask & ~chip->reg_direction))
> > +			return 0;
> > +
> > +		type = __IRQT_RISEDGE | __IRQT_FALEDGE;
> > +	}
> > +
> > +	gpio_direction_input(irq_to_gpio(irq));
>
> Hmm, I'd rather see this be "if it's not an input, fail".
>

OK

>
> > +
> > +	if (type & __IRQT_RISEDGE)
> > +		chip->irq_rising_edge |= mask;
> > +	else
> > +		chip->irq_rising_edge &= ~mask;
> > +
> > +	if (type & __IRQT_FALEDGE)
> > +		chip->irq_falling_edge |= mask;
> > +	else
> > +		chip->irq_falling_edge &= ~mask;
>
> And I'd rather see this be "if it's not BOTHEDGES or NONE, fail".
>
> At least with BOTHEDGES, the irq handlers must be prepared to
> cope with some cases of seemingly-lost IRQs, but with single
> edge triggering they generally expect the hardware to latch
> things as usual.
>

As you said above, even if the irq handler reads out the register
and find the IRQ being possibly lost, it is still not able to tell
which pin level change triggers this...

>
> > +
> > +	return 0;
> > +}
> > +
> > +static int pca9539_init_irq(struct pca9539_chip *chip)
> > +{
> > +	struct irq_chip *ic = &chip->irq_chip;
> > +	int ret, irq, irq_start;
> > +
> > +	/* initial input register value for IRQ level change detection */
> > +	ret = pca9539_read_reg(chip, PCA9539_INPUT, &chip->last_input);
>
> ... which isn't updated on most read-input-pins codepaths ...
>
> > +	if (ret)
> > +		return -EIO;
>
> "return ret"
>

OK

>
> > +
> > +	chip->irq = chip->client->irq;
> > +	chip->irq_start = irq_start = gpio_to_irq(chip->gpio_start);
> > +
> > +	/* do not install GPIO interrupts for the chip if
> > +	 * 1. the PCA9539 interrupt line is not used
> > +	 * 2. or the GPIO interrupt number exceeds NR_IRQS
> > +	 */
> > +	if (chip->irq <= 0 || irq_start + NR_PCA9539_GPIOS >= NR_IRQS)
>
> Also, if irq_start < 0 ...

OK

>
>
> > +		return -EINVAL;
> > +
> > +	chip->irq_mask	= 0;
> > +	chip->irq_rising_edge  = 0;
> > +	chip->irq_falling_edge = 0;
> > +
> > +	ic->ack = pca9539_irq_ack;
> > +	ic->mask = pca9539_irq_mask;
> > +	ic->unmask = pca9539_irq_unmask;
> > +	ic->set_type = pca9539_irq_set_type;
> > +
> > +	for (irq = irq_start; irq < irq_start + NR_PCA9539_GPIOS; irq++) {
> > +		set_irq_chip(irq, ic);
> > +		set_irq_chip_data(irq, chip);
> > +		set_irq_handler(irq, handle_edge_irq);
> > +		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> > +	}
> > +
> > +	set_irq_type(chip->irq, IRQT_FALLING);
> > +	set_irq_data(chip->irq, chip);
> > +	set_irq_chained_handler(chip->irq, pca9539_irq_demux);
> > +
> > +	INIT_WORK(&chip->irq_work, pca9539_irq_work);
> > +	return 0;
> > +}
> > +#else
> > +static inline int pca9539_init_irq(struct pca9539_chip *chip)
> > +{
> > +	return 0;
> > +}
> > +#endif /* CONFIG_GPIO_PCA9539_GENERIC_IRQ */
> > +
> >  static int __devinit pca9539_probe(struct i2c_client *client)
> >  {
> >  	struct pca9539_platform_data *pdata;
>

---
Cheers
- eric