@@ -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);+}elseif(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");gotoerr_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);
From: Stephen Warren <hidden> Date: 2011-11-30 21:07:40
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 <redacted>
@@ -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);+}elseif(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");gotoerr_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
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:
quoted
From: Rakesh Iyer <redacted>
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 <redacted>
---
arch/arm/mach-tegra/include/mach/kbc.h | 1 +
drivers/input/keyboard/tegra-kbc.c | 20 ++++++++++++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
@@ -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);+}elseif(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
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
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
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:
quoted
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
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
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:
quoted
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
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:
quoted
Hi Rakesh,
On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
quoted
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.
quoted
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.
quoted
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?
quoted
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
________________________________________
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:
quoted
Hi Rakesh,
On Thu, Dec 01, 2011 at 01:09:59PM -0800, Rakesh Iyer wrote:
quoted
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.
quoted
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.
quoted
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.
quoted
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.
quoted
Is this scenario not possible?
Thanks.
With that being the case do you think the fix makes sense?
quoted
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.
quoted
Thanks.
quoted
--
Dmitry
So can I assume this patch is accepted?
Thanks and Regards
Rakesh
@@ -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);+}elseif(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");gotoerr_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