Re: Suspending with keypad bug (RFC)

andrzej zaborowski
2006-08-10T04:55:26Z

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On 06/08/06, Juha Yrjölä <juha.yrjola@solidboot.com> wrote:
> andrzej zaborowski wrote:
>
> > I came up with a different fix, in the keypad driver but it's a bit
> > more intrusive. With this the device sleeps peacefully until another
> > key-press is detected. What should be the correct behavior?
> > I'm attaching both changes.
>
> This approach is better; the generic PM code should not have to care
> that much about different wakeup sources.
>
> I'm not exactly sure in which state the system is when the suspend
> function is called. Can there still be timer activity, for example?

I think yes, some suspend functions may rely on timers because they
are not obliged to return immdiately.

> Nevertheless, you're safer doing a spin_lock_irqsave() in the suspend
> function first, and releasing the lock only after setting the suspended
> variable and re-enabling the interrupt.
>
> You should also check whether you're actually running on OMAP1 before
> you write to the register.
>
> In the IRQ handler, you should also do a spin_lock_irqsave() before
> accessing the suspended variable.

Thanks for help! I'm attaching the corrected version, it works nicely.

In the irq handler I release the lock just after accessing the
"suspended" variable because as I understand an irq handler will never
be interrupted by something that calls the suspend function.

>
> Cheers,
> Juha
>

--
balrog 2oo6

[-- Attachment #2: linux-keypad-suspend2.patch --]
[-- Type: application/octet-stream, Size: 2112 bytes --]

--- a/drivers/input/keyboard/omap-keypad.c 2006-08-05 05:18:01.000000000 +0000
+++ b/drivers/input/keyboard/omap-keypad.c 2006-08-10 06:19:11.000000000 +0000
@@ -33,6 +33,7 @@
#include <linux/delay.h>
#include <linux/platform_device.h>
#include <linux/mutex.h>
+#include <linux/spinlock.h>
#include <asm/arch/irqs.h>
#include <asm/arch/gpio.h>
#include <asm/arch/hardware.h>
@@ -59,6 +60,8 @@ struct omap_kp {
int irq;
unsigned int rows;
unsigned int cols;
+ int suspended;
+ spinlock_t suspend_lock;
};

DECLARE_TASKLET_DISABLED(kp_tasklet, omap_kp_tasklet, 0);
@@ -99,6 +102,13 @@ static irqreturn_t omap_kp_interrupt(int
struct pt_regs *regs)
{
struct omap_kp *omap_kp = dev_id;
+ unsigned long flags;
+ spin_lock_irqsave(&omap_kp->suspend_lock, flags);
+
+ if (omap_kp->suspended)
+ return IRQ_HANDLED;
+
+ spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);

/* disable keyboard interrupt and schedule for handling */
if (cpu_is_omap24xx()) {
@@ -271,15 +281,29 @@ static DEVICE_ATTR(enable, S_IRUGO | S_I
#ifdef CONFIG_PM
static int omap_kp_suspend(struct platform_device *dev, pm_message_t state)
{
- /* Nothing yet */
+ struct omap_kp *omap_kp = platform_get_drvdata(dev);
+ unsigned long flags;
+ spin_lock_irqsave(&omap_kp->suspend_lock, flags);
+
+ /*
+ * Re-enable the interrupt in case it has been masked by the
+ * handler and a key is still pressed. We need the interrupt
+ * to wake us up from suspended.
+ */
+ if (cpu_class_is_omap1())
+ omap_writew(0, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);

+ omap_kp->suspended = 1;
+
+ spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
return 0;
}

static int omap_kp_resume(struct platform_device *dev)
{
- /* Nothing yet */
+ struct omap_kp *omap_kp = platform_get_drvdata(dev);

+ omap_kp->suspended = 0;
return 0;
}
#else
@@ -351,6 +375,9 @@ static int __init omap_kp_probe(struct p
}
}

+ spin_lock_init(&omap_kp->suspend_lock);
+ omap_kp->suspended = 0;
+
init_timer(&omap_kp->timer);
omap_kp->timer.function = omap_kp_timer;
omap_kp->timer.data = (unsigned long) omap_kp;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]