| Message ID | CAAwP0s0e2x33jZQhFRY+Hoo61SFqYKRgWk=1mL2+DNxSDJHz+A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas <martinez.javier@gmail.com> wrote: > Is the following inlined patch [1] what you were thinking that would > be the right approach? This looks sort of OK, but I'm still struggling with the question of what we could do to help other implementations facing the same issue. This is a pretty hard design pattern to properly replicate in every such driver is it not? Hmmmm Yours, Linus Walleij
On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote: ... > Is the following inlined patch [1] what you were thinking that would > be the right approach? ... > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c ... > +static int omap_gpio_irq_request(struct irq_data *d) > +{ > + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > + > + return omap_gpio_request(&bank->chip, d->hwirq); If you want the GPIO usage to be known to the GPIO subsystem, then wouldn't you call gpio_request() here rather than omap_gpio_request()? The above code will certainly do enough so that the OMAP GPIO HW is fully enabled as you need, but I thought the idea was to also prevent some other code successfully running gpio_request() on that same GPIO?
On 04/14/2013 02:53 PM, Linus Walleij wrote: > On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas > <martinez.javier@gmail.com> wrote: > >> Is the following inlined patch [1] what you were thinking that would >> be the right approach? > > This looks sort of OK, but I'm still struggling with the question of > what we could do to help other implementations facing the same issue. > > This is a pretty hard design pattern to properly replicate in every such > driver is it not? Well, instead of adding .request_irq() to the irqchip, and then making each driver call gpio_request() from the implementation, perhaps you could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, and if it gets back a non-error response, the IRQ core could call gpio_request(). That means that the change to each GPIO+IRQ driver is simply to implement a standalone data transformation function .irq_to_gpio(). Now, this does re-introduce irq_to_gpio() in some way, but with the following advantages: 1) The implementation is per-controller, not a single global function, so isn't introducing any kind of centralized mapping scheme again. 2) This irq-chip-specific .irq_to_gpio() would only be implemented for IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. Its potential existence doesn't imply that all IRQ chips need implement this; it would be very specifically be for this one particular case. So, I think it's reasonable to introduce this.
On 04/15/2013 11:53 AM, Stephen Warren wrote: > On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote: > ... >> Is the following inlined patch [1] what you were thinking that would >> be the right approach? > ... >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > ... >> +static int omap_gpio_irq_request(struct irq_data *d) >> +{ >> + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); >> + >> + return omap_gpio_request(&bank->chip, d->hwirq); > > If you want the GPIO usage to be known to the GPIO subsystem, then > wouldn't you call gpio_request() here rather than omap_gpio_request()? > The above code will certainly do enough so that the OMAP GPIO HW is > fully enabled as you need, but I thought the idea was to also prevent > some other code successfully running gpio_request() on that same GPIO? Also, although omap gpios default to being inputs, we should not assume that. So may be you should call gpio_request_one() here passing as flags GPIOF_IN to configure as an input. Cheers Jon
On Mon, Apr 15, 2013 at 6:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/14/2013 02:53 PM, Linus Walleij wrote: >> This is a pretty hard design pattern to properly replicate in every such >> driver is it not? > > Well, instead of adding .request_irq() to the irqchip, and then making > each driver call gpio_request() from the implementation, perhaps you > could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, > and if it gets back a non-error response, the IRQ core could call > gpio_request(). That means that the change to each GPIO+IRQ driver is > simply to implement a standalone data transformation function > .irq_to_gpio(). > > Now, this does re-introduce irq_to_gpio() in some way, but with the > following advantages: > > 1) The implementation is per-controller, not a single global function, > so isn't introducing any kind of centralized mapping scheme again. > > 2) This irq-chip-specific .irq_to_gpio() would only be implemented for > IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. > Its potential existence doesn't imply that all IRQ chips need implement > this; it would be very specifically be for this one particular case. I sort of like the idea. It'd have to go past Grant and Thomas G though. A problem is that this will result in the same kind of dilemma that the pinctrl subsystem has been facing with mapping the global GPIO numbers back into local numbers (as is the nature of IRQ numbers). Such as is done by the GPIO ranges in pinctrl. The good part is that we now know how to actually contain that, even how to do it in DT. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 685e850..e035e64 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -811,6 +811,13 @@ static void gpio_unmask_irq(struct irq_data *d) spin_unlock_irqrestore(&bank->lock, flags); } +static int omap_gpio_irq_request(struct irq_data *d) +{ + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); + + return omap_gpio_request(&bank->chip, d->hwirq); +} + static struct irq_chip gpio_irq_chip = { .name = "GPIO", .irq_shutdown = gpio_irq_shutdown, @@ -819,6 +826,7 @@ static struct irq_chip gpio_irq_chip = { .irq_unmask = gpio_unmask_irq, .irq_set_type = gpio_irq_type, .irq_set_wake = gpio_wake_enable, + .irq_request = omap_gpio_irq_request, }; /*---------------------------------------------------------------------*/ @@ -1028,6 +1036,7 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank, unsigned int irq_start, ct->chip.irq_mask = irq_gc_mask_set_bit; ct->chip.irq_unmask = irq_gc_mask_clr_bit; ct->chip.irq_set_type = gpio_irq_type; + ct->chip.irq_request = omap_gpio_irq_request; if (bank->regs->wkup_en) ct->chip.irq_set_wake = gpio_wake_enable, diff --git a/include/linux/irq.h b/include/linux/irq.h index 0e8e3a6..85596cc 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -303,6 +303,7 @@ struct irq_chip { void (*irq_shutdown)(struct irq_data *data); void (*irq_enable)(struct irq_data *data); void (*irq_disable)(struct irq_data *data); + int (*irq_request)(struct irq_data *data); void (*irq_ack)(struct irq_data *data); void (*irq_mask)(struct irq_data *data); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index fa17855..a4bd4f7 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -588,6 +588,12 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned int irq, unmask = 1; } + if (chip->irq_request) { + ret = chip->irq_request(&desc->irq_data); + if (ret) + return ret; + } + /* caller masked out all except trigger mode flags */ ret = chip->irq_set_type(&desc->irq_data, flags);