From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Brownell <david-b@pacbell.net>
Subject: Re: [PATCH] atmel_lcdfb: suspend/resume support
Date: Thu, 13 Mar 2008 11:19:37 -0800
Message-ID: <200803131219.38154.david-b@pacbell.net>
References: <47D53CFC.8050907@atmel.com> <20080313162454.3859ef1a@hskinnemo-gx620.norway.atmel.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Return-path: <linux-kernel-owner+glk-linux-kernel-3=40m.gmane.org-S1757045AbYCMTTv@vger.kernel.org>
In-Reply-To: <20080313162454.3859ef1a@hskinnemo-gx620.norway.atmel.com>
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <fbdev-devel.lists.sourceforge.net>
Content-Type: text/plain; charset="us-ascii"
To: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>, linux-fbdev-devel@lists.sourceforge.net, Linux Kernel list <linux-kernel@vger.kernel.org>, "Antonino A. Daplas" <adaplas@gmail.com>, ARM Linux Mailing List <linux-arm-kernel@lists.arm.linux.org.uk>, Haavard Skinnemoen <hskinnemoen@atmel.com>, Sedji GAOUAOU <sedji.gaouaou@atmel.com>, Patrice VILCHEZ <patrice.vilchez@atmel.com>, Andrew Victor <linux@maxim.org.za>, Andrew Morton <akpm@linux-foundation.org>

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