Re: [PATCH] atmel_lcdfb: suspend/resume support

[David Brownell <david-b@pacbell.net>]

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;
 }