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

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

[Uwe Kleine-König]

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

[PATCH v2 1/3] video: fbdev: imxfb: fix semantic of .get_power and .set_power

[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 <u.kleine-koenig@pengutronix.de>
---

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(-)

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

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

[Uwe Kleine-König]

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

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(+)

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

[PATCH v2 3/3] video: fbdev: imxfb: add some error handling

[Uwe Kleine-König]

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 <u.kleine-koenig@pengutronix.de>
---

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(-)

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

[PATCH v2 1/3] video: fbdev: imxfb: fix semantic of .get_power and .set_power

[Philipp Zabel]

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 <u.kleine-koenig@pengutronix.de>

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

regards
Philipp

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

[Tomi Valkeinen]

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

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

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

[Uwe Kleine-König]

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

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

[Tomi Valkeinen]

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:
>> On 04/05/16 12:43, Uwe Kleine-K?nig wrote:
>>> 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?

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.

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

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