[PATCH v2 0/3] video: fbdev: imxfb: make it work again

Hello,

this is v2 of the series which addresses the review comments I got vor
(implicit) v1.

For patch 2 the question is still open if this is the right fix, but
without this the display doesn't stay on. Patches 1 and 3 should be
applicable independant of patch 2.

Best regards
Uwe

Uwe Kleine-König (3):
  video: fbdev: imxfb: fix semantic of .get_power and .set_power
  video: fbdev: imxfb: enable lcd regulator in .probe
  video: fbdev: imxfb: add some error handling

 drivers/video/fbdev/imxfb.c | 53 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 12 deletions(-)

-- 
2.8.0.rc3

.set_power gets passed an FB_BLANK_XXX value, not a bool. So 0 signals
on; and >1 means off. The same applies for return values of .get_power.

Signed-off-by: Uwe Kleine-König <redacted>
---

Changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-2-git-send-email-u.kleine-koenig@pengutronix.de

 - Make it explicit that we're working on FB_BLANK_XXX values,
   suggested by Philipp Zabel.

 drivers/video/fbdev/imxfb.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 76b6a7784b06..6d402c1a0f2b 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c

@@ -758,10 +758,11 @@ static int imxfb_lcd_get_power(struct lcd_device *lcddev)
 {
 	struct imxfb_info *fbi = dev_get_drvdata(&lcddev->dev);
 
-	if (!IS_ERR(fbi->lcd_pwr))
-		return regulator_is_enabled(fbi->lcd_pwr);
+	if (!IS_ERR(fbi->lcd_pwr) &&
+	    !regulator_is_enabled(fbi->lcd_pwr))
+		return FB_BLANK_POWERDOWN;
 
-	return 1;
+	return FB_BLANK_UNBLANK;
 }
 
 static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)

@@ -769,7 +770,7 @@ static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
 	struct imxfb_info *fbi = dev_get_drvdata(&lcddev->dev);
 
 	if (!IS_ERR(fbi->lcd_pwr)) {
-		if (power)
+		if (power = FB_BLANK_UNBLANK)
 			return regulator_enable(fbi->lcd_pwr);
 		else
 			return regulator_disable(fbi->lcd_pwr);

-- 
2.8.0.rc3

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 <redacted>
---

No changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-3-git-send-email-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 6d402c1a0f2b..18388ca178a2 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c

@@ -995,8 +995,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);
 

-- 
2.8.0.rc3

clk_prepare_enable can fail and if it does the controller must not be
considered enabled. So check for errors, properly unwind and give the
error code back to the caller.

While touching the clock code also enable the clocks in the same
direction and disable in reverse order.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Uwe Kleine-König <redacted>
---

Changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-4-git-send-email-u.kleine-koenig@pengutronix.de

 - Some simplifications suggested and Reviewed-by Philipp Zabel
 - Rework goto labels to have them at the end of the function
   (Tomi Valkeinen).

 drivers/video/fbdev/imxfb.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 18388ca178a2..23a623688f1e 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c

@@ -473,11 +473,12 @@ static int imxfb_set_par(struct fb_info *info)
 	return 0;
 }
 
-static void imxfb_enable_controller(struct imxfb_info *fbi)
+static int imxfb_enable_controller(struct imxfb_info *fbi)
 {
+	int ret;
 
 	if (fbi->enabled)
-		return;
+		return 0;
 
 	pr_debug("Enabling LCD controller\n");
 

@@ -496,10 +497,29 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
 	 */
 	writel(RMCR_LCDC_EN_MX1, fbi->regs + LCDC_RMCR);
 
-	clk_prepare_enable(fbi->clk_ipg);
-	clk_prepare_enable(fbi->clk_ahb);
-	clk_prepare_enable(fbi->clk_per);
+	ret = clk_prepare_enable(fbi->clk_ipg);
+	if (ret)
+		goto err_enable_ipg;
+
+	ret = clk_prepare_enable(fbi->clk_ahb);
+	if (ret)
+		goto err_enable_ahb;
+
+	ret = clk_prepare_enable(fbi->clk_per);
+	if (ret)
+		goto err_enable_per;
+
 	fbi->enabled = true;
+	return 0;
+
+err_enable_per:
+	clk_disable_unprepare(fbi->clk_ahb);
+err_enable_ahb:
+	clk_disable_unprepare(fbi->clk_ipg);
+err_enable_ipg:
+	writel(0, fbi->regs + LCDC_RMCR);
+
+	return ret;
 }
 
 static void imxfb_disable_controller(struct imxfb_info *fbi)

@@ -510,8 +530,8 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
 	pr_debug("Disabling LCD controller\n");
 
 	clk_disable_unprepare(fbi->clk_per);
-	clk_disable_unprepare(fbi->clk_ipg);
 	clk_disable_unprepare(fbi->clk_ahb);
+	clk_disable_unprepare(fbi->clk_ipg);
 	fbi->enabled = false;
 
 	writel(0, fbi->regs + LCDC_RMCR);

@@ -532,8 +552,7 @@ static int imxfb_blank(int blank, struct fb_info *info)
 		break;
 
 	case FB_BLANK_UNBLANK:
-		imxfb_enable_controller(fbi);
-		break;
+		return imxfb_enable_controller(fbi);
 	}
 	return 0;
 }

-- 
2.8.0.rc3

Am Mittwoch, den 04.05.2016, 11:43 +0200 schrieb Uwe Kleine-König:

.set_power gets passed an FB_BLANK_XXX value, not a bool. So 0 signals
on; and >1 means off. The same applies for return values of .get_power.

Signed-off-by: Uwe Kleine-König <redacted>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Hi,

On 04/05/16 12:43, Uwe Kleine-König wrote:

Hello,

this is v2 of the series which addresses the review comments I got vor
(implicit) v1.

For patch 2 the question is still open if this is the right fix, but
without this the display doesn't stay on. Patches 1 and 3 should be
applicable independant of patch 2.

I picked patches 1 and 3, they look fine.

I still think patch 2 is just broken, it doesn't make sense to me.

If the regulator is enabled in probe, then it's always on, and
imxfb_lcd_set_power() should be removed as it never has any effect. But
that doesn't sound correct, as presumably the imxfb_lcd_set_power() has
worked at some point.

And shouldn't the regulator be disabled at least when suspending?

 Tomi

Hello Tomi,

On Tue, May 10, 2016 at 11:47:38AM +0300, Tomi Valkeinen wrote:

On 04/05/16 12:43, Uwe Kleine-König wrote:

quoted

this is v2 of the series which addresses the review comments I got vor
(implicit) v1.

For patch 2 the question is still open if this is the right fix, but
without this the display doesn't stay on. Patches 1 and 3 should be
applicable independant of patch 2.

I picked patches 1 and 3, they look fine.

Thanks.

I still think patch 2 is just broken, it doesn't make sense to me.

What do you think should happen during startup? Something should call
the set_power callback to enable the device? Or should that only happen
when something writes to /dev/fb0?

If the regulator is enabled in probe, then it's always on, and
imxfb_lcd_set_power() should be removed as it never has any effect. But
that doesn't sound correct, as presumably the imxfb_lcd_set_power() has
worked at some point.

I think it worked back when unused regulators were not disabled during
boot.

And shouldn't the regulator be disabled at least when suspending?

Yeah, but maybe the core should call set_power(off) then? (Don't know,
maybe that cannot work.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On 10/05/16 12:05, Uwe Kleine-König wrote:

Hello Tomi,

On Tue, May 10, 2016 at 11:47:38AM +0300, Tomi Valkeinen wrote:

quoted

On 04/05/16 12:43, Uwe Kleine-König wrote:

quoted

this is v2 of the series which addresses the review comments I got vor
(implicit) v1.

For patch 2 the question is still open if this is the right fix, but
without this the display doesn't stay on. Patches 1 and 3 should be
applicable independant of patch 2.

I picked patches 1 and 3, they look fine.

Thanks.

quoted

I still think patch 2 is just broken, it doesn't make sense to me.

What do you think should happen during startup? Something should call
the set_power callback to enable the device? Or should that only happen
when something writes to /dev/fb0?

I think the panel should be enabled at startup, as with fbdev there's no
specific enable call. So I don't have a problem with enabling the panel
at startup, but just that the regulator enables/disables don't seem to
match.

quoted

If the regulator is enabled in probe, then it's always on, and
imxfb_lcd_set_power() should be removed as it never has any effect. But
that doesn't sound correct, as presumably the imxfb_lcd_set_power() has
worked at some point.

I think it worked back when unused regulators were not disabled during
boot.

Is imxfb_lcd_set_power() ever called, or just not at startup? If it is
called properly later, then perhaps you just need to kick it at startup
time to enable it. Maybe imxfb_lcd_set_power(lcd, FB_BLANK_UNBLANK);

Did you try unloading the module (both when the LCD is enabled and when
it's disabled), and seeing that the regulator gets disabled when you
unload it?

 Tomi