[PATCH] atmel_lcdfb: suspend/resume support

[PATCH] atmel_lcdfb: suspend/resume support

[Nicolas Ferre]

From: David Brownell <dbrownell@users.sourceforge.net>

Teach atmel_lcdfb driver how to suspend/resume.

Note that the backlight control should probably do more of the same stuff:
turning off display power (more than just the backlight) and stopping the
clocks (and dma to drive the no-longer-seen display).  No point in wasting
power to generate images that can't be observed, after all...

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
It applies on top of :
atmel_lcdfb-dont-initialize-a-pre-allocated-framebuffer.patch
already in the -mm tree.

 drivers/video/atmel_lcdfb.c |   36 ++++++++++++++++++++++++++++++++++--
 include/video/atmel_lcdc.h  |    1 +
 2 files changed, 35 insertions(+), 2 deletions(-)

--- linux-2.6.x-rc.orig/drivers/video/atmel_lcdfb.c
+++ linux-2.6.x-rc/drivers/video/atmel_lcdfb.c
@@ -909,10 +909,42 @@ static int __exit atmel_lcdfb_remove(str
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+static int atmel_lcdfb_suspend(struct platform_device *pdev, pm_message_t mesg)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct atmel_lcdfb_info *sinfo = info->par;
+
+	sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
+	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
+	if (sinfo->atmel_lcdfb_power_control)
+		sinfo->atmel_lcdfb_power_control(0);
+	atmel_lcdfb_stop_clock(sinfo);
+	return 0;
+}
+
+static int atmel_lcdfb_resume(struct platform_device *pdev)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct atmel_lcdfb_info *sinfo = info->par;
+
+	atmel_lcdfb_start_clock(sinfo);
+	if (sinfo->atmel_lcdfb_power_control)
+		sinfo->atmel_lcdfb_power_control(1);
+	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
+	return 0;
+}
+
+#else
+#define atmel_lcdfb_suspend	NULL
+#define atmel_lcdfb_resume	NULL
+#endif
+
 static struct platform_driver atmel_lcdfb_driver = {
 	.remove		= __exit_p(atmel_lcdfb_remove),
-
-// FIXME need suspend, resume
+	.suspend	= atmel_lcdfb_suspend,
+	.resume		= atmel_lcdfb_resume,
 
 	.driver		= {
 		.name	= "atmel_lcdfb",
--- linux-2.6.x-rc.orig/include/video/atmel_lcdc.h
+++ linux-2.6.x-rc/include/video/atmel_lcdc.h
@@ -39,6 +39,7 @@ struct atmel_lcdfb_info {
 	u8			bl_power;
 #endif
 	bool			lcdcon_is_backlight;
+	u8			saved_lcdcon;
 
 	u8			default_bpp;
 	unsigned int		default_lcdcon2;

Re: [PATCH] atmel_lcdfb: suspend/resume support

[Haavard Skinnemoen]

On Mon, 10 Mar 2008 14:51:56 +0100
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> +static int atmel_lcdfb_suspend(struct platform_device *pdev, pm_message_t mesg)
> +{
> +	struct fb_info *info = platform_get_drvdata(pdev);
> +	struct atmel_lcdfb_info *sinfo = info->par;
> +
> +	sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);

You're saving CONTRAST_VAL into a field called saved_lcdcon even though
it has nothing to do with LCDCON1 or LCDCON2...

> +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);

...then you're altering CONTRAST_CTR...

> +}
> +
> +static int atmel_lcdfb_resume(struct platform_device *pdev)
> +{

> +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);

...and later restoring the saved value of CONTRAST_VAL into CONTRAST_CTR.

Confused.

> @@ -39,6 +39,7 @@ struct atmel_lcdfb_info {
>  	u8			bl_power;
>  #endif
>  	bool			lcdcon_is_backlight;
> +	u8			saved_lcdcon;

All of the registers involved are 32 bits wide, although the
interesting bits are all in the low byte. Do we really want to save
three bytes in the struct that badly?

Haavard

Re: [PATCH] atmel_lcdfb: suspend/resume support

[David Brownell]

On Thursday 13 March 2008, Haavard Skinnemoen wrote:

> > +	sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> 
> You're saving CONTRAST_VAL into a field called saved_lcdcon even though
> it has nothing to do with LCDCON1 or LCDCON2...

Yeah, why don't registers named LCDCONx have nothing
to do with LCD CONtrast?  Better to have named the PWM
registers PWM ... and say they could be used for either
contrast or backlight control.

The saved contrast/backlight counter value CONTRAST_VAL is
one of two PWM control registers.


> > +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
> 
> ...then you're altering CONTRAST_CTR...

That's forces the contrast signal low and disables the
PWM counter, so it won't "randomly" leave the PWM output
high when the clock is stopped (leaving at least some
displays lit during suspend).

It's possible that only CTR needed to be saved; all the
backlight support in this driver still needs work.


> > +}
> > +
> > +static int atmel_lcdfb_resume(struct platform_device *pdev)
> > +{
> 
> > +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
> 
> ...and later restoring the saved value of CONTRAST_VAL into CONTRAST_CTR.
> 
> Confused.

Yeah, that looks wrong; the patch below makes more sense.
Though it *does* behave right for some reason...

- Dave


--- sam9.orig/drivers/video/atmel_lcdfb.c	2008-03-13 12:14:08.000000000 -0700
+++ sam9/drivers/video/atmel_lcdfb.c	2008-03-13 10:40:54.000000000 -0700
@@ -926,7 +926,10 @@ static int atmel_lcdfb_resume(struct pla
 	atmel_lcdfb_start_clock(sinfo);
 	if (sinfo->atmel_lcdfb_power_control)
 		sinfo->atmel_lcdfb_power_control(1);
-	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
+	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, sinfo->saved_lcdcon);
+	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR,
+			sinfo->saved_lcdcon ? contrast_ctr : 0);
+
 	return 0;
 }
 

Re: [PATCH] atmel_lcdfb: suspend/resume support

[Haavard Skinnemoen]

On Thu, 13 Mar 2008 11:19:37 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Thursday 13 March 2008, Haavard Skinnemoen wrote:
> 
> > > +	sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> > 
> > You're saving CONTRAST_VAL into a field called saved_lcdcon even though
> > it has nothing to do with LCDCON1 or LCDCON2...
> 
> Yeah, why don't registers named LCDCONx have nothing
> to do with LCD CONtrast?  Better to have named the PWM
> registers PWM ... and say they could be used for either
> contrast or backlight control.

While I admit you have a point, I think it's easier to change the name
of that field than changing the hardware documentation at this point ;)

> The saved contrast/backlight counter value CONTRAST_VAL is
> one of two PWM control registers.

I know.

> > > +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
> > 
> > ...then you're altering CONTRAST_CTR...
> 
> That's forces the contrast signal low and disables the
> PWM counter, so it won't "randomly" leave the PWM output
> high when the clock is stopped (leaving at least some
> displays lit during suspend).

I'm not saying it's the wrong thing to do. I just think it's strange
that you alter a different register than the one you saved.

> It's possible that only CTR needed to be saved; all the
> backlight support in this driver still needs work.

Yes, I do think we can assume VAL stays unchanged during suspend.

> > > +}
> > > +
> > > +static int atmel_lcdfb_resume(struct platform_device *pdev)
> > > +{
> > 
> > > +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
> > 
> > ...and later restoring the saved value of CONTRAST_VAL into CONTRAST_CTR.
> > 
> > Confused.
> 
> Yeah, that looks wrong; the patch below makes more sense.
> Though it *does* behave right for some reason...

Yes, that looks better. Perhaps you used a contrast value that happened
to set the right bits when written to CTR?

I still think the name of the saved_lcdcon field is confusing though.

Haavard