Re: [PATCH] atmel_lcdfb: backlight control

[Haavard Skinnemoen]

On Fri, 18 Jan 2008 15:44:35 +0100
Nicolas Ferre <nicolas.ferre@xxxxxxxxx> wrote:

> From: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> 
> On the sam9 EK boards, the LCD backlight is hooked up to a PWM output
> from the LCD controller.  It's controlled by "contrast" registers though.
> 
> This patch lets boards declare that they have that kind of backlight
> control.  The driver can then export this control, letting screenblank
> and other operations actually take effect ... reducing the typically
> substantial power drain from the backlight.
> 
> Signed-off-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxx>

Looks good to me. It won't affect any current AVR32 boards, although
that may change in the future.

A couple of minor comments below.

> +static struct backlight_ops atmel_lcdc_bl_ops = {
> +	.update_status = atmel_bl_update_status,
> +	.get_brightness = atmel_bl_get_brightness,
> +};

This can be const, no?

No it can't, since backlight_device_register() takes a non-const
pointer for some reason...

> +#else
> +
> +static inline void init_backlight(struct atmel_lcdfb_info *sinfo)
> +{
> +	dev_warn(&sinfo->pdev->dev, "backlight control is not available\n");
> +}
> +
> +static inline void exit_contrast(struct atmel_lcdfb_info *sinfo)
> +{
> +}
> +
> +#endif

This looks a bit asymmetric...

> +static inline void init_contrast(struct atmel_lcdfb_info *sinfo)
> +{
> +	/* have some default contrast/backlight settings */
> +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, contrast_ctr);
> +	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, ATMEL_LCDC_CVAL_DEFAULT);
> +
> +	if (sinfo->lcdcon_is_backlight)
> +		init_backlight(sinfo);
> +}
> +
>  
>  static struct fb_fix_screeninfo atmel_lcdfb_fix __initdata = {
>  	.type		= FB_TYPE_PACKED_PIXELS,
> @@ -370,10 +472,6 @@ static int atmel_lcdfb_set_par(struct fb
>  	/* Disable all interrupts */
>  	lcdc_writel(sinfo, ATMEL_LCDC_IDR, ~0UL);
>  
> -	/* Set contrast */
> -	value = ATMEL_LCDC_PS_DIV8 | ATMEL_LCDC_POL_POSITIVE | ATMEL_LCDC_ENA_PWMENABLE;
> -	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, value);
> -	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, ATMEL_LCDC_CVAL_DEFAULT);
>  	/* ...wait for DMA engine to become idle... */
>  	while (lcdc_readl(sinfo, ATMEL_LCDC_DMACON) & ATMEL_LCDC_DMABUSY)
>  		msleep(10);
> @@ -577,6 +675,7 @@ static int __init atmel_lcdfb_probe(stru
>  		sinfo->default_monspecs = pdata_sinfo->default_monspecs;
>  		sinfo->atmel_lcdfb_power_control = pdata_sinfo->atmel_lcdfb_power_control;
>  		sinfo->guard_time = pdata_sinfo->guard_time;
> +		sinfo->lcdcon_is_backlight = pdata_sinfo->lcdcon_is_backlight;
>  	} else {
>  		dev_err(dev, "cannot get default configuration\n");
>  		goto free_info;
> @@ -670,6 +769,9 @@ static int __init atmel_lcdfb_probe(stru
>  		goto release_mem;
>  	}
>  
> +	/* Initialize PWM for contrast or backlight ("off") */
> +	init_contrast(sinfo);
> +
>  	/* interrupt */
>  	ret = request_irq(sinfo->irq_base, atmel_lcdfb_interrupt, 0, pdev->name, info);
>  	if (ret) {
> @@ -755,6 +857,7 @@ static int __exit atmel_lcdfb_remove(str
>  	if (!sinfo)
>  		return 0;
>  
> +	exit_contrast(sinfo);

Missing exit_contrast() in probe() error path?

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/