[PATCH 0/6] video: s3c-fb: Use runtime suspend while blanked

[PATCH 0/6] video: s3c-fb: Use runtime suspend while blanked

[Mark Brown]

This patch series (the first two of which I posted the other day)
improves the runtime power management in the s3c-fb driver by moving it
to keeping the device runtime suspended when the screen is powered down
by userspace.  This saves a trivial amount of power in the framebuffer
controller and allows the SoC core code to enter system wide idle
states which offer much more substantial gains.

Currently it's only lightly tested but it certainly seems to the right
thing for me.

Mark Brown (6):
      video: s3c-fb: Make runtime PM functional again
      video: s3c-fb: Use s3c_fb_enable() to enable the framebuffer
      video: s3c-fb: Disable runtime PM in error paths from probe
      video: s3c-fb: Take a runtime PM reference when unblanked
      video: s3c-fb: Hold runtime PM references when touching registers
      video: s3c-fb: Don't keep device runtime active when open

 drivers/video/s3c-fb.c |  127 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 92 insertions(+), 35 deletions(-)

[PATCH 1/6] 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 a0b3fd6..b1a75a0 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1038,8 +1038,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,
@@ -1446,6 +1468,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, sfb);
+	pm_runtime_put_sync(sfb->dev);
 
 	return 0;
 
@@ -1485,6 +1508,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]);
@@ -1510,7 +1535,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);
@@ -1531,8 +1556,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;
 }
 
@@ -1544,7 +1567,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)
@@ -1583,19 +1605,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))
@@ -1918,10 +1932,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,
@@ -1930,7 +1941,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

[PATCH 2/6] video: s3c-fb: Use s3c_fb_enable() to enable the framebuffer

[Mark Brown]

The s3c-fb driver has a function called s3c_fb_enable() which turns on
and off the physical output. However it is only actually used in paths
which disable the screen, the enabling just writes to the register. Make
the code less confusing by ensuring that the enable also goes through
the same path.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/s3c-fb.c |   55 ++++++++++++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index b1a75a0..be4c218 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -441,6 +441,32 @@ static void shadow_protect_win(struct s3c_fb_win *win, bool protect)
 }
 
 /**
+ * s3c_fb_enable() - Set the state of the main LCD output
+ * @sfb: The main framebuffer state.
+ * @enable: The state to set.
+ */
+static void s3c_fb_enable(struct s3c_fb *sfb, int enable)
+{
+	u32 vidcon0 = readl(sfb->regs + VIDCON0);
+
+	if (enable)
+		vidcon0 |= VIDCON0_ENVID | VIDCON0_ENVID_F;
+	else {
+		/* see the note in the framebuffer datasheet about
+		 * why you cannot take both of these bits down at the
+		 * same time. */
+
+		if (!(vidcon0 & VIDCON0_ENVID))
+			return;
+
+		vidcon0 |= VIDCON0_ENVID;
+		vidcon0 &= ~VIDCON0_ENVID_F;
+	}
+
+	writel(vidcon0, sfb->regs + VIDCON0);
+}
+
+/**
  * s3c_fb_set_par() - framebuffer request to set new framebuffer state.
  * @info: The framebuffer to change.
  *
@@ -510,9 +536,10 @@ static int s3c_fb_set_par(struct fb_info *info)
 		if (sfb->variant.is_2443)
 			data |= (1 << 5);
 
-		data |= VIDCON0_ENVID | VIDCON0_ENVID_F;
 		writel(data, regs + VIDCON0);
 
+		s3c_fb_enable(sfb, 1);
+
 		data = VIDTCON0_VBPD(var->upper_margin - 1) |
 		       VIDTCON0_VFPD(var->lower_margin - 1) |
 		       VIDTCON0_VSPW(var->vsync_len - 1);
@@ -761,32 +788,6 @@ static int s3c_fb_setcolreg(unsigned regno,
 }
 
 /**
- * s3c_fb_enable() - Set the state of the main LCD output
- * @sfb: The main framebuffer state.
- * @enable: The state to set.
- */
-static void s3c_fb_enable(struct s3c_fb *sfb, int enable)
-{
-	u32 vidcon0 = readl(sfb->regs + VIDCON0);
-
-	if (enable)
-		vidcon0 |= VIDCON0_ENVID | VIDCON0_ENVID_F;
-	else {
-		/* see the note in the framebuffer datasheet about
-		 * why you cannot take both of these bits down at the
-		 * same time. */
-
-		if (!(vidcon0 & VIDCON0_ENVID))
-			return;
-
-		vidcon0 |= VIDCON0_ENVID;
-		vidcon0 &= ~VIDCON0_ENVID_F;
-	}
-
-	writel(vidcon0, sfb->regs + VIDCON0);
-}
-
-/**
  * s3c_fb_blank() - blank or unblank the given window
  * @blank_mode: The blank state from FB_BLANK_*
  * @info: The framebuffer to blank.
-- 
1.7.7.3

[PATCH 3/6] video: s3c-fb: Disable runtime PM in error paths from probe

[Mark Brown]

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

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index be4c218..2e0eef0 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1464,7 +1464,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 			dev_err(dev, "failed to create window %d\n", win);
 			for (; win >= 0; win--)
 				s3c_fb_release_win(sfb, sfb->windows[win]);
-			goto err_irq;
+			goto err_pm_runtime;
 		}
 	}
 
@@ -1473,7 +1473,8 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_irq:
+err_pm_runtime:
+	pm_runtime_put_sync(sfb->dev);
 	free_irq(sfb->irq_no, sfb);
 
 err_ioremap:
@@ -1483,6 +1484,8 @@ err_req_region:
 	release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
 
 err_lcd_clk:
+	pm_runtime_disable(sfb->dev);
+
 	if (!sfb->variant.has_clksel) {
 		clk_disable(sfb->lcd_clk);
 		clk_put(sfb->lcd_clk);
-- 
1.7.7.3

[PATCH 4/6] video: s3c-fb: Take a runtime PM reference when unblanked

[Mark Brown]

When the framebuffer is unblanked hold a runtime PM reference. This
prevents us powering down when userspace has left an image on the
framebuffer and prepares the way for being able to power down the hardware
when an application still has the device open.

Since we now hold a runtime PM reference whenever the display is unblanked
there is no need for the runtime power management to disable and enable
the display, and doing so would lead to runtime PM trying to recurse into
itself when called from the blanking code, so split the runtime PM into
separate functions which only deal with the clocks.  The PM core will
runtime resume the device prior to system suspend.

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

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 2e0eef0..688b9d8 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -192,6 +192,7 @@ struct s3c_fb_vsync {
  * @regs: The mapped hardware registers.
  * @variant: Variant information for this hardware.
  * @enabled: A bitmask of enabled hardware windows.
+ * @output_on: Flag if the physical output is enabled.
  * @pdata: The platform configuration data passed with the device.
  * @windows: The hardware windows that have been claimed.
  * @irq_no: IRQ line number
@@ -208,6 +209,7 @@ struct s3c_fb {
 	struct s3c_fb_variant	 variant;
 
 	unsigned char		 enabled;
+	bool			 output_on;
 
 	struct s3c_fb_platdata	*pdata;
 	struct s3c_fb_win	*windows[S3C_FB_MAX_WIN];
@@ -449,21 +451,28 @@ static void s3c_fb_enable(struct s3c_fb *sfb, int enable)
 {
 	u32 vidcon0 = readl(sfb->regs + VIDCON0);
 
-	if (enable)
+	if (enable && !sfb->output_on)
+		pm_runtime_get_sync(sfb->dev);
+
+	if (enable) {
 		vidcon0 |= VIDCON0_ENVID | VIDCON0_ENVID_F;
-	else {
+	} else {
 		/* see the note in the framebuffer datasheet about
 		 * why you cannot take both of these bits down at the
 		 * same time. */
 
-		if (!(vidcon0 & VIDCON0_ENVID))
-			return;
-
-		vidcon0 |= VIDCON0_ENVID;
-		vidcon0 &= ~VIDCON0_ENVID_F;
+		if (vidcon0 & VIDCON0_ENVID) {
+			vidcon0 |= VIDCON0_ENVID;
+			vidcon0 &= ~VIDCON0_ENVID_F;
+		}
 	}
 
 	writel(vidcon0, sfb->regs + VIDCON0);
+
+	if (!enable && sfb->output_on)
+		pm_runtime_put_sync(sfb->dev);
+
+	sfb->output_on = enable;
 }
 
 /**
@@ -1539,7 +1548,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int s3c_fb_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1609,11 +1618,40 @@ 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)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_fb *sfb = platform_get_drvdata(pdev);
+
+	if (!sfb->variant.has_clksel)
+		clk_disable(sfb->lcd_clk);
+
+	clk_disable(sfb->bus_clk);
+
+	return 0;
+}
+
+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);
+	struct s3c_fb_platdata *pd = sfb->pdata;
+
+	clk_enable(sfb->bus_clk);
+
+	if (!sfb->variant.has_clksel)
+		clk_enable(sfb->lcd_clk);
+
+	/* setup gpio and output polarity controls */
+	pd->setup_gpio();
+	writel(pd->vidcon1, sfb->regs + VIDCON1);
+
+	return 0;
+}
+#endif
 
 #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
 #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))
@@ -1936,7 +1974,11 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
 
-static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
+static const struct dev_pm_ops s3cfb_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 struct platform_driver s3c_fb_driver = {
 	.probe		= s3c_fb_probe,
-- 
1.7.7.3

[PATCH 5/6] video: s3c-fb: Hold runtime PM references when touching registers

[Mark Brown]

Take a runtime PM reference whenever updating registers in preparation
for suspending the device when the framebuffer is blanked.

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

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 688b9d8..84cf631 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -496,6 +496,8 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
 
+	pm_runtime_get_sync(sfb->dev);
+
 	shadow_protect_win(win, 1);
 
 	switch (var->bits_per_pixel) {
@@ -692,6 +694,8 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	shadow_protect_win(win, 0);
 
+	pm_runtime_put_sync(sfb->dev);
+
 	return 0;
 }
 
@@ -763,6 +767,8 @@ static int s3c_fb_setcolreg(unsigned regno,
 	dev_dbg(sfb->dev, "%s: win %d: %d => rgb=%d/%d/%d\n",
 		__func__, win->index, regno, red, green, blue);
 
+	pm_runtime_get_sync(sfb->dev);
+
 	switch (info->fix.visual) {
 	case FB_VISUAL_TRUECOLOR:
 		/* true-colour, use pseudo-palette */
@@ -790,9 +796,11 @@ static int s3c_fb_setcolreg(unsigned regno,
 		break;
 
 	default:
+		pm_runtime_put_sync(sfb->dev);
 		return 1;	/* unknown type */
 	}
 
+	pm_runtime_put_sync(sfb->dev);
 	return 0;
 }
 
@@ -812,6 +820,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 
 	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
 
+	pm_runtime_get_sync(sfb->dev);
+
 	wincon = readl(sfb->regs + sfb->variant.wincon + (index * 4));
 
 	switch (blank_mode) {
@@ -839,6 +849,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	case FB_BLANK_VSYNC_SUSPEND:
 	case FB_BLANK_HSYNC_SUSPEND:
 	default:
+		pm_runtime_put_sync(sfb->dev);
 		return 1;
 	}
 
@@ -869,6 +880,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 		shadow_protect_win(win, 0);
 	}
 
+	pm_runtime_put_sync(sfb->dev);
+
 	return 0;
 }
 
@@ -891,6 +904,8 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 	void __iomem *buf	= sfb->regs + win->index * 8;
 	unsigned int start_boff, end_boff;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	/* Offset in bytes to the start of the displayed area */
 	start_boff = var->yoffset * info->fix.line_length;
 	/* X offset depends on the current bpp */
@@ -909,6 +924,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 			break;
 		default:
 			dev_err(sfb->dev, "invalid bpp\n");
+			pm_runtime_put_sync(sfb->dev);
 			return -EINVAL;
 		}
 	}
@@ -924,6 +940,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 
 	shadow_protect_win(win, 0);
 
+	pm_runtime_put_sync(sfb->dev);
 	return 0;
 }
 
@@ -1013,11 +1030,16 @@ static int s3c_fb_wait_for_vsync(struct s3c_fb *sfb, u32 crtc)
 	if (crtc != 0)
 		return -ENODEV;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	count = sfb->vsync_info.count;
 	s3c_fb_enable_irq(sfb);
 	ret = wait_event_interruptible_timeout(sfb->vsync_info.wait,
 				       count != sfb->vsync_info.count,
 				       msecs_to_jiffies(VSYNC_TIMEOUT_MSEC));
+
+	pm_runtime_put_sync(sfb->dev);
+
 	if (ret == 0)
 		return -ETIMEDOUT;
 
-- 
1.7.7.3

[PATCH 6/6] video: s3c-fb: Don't keep device runtime active when open

[Mark Brown]

Allow the controller to be runtime suspended when the screen is blanked
by not taking a runtime reference while the device is open. This allows
greater system wide power savings when used with a standard application
layer and ensures that the screen does not blank unless requested.

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

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 84cf631..0c63b69 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1070,30 +1070,8 @@ 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,
-- 
1.7.7.3

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

[Jingoo Han]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, December 27, 2011 11:16 PM
> To: Jingoo Han; Florian Tobias Schandinat
> Cc: linux-fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Brown
> Subject: [PATCH 1/6] 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.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

	Acked-by: Jingoo Han <jg1.han@samsung.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 a0b3fd6..b1a75a0 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1038,8 +1038,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,
> @@ -1446,6 +1468,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>  	}
> 
>  	platform_set_drvdata(pdev, sfb);
> +	pm_runtime_put_sync(sfb->dev);
> 
>  	return 0;
> 
> @@ -1485,6 +1508,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]);
> @@ -1510,7 +1535,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);
> @@ -1531,8 +1556,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;
>  }
> 
> @@ -1544,7 +1567,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)
> @@ -1583,19 +1605,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))
> @@ -1918,10 +1932,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,
> @@ -1930,7 +1941,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 3/6] video: s3c-fb: Disable runtime PM in error paths from probe

[Jingoo Han]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, December 27, 2011 11:16 PM
> To: Jingoo Han; Florian Tobias Schandinat
> Cc: linux-fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Brown
> Subject: [PATCH 3/6] video: s3c-fb: Disable runtime PM in error paths from probe
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

		Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  drivers/video/s3c-fb.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index be4c218..2e0eef0 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1464,7 +1464,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>  			dev_err(dev, "failed to create window %d\n", win);
>  			for (; win >= 0; win--)
>  				s3c_fb_release_win(sfb, sfb->windows[win]);
> -			goto err_irq;
> +			goto err_pm_runtime;
>  		}
>  	}
> 
> @@ -1473,7 +1473,8 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> 
>  	return 0;
> 
> -err_irq:
> +err_pm_runtime:
> +	pm_runtime_put_sync(sfb->dev);
>  	free_irq(sfb->irq_no, sfb);
> 
>  err_ioremap:
> @@ -1483,6 +1484,8 @@ err_req_region:
>  	release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
> 
>  err_lcd_clk:
> +	pm_runtime_disable(sfb->dev);
> +
>  	if (!sfb->variant.has_clksel) {
>  		clk_disable(sfb->lcd_clk);
>  		clk_put(sfb->lcd_clk);
> --
> 1.7.7.3

RE: [PATCH 4/6] video: s3c-fb: Take a runtime PM reference when unblanked

[Jingoo Han]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, December 27, 2011 11:16 PM
> To: Jingoo Han; Florian Tobias Schandinat
> Cc: linux-fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Brown
> Subject: [PATCH 4/6] video: s3c-fb: Take a runtime PM reference when unblanked
> 
> When the framebuffer is unblanked hold a runtime PM reference. This
> prevents us powering down when userspace has left an image on the
> framebuffer and prepares the way for being able to power down the hardware
> when an application still has the device open.
> 
> Since we now hold a runtime PM reference whenever the display is unblanked
> there is no need for the runtime power management to disable and enable
> the display, and doing so would lead to runtime PM trying to recurse into
> itself when called from the blanking code, so split the runtime PM into
> separate functions which only deal with the clocks.  The PM core will
> runtime resume the device prior to system suspend.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

		Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  drivers/video/s3c-fb.c |   66 +++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 2e0eef0..688b9d8 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -192,6 +192,7 @@ struct s3c_fb_vsync {
>   * @regs: The mapped hardware registers.
>   * @variant: Variant information for this hardware.
>   * @enabled: A bitmask of enabled hardware windows.
> + * @output_on: Flag if the physical output is enabled.
>   * @pdata: The platform configuration data passed with the device.
>   * @windows: The hardware windows that have been claimed.
>   * @irq_no: IRQ line number
> @@ -208,6 +209,7 @@ struct s3c_fb {
>  	struct s3c_fb_variant	 variant;
> 
>  	unsigned char		 enabled;
> +	bool			 output_on;
> 
>  	struct s3c_fb_platdata	*pdata;
>  	struct s3c_fb_win	*windows[S3C_FB_MAX_WIN];
> @@ -449,21 +451,28 @@ static void s3c_fb_enable(struct s3c_fb *sfb, int enable)
>  {
>  	u32 vidcon0 = readl(sfb->regs + VIDCON0);
> 
> -	if (enable)
> +	if (enable && !sfb->output_on)
> +		pm_runtime_get_sync(sfb->dev);
> +
> +	if (enable) {
>  		vidcon0 |= VIDCON0_ENVID | VIDCON0_ENVID_F;
> -	else {
> +	} else {
>  		/* see the note in the framebuffer datasheet about
>  		 * why you cannot take both of these bits down at the
>  		 * same time. */
> 
> -		if (!(vidcon0 & VIDCON0_ENVID))
> -			return;
> -
> -		vidcon0 |= VIDCON0_ENVID;
> -		vidcon0 &= ~VIDCON0_ENVID_F;
> +		if (vidcon0 & VIDCON0_ENVID) {
> +			vidcon0 |= VIDCON0_ENVID;
> +			vidcon0 &= ~VIDCON0_ENVID_F;
> +		}
>  	}
> 
>  	writel(vidcon0, sfb->regs + VIDCON0);
> +
> +	if (!enable && sfb->output_on)
> +		pm_runtime_put_sync(sfb->dev);
> +
> +	sfb->output_on = enable;
>  }
> 
>  /**
> @@ -1539,7 +1548,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
>  static int s3c_fb_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -1609,11 +1618,40 @@ 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)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct s3c_fb *sfb = platform_get_drvdata(pdev);
> +
> +	if (!sfb->variant.has_clksel)
> +		clk_disable(sfb->lcd_clk);
> +
> +	clk_disable(sfb->bus_clk);
> +
> +	return 0;
> +}
> +
> +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);
> +	struct s3c_fb_platdata *pd = sfb->pdata;
> +
> +	clk_enable(sfb->bus_clk);
> +
> +	if (!sfb->variant.has_clksel)
> +		clk_enable(sfb->lcd_clk);
> +
> +	/* setup gpio and output polarity controls */
> +	pd->setup_gpio();
> +	writel(pd->vidcon1, sfb->regs + VIDCON1);
> +
> +	return 0;
> +}
> +#endif
> 
>  #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
>  #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))
> @@ -1936,7 +1974,11 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
> 
> -static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
> +static const struct dev_pm_ops s3cfb_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 struct platform_driver s3c_fb_driver = {
>  	.probe		= s3c_fb_probe,
> --
> 1.7.7.3

RE: [PATCH 5/6] video: s3c-fb: Hold runtime PM references when touching registers

[Jingoo Han]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, December 27, 2011 11:16 PM
> To: Jingoo Han; Florian Tobias Schandinat
> Cc: linux-fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Brown
> Subject: [PATCH 5/6] video: s3c-fb: Hold runtime PM references when touching registers
> 
> Take a runtime PM reference whenever updating registers in preparation
> for suspending the device when the framebuffer is blanked.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

		Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  drivers/video/s3c-fb.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 688b9d8..84cf631 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -496,6 +496,8 @@ static int s3c_fb_set_par(struct fb_info *info)
> 
>  	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	shadow_protect_win(win, 1);
> 
>  	switch (var->bits_per_pixel) {
> @@ -692,6 +694,8 @@ static int s3c_fb_set_par(struct fb_info *info)
> 
>  	shadow_protect_win(win, 0);
> 
> +	pm_runtime_put_sync(sfb->dev);
> +
>  	return 0;
>  }
> 
> @@ -763,6 +767,8 @@ static int s3c_fb_setcolreg(unsigned regno,
>  	dev_dbg(sfb->dev, "%s: win %d: %d => rgb=%d/%d/%d\n",
>  		__func__, win->index, regno, red, green, blue);
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	switch (info->fix.visual) {
>  	case FB_VISUAL_TRUECOLOR:
>  		/* true-colour, use pseudo-palette */
> @@ -790,9 +796,11 @@ static int s3c_fb_setcolreg(unsigned regno,
>  		break;
> 
>  	default:
> +		pm_runtime_put_sync(sfb->dev);
>  		return 1;	/* unknown type */
>  	}
> 
> +	pm_runtime_put_sync(sfb->dev);
>  	return 0;
>  }
> 
> @@ -812,6 +820,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
> 
>  	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	wincon = readl(sfb->regs + sfb->variant.wincon + (index * 4));
> 
>  	switch (blank_mode) {
> @@ -839,6 +849,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  	case FB_BLANK_VSYNC_SUSPEND:
>  	case FB_BLANK_HSYNC_SUSPEND:
>  	default:
> +		pm_runtime_put_sync(sfb->dev);
>  		return 1;
>  	}
> 
> @@ -869,6 +880,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
>  		shadow_protect_win(win, 0);
>  	}
> 
> +	pm_runtime_put_sync(sfb->dev);
> +
>  	return 0;
>  }
> 
> @@ -891,6 +904,8 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
>  	void __iomem *buf	= sfb->regs + win->index * 8;
>  	unsigned int start_boff, end_boff;
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	/* Offset in bytes to the start of the displayed area */
>  	start_boff = var->yoffset * info->fix.line_length;
>  	/* X offset depends on the current bpp */
> @@ -909,6 +924,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
>  			break;
>  		default:
>  			dev_err(sfb->dev, "invalid bpp\n");
> +			pm_runtime_put_sync(sfb->dev);
>  			return -EINVAL;
>  		}
>  	}
> @@ -924,6 +940,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
> 
>  	shadow_protect_win(win, 0);
> 
> +	pm_runtime_put_sync(sfb->dev);
>  	return 0;
>  }
> 
> @@ -1013,11 +1030,16 @@ static int s3c_fb_wait_for_vsync(struct s3c_fb *sfb, u32 crtc)
>  	if (crtc != 0)
>  		return -ENODEV;
> 
> +	pm_runtime_get_sync(sfb->dev);
> +
>  	count = sfb->vsync_info.count;
>  	s3c_fb_enable_irq(sfb);
>  	ret = wait_event_interruptible_timeout(sfb->vsync_info.wait,
>  				       count != sfb->vsync_info.count,
>  				       msecs_to_jiffies(VSYNC_TIMEOUT_MSEC));
> +
> +	pm_runtime_put_sync(sfb->dev);
> +
>  	if (ret == 0)
>  		return -ETIMEDOUT;
> 
> --
> 1.7.7.3

RE: [PATCH 6/6] video: s3c-fb: Don't keep device runtime active when open

[Jingoo Han]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, December 27, 2011 11:16 PM
> To: Jingoo Han; Florian Tobias Schandinat
> Cc: linux-fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Brown
> Subject: [PATCH 6/6] video: s3c-fb: Don't keep device runtime active when open
> 
> Allow the controller to be runtime suspended when the screen is blanked
> by not taking a runtime reference while the device is open. This allows
> greater system wide power savings when used with a standard application
> layer and ensures that the screen does not blank unless requested.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

		Acked-by: Jingoo Han <jg1.han@samsung.com>

> ---
>  drivers/video/s3c-fb.c |   22 ----------------------
>  1 files changed, 0 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 84cf631..0c63b69 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1070,30 +1070,8 @@ 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,
> --
> 1.7.7.3

Re: [PATCH 0/6] video: s3c-fb: Use runtime suspend while blanked

[Florian Tobias Schandinat]

On 12/27/2011 02:15 PM, Mark Brown wrote:
> This patch series (the first two of which I posted the other day)
> improves the runtime power management in the s3c-fb driver by moving it
> to keeping the device runtime suspended when the screen is powered down
> by userspace.  This saves a trivial amount of power in the framebuffer
> controller and allows the SoC core code to enter system wide idle
> states which offer much more substantial gains.
> 
> Currently it's only lightly tested but it certainly seems to the right
> thing for me.

Applied all 6 patches of this series.


Thanks,

Florian Tobias Schandinat

> 
> Mark Brown (6):
>       video: s3c-fb: Make runtime PM functional again
>       video: s3c-fb: Use s3c_fb_enable() to enable the framebuffer
>       video: s3c-fb: Disable runtime PM in error paths from probe
>       video: s3c-fb: Take a runtime PM reference when unblanked
>       video: s3c-fb: Hold runtime PM references when touching registers
>       video: s3c-fb: Don't keep device runtime active when open
> 
>  drivers/video/s3c-fb.c |  127 ++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 92 insertions(+), 35 deletions(-)
> 
>