| Message ID | alpine.LFD.2.02.1109021027360.2723@ionos (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM: > On Thu, 4 Aug 2011, Stephen Warren wrote: > > > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > > it can't be used as anything else; it should be requested. Enhance the > > core interrupt code to call gpio_request() and gpio_direction_input() for > > any IRQ that is also a GPIO. This prevents duplication of these calls in > > each driver that uses an IRQ. > > This is very much the wrong approach. If you think it through then the > irq setup code might end up with tons of other subsystem specific > setup thingies, e.g. PCI ..... > > The right thing to do is to add a irq_configure() function pointer to > the irq chip and provide a common function for gpios in gpiolib, which > is then used by the particular GPIO irq chip implementation. Sorry, could you expand on this some more. The whole point of these patches is that it's impossible to convert from IRQ number to GPIO number. Some drivers use just an IRQ, and don't care if it's a GPIO. They work fine currently. Some drivers use just a GPIO; that's not relevant to these patches. Some drivers use something that's both an IRQ and a GPIO. Historically, this has worked by passing the IRQ to the driver, and then the driver calling irq_to_gpio() to get the GPIO ID. Since irq_to_gpio() is being removed, this is no longer possible. The whole point of the removal was that it's not possible in general to convert from IRQ to GPIO, so I'm not sure exactly what you're proposing irq_configure() or gpiolib do to centralize this? Thanks for any insight.
Stephen Warren wrote at Friday, September 02, 2011 9:25 AM: > Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM: > > On Thu, 4 Aug 2011, Stephen Warren wrote: > > > > > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > > > it can't be used as anything else; it should be requested. Enhance the > > > core interrupt code to call gpio_request() and gpio_direction_input() for > > > any IRQ that is also a GPIO. This prevents duplication of these calls in > > > each driver that uses an IRQ. > > > > This is very much the wrong approach. If you think it through then the > > irq setup code might end up with tons of other subsystem specific > > setup thingies, e.g. PCI ..... > > > > The right thing to do is to add a irq_configure() function pointer to > > the irq chip and provide a common function for gpios in gpiolib, which > > is then used by the particular GPIO irq chip implementation. > > Sorry, could you expand on this some more. Uggh. Sorry; just ignore this response; I got it confused with the responses to my proposed I2C changes. I've long-since dropped the patch that you responded to.
On Fri, 2 Sep 2011, Stephen Warren wrote: > Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM: > > On Thu, 4 Aug 2011, Stephen Warren wrote: > > > > > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > > > it can't be used as anything else; it should be requested. Enhance the > > > core interrupt code to call gpio_request() and gpio_direction_input() for > > > any IRQ that is also a GPIO. This prevents duplication of these calls in > > > each driver that uses an IRQ. > > > > This is very much the wrong approach. If you think it through then the > > irq setup code might end up with tons of other subsystem specific > > setup thingies, e.g. PCI ..... > > > > The right thing to do is to add a irq_configure() function pointer to > > the irq chip and provide a common function for gpios in gpiolib, which > > is then used by the particular GPIO irq chip implementation. > > Sorry, could you expand on this some more. > > The whole point of these patches is that it's impossible to convert from > IRQ number to GPIO number. > > Some drivers use just an IRQ, and don't care if it's a GPIO. They work > fine currently. > > Some drivers use just a GPIO; that's not relevant to these patches. > > Some drivers use something that's both an IRQ and a GPIO. Historically, > this has worked by passing the IRQ to the driver, and then the driver > calling irq_to_gpio() to get the GPIO ID. Since irq_to_gpio() is being > removed, this is no longer possible. The whole point of the removal was > that it's not possible in general to convert from IRQ to GPIO, so I'm not > sure exactly what you're proposing irq_configure() or gpiolib do to > centralize this? chip->irq_configure() calls a irqchip function if set. So now the code which implements the irq functionality for GPIO definitely knows how to map the irq number to the GPIO pin, otherwise it would not be able to handle the mask/ack/unmask function either. The drivers which just request an irq are oblivious about that: request_irq(irq) if (chip->irq_configure) chip->irq_configure() configure_gpio_pin() Whether configure_gpio_pin() is a function which is implemented per GPIO driver or a common function in gpiolib is not relevant for the irq core code. Thanks, tglx
diff --git a/include/linux/irq.h b/include/linux/irq.h index 5951730..33ba4b8 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -265,6 +265,7 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d) * struct irq_chip - hardware interrupt chip descriptor * * @name: name for /proc/interrupts + * @irq_configure: configure an interrupt (optional) * @irq_startup: start up the interrupt (defaults to ->enable if NULL) * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) * @irq_enable: enable the interrupt (defaults to chip->unmask if NULL) @@ -289,9 +290,14 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d) * @flags: chip specific flags * * @release: release function solely used by UML + * + * If @irq_configure is provided, it's called from setup_irq prior to + * enabling the interrupt. irq_configure should return 0 on success or + * an appropriate error code. */ struct irq_chip { const char *name; + int (*irq_configure)(struct irq_data *data); unsigned int (*irq_startup)(struct irq_data *data); void (*irq_shutdown)(struct irq_data *data); void (*irq_enable)(struct irq_data *data); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 9b956fa..d5e6a58 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -999,6 +999,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (!shared) { init_waitqueue_head(&desc->wait_for_threads); + /* Configure the interrupt */ + if (desc->chip->irq_configure) { + ret = desc->chip->irq_configure(&desc->irq_data); + if (ret) + goto out_mask; + } + /* Setup the type (level, edge polarity) if configured: */ if (new->flags & IRQF_TRIGGER_MASK) { ret = __irq_set_trigger(desc, irq,