linux-kernel - Re: [PATCH] atmel_lcdfb: suspend/resume support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 13 Mar 2008 11:19:37 -0800
From:	David Brownell <david-b@...bell.net>
To:	Haavard Skinnemoen <haavard.skinnemoen@...el.com>
Cc:	Nicolas Ferre <nicolas.ferre@...el.com>,
	linux-fbdev-devel@...ts.sourceforge.net,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	"Antonino A. Daplas" <adaplas@...il.com>,
	ARM Linux Mailing List 
	<linux-arm-kernel@...ts.arm.linux.org.uk>,
	Haavard Skinnemoen <hskinnemoen@...el.com>,
	Sedji GAOUAOU <sedji.gaouaou@...el.com>,
	Patrice VILCHEZ <patrice.vilchez@...el.com>,
	Andrew Victor <linux@...im.org.za>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] atmel_lcdfb: suspend/resume support

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/