RE: [PATCH] s3c-fb: add support for runtime pm

[Jonghun Han <jonghun.han@samsung.com>]

Hi Jingoo,

How about adding pm_runtime_* in s3c_fb_blank ?
In FB_BLANK_POWERDOWN state no need to enable clock or power-domain.

Best regards,

> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> owner@vger.kernel.org] On Behalf Of Jonghun Han
> Sent: Tuesday, December 21, 2010 5:30 PM
> To: 'Jingoo Han'; linux-fbdev@vger.kernel.org; 'Paul Mundt';
linux-samsung-
> soc@vger.kernel.org
> Cc: ben-linux@fluff.org; 'Ilho Lee'
> Subject: RE: [PATCH] s3c-fb: add support for runtime pm
> 
> Hi Jingoo,
> 
> Here is a quick review.
> 
> Best regards,
> 
> Jingoo Han wrote:
> 
> > This patch adds support for runtime pm using the functions.
> >  - pm_runtime_get_sync()
> >  - pm_runtime_put_sync()
> >
> > pm_runtime_get_sync() and pm_runtime_put_sync() are called when open
> > or release function of framebufer driver is called to inform the
> > system if
> hardware is
> > idle or not.
> >
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > ---
> >  drivers/video/s3c-fb.c |  111
> > ++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 107 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c index
> f9aca9d..83ce9a0
> > 100644
> > --- a/drivers/video/s3c-fb.c
> > +++ b/drivers/video/s3c-fb.c
> 
> <snip>
> 
> > +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,
> 
> How about use pm_runtime_put (sfb->dev) instead
> pm_runtime_put_sync(sfb->dev) ?
> In my opinion no need to sync option in *put* case.
> 
> > @@ -1322,6 +1345,8 @@ static int __devinit s3c_fb_probe(struct
> platform_device
> > *pdev)
> >
> >  	clk_enable(sfb->bus_clk);
> >
> > +	pm_runtime_enable(sfb->dev);
> > +
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	if (!res) {
> >  		dev_err(dev, "failed to find registers\n"); @@ -1360,6
> +1385,9
> 
> How about move pm_runtime_enable upper pm_runtime_get_sync in
s3c_fb_probe?
> 
> > @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> >
> >  	dev_dbg(dev, "got resources (regs %p), probing windows\n",
> sfb->regs);
> >
> > +	platform_set_drvdata(pdev, sfb);
> > +	pm_runtime_get_sync(sfb->dev);
> > +
> >  	/* setup gpio and output polarity controls */
> >
> >  	pd->setup_gpio();
> 
> platform_set_drvdata(pdev, sfb); already exists line# 1402.
> 
> <snip>
> >
> > @@ -1434,6 +1463,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]); @@ -
> 
> sfb->variant.nr_windows is better than S3C_FB_MAX_WIN.
> 
> > 1450,12 +1481,74 @@ static int __devexit s3c_fb_remove(struct
> platform_device
> > *pdev)
> >
> <snip>
> > +
> > +	for (win_no = S3C_FB_MAX_WIN - 1; win_no >= 0; win_no--) {
> 
> Ditto
> 
> > +		win = sfb->windows[win_no];
> > +		if (!win)
> > +			continue;
> > +
> > +		/* use the blank function to push into power-down */
> > +		s3c_fb_blank(FB_BLANK_POWERDOWN, win->fbinfo);
> > +	}
> > +
> > +	clk_disable(sfb->bus_clk);
> > +	return 0;
> > +}
> > +
> > +static int s3c_fb_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;
> > +	struct s3c_fb_win *win;
> > +	int win_no;
> > +
> > +	clk_enable(sfb->bus_clk);
> > +
> > +	/* setup registers */
> > +	writel(pd->vidcon1, sfb->regs + VIDCON1);
> > +
> > +	/* zero all windows before we do anything */
> > +	for (win_no = 0; win_no < sfb->variant.nr_windows; win_no++)
> > +		s3c_fb_clear_win(sfb, win_no);
> 
> Is it right "win_no < sfb->variant.nr_windows" not "nr_windows - 1" ?

Sorry my mistake.

> 
> > +
> > +	for (win_no = 0; win_no < sfb->variant.nr_windows - 1; win_no++) {
> > +		void __iomem *regs = sfb->regs + sfb->variant.keycon;
> > +
> > +		regs += (win_no * 8);
> > +		writel(0xffffff, regs + WKEYCON0);
> > +		writel(0xffffff, regs + WKEYCON1);
> > +	}
> > +
> > +	/* restore framebuffers */
> > +	for (win_no = 0; win_no < S3C_FB_MAX_WIN; win_no++) {
> > +		win = sfb->windows[win_no];
> > +		if (!win)
> > +			continue;
> > +
> > +		dev_dbg(&pdev->dev, "resuming window %d\n", win_no);
> > +		s3c_fb_set_par(win->fbinfo);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> How about merge three for loop ?

So how about merge two for loops ?

> 
> In my opinion s3c_fb_blank(FB_BLANK_UNBLANK, win->fbinfo) should be called
> in resume.
> Because sfb->enabled was cleared in suspend function using
> s3c_fb_blank(FB_BLANK_POWERDOWN, win->fbinfo).
> 
> <snip>
> 
> >
> > @@ -1710,15 +1807,21 @@ static struct platform_device_id
> s3c_fb_driver_ids[] =
> > {  };  MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
> >
> > +static const struct dev_pm_ops s3cfb_pm_ops = {
> > +	.suspend	= s3c_fb_suspend,
> > +	.resume		= s3c_fb_resume,
> > +	.runtime_suspend	= s3c_fb_runtime_suspend,
> > +	.runtime_resume		= s3c_fb_runtime_resume,
> > +};
> 
> s3c_fb_suspend and s3c_fb_resume are exactly same with
> s3c_fb_runtime_suspend s3c_fb_runtime_resume.
> Why don't you register the same function ?
> 
> <snip>
> >
> > --
> > 1.6.2.5
> >
> > --
> > 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
> 
> --
> 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