Hi Kevin,
> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> arch/arm/plat-omap/gpio.c | 14 ++++----------
> 1 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index d3fa41e..210a1c0 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -921,13 +921,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank,
> int gpio, int enable)
> case METHOD_MPUIO:
> case METHOD_GPIO_1610:
> spin_lock_irqsave(&bank->lock, flags);
> - if (enable) {
> + if (enable)
> bank->suspend_wakeup |= (1 << gpio);
> - enable_irq_wake(bank->irq);
> - } else {
> - disable_irq_wake(bank->irq);
> + else
> bank->suspend_wakeup &= ~(1 << gpio);
> - }
> spin_unlock_irqrestore(&bank->lock, flags);
> return 0;
> #endif
> @@ -940,13 +937,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank,
> int gpio, int enable)
> return -EINVAL;
> }
> spin_lock_irqsave(&bank->lock, flags);
> - if (enable) {
> + if (enable)
> bank->suspend_wakeup |= (1 << gpio);
> - enable_irq_wake(bank->irq);
> - } else {
> - disable_irq_wake(bank->irq);
> + else
> bank->suspend_wakeup &= ~(1 << gpio);
> - }
> spin_unlock_irqrestore(&bank->lock, flags);
> return 0;
> #endif
I have been looking into an issue that appears to be related to this. I have the following questions with regard to this patch.
1). Although, I agree with the above change was there any reason why we did not add some code to set/clear the appropriate bit in the WKUENA register in this function? If this function is called via a call to set_irq_wake it will not modify the WKUENA register. Therefore, we were thinking of something along the lines of:
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 17d7afe..895c548 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -941,10 +941,13 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
return -EINVAL;
}
spin_lock_irqsave(&bank->lock, flags);
- if (enable)
+ if (enable) {
bank->suspend_wakeup |= (1 << gpio);
- else
+ __raw_writel(1 << gpio, bank->base + OMAP24XX_GPIO_SETWKUENA);
+ } else {
+ __raw_writel(1 << gpio, bank->base + OMAP24XX_GPIO_CLEARWKUENA);
bank->suspend_wakeup &= ~(1 << gpio);
+ }
spin_unlock_irqrestore(&bank->lock, flags);
return 0;
#endif
2). We have found that a call to request_irq results in a call to the function "set_24xx_gpio_triggering()" (for OMAP3430). In addition to configuring the triggering for a given GPIO, this function is also programming the WKUENA register. Hence, the wake-up enable is enabled for GPIO without calling set_irq_wake().
I am not sure if this is the intended behaviour or if drivers should explicitly be calling set_irq_wake to enable/disable the wake-up.
The other problem with this is that once request_irq is called for a gpio, then even if we call disable_irq the wake-up event is not cleared. So this means that a gpio event will still wake-up the device without the gpio being enabled and because the gpio is disabled the event will never be cleared and hence will prevent retention.
So should the wake-up event only be enabled/disabled by a call to set_irq_wake() or should we make sure that calling disable_irq in turn calls gpio_mask_irq and make sure this clears the wake-up event?
Cheers
Jon
--
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
