[PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[Viresh Kumar]

From: Deepak Sikri <deepak.sikri@st.com>

This patch handles the fix for unbalanced irq for the cases when
enable_irq_wake fails, and a warning related to same is displayed
on the console. The workaround is handled at the driver level.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Rajeev Kumar <rajeev-dlh.kumar@st.com>
---
 drivers/input/keyboard/spear-keyboard.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 0e7f686..6eb2dd8 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -58,6 +58,7 @@ struct spear_kbd {
 	void __iomem *io_base;
 	struct clk *clk;
 	unsigned int irq;
+	unsigned int irq_wake;
 	unsigned short last_key;
 	unsigned int mode;
 	unsigned short keycodes[256];
@@ -287,8 +288,10 @@ static int spear_kbd_suspend(struct device *dev)
 	if (input_dev->users)
 		clk_enable(kbd->clk);
 
-	if (device_may_wakeup(&pdev->dev))
-		enable_irq_wake(kbd->irq);
+	if (device_may_wakeup(&pdev->dev)) {
+		if (!enable_irq_wake(kbd->irq))
+			kbd->irq_wake = 1;
+	}
 
 	mutex_unlock(&input_dev->mutex);
 
@@ -303,8 +306,12 @@ static int spear_kbd_resume(struct device *dev)
 
 	mutex_lock(&input_dev->mutex);
 
-	if (device_may_wakeup(&pdev->dev))
-		disable_irq_wake(kbd->irq);
+	if (device_may_wakeup(&pdev->dev)) {
+		if (kbd->irq_wake) {
+			kbd->irq_wake = 0;
+			disable_irq_wake(kbd->irq);
+		}
+	}
 
 	if (input_dev->users)
 		clk_enable(kbd->clk);
-- 
1.7.8.110.g4cb5d

[PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Viresh Kumar]

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/input/keyboard/spear-keyboard.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..6e83828 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};
+static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 #endif
 
 static struct platform_driver spear_kbd_driver = {
-- 
1.7.8.110.g4cb5d

Re: [PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Viresh Kumar]

On 2/23/2012 3:10 PM, Viresh KUMAR wrote:
> Thaw and poweroff routines are missing for spear-keyboard. They are required for:
> - Error case scenarios during freeze
> - Using test features, of hibernate.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Sorry for the duplicate SOB :(

-- 
viresh

Re: [PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Shubhrajyoti]

On Thursday 23 February 2012 03:10 PM, Viresh Kumar wrote:
> Thaw and poweroff routines are missing for spear-keyboard. They are required for:
> - Error case scenarios during freeze
> - Using test features, of hibernate.
Not a comment doubt

When is thaw called and when is power off called?
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/input/keyboard/spear-keyboard.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
> index 6eb2dd8..6e83828 100644
> --- a/drivers/input/keyboard/spear-keyboard.c
> +++ b/drivers/input/keyboard/spear-keyboard.c
> @@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
>  	return 0;
>  }
>  
> -static const struct dev_pm_ops spear_kbd_pm_ops = {
> -	.suspend	= spear_kbd_suspend,
> -	.resume		= spear_kbd_resume,
> -};
Now that you have converted to simple dev pm can you remove the #ifdef
from pm.

> +static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
>  #endif
>  
>  static struct platform_driver spear_kbd_driver = {

Re: [PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Viresh Kumar]

On 2/23/2012 4:59 PM, Shubhrajyoti wrote:
>> > - Using test features, of hibernate.
> Not a comment doubt
> 
> When is thaw called and when is power off called?

Firstly freeze is called, after that snapshot of memory is taken
(to be saved in disk). Now, we need resume harddisk, so we get our
thaw routines called (analogous to resume). Now system is up again. Save
image to disk and call poweroff() to finally shutdown.

Refer Documentation/power/devices.txt to get complete sequence of calls.

>> > diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
>> > index 6eb2dd8..6e83828 100644
>> > --- a/drivers/input/keyboard/spear-keyboard.c
>> > +++ b/drivers/input/keyboard/spear-keyboard.c
>> > @@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
>> >  	return 0;
>> >  }
>> >  
>> > -static const struct dev_pm_ops spear_kbd_pm_ops = {
>> > -	.suspend	= spear_kbd_suspend,
>> > -	.resume		= spear_kbd_resume,
>> > -};
> Now that you have converted to simple dev pm can you remove the #ifdef
> from pm.
> 

I didn't get it completely. CONFIG_PM Macro's are still required.

-- 
viresh

Re: [PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Viresh Kumar]

On 2/23/2012 5:26 PM, Viresh Kumar wrote:
>>>> >> > -static const struct dev_pm_ops spear_kbd_pm_ops = {
>>>> >> > -	.suspend	= spear_kbd_suspend,
>>>> >> > -	.resume		= spear_kbd_resume,
>>>> >> > -};
>> > Now that you have converted to simple dev pm can you remove the #ifdef
>> > from pm.
>> > 
> I didn't get it completely. CONFIG_PM Macro's are still required.

Sorry. Got it now. Will remove it. :)

-- 
viresh

Re: [PATCH 2/2] Input/spear-keyboard: Provide thaw and poweroff routines

[Shubhrajyoti]

On Thursday 23 February 2012 05:26 PM, Viresh Kumar wrote:
> On 2/23/2012 4:59 PM, Shubhrajyoti wrote:
>>>> - Using test features, of hibernate.
>> Not a comment doubt
>>
>> When is thaw called and when is power off called?
> Firstly freeze is called, after that snapshot of memory is taken
> (to be saved in disk). Now, we need resume harddisk, so we get our
> thaw routines called (analogous to resume). Now system is up again. Save
> image to disk and call poweroff() to finally shutdown.
>
> Refer Documentation/power/devices.txt to get complete sequence of calls.
Makes sense thanks.
>>>> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
>>>> index 6eb2dd8..6e83828 100644
>>>> --- a/drivers/input/keyboard/spear-keyboard.c
>>>> +++ b/drivers/input/keyboard/spear-keyboard.c
>>>> @@ -321,10 +321,7 @@ static int spear_kbd_resume(struct device *dev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static const struct dev_pm_ops spear_kbd_pm_ops = {
>>>> -	.suspend	= spear_kbd_suspend,
>>>> -	.resume		= spear_kbd_resume,
>>>> -};
>> Now that you have converted to simple dev pm can you remove the #ifdef
>> from pm.
>>
> I didn't get it completely. CONFIG_PM Macro's are still required.
The dev pm makes it empty so not required.

[PATCH V2] Input/spear-keyboard: Provide thaw and poweroff routines

[Viresh Kumar]

Thaw and poweroff routines are missing for spear-keyboard. They are required for:
- Error case scenarios during freeze
- Using test features, of hibernate.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Rajeev Kumar <rajeev-dlh.kumar@st.com>
---
Changes since V1:
- moved spear_kbd_pm_ops's definition/usage outside of CONFIG_PM macro

 drivers/input/keyboard/spear-keyboard.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 6eb2dd8..782e947 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -320,22 +320,17 @@ static int spear_kbd_resume(struct device *dev)
 
 	return 0;
 }
-
-static const struct dev_pm_ops spear_kbd_pm_ops = {
-	.suspend	= spear_kbd_suspend,
-	.resume		= spear_kbd_resume,
-};
 #endif
 
+static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
+
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
 	.remove		= __devexit_p(spear_kbd_remove),
 	.driver		= {
 		.name	= "keyboard",
 		.owner	= THIS_MODULE,
-#ifdef CONFIG_PM
 		.pm	= &spear_kbd_pm_ops,
-#endif
 	},
 };
 module_platform_driver(spear_kbd_driver);
-- 
1.7.8.110.g4cb5d

Re: [PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[Dmitry Torokhov]

On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:
> From: Deepak Sikri <deepak.sikri@st.com>
> 
> This patch handles the fix for unbalanced irq for the cases when
> enable_irq_wake fails, and a warning related to same is displayed
> on the console. The workaround is handled at the driver level.

So why is the device marked as wakeup capable?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[deepaksi]

Hi,


On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:
> On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:
>> From: Deepak Sikri<deepak.sikri@st.com>
>>
>> This patch handles the fix for unbalanced irq for the cases when
>> enable_irq_wake fails, and a warning related to same is displayed
>> on the console. The workaround is handled at the driver level.
> So why is the device marked as wakeup capable?

The device is capable of wake up. There are cases in which the 
enable_irq_wake call may fail.

This was specifically observed for ARM cortex architecture kernel 
implementation for GIC (PL390).
The 2.6.37 kernel versions and before do not have the callbacks related 
to function that help to set up
the wake up interrupts  in GIC, and enable_irq_wake returns with an 
error code -ENXIO.

Now if the user tries to disable the wake up interrupts in the resume 
routines without checking in for the cases
where the enable_irq_wake failed, kernel returns a warning related to 
unbalanced irq as we are trying to disable
an interrupt which was not enabled.

However, in case the enable_irq_wake is successful ( as was in case of 
ARM9 architectures with VIC callbacks
present for setting up wake up interrupts present), this check allowed 
to keep track of success cases.



Regards
Deepak

Re: [PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[Dmitry Torokhov]

On Fri, Feb 24, 2012 at 02:28:25PM +0530, deepaksi wrote:
> Hi,
> 
> 
> On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:
> >On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:
> >>From: Deepak Sikri<deepak.sikri@st.com>
> >>
> >>This patch handles the fix for unbalanced irq for the cases when
> >>enable_irq_wake fails, and a warning related to same is displayed
> >>on the console. The workaround is handled at the driver level.
> >So why is the device marked as wakeup capable?
> 
> The device is capable of wake up. There are cases in which the
> enable_irq_wake call may fail.

In this case I'd say the proper action would be to abort suspend instead
of continuing.

> This was specifically observed for ARM cortex architecture kernel
> implementation for GIC (PL390).
> The 2.6.37 kernel versions and before do not have the callbacks
> related to function that help to set up
> the wake up interrupts  in GIC, and enable_irq_wake returns with an
> error code -ENXIO.

Right, but current mainline does have it set up properly, correct? IOW
we do not really expect enable_irq_wake() to fail if device is wakeup
capable, correct?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input/spear-keyboard: Fix for balancing the enable_irq_wake in Power Mgmt

[deepaksi]

On 2/24/2012 2:32 PM, Dmitry Torokhov wrote:
> On Fri, Feb 24, 2012 at 02:28:25PM +0530, deepaksi wrote:
>> Hi,
>>
>>
>> On 2/24/2012 2:04 PM, Dmitry Torokhov wrote:
>>> On Thu, Feb 23, 2012 at 03:10:51PM +0530, Viresh Kumar wrote:
>>>> From: Deepak Sikri<deepak.sikri@st.com>
>>>>
>>>> This patch handles the fix for unbalanced irq for the cases when
>>>> enable_irq_wake fails, and a warning related to same is displayed
>>>> on the console. The workaround is handled at the driver level.
>>> So why is the device marked as wakeup capable?
>> The device is capable of wake up. There are cases in which the
>> enable_irq_wake call may fail.
> In this case I'd say the proper action would be to abort suspend instead
> of continuing.

ok

>> This was specifically observed for ARM cortex architecture kernel
>> implementation for GIC (PL390).
>> The 2.6.37 kernel versions and before do not have the callbacks
>> related to function that help to set up
>> the wake up interrupts  in GIC, and enable_irq_wake returns with an
>> error code -ENXIO.
> Right, but current mainline does have it set up properly, correct? IOW
> we do not really expect enable_irq_wake() to fail if device is wakeup
> capable, correct?
>
> Thanks.
>

I agree the current mainline does have the support now. This was added 
some time back for 2.6.37.
Please do not consider this patch for the mainline.

Thanks
Deepak