diff mbox

[2/3] video: fbdev: imxfb: enable lcd regulator in .probe

Message ID 1457380425-20244-3-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Uwe Kleine-König March 7, 2016, 7:53 p.m. UTC
This asserts that the display is on after the driver is initialized.
Otherwise, depending on how the boot loader handled the display, it is
either disabled as the regulator doesn't seem in use, or it stays off.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/fbdev/imxfb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Tomi Valkeinen March 11, 2016, 11:22 a.m. UTC | #1
On 07/03/16 21:53, Uwe Kleine-König wrote:
> This asserts that the display is on after the driver is initialized.
> Otherwise, depending on how the boot loader handled the display, it is
> either disabled as the regulator doesn't seem in use, or it stays off.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/fbdev/imxfb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index c5fcedde2a60..3dd2824e6773 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -979,8 +979,17 @@ static int imxfb_probe(struct platform_device *pdev)
>  	imxfb_enable_controller(fbi);
>  	fbi->pdev = pdev;
>  
> +	if (!IS_ERR(fbi->lcd_pwr)) {
> +		ret = regulator_enable(fbi->lcd_pwr);
> +		if (ret)
> +			goto failed_regulator;
> +	}
> +
>  	return 0;
>  
> +failed_regulator:
> +	imxfb_disable_controller(fbi);
> +
>  failed_lcd:
>  	unregister_framebuffer(info);

So I didn't go through the code in detail, but this doesn't look correct
to me.

Where is the regulator disabled which now gets enabled in probe?

imxfb_lcd_set_power() handles the regulator enable/disable, so doesn't
this mean the regulator would always be enabled? You first enable it in
probe, then imxfb_lcd_set_power() enables it at some point (?), so the
enable-count is two then.

To be honest, I've never used 'struct lcd_ops', but I think the enabling
of the regulator should happen somehow via that. If the regulator needs
to be enabled at probe time, then the probe should somehow cause
lcd_ops->set_power to get called.

Why does the regulator need to be enabled at probe? Or are you saying
imxfb_lcd_set_power() is never called in your case?

 Tomi
diff mbox

Patch

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index c5fcedde2a60..3dd2824e6773 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -979,8 +979,17 @@  static int imxfb_probe(struct platform_device *pdev)
 	imxfb_enable_controller(fbi);
 	fbi->pdev = pdev;
 
+	if (!IS_ERR(fbi->lcd_pwr)) {
+		ret = regulator_enable(fbi->lcd_pwr);
+		if (ret)
+			goto failed_regulator;
+	}
+
 	return 0;
 
+failed_regulator:
+	imxfb_disable_controller(fbi);
+
 failed_lcd:
 	unregister_framebuffer(info);