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.

---

Changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-2-git-send-email-u.kleine-koenig at 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(-)

index 76b6a7784b06..6d402c1a0f2b 100644
@@ -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

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

regards
Philipp

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.

---

No changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-3-git-send-email-u.kleine-koenig at pengutronix.de

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

index 6d402c1a0f2b..18388ca178a2 100644
@@ -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.

---

Changes since (implicit) v1, sent with
Message-Id: 1457380425-20244-4-git-send-email-u.kleine-koenig at 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(-)

index 18388ca178a2..23a623688f1e 100644
@@ -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

Hi,

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160510/10c1277f/attachment.sig>

Hello Tomi,

Thanks.

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 it worked back when unused regulators were not disabled during boot.

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

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160510/8144c3b2/attachment.sig>