Re: [PATCH] atmel_lcdfb: suspend/resume support

[Haavard Skinnemoen]

On Mon, 10 Mar 2008 14:51:56 +0100
Nicolas Ferre <nicolas.ferre@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/