[PATCH] video: s3c-fb: make runtime pm functions static

[PATCH] video: s3c-fb: make runtime pm functions static

[Jingoo Han]

This patch makes runtime pm functions static.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/s3c-fb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 6817d18..832c7e5 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1547,7 +1547,7 @@ static int s3c_fb_resume(struct device *dev)
 	return 0;
 }
 
-int s3c_fb_runtime_suspend(struct device *dev)
+static int s3c_fb_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct s3c_fb *sfb = platform_get_drvdata(pdev);
@@ -1567,7 +1567,7 @@ int s3c_fb_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-int s3c_fb_runtime_resume(struct device *dev)
+static int s3c_fb_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct s3c_fb *sfb = platform_get_drvdata(pdev);
-- 
1.7.1

Re: [PATCH] video: s3c-fb: make runtime pm functions static

[Paul Mundt]

On Mon, Apr 11, 2011 at 04:22:58PM +0900, Jingoo Han wrote:
> This patch makes runtime pm functions static.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

On Mon, Apr 11, 2011 at 04:25:37PM +0900, Jingoo Han wrote:
> The spinlock is added to interrupt routine to ensure that the driver
> is protected against multiple accesses.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

Both applied, thanks.

[PATCH] video: s3c-fb: Make runtime PM functional again

[Mark Brown]

The change in "video: s3c-fb: modify runtime pm functions" (commit
35784b) renders the runtime power management for the device completely
ineffectual as while it leaves runtime power management notionally
enabled a runtime power reference is held for the entire time the device
is registered meaning it will never actually do anything.

A further issue is introduced as runtime power management is added
during the system suspend path which is not something which drivers are
supposed to do and would interact poorly if there were any operations
done in the runtime power management callbacks.

While this does make things simpler (the main motivation for the
original change) it will not only cause us to use more power in the
framebuffer controller but will also prevent us entering lower power
domain and SoC wide states as we can never power down the domain
containing the device.  Since neither of these things is desirable
revert the change.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/video/s3c-fb.c |   51 +++++++++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index e531ebc..690b44e 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1036,8 +1036,30 @@ static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	return ret;
 }
 
+static int s3c_fb_open(struct fb_info *info, int user)
+{
+	struct s3c_fb_win *win = info->par;
+	struct s3c_fb *sfb = win->parent;
+
+	pm_runtime_get_sync(sfb->dev);
+
+	return 0;
+}
+
+static int s3c_fb_release(struct fb_info *info, int user)
+{
+	struct s3c_fb_win *win = info->par;
+	struct s3c_fb *sfb = win->parent;
+
+	pm_runtime_put_sync(sfb->dev);
+
+	return 0;
+}
+
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
+	.fb_open	= s3c_fb_open,
+	.fb_release	= s3c_fb_release,
 	.fb_check_var	= s3c_fb_check_var,
 	.fb_set_par	= s3c_fb_set_par,
 	.fb_blank	= s3c_fb_blank,
@@ -1436,6 +1458,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, sfb);
+	pm_runtime_put_sync(sfb->dev);
 
 	return 0;
 
@@ -1468,6 +1491,8 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	struct s3c_fb *sfb = platform_get_drvdata(pdev);
 	int win;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	for (win = 0; win < S3C_FB_MAX_WIN; win++)
 		if (sfb->windows[win])
 			s3c_fb_release_win(sfb, sfb->windows[win]);
@@ -1488,7 +1513,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 static int s3c_fb_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1509,8 +1534,6 @@ static int s3c_fb_suspend(struct device *dev)
 		clk_disable(sfb->lcd_clk);
 
 	clk_disable(sfb->bus_clk);
-	pm_runtime_put_sync(sfb->dev);
-
 	return 0;
 }
 
@@ -1522,7 +1545,6 @@ static int s3c_fb_resume(struct device *dev)
 	struct s3c_fb_win *win;
 	int win_no;
 
-	pm_runtime_get_sync(sfb->dev);
 	clk_enable(sfb->bus_clk);
 
 	if (!sfb->variant.has_clksel)
@@ -1561,19 +1583,11 @@ static int s3c_fb_resume(struct device *dev)
 
 	return 0;
 }
+#else
+#define s3c_fb_suspend NULL
+#define s3c_fb_resume  NULL
 #endif
 
-#ifdef CONFIG_PM_RUNTIME
-static int s3c_fb_runtime_suspend(struct device *dev)
-{
-	return 0;
-}
-
-static int s3c_fb_runtime_resume(struct device *dev)
-{
-	return 0;
-}
-#endif
 
 #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
 #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))
@@ -1896,10 +1910,7 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
 
-static const struct dev_pm_ops s3c_fb_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(s3c_fb_suspend, s3c_fb_resume)
-	SET_RUNTIME_PM_OPS(s3c_fb_runtime_suspend, s3c_fb_runtime_resume, NULL)
-};
+static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
 
 static struct platform_driver s3c_fb_driver = {
 	.probe		= s3c_fb_probe,
@@ -1908,7 +1919,7 @@ static struct platform_driver s3c_fb_driver = {
 	.driver		= {
 		.name	= "s3c-fb",
 		.owner	= THIS_MODULE,
-		.pm	= &s3c_fb_pm_ops,
+		.pm	= &s3cfb_pm_ops,
 	},
 };
 
-- 
1.7.7.3

RE: [PATCH] video: s3c-fb: Make runtime PM functional again

[Jingoo Han]

Hi, Mark

Mark wrote:
> -----Original Message-----
> Subject: [PATCH] video: s3c-fb: Make runtime PM functional again
> 
> The change in "video: s3c-fb: modify runtime pm functions" (commit
> 35784b) renders the runtime power management for the device completely
> ineffectual as while it leaves runtime power management notionally
> enabled a runtime power reference is held for the entire time the device
> is registered meaning it will never actually do anything.
> 
> A further issue is introduced as runtime power management is added
> during the system suspend path which is not something which drivers are
> supposed to do and would interact poorly if there were any operations
> done in the runtime power management callbacks.
> 
> While this does make things simpler (the main motivation for the
> original change) it will not only cause us to use more power in the
> framebuffer controller but will also prevent us entering lower power
> domain and SoC wide states as we can never power down the domain
> containing the device.  Since neither of these things is desirable
> revert the change.
The main difference is as follows:
If no fb windows are opened.
	- Your patch: LCD block power is off
	- My patch: LCD block power is on still.

However, after booting, probing, open is called from platform system, soon.
And, the default window is always opened. This means that at least one window
is opened after booting. So, I don't think that "video: s3c-fb: modify runtime
pm functions" (commit: 35784b) cause us to use more power in the framebuffer
controller.

My intention is that runtime pm is used only for LCD block power off when
suspend and resume are called.

> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/video/s3c-fb.c |   51 +++++++++++++++++++++++++++++------------------
>  1 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index e531ebc..690b44e 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1036,8 +1036,30 @@ static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  	return ret;
>  }
> 
> +static int s3c_fb_open(struct fb_info *info, int user)
> +{
> +	struct s3c_fb_win *win = info->par;
> +	struct s3c_fb *sfb = win->parent;
> +
> +	pm_runtime_get_sync(sfb->dev);
> +
> +	return 0;
> +}
> +
> +static int s3c_fb_release(struct fb_info *info, int user)
> +{
> +	struct s3c_fb_win *win = info->par;
> +	struct s3c_fb *sfb = win->parent;
> +
> +	pm_runtime_put_sync(sfb->dev);
> +
> +	return 0;
> +}
> +
>  static struct fb_ops s3c_fb_ops = {
>  	.owner		= THIS_MODULE,
> +	.fb_open	= s3c_fb_open,
> +	.fb_release	= s3c_fb_release,
>  	.fb_check_var	= s3c_fb_check_var,
>  	.fb_set_par	= s3c_fb_set_par,
>  	.fb_blank	= s3c_fb_blank,
> @@ -1436,6 +1458,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>  	}
> 
>  	platform_set_drvdata(pdev, sfb);
> +	pm_runtime_put_sync(sfb->dev);
> 
>  	return 0;
> 
> @@ -1468,6 +1491,8 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
>  	struct s3c_fb *sfb = platform_get_drvdata(pdev);
>  	int win;
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	for (win = 0; win < S3C_FB_MAX_WIN; win++)
>  		if (sfb->windows[win])
>  			s3c_fb_release_win(sfb, sfb->windows[win]);
> @@ -1488,7 +1513,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
>  static int s3c_fb_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -1509,8 +1534,6 @@ static int s3c_fb_suspend(struct device *dev)
>  		clk_disable(sfb->lcd_clk);
> 
>  	clk_disable(sfb->bus_clk);
> -	pm_runtime_put_sync(sfb->dev);
> -
>  	return 0;
>  }
> 
> @@ -1522,7 +1545,6 @@ static int s3c_fb_resume(struct device *dev)
>  	struct s3c_fb_win *win;
>  	int win_no;
> 
> -	pm_runtime_get_sync(sfb->dev);
>  	clk_enable(sfb->bus_clk);
> 
>  	if (!sfb->variant.has_clksel)
> @@ -1561,19 +1583,11 @@ static int s3c_fb_resume(struct device *dev)
> 
>  	return 0;
>  }
> +#else
> +#define s3c_fb_suspend NULL
> +#define s3c_fb_resume  NULL
>  #endif
> 
> -#ifdef CONFIG_PM_RUNTIME
> -static int s3c_fb_runtime_suspend(struct device *dev)
> -{
> -	return 0;
> -}
> -
> -static int s3c_fb_runtime_resume(struct device *dev)
> -{
> -	return 0;
> -}
> -#endif
> 
>  #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
>  #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))
> @@ -1896,10 +1910,7 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
> 
> -static const struct dev_pm_ops s3c_fb_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(s3c_fb_suspend, s3c_fb_resume)
> -	SET_RUNTIME_PM_OPS(s3c_fb_runtime_suspend, s3c_fb_runtime_resume, NULL)
> -};
> +static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
> 
>  static struct platform_driver s3c_fb_driver = {
>  	.probe		= s3c_fb_probe,
> @@ -1908,7 +1919,7 @@ static struct platform_driver s3c_fb_driver = {
>  	.driver		= {
>  		.name	= "s3c-fb",
>  		.owner	= THIS_MODULE,
> -		.pm	= &s3c_fb_pm_ops,
> +		.pm	= &s3cfb_pm_ops,
>  	},
>  };
> 
> --
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video: s3c-fb: Make runtime PM functional again

[Mark Brown]

On Tue, Dec 27, 2011 at 01:40:47PM +0900, Jingoo Han wrote:

> > While this does make things simpler (the main motivation for the
> > original change) it will not only cause us to use more power in the
> > framebuffer controller but will also prevent us entering lower power
> > domain and SoC wide states as we can never power down the domain
> > containing the device.  Since neither of these things is desirable
> > revert the change.

> The main difference is as follows:
> If no fb windows are opened.
> 	- Your patch: LCD block power is off
> 	- My patch: LCD block power is on still.

By holding the LCD block power on your patch will also prevent the SoC
using lower power modes like STOP and DEEP-STOP on the S3C6410.

> However, after booting, probing, open is called from platform system, soon.
> And, the default window is always opened. This means that at least one window

This only happens if you enable CONFIG_FRAMEBUFFER_CONSOLE as that
causes the console code to become a framebuffer client.

> is opened after booting. So, I don't think that "video: s3c-fb: modify runtime
> pm functions" (commit: 35784b) cause us to use more power in the framebuffer
> controller.

If the console isn't using the framebuffer then whenever userspace
releases we'll be able to runtime suspend the framebuffer.  This isn't
ideal as what userspace expects is that it can hold the device open and
use blanking to save power - I'm working on patches to do this - but
it's a start.

> My intention is that runtime pm is used only for LCD block power off when
> suspend and resume are called.

This isn't runtime PM at all, it's system PM which for some reason calls
runtime PM functions.  At best the runtime PM calls won't do anything as
they're inhibited during system PM.