[PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[riyer-DDmLM1+adcrQT0dZR+AlfA]

From: Rakesh Iyer <riyer-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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);
 
@@ -754,6 +760,7 @@ static int tegra_kbc_suspend(struct device *dev)
 		tegra_kbc_setup_wakekeys(kbc, true);
 		msleep(30);
 
+		kbc->keypress_caused_wake = false;
 		enable_irq_wake(kbc->irq);
 	} else {
 		if (kbc->idev->users)
@@ -780,7 +787,12 @@ static int tegra_kbc_resume(struct device *dev)
 
 		tegra_kbc_set_fifo_interrupt(kbc, true);
 
-		enable_irq(kbc->irq);
+		if (kbc->keypress_caused_wake && kbc->wakeup_key) {
+			input_report_key(kbc->idev, kbc->wakeup_key, 1);
+			input_sync(kbc->idev);
+			input_report_key(kbc->idev, kbc->wakeup_key, 0);
+			input_sync(kbc->idev);
+		}
 	} else {
 		if (kbc->idev->users)
 			err = tegra_kbc_start(kbc);
-- 
1.7.4.1

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

[Stephen Warren]

Rakesh Iyer wrote at Wednesday, November 30, 2011 1:44 PM:
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

-- 
nvpublic

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[Dmitry Torokhov]

Hi Rakesh,

On Wed, Nov 30, 2011 at 12:43:31PM -0800, riyer@nvidia.com wrote:
> From: Rakesh Iyer <riyer@nvidia.com>
> 
> 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@nvidia.com>
> ---
>  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

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

[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@gmail.com]
> Sent: Wednesday, November 30, 2011 1:20 PM
> To: Rakesh Iyer
> Cc: rydberg@euromail.se; Stephen Warren; Laxman Dewangan; linux-
> kernel@vger.kernel.org; linux-input@vger.kernel.org; linux-tegra@vger.kernel.org
> 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@nvidia.com wrote:
> > From: Rakesh Iyer <riyer@nvidia.com>
> >
> > 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@nvidia.com>
> > ---
> >  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

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[Dmitry Torokhov]

On Wed, Nov 30, 2011 at 02:14:03PM -0800, Rakesh Iyer wrote:
> 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.

I do not think you can guarantee this though because if user touches
keyboard "too early", before your resume method had a chance to disable
kbc interrupt as a wakeup source, you are still going to get that
interrupt and deliver KEY_POWER even though KBC is not the actual wakeup
source.

So don't over-complicate it. If hardware can't detect actual key pressed
just emit KEY_POWER if a key was at any time between calls to
tegra_kbc_suspend() and tegra_kbc_resume().

BTW, could you please have your MUA wrap long lines around 75 column or so?

Thanks.

-- 
Dmitry

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

[Rakesh Iyer]

Hello Dmitry.

Sorry for the wrap issue, my Outlook does not seem to obey the settings.

I wanted to explain the tegra system resume path implementation so I can justify 
why I am doing this complicated fix and why I feel it will guarantee the resume 
is due to keypress.

The tegra wake resume code is registered as a syscore ops.
When the system is resumed due to a wake event, the suspend_enter (after wakeup) 
routine will invoke the tegra syscoreops_resume method and that routine will propagate 
the wake event to the individual ISR's through genirq.
If kbc was wake source, kbc_isr will be invoked in this execution path.

If system is resumed due to other reason, the tegra_syscoreops_resume code will not 
find the event.

In the kbc we ignore all keypresses until kbc_resume re-enables the fifo interrupt.
So the only way to generate this key would be if the tegra_syscoreops_resume 
finds that kbc was wake source

Maybe my understanding is wrong. Please feel free to correct me.

Regards
Rakesh

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, November 30, 2011 11:27 PM
> To: Rakesh Iyer
> Cc: rydberg@euromail.se; Stephen Warren; Laxman Dewangan; linux-
> kernel@vger.kernel.org; linux-input@vger.kernel.org; linux-tegra@vger.kernel.org
> Subject: Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.
> 
> On Wed, Nov 30, 2011 at 02:14:03PM -0800, Rakesh Iyer wrote:
> > 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.
> 
> I do not think you can guarantee this though because if user touches
> keyboard "too early", before your resume method had a chance to disable
> kbc interrupt as a wakeup source, you are still going to get that
> interrupt and deliver KEY_POWER even though KBC is not the actual wakeup
> source.
> 
> So don't over-complicate it. If hardware can't detect actual key pressed
> just emit KEY_POWER if a key was at any time between calls to
> tegra_kbc_suspend() and tegra_kbc_resume().
> 
> BTW, could you please have your MUA wrap long lines around 75 column or so?
> 
> Thanks.
> 
> --
> Dmitry

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[Dmitry Torokhov]

Hi Rakesh,

On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
> Hello Dmitry.
> 
> Sorry for the wrap issue, my Outlook does not seem to obey the settings.
> 
> I wanted to explain the tegra system resume path implementation so I can justify 
> why I am doing this complicated fix and why I feel it will guarantee the resume 
> is due to keypress.
> 
> The tegra wake resume code is registered as a syscore ops.
> When the system is resumed due to a wake event, the suspend_enter (after wakeup) 
> routine will invoke the tegra syscoreops_resume method and that routine will propagate 
> the wake event to the individual ISR's through genirq.
> If kbc was wake source, kbc_isr will be invoked in this execution path.
> 
> If system is resumed due to other reason, the tegra_syscoreops_resume code will not 
> find the event.

Consider the following sequence:

1. Something other than keyboard generates wakeup event
2. It's IRQ fires up and gets serviced
3. System starts resuming devices
4. User presses a key on the keypad while it is still suspended _and_
   registered as a wakeup source
5. Keypad's ISR runs as well and you decide that KEY_POWER should be
   reported even though keypad wasn't the real reason the system
   woke up.

Is this scenario not possible?

Thanks.

-- 
Dmitry

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[riyer]

Hello Dmitry.
Please find replies inline.

On Sun, 2011-12-04 at 00:50 -0800, Dmitry Torokhov wrote:
> Hi Rakesh,
> 
> On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
> > Hello Dmitry.
> > 
> > Sorry for the wrap issue, my Outlook does not seem to obey the settings.
> > 
> > I wanted to explain the tegra system resume path implementation so I can justify 
> > why I am doing this complicated fix and why I feel it will guarantee the resume 
> > is due to keypress.
> > 
> > The tegra wake resume code is registered as a syscore ops.
> > When the system is resumed due to a wake event, the suspend_enter (after wakeup) 
> > routine will invoke the tegra syscoreops_resume method and that routine will propagate 
> > the wake event to the individual ISR's through genirq.
> > If kbc was wake source, kbc_isr will be invoked in this execution path.
> > 
> > If system is resumed due to other reason, the tegra_syscoreops_resume code will not 
> > find the event.
> 
> Consider the following sequence:
> 
> 1. Something other than keyboard generates wakeup event
> 2. It's IRQ fires up and gets serviced
At this point syscoreops_resume has finished all its wakeup processing.

> 3. System starts resuming devices
> 4. User presses a key on the keypad while it is still suspended _and_
>    registered as a wakeup source
This will have no impact on the system and keypad ISR will not be
invoked.
> 5. Keypad's ISR runs as well and you decide that KEY_POWER should be
>    reported even though keypad wasn't the real reason the system
>    woke up.
The interrupt line we use to detect wakeup processing is the keypress
interrupt which is disabled and will never cause the ISR invocation from
a device interrupt(i.e. the PIC).

In other words KBC isr gets invoked for 2 reasons
a) FIFO interrupt is generated, which will not happen as long as
scanning logic is disabled until kbc_resume executes.
b) The syscoreops_resume codepath calls into the ISR after finding KBC
was a wakesource. If this happens it will happen only when the kernel
resumes from its suspend code path.
> 
> Is this scenario not possible?
> 
> Thanks.
> 
With that being the case do you think the fix makes sense?

Regards
Rakesh

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[Dmitry Torokhov]

On Sun, Dec 04, 2011 at 05:18:17PM -0800, riyer wrote:
> Hello Dmitry.
> Please find replies inline.
> 
> On Sun, 2011-12-04 at 00:50 -0800, Dmitry Torokhov wrote:
> > Hi Rakesh,
> > 
> > On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
> > > Hello Dmitry.
> > > 
> > > Sorry for the wrap issue, my Outlook does not seem to obey the settings.
> > > 
> > > I wanted to explain the tegra system resume path implementation so I can justify 
> > > why I am doing this complicated fix and why I feel it will guarantee the resume 
> > > is due to keypress.
> > > 
> > > The tegra wake resume code is registered as a syscore ops.
> > > When the system is resumed due to a wake event, the suspend_enter (after wakeup) 
> > > routine will invoke the tegra syscoreops_resume method and that routine will propagate 
> > > the wake event to the individual ISR's through genirq.
> > > If kbc was wake source, kbc_isr will be invoked in this execution path.
> > > 
> > > If system is resumed due to other reason, the tegra_syscoreops_resume code will not 
> > > find the event.
> > 
> > Consider the following sequence:
> > 
> > 1. Something other than keyboard generates wakeup event
> > 2. It's IRQ fires up and gets serviced
> At this point syscoreops_resume has finished all its wakeup processing.
> 
> > 3. System starts resuming devices
> > 4. User presses a key on the keypad while it is still suspended _and_
> >    registered as a wakeup source
> This will have no impact on the system and keypad ISR will not be
> invoked.
> > 5. Keypad's ISR runs as well and you decide that KEY_POWER should be
> >    reported even though keypad wasn't the real reason the system
> >    woke up.
> The interrupt line we use to detect wakeup processing is the keypress
> interrupt which is disabled and will never cause the ISR invocation from
> a device interrupt(i.e. the PIC).
> 
> In other words KBC isr gets invoked for 2 reasons
> a) FIFO interrupt is generated, which will not happen as long as
> scanning logic is disabled until kbc_resume executes.
> b) The syscoreops_resume codepath calls into the ISR after finding KBC
> was a wakesource. If this happens it will happen only when the kernel
> resumes from its suspend code path.

Is this code already in mainline or still somewhere else?

> > 
> > Is this scenario not possible?
> > 
> > Thanks.
> > 
> With that being the case do you think the fix makes sense?

OK, yes. I do not think it is that important if we occasionally report
KEY_POWER even if KBC was not a true wakeup source but that's fine.

Thanks.

-- 
Dmitry

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

[Rakesh Iyer]

________________________________________
From: Dmitry Torokhov [dmitry.torokhov@gmail.com]
Sent: Sunday, December 04, 2011 8:27 PM
To: Rakesh Iyer
Cc: rydberg@euromail.se; Stephen Warren; Laxman Dewangan; linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; linux-tegra@vger.kernel.org
Subject: Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

On Sun, Dec 04, 2011 at 05:18:17PM -0800, riyer wrote:
> Hello Dmitry.
> Please find replies inline.
>
> On Sun, 2011-12-04 at 00:50 -0800, Dmitry Torokhov wrote:
> > Hi Rakesh,
> >
> > On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
> > > Hello Dmitry.
> > >
> > > Sorry for the wrap issue, my Outlook does not seem to obey the settings.
> > >
> > > I wanted to explain the tegra system resume path implementation so I can justify
> > > why I am doing this complicated fix and why I feel it will guarantee the resume
> > > is due to keypress.
> > >
> > > The tegra wake resume code is registered as a syscore ops.
> > > When the system is resumed due to a wake event, the suspend_enter (after wakeup)
> > > routine will invoke the tegra syscoreops_resume method and that routine will propagate
> > > the wake event to the individual ISR's through genirq.
> > > If kbc was wake source, kbc_isr will be invoked in this execution path.
> > >
> > > If system is resumed due to other reason, the tegra_syscoreops_resume code will not
> > > find the event.
> >
> > Consider the following sequence:
> >
> > 1. Something other than keyboard generates wakeup event
> > 2. It's IRQ fires up and gets serviced
> At this point syscoreops_resume has finished all its wakeup processing.
>
> > 3. System starts resuming devices
> > 4. User presses a key on the keypad while it is still suspended _and_
> >    registered as a wakeup source
> This will have no impact on the system and keypad ISR will not be
> invoked.
> > 5. Keypad's ISR runs as well and you decide that KEY_POWER should be
> >    reported even though keypad wasn't the real reason the system
> >    woke up.
> The interrupt line we use to detect wakeup processing is the keypress
> interrupt which is disabled and will never cause the ISR invocation from
> a device interrupt(i.e. the PIC).
>
> In other words KBC isr gets invoked for 2 reasons
> a) FIFO interrupt is generated, which will not happen as long as
> scanning logic is disabled until kbc_resume executes.
> b) The syscoreops_resume codepath calls into the ISR after finding KBC
> was a wakesource. If this happens it will happen only when the kernel
> resumes from its suspend code path.

>> Is this code already in mainline or still somewhere else?
 
I think this code has not made it to mainline yet. 
Its in the Android and ChromeOs kernel trees.

> >
> > Is this scenario not possible?
> >
> > Thanks.
> >
> With that being the case do you think the fix makes sense?

>> OK, yes. I do not think it is that important if we occasionally report
>> KEY_POWER even if KBC was not a true wakeup source but that's fine.

>>Thanks.

>>--
>> Dmitry

So can I assume this patch is accepted?

Thanks and Regards
Rakesh

Re: [PATCH v1] Input: tegra-kbc - report wakeup key for some platforms.

[Dmitry Torokhov]

Hi Rakesh,

On Wed, Nov 30, 2011 at 12:43:31PM -0800, riyer@nvidia.com wrote:
> From: Rakesh Iyer <riyer@nvidia.com>
> 
> 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@nvidia.com>
> ---
>  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);
>  
> @@ -754,6 +760,7 @@ static int tegra_kbc_suspend(struct device *dev)
>  		tegra_kbc_setup_wakekeys(kbc, true);
>  		msleep(30);
>  
> +		kbc->keypress_caused_wake = false;
>  		enable_irq_wake(kbc->irq);
>  	} else {
>  		if (kbc->idev->users)
> @@ -780,7 +787,12 @@ static int tegra_kbc_resume(struct device *dev)
>  
>  		tegra_kbc_set_fifo_interrupt(kbc, true);
>  
> -		enable_irq(kbc->irq);
> +		if (kbc->keypress_caused_wake && kbc->wakeup_key) {
> +			input_report_key(kbc->idev, kbc->wakeup_key, 1);
> +			input_sync(kbc->idev);
> +			input_report_key(kbc->idev, kbc->wakeup_key, 0);
> +			input_sync(kbc->idev);

Would you mind moving this block into the ISR itself? If we leave ISR
running then there is no reason to postpone event generation, is there?
If not then we can get rid of kbc->keypress_caused_wake.

Thanks.

-- 
Dmitry