>From 02227060687d8ce8254714d9812e19b815463dd4 Mon Sep 17 00:00:00 2001
From: Alek Du <[email protected]>
Date: Wed, 1 Jul 2009 17:07:00 +0800
Subject: [PATCH] gpio: pca953x: add irq_chip for irq demuxing
This patch is required when using this gpio driver with gpio_keys driver
Signed-off-by: Alek Du <[email protected]>
CC: David Brownell <[email protected]>
---
drivers/gpio/pca953x.c | 54 +++++++++++++++++++++++++++++++++++++++++++
include/linux/i2c/pca953x.h | 3 ++
2 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index 8ab1308..47c1d99 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/i2c.h>
#include <linux/i2c/pca953x.h>
#ifdef CONFIG_OF_GPIO
@@ -51,6 +52,7 @@ MODULE_DEVICE_TABLE(i2c, pca953x_id);
struct pca953x_chip {
unsigned gpio_start;
+ unsigned irq_base;
uint16_t reg_output;
uint16_t reg_direction;
@@ -183,6 +185,13 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
chip->reg_output = reg_val;
}
+static int pca953x_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct pca953x_chip *chip = container_of(gc, struct pca953x_chip,
+ gpio_chip);
+ return chip->irq_base + offset;
+}
+
static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
{
struct gpio_chip *gc;
@@ -193,6 +202,7 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
gc->direction_output = pca953x_gpio_direction_output;
gc->get = pca953x_gpio_get_value;
gc->set = pca953x_gpio_set_value;
+ gc->to_irq = pca953x_gpio_to_irq;
gc->can_sleep = 1;
gc->base = chip->gpio_start;
@@ -251,6 +261,34 @@ pca953x_get_alt_pdata(struct i2c_client *client)
}
#endif
+/* the irq_chip at least needs one handler */
+static void pca953x_irq_unmask(unsigned irq)
+{
+}
+
+static struct irq_chip pca953x_irqchip = {
+ .name = "pca953x_irqchip",
+ .unmask = pca953x_irq_unmask,
+};
+
+static void pca953x_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+ struct pca953x_chip *chip = (struct pca953x_chip *)get_irq_data(irq);
+ int i;
+
+ if (desc->chip->ack)
+ desc->chip->ack(irq);
+ /* we must call all sub-irqs, since there is no way to read
+ * I2C gpio expander's status in irq context. The driver itself
+ * would be reponsible to check if the irq is for him.
+ */
+ for (i = 0; i < chip->gpio_chip.ngpio; i++)
+ generic_handle_irq(chip->irq_base + i);
+
+ if (desc->chip->unmask)
+ desc->chip->unmask(irq);
+}
+
static int __devinit pca953x_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -284,6 +322,8 @@ static int __devinit pca953x_probe(struct i2c_client *client,
chip->names = pdata->names;
+ chip->irq_base = pdata->irq_base;
+
/* initialize cached registers from their original values.
* we can't share this chip with another i2c master.
*/
@@ -315,6 +355,20 @@ static int __devinit pca953x_probe(struct i2c_client *client,
}
i2c_set_clientdata(client, chip);
+
+ if (chip->irq_base) {
+ int i;
+
+ set_irq_type(client->irq,
+ IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING);
+ set_irq_chained_handler(client->irq, pca953x_irq_handler);
+ set_irq_data(client->irq, chip);
+ for (i = 0; i < chip->gpio_chip.ngpio; i++) {
+ set_irq_chip_and_handler_name(i + chip->irq_base,
+ &pca953x_irqchip, handle_simple_irq, "demux");
+ set_irq_chip_data(i + chip->irq_base, chip);
+ }
+ }
return 0;
out_failed:
diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
index 81736d6..bf7c0a3 100644
--- a/include/linux/i2c/pca953x.h
+++ b/include/linux/i2c/pca953x.h
@@ -4,6 +4,9 @@ struct pca953x_platform_data {
/* number of the first GPIO */
unsigned gpio_base;
+ /* number of the first IRQ */
+ unsigned irq_base;
+
/* initial polarity inversion setting */
uint16_t invert;
--
1.6.0.4
On Friday 03 July 2009, Alek Du wrote:
> From 02227060687d8ce8254714d9812e19b815463dd4 Mon Sep 17 00:00:00 2001
> From: Alek Du <[email protected]>
> Date: Wed, 1 Jul 2009 17:07:00 +0800
> Subject: [PATCH] gpio: pca953x: add irq_chip for irq demuxing
>
> This patch is required when using this gpio driver with gpio_keys driver
Two fundamental comments:
- I don't think these chips (or pcf857x chips) present the
model that irq_chip support implies ... what they expose
is more like a "poll now" hint than what a SoC-integrated
GPIO does.
More complex external chips (like mcp23s08 gpio expanders,
or some MFDs) can implement what an irq_chip impies ...
they latch status, provide pin-level control for IRQ masking,
acking, and trigger modes, etc.
- You're using a model -- need to use it! -- which has gotten
zero support or cooperation from the folk who currently
claim ownership of genirq.
Specifically: IRQ chaining through an irq_chip whose
operation requires methods that sleep.
In short, I wouldn't try to use irq_chip in these cases ...
but if you strongly believe that's the answer, doing it
"right" is going require (overdue) genirq updates.
Plus ... last time I looked, some of the procedures you
call were not available to modular drivers. Raising
two technical issues (in addition to the ones below):
- There is no code handling the case where this I2c driver
gets unbound. The relevant genirq state would need to
be cleaned up.
- This code is still kicking in for non-modular builds.
(Which could be OK if those procedures are now available
to modules.)
> Signed-off-by: Alek Du <[email protected]>
> CC: David Brownell <[email protected]>
> ---
> drivers/gpio/pca953x.c | 54 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/i2c/pca953x.h | 3 ++
> 2 files changed, 57 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
> index 8ab1308..47c1d99 100644
> --- a/drivers/gpio/pca953x.c
> +++ b/drivers/gpio/pca953x.c
> @@ -13,6 +13,7 @@
>
> #include <linux/module.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/i2c.h>
> #include <linux/i2c/pca953x.h>
> #ifdef CONFIG_OF_GPIO
> @@ -51,6 +52,7 @@ MODULE_DEVICE_TABLE(i2c, pca953x_id);
>
> struct pca953x_chip {
> unsigned gpio_start;
> + unsigned irq_base;
> uint16_t reg_output;
> uint16_t reg_direction;
>
> @@ -183,6 +185,13 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> chip->reg_output = reg_val;
> }
>
> +static int pca953x_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct pca953x_chip *chip = container_of(gc, struct pca953x_chip,
> + gpio_chip);
> + return chip->irq_base + offset;
> +}
> +
> static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
> {
> struct gpio_chip *gc;
> @@ -193,6 +202,7 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
> gc->direction_output = pca953x_gpio_direction_output;
> gc->get = pca953x_gpio_get_value;
> gc->set = pca953x_gpio_set_value;
> + gc->to_irq = pca953x_gpio_to_irq;
> gc->can_sleep = 1;
>
> gc->base = chip->gpio_start;
> @@ -251,6 +261,34 @@ pca953x_get_alt_pdata(struct i2c_client *client)
> }
> #endif
>
> +/* the irq_chip at least needs one handler */
> +static void pca953x_irq_unmask(unsigned irq)
> +{
> +}
> +
> +static struct irq_chip pca953x_irqchip = {
> + .name = "pca953x_irqchip",
Just "pca953x" would suffice. And it wouldn't cause
misdisplaying of /proc/interrupts output. :)
> + .unmask = pca953x_irq_unmask,
> +};
> +
> +static void pca953x_irq_handler(unsigned irq, struct irq_desc *desc)
> +{
> + struct pca953x_chip *chip = (struct pca953x_chip *)get_irq_data(irq);
> + int i;
> +
> + if (desc->chip->ack)
> + desc->chip->ack(irq);
This top-level IRQ chaining handler is clelarly incorrect.
Reading from a pca9539 spec I have handy:
Resetting the interrupt circuit is achieved when
data on the port is changed to the original setting,
data is read from the port that generated the
interrupt, or in a Stop event.
You're not guaranteeing *any* of those happens. Since
the chip's nINT output is level-active, it's possible
that the hardware is still raising this interrupt when
this hander returns ...
The *best* you could do here would be to ensure this
chaining handler runs in thread context, then have it
read the GPIO input port register(s) to ensure nINT
is cleared.
> + /* we must call all sub-irqs, since there is no way to read
> + * I2C gpio expander's status in irq context. The driver itself
> + * would be reponsible to check if the irq is for him.
> + */
> + for (i = 0; i < chip->gpio_chip.ngpio; i++)
> + generic_handle_irq(chip->irq_base + i);
You should only do that for pins configured as inputs.
The nIRQ signal is not triggered for changes on output pins.
Also, see the second point above. So far as I know, the
dragons guarding the genirq den are still intent on not
providing any support for chained handlers in cases like
this one...
> +
> + if (desc->chip->unmask)
> + desc->chip->unmask(irq);
> +}
> +
> static int __devinit pca953x_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -284,6 +322,8 @@ static int __devinit pca953x_probe(struct i2c_client *client,
>
> chip->names = pdata->names;
>
> + chip->irq_base = pdata->irq_base;
> +
> /* initialize cached registers from their original values.
> * we can't share this chip with another i2c master.
> */
> @@ -315,6 +355,20 @@ static int __devinit pca953x_probe(struct i2c_client *client,
> }
>
> i2c_set_clientdata(client, chip);
> +
> + if (chip->irq_base) {
> + int i;
> +
> + set_irq_type(client->irq,
> + IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING);
Won't work on all hardware. And surely you'd want just
EDGE_FALLING, so you get only half as many IRQs, if you
happen to be hooking this up to an interrupt line which
supports edge-sensitive IRQ triggers (and not through
some kind of signal inverter)?
> + set_irq_chained_handler(client->irq, pca953x_irq_handler);
I'd set the handler *last* in case one of the pins
changes between here and the time you've finished
setting up the chained-to IRQs ...
> + set_irq_data(client->irq, chip);
> + for (i = 0; i < chip->gpio_chip.ngpio; i++) {
> + set_irq_chip_and_handler_name(i + chip->irq_base,
> + &pca953x_irqchip, handle_simple_irq, "demux");
> + set_irq_chip_data(i + chip->irq_base, chip);
This is insufficient. IRQF_VALID isn't necessarily
going to be set. And ... I suppose you haven't
actually run this with LOCKDEP? If you do, you
will likely notice you need to set the lock class
for those chained-to interrupts so it's something
different from the class of the chained-from IRQ.
> + }
> + }
> return 0;
>
> out_failed:
> diff --git a/include/linux/i2c/pca953x.h b/include/linux/i2c/pca953x.h
> index 81736d6..bf7c0a3 100644
> --- a/include/linux/i2c/pca953x.h
> +++ b/include/linux/i2c/pca953x.h
> @@ -4,6 +4,9 @@ struct pca953x_platform_data {
> /* number of the first GPIO */
> unsigned gpio_base;
>
> + /* number of the first IRQ */
> + unsigned irq_base;
> +
> /* initial polarity inversion setting */
> uint16_t invert;
>
> --
> 1.6.0.4
>
>
David,
Thanks for the review.
Basically my idea is *not* actually demuxing the I2C gpio interrupts. Since the demuxing need send I2C command to the GPIO chip, which has big latency.
My idea is, when this GPIO chip hooked to an edge sensitive interrupt line, we could use this interrupt handler to call all second level IRQs. The hooked edge detect interrupt line "automatically" masks the interrupt when the GPIO pin state is not restored to original state.
>> +static void pca953x_irq_handler(unsigned irq, struct irq_desc *desc)
>> +{
>> + struct pca953x_chip *chip = (struct pca953x_chip *)get_irq_data(irq);
>> + int i;
>> +
>> + if (desc->chip->ack)
>> + desc->chip->ack(irq);
>
>This top-level IRQ chaining handler is clelarly incorrect.
>Reading from a pca9539 spec I have handy:
>
> Resetting the interrupt circuit is achieved when
> data on the port is changed to the original setting,
> data is read from the port that generated the
> interrupt, or in a Stop event.
>
>You're not guaranteeing *any* of those happens. Since
>the chip's nINT output is level-active, it's possible
>that the hardware is still raising this interrupt when
>this hander returns ...
That's why we connected the nINT output to an edge sensitive interrupt line.
The IRQ would be triggered only once if the nINT keeps level-low. The driver that requires the gpio pin IRQ needs to clear the interrupt source.
>> + /* we must call all sub-irqs, since there is no way to read
>> + * I2C gpio expander's status in irq context. The driver itself
>> + * would be reponsible to check if the irq is for him.
>> + */
>> + for (i = 0; i < chip->gpio_chip.ngpio; i++)
>> + generic_handle_irq(chip->irq_base + i);
>
>You should only do that for pins configured as inputs.
>The nIRQ signal is not triggered for changes on output pins.
Ok, but those output pins, actually nobody will install IRQ handlers for them.
>> + if (chip->irq_base) {
>> + int i;
>> +
>> + set_irq_type(client->irq,
>> + IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING);
>
>Won't work on all hardware. And surely you'd want just
>EDGE_FALLING, so you get only half as many IRQs, if you
>happen to be hooking this up to an interrupt line which
>supports edge-sensitive IRQ triggers (and not through
>some kind of signal inverter)?
I set the IRQ type to EDGE_FALLING and EDGE_RISING because the nINT signal of the GPIO is low-active. So when one of the input pins transit from high/low, it will trigger two interrupts on the hooked up interrupt line.
Thanks,
Alek