RE: [PATCH v1] Input: tegra-kbc - report wakeup key for someplatforms.

[Rakesh Iyer]

Thanks Dmitry.

Since there can be multiple wake causes I wanted to isolate wake key generation to the case where keyboard actually generated the wake interrupt.
If the system was woken by some other source and keypress occurs before resume is invoked we will pass on an unintended wake key.

Leaving the disable_irq as is,  can I can enable the interrupts at the end of the suspend routine?

Note with the FIFO interrupt disabled, there will be no interrupt after that point from the device unless it's a Wake interrupt.

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Wednesday, November 30, 2011 1:20 PM
> To: Rakesh Iyer
> Cc: rydberg@xxxxxxxxxxx; Stephen Warren; Laxman Dewangan; linux-
> kernel@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.
> 
> Hi Rakesh,
> 
> On Wed, Nov 30, 2011 at 12:43:31PM -0800, riyer@xxxxxxxxxx wrote:
> > From: Rakesh Iyer <riyer@xxxxxxxxxx>
> >
> > Tegra kbc cannot detect exact keypress causing wakeup in interrupt mode.
> > Allow wakeup keypress to be reported for certain platforms.
> >
> > Signed-off-by: Rakesh Iyer <riyer@xxxxxxxxxx>
> > ---
> >  arch/arm/mach-tegra/include/mach/kbc.h |    1 +
> >  drivers/input/keyboard/tegra-kbc.c     |   20 ++++++++++++++++----
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/mach-tegra/include/mach/kbc.h b/arch/arm/mach-
> tegra/include/mach/kbc.h
> > index 4f3572a..20bb054 100644
> > --- a/arch/arm/mach-tegra/include/mach/kbc.h
> > +++ b/arch/arm/mach-tegra/include/mach/kbc.h
> > @@ -53,6 +53,7 @@ struct tegra_kbc_platform_data {
> >  	struct tegra_kbc_pin_cfg pin_cfg[KBC_MAX_GPIO];
> >  	const struct matrix_keymap_data *keymap_data;
> >
> > +	u32 wakeup_key;
> >  	bool wakeup;
> >  	bool use_fn_map;
> >  	bool use_ghost_filter;
> > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> > index cf3228b..ceb1185 100644
> > --- a/drivers/input/keyboard/tegra-kbc.c
> > +++ b/drivers/input/keyboard/tegra-kbc.c
> > @@ -52,6 +52,7 @@
> >  /* KBC Interrupt Register */
> >  #define KBC_INT_0	0x4
> >  #define KBC_INT_FIFO_CNT_INT_STATUS	(1 << 2)
> > +#define KBC_INT_KEYPRESS_INT_STATUS	(1 << 0)
> >
> >  #define KBC_ROW_CFG0_0	0x8
> >  #define KBC_COL_CFG0_0	0x18
> > @@ -74,10 +75,12 @@ struct tegra_kbc {
> >  	unsigned int cp_to_wkup_dly;
> >  	bool use_fn_map;
> >  	bool use_ghost_filter;
> > +	bool keypress_caused_wake;
> >  	const struct tegra_kbc_platform_data *pdata;
> >  	unsigned short keycode[KBC_MAX_KEY * 2];
> >  	unsigned short current_keys[KBC_MAX_KPENT];
> >  	unsigned int num_pressed_keys;
> > +	u32 wakeup_key;
> >  	struct timer_list timer;
> >  	struct clk *clk;
> >  };
> > @@ -409,6 +412,9 @@ static irqreturn_t tegra_kbc_isr(int irq, void *args)
> >  		 */
> >  		tegra_kbc_set_fifo_interrupt(kbc, false);
> >  		mod_timer(&kbc->timer, jiffies + kbc->cp_dly_jiffies);
> > +	} else if (val & KBC_INT_KEYPRESS_INT_STATUS) {
> > +		/* We can be here only through system resume path */
> > +		kbc->keypress_caused_wake = true;
> >  	}
> >
> >  	spin_unlock_irqrestore(&kbc->lock, flags);
> > @@ -674,9 +680,10 @@ static int __devinit tegra_kbc_probe(struct platform_device
> *pdev)
> >  	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
> >  	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
> >  				   input_dev->keycode, input_dev->keybit);
> > +	kbc->wakeup_key = pdata->wakeup_key;
> >
> > -	err = request_irq(kbc->irq, tegra_kbc_isr, IRQF_TRIGGER_HIGH,
> > -			  pdev->name, kbc);
> > +	err = request_irq(kbc->irq, tegra_kbc_isr,
> > +			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name,
> kbc);
> >  	if (err) {
> >  		dev_err(&pdev->dev, "failed to request keyboard IRQ\n");
> >  		goto err_put_clk;
> > @@ -738,7 +745,6 @@ static int tegra_kbc_suspend(struct device *dev)
> >
> >  	mutex_lock(&kbc->idev->mutex);
> >  	if (device_may_wakeup(&pdev->dev)) {
> > -		disable_irq(kbc->irq);
> >  		del_timer_sync(&kbc->timer);
> >  		tegra_kbc_set_fifo_interrupt(kbc, false);
> 
> This disturbs locking rules and allows timer to run past this point.
> Instead of keeping interrupt enabled can't you simply read controller
> state in tegra_kbc_resume (before enabling interrupt) and emit the
> keycode if you detect KBC_INT_KEYPRESS_INT_STATUS condition?
> 
> Thanks.
> 
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/