Messages in this thread |  | | | From | David Brownell <> | | Subject | Re: [PATCH] gpio: pca953x: add irq_chip for irq demuxing | | Date | Fri, 3 Jul 2009 09:53:53 -0700 |
On Friday 03 July 2009, Alek Du wrote: > From 02227060687d8ce8254714d9812e19b815463dd4 Mon Sep 17 00:00:00 2001 > From: Alek Du <alek.du@intel.com> > 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 <alek.du@intel.com> > CC: David Brownell <david-b@pacbell.net> > --- > 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 > >
|  |