[PATCH v6 8/8] Input: omap4 - pm runtime

[PATCH v6 8/8] Input: omap4 - pm runtime

[Abraham Arce]

Enable pm runtime in driver

Reviewed-by: Basak, Partha <p-basak2@ti.com>
Signed-off-by: Abraham Arce <x0066660@ti.com>
---
 drivers/input/keyboard/omap4-keypad.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 45bd097..ed47e9a 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -29,6 +29,7 @@
 #include <linux/io.h>
 #include <linux/input.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 
 #include <plat/omap4-keypad.h>
 
@@ -239,6 +240,9 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
 	matrix_keypad_build_keymap(pdata->keymap_data, row_shift,
 			input_dev->keycode, input_dev->keybit);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	omap4_keypad_config(keypad_data);
 
 	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
@@ -277,6 +281,9 @@ static int __devexit omap4_keypad_remove(struct platform_device *pdev)
 	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
 	struct resource *res;
 
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
 	free_irq(keypad_data->irq, keypad_data);
 	input_unregister_device(keypad_data->input);
 
-- 
1.6.3.3

Re: [PATCH v6 8/8] Input: omap4 - pm runtime

[Kevin Hilman]

Abraham Arce <x0066660@ti.com> writes:

> Enable pm runtime in driver

So power is enabled on probe and cut on _remove().  Did you consider
doing any more fine grained PM for this device?  For example, cutting
power after some inactivity timer and re-enabling on a
keypress/interrupt?

Kevin



> Reviewed-by: Basak, Partha <p-basak2@ti.com>
> Signed-off-by: Abraham Arce <x0066660@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index 45bd097..ed47e9a 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -29,6 +29,7 @@
>  #include <linux/io.h>
>  #include <linux/input.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <plat/omap4-keypad.h>
>  
> @@ -239,6 +240,9 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>  	matrix_keypad_build_keymap(pdata->keymap_data, row_shift,
>  			input_dev->keycode, input_dev->keybit);
>  
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	omap4_keypad_config(keypad_data);
>  
>  	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
> @@ -277,6 +281,9 @@ static int __devexit omap4_keypad_remove(struct platform_device *pdev)
>  	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
>  	struct resource *res;
>  
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +
>  	free_irq(keypad_data->irq, keypad_data);
>  	input_unregister_device(keypad_data->input);

RE: [PATCH v6 8/8] Input: omap4 - pm runtime

[Varadarajan, Charulatha]

Abraham,

> -----Original Message-----
> To: Arce, Abraham
> Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v6 8/8] Input: omap4 - pm runtime
> 
> Abraham Arce <x0066660@ti.com> writes:
> 
> > Enable pm runtime in driver
> 
> So power is enabled on probe and cut on _remove().  Did you consider
> doing any more fine grained PM for this device?  For example, cutting
> power after some inactivity timer and re-enabling on a
> keypress/interrupt?
> 
> Kevin
> 
> 
> 
> > Reviewed-by: Basak, Partha <p-basak2@ti.com>
> > Signed-off-by: Abraham Arce <x0066660@ti.com>
> > ---
> >  drivers/input/keyboard/omap4-keypad.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/omap4-keypad.c 
> b/drivers/input/keyboard/omap4-keypad.c
> > index 45bd097..ed47e9a 100644
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/io.h>
> >  #include <linux/input.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include <plat/omap4-keypad.h>
> >  
> > @@ -239,6 +240,9 @@ static int __devinit 
> omap4_keypad_probe(struct platform_device *pdev)
> >  	matrix_keypad_build_keymap(pdata->keymap_data, row_shift,
> >  			input_dev->keycode, input_dev->keybit);
> >  
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);

In patch 1, kbd is included in the hwmods list. Hence kbd would be reset
during init and clocks would be disabled. Probe is not doing any clock
enable before accessing the kbd registers. Later, only in this patch,
pm_runtime_get_sync() is being done.

Ideally, once the kbd is included in the hwmod list, clock_enable()
shall be used before accessing kbd registers and later in this patch
pm_runtime_get_sync() shall be used while removing usage of clk_enable().

-V Charulatha

> > +
> >  	omap4_keypad_config(keypad_data);
> >  
> >  	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
> > @@ -277,6 +281,9 @@ static int __devexit 
> omap4_keypad_remove(struct platform_device *pdev)
> >  	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> >  	struct resource *res;
> >  
> > +	pm_runtime_put_sync(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +
> >  	free_irq(keypad_data->irq, keypad_data);
> >  	input_unregister_device(keypad_data->input);

RE: [PATCH v6 8/8] Input: omap4 - pm runtime

[Datta, Shubhrajyoti]

Kevin,
> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Thursday, September 30, 2010 7:21 PM
> To: Arce, Abraham
> Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v6 8/8] Input: omap4 - pm runtime
> 
> Abraham Arce <x0066660@ti.com> writes:
> 
> > Enable pm runtime in driver
> 
> So power is enabled on probe and cut on _remove().  Did you consider
> doing any more fine grained PM for this device?  For example, cutting
> power after some inactivity timer and re-enabling on a
> keypress/interrupt?
My idea is that the clock needs to be on to get interrupts (OMAP4 the control is at module level and  ick/fclk level control is difficult). So disabling will prevent interrupts. 
The keypad is in wakeup domain which is always on. So the power impact may be minimal.

What do you think.
> 
> Kevin
> 
> 
> 
> > Reviewed-by: Basak, Partha <p-basak2@ti.com>
> > Signed-off-by: Abraham Arce <x0066660@ti.com>
> > ---
> >  drivers/input/keyboard/omap4-keypad.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/omap4-keypad.c
> b/drivers/input/keyboard/omap4-keypad.c
> > index 45bd097..ed47e9a 100644
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/io.h>
> >  #include <linux/input.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include <plat/omap4-keypad.h>
> >
> > @@ -239,6 +240,9 @@ static int __devinit omap4_keypad_probe(struct
> platform_device *pdev)
> >  	matrix_keypad_build_keymap(pdata->keymap_data, row_shift,
> >  			input_dev->keycode, input_dev->keybit);
> >
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> > +
> >  	omap4_keypad_config(keypad_data);
> >
> >  	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
> > @@ -277,6 +281,9 @@ static int __devexit omap4_keypad_remove(struct
> platform_device *pdev)
> >  	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> >  	struct resource *res;
> >
> > +	pm_runtime_put_sync(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +
> >  	free_irq(keypad_data->irq, keypad_data);
> >  	input_unregister_device(keypad_data->input);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 8/8] Input: omap4 - pm runtime

[Basak, Partha]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Datta, Shubhrajyoti
> Sent: Monday, December 06, 2010 6:30 PM
> To: Kevin Hilman; Arce, Abraham
> Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: RE: [PATCH v6 8/8] Input: omap4 - pm runtime
> 
> 
> Kevin,
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:linux-input-
> > owner@vger.kernel.org] On Behalf Of Kevin Hilman
> > Sent: Thursday, September 30, 2010 7:21 PM
> > To: Arce, Abraham
> > Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH v6 8/8] Input: omap4 - pm runtime
> >
> > Abraham Arce <x0066660@ti.com> writes:
> >
> > > Enable pm runtime in driver
> >
> > So power is enabled on probe and cut on _remove().  Did you consider
> > doing any more fine grained PM for this device?  For example, cutting
> > power after some inactivity timer and re-enabling on a
> > keypress/interrupt?
> My idea is that the clock needs to be on to get interrupts (OMAP4 the
> control is at module level and  ick/fclk level control is difficult).
> So disabling will prevent interrupts.
> The keypad is in wakeup domain which is always on. So the power impact
> may be minimal.
> 
> What do you think.

Kevin, I hope you are OK with this. 

> >
> > Kevin
> >
> >
> >
> > > Reviewed-by: Basak, Partha <p-basak2@ti.com>
> > > Signed-off-by: Abraham Arce <x0066660@ti.com>
> > > ---
> > >  drivers/input/keyboard/omap4-keypad.c |    7 +++++++
> > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/omap4-keypad.c
> > b/drivers/input/keyboard/omap4-keypad.c
> > > index 45bd097..ed47e9a 100644
> > > --- a/drivers/input/keyboard/omap4-keypad.c
> > > +++ b/drivers/input/keyboard/omap4-keypad.c
> > > @@ -29,6 +29,7 @@
> > >  #include <linux/io.h>
> > >  #include <linux/input.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/pm_runtime.h>
> > >
> > >  #include <plat/omap4-keypad.h>
> > >
> > > @@ -239,6 +240,9 @@ static int __devinit omap4_keypad_probe(struct
> > platform_device *pdev)
> > >  	matrix_keypad_build_keymap(pdata->keymap_data, row_shift,
> > >  			input_dev->keycode, input_dev->keybit);
> > >
> > > +	pm_runtime_enable(&pdev->dev);
> > > +	pm_runtime_get_sync(&pdev->dev);
> > > +
> > >  	omap4_keypad_config(keypad_data);
> > >
> > >  	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
> > > @@ -277,6 +281,9 @@ static int __devexit omap4_keypad_remove(struct
> > platform_device *pdev)
> > >  	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > >  	struct resource *res;
> > >
> > > +	pm_runtime_put_sync(&pdev->dev);
> > > +	pm_runtime_disable(&pdev->dev);
> > > +
> > >  	free_irq(keypad_data->irq, keypad_data);
> > >  	input_unregister_device(keypad_data->input);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 8/8] Input: omap4 - pm runtime

[Kevin Hilman]

"Datta, Shubhrajyoti" <shubhrajyoti@ti.com> writes:

[...]

>> Subject: Re: [PATCH v6 8/8] Input: omap4 - pm runtime
>> 
>> Abraham Arce <x0066660@ti.com> writes:
>> 
>> > Enable pm runtime in driver
>> 
>> So power is enabled on probe and cut on _remove().  Did you consider
>> doing any more fine grained PM for this device?  For example, cutting
>> power after some inactivity timer and re-enabling on a
>> keypress/interrupt?

> My idea is that the clock needs to be on to get interrupts (OMAP4 the
> control is at module level and ick/fclk level control is
> difficult). So disabling will prevent interrupts.  The keypad is in
> wakeup domain which is always on. So the power impact may be minimal.
>
> What do you think.

I think the changelog needs to be updated to describe the reasons behind
the decisions made.

Kevin