Cory Maccarrone <darkstar6262@xxxxxxxxx> writes:
> Some chips, namely any OMAP1 chips using METHOD_MPUIO,
> OMAP15xx and OMAP7xx, cannot be setup to respond to on-chip GPIO
> interrupts in both rising and falling edge directions -- they can
> only respond to one direction or the other, depending on how the
> ICR is configured.
>
> Additionally, current code forces rising edge detection if both
> flags are specified:
>
> if (trigger & IRQ_TYPE_EDGE_RISING)
> l |= 1 << gpio;
> else if (trigger & IRQ_TYPE_EDGE_FALLING)
> l &= ~(1 << gpio);
> else
> goto bad;
>
> This change implements a toggle function that will modify the ICR
> to flip the direction of interrupt for IRQs that are requested with
> both rising and falling flags. The toggle function is not called
> for chips and GPIOs it does not apply to through the use of a flip_mask
> that's added on a per-bank basis. The mask is only set for those
> GPIOs where a toggle is necessary. Edge detection starts out the
> same as above with FALLING mode first.
>
> The toggle happens on EACH interrupt; without it, we have the
> following sequence of actions on GPIO transition:
>
> ICR GPIO Result
> 0x1 0 -> 1 (rising) Interrupt
> 0x1 1 -> 0 (falling) No interrupt
>
> (set ICR to 0x0 manually)
> 0x0 0 -> 1 (rising) No interrupt
> 0x0 1 -> 0 (falling) Interrupt
>
> That is, with the ICR set to 1 for a gpio, only rising edge interrupts
> are caught, and with it set to 0, only falling edge interrupts are
> caught. If we add in the toggle, we get this:
>
> ICR GPIO Result
> 0x1 0 -> 1 (rising) Interrupt (ICR set to 0x0)
> 0x0 1 -> 0 (falling) Interrupt (ICR set to 0x1)
> 0x1 0 -> 1 ...
>
> so, both rising and falling are caught, per the request for both
> (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING).
>
> Signed-off-by: Cory Maccarrone <darkstar6262@xxxxxxxxx>
Much better changelog, very thorough. And I like the approach on this
version better as well.
Please run this through scripts/checkpatch.pl and fix the issues
reported there.
Just a few minor comments below...
> ---
> arch/arm/plat-omap/gpio.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 58 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 055160e..a05c83d 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -192,6 +192,7 @@ struct gpio_bank {
> u32 saved_risingdetect;
> #endif
> u32 level_mask;
> + u32 flip_mask;
Maybe toggle_mask is a better name since you call this toggle in the
rest of your code.
> spinlock_t lock;
> struct gpio_chip chip;
> struct clk *dbck;
> @@ -749,6 +750,44 @@ static inline void set_24xx_gpio_triggering(struct gpio_bank *bank, int gpio,
> }
> #endif
>
> +/*
> + * This only applies to chips that can't do both rising and falling edge
> + * detection at once. For all other chips, this function is a noop.
> + */
> +static void _toggle_gpio_edge_triggering(struct gpio_bank *bank, int gpio)
> +{
> + void __iomem *reg = bank->base;
> + u32 l = 0;
> +
> + switch (bank->method) {
> +#ifdef CONFIG_ARCH_OMAP1
> + case METHOD_MPUIO:
> + reg += OMAP_MPUIO_GPIO_INT_EDGE;
> + break;
> +#endif
> +#ifdef CONFIG_ARCH_OMAP15XX
> + case METHOD_GPIO_1510:
> + reg += OMAP1510_GPIO_INT_CONTROL;
> + break;
> +#endif
> +#if defined(CONFIG_ARCH_OMAP730) || defined(CONFIG_ARCH_OMAP850)
> + case METHOD_GPIO_7XX:
> + reg += OMAP7XX_GPIO_INT_CONTROL;
> + break;
> +#endif
> + default:
> + return;
> + }
> +
> + l = __raw_readl(reg);
> + if ((l >> gpio) & 1)
> + l &= ~(1 << gpio);
> + else
> + l |= 1 << gpio;
> +
> + __raw_writel(l, reg);
> +}
> +
> static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> {
> void __iomem *reg = bank->base;
> @@ -759,6 +798,8 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> case METHOD_MPUIO:
> reg += OMAP_MPUIO_GPIO_INT_EDGE;
> l = __raw_readl(reg);
> + if (trigger & IRQ_TYPE_EDGE_RISING && trigger & IRQ_TYPE_EDGE_FALLING)
You could use this instead
if (trigger & IRQ_TYPE_EDGE_BOTH)
and it will also solve the checkpatch warning about the long lines
here.
same for other instances of this below.
> + bank->flip_mask |= 1 << gpio;
> if (trigger & IRQ_TYPE_EDGE_RISING)
> l |= 1 << gpio;
> else if (trigger & IRQ_TYPE_EDGE_FALLING)
> @@ -771,6 +812,8 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> case METHOD_GPIO_1510:
> reg += OMAP1510_GPIO_INT_CONTROL;
> l = __raw_readl(reg);
> + if (trigger & IRQ_TYPE_EDGE_RISING && trigger & IRQ_TYPE_EDGE_FALLING)
> + bank->flip_mask |= 1 << gpio;
> if (trigger & IRQ_TYPE_EDGE_RISING)
> l |= 1 << gpio;
> else if (trigger & IRQ_TYPE_EDGE_FALLING)
> @@ -803,6 +846,8 @@ static int _set_gpio_triggering(struct gpio_bank *bank, int gpio, int trigger)
> case METHOD_GPIO_7XX:
> reg += OMAP7XX_GPIO_INT_CONTROL;
> l = __raw_readl(reg);
> + if (trigger & IRQ_TYPE_EDGE_RISING && trigger & IRQ_TYPE_EDGE_FALLING)
> + bank->flip_mask |= 1 << gpio;
> if (trigger & IRQ_TYPE_EDGE_RISING)
> l |= 1 << gpio;
> else if (trigger & IRQ_TYPE_EDGE_FALLING)
> @@ -1217,7 +1262,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> {
> void __iomem *isr_reg = NULL;
> u32 isr;
> - unsigned int gpio_irq;
> + unsigned int gpio_irq, gpio_index;
> struct gpio_bank *bank;
> u32 retrigger = 0;
> int unmasked = 0;
> @@ -1284,9 +1329,21 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>
> gpio_irq = bank->virtual_irq_start;
> for (; isr != 0; isr >>= 1, gpio_irq++) {
> + gpio_index = get_gpio_index(irq_to_gpio(gpio_irq));
> +
> if (!(isr & 1))
> continue;
>
For better optimizaion for builds without OMAP1 support, you should wrap
this part:
#ifdef CONFIG_ARCH_OMAP1
> + /*
> + * Some chips can't respond to both rising and falling at the
> + * same time. If this irq was requested with both flags, we
> + * need to flip the ICR data for the IRQ to respond to the
> + * IRQ for the opposite direction. This will be indicated in
> + * the bank flip_mask.
> + */
> + if ((bank->flip_mask >> gpio_index) & 1)
minor nit, but elsewhere in the GPIO code we tend to mask with (1 <<
gpio_index). So, for consistency, this should be
if (bank->flip_mask && (1 << gpio_index))
> + _toggle_gpio_edge_triggering(bank, gpio_index);
> +
> generic_handle_irq(gpio_irq);
> }
> }
#endif
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
