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

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

[Uwe Kleine-König]

Hello,

it seems the imxfb driver stopped working quite some time ago. Here come
some fixes to convince the driver to operate again. This is a cleanup of
the RFC patch "video: fbdev: imxfb: make the driver cooperate" I sent
last week (Message-Id:
1456829223-1526-1-git-send-email-u.kleine-koenig@pengutronix.de).

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 | 48 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

-- 
2.7.0

[PATCH 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>
---
 drivers/video/fbdev/imxfb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index cee88603efc9..c5fcedde2a60 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -759,9 +759,9 @@ 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);
+		return !regulator_is_enabled(fbi->lcd_pwr);
 
-	return 1;
+	return 0;
 }
 
 static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
@@ -769,7 +769,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)
 			return regulator_enable(fbi->lcd_pwr);
 		else
 			return regulator_disable(fbi->lcd_pwr);
-- 
2.7.0

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

[Philipp Zabel]

Hi Uwe,

Am Montag, den 07.03.2016, 20:53 +0100 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.

I'd try to somehow work this information into the code to avoid future
confusion.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/fbdev/imxfb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index cee88603efc9..c5fcedde2a60 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -759,9 +759,9 @@ 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);
> +		return !regulator_is_enabled(fbi->lcd_pwr);
>  
> -	return 1;
> +	return 0;

How about making it explicit:

	if (!IS_ERR(fbi->lcd_pwr) &&
	    !regulator_is_enabled(fbi->lcd_pwr))
		return FB_BLANK_POWERDOWN;

	return FB_BLANK_UNBLANK;

>  }
>  
>  static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
> @@ -769,7 +769,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)

Same here:
		if (power = FB_BLANK_UNBLANK)

>  			return regulator_enable(fbi->lcd_pwr);
>  		else
>  			return regulator_disable(fbi->lcd_pwr);

regards
Philipp

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

[Uwe Kleine-König]

Hello,

On Tue, Mar 08, 2016 at 08:55:30AM +0100, Philipp Zabel wrote:
> Hi Uwe,
> 
> Am Montag, den 07.03.2016, 20:53 +0100 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.
> 
> I'd try to somehow work this information into the code to avoid future
> confusion.

I integrated your changes into my code, you're obviously right here.

Jean-Christophe, Tomi: Do you agree in principle with these changes? If
so I can resend. If you won't take the changes anyhow, I wouldn't.

Best regards
Uwe

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

[PATCH 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>
---
 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);
 
-- 
2.7.0

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

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1818 bytes --]


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Uwe Kleine-König]

Hello Tomi,

On Fri, Mar 11, 2016 at 01:22:40PM +0200, Tomi Valkeinen wrote:
> 
> 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?

It isn't, the display should be on after all :-)

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

imxfb_lcd_set_power isn't called during boot.

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

Right. I just confirmed that with next-20160419 with this patch on top:

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 76b6a7784b06..93b803ebd61d 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -1,3 +1,4 @@
+#define DEBUG
 /*
  *  Freescale i.MX Frame Buffer device driver
  *
@@ -768,6 +769,7 @@ static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
 {
 	struct imxfb_info *fbi = dev_get_drvdata(&lcddev->dev);
 
+	dev_dbg(&lcddev->dev, "%s(power=%d)\n", __func__, power);
 	if (!IS_ERR(fbi->lcd_pwr)) {
 		if (power)
 			return regulator_enable(fbi->lcd_pwr);

With that:

# dmesg | grep -iE '(lcd|fb|power)'
[    0.562633] backlight supply power not found, using dummy regulator
[    0.568246] imx-fb 53fbc000.lcdc: i.MX Framebuffer driver
[    0.568326] imxfb_init_fbinfo
[    0.576275] Enabling LCD controller
[    1.652166] sdhci-esdhc-imx 53fb4000.esdhc: Got CD GPIO
[    1.658109] sdhci-esdhc-imx 53fb4000.esdhc: Got WP GPIO
[    1.703638] mmc0: SDHCI controller on 53fb4000.esdhc [53fb4000.esdhc] using DMA
[    4.094923] lcd supply: disabling

The bootloader enables the lcd before booting into Linux, with the last
message the display gets disabled.

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello Tomi, hello Jean-Christophe,

On Wed, Apr 20, 2016 at 09:17:27PM +0200, Uwe Kleine-König wrote:
> On Fri, Mar 11, 2016 at 01:22:40PM +0200, Tomi Valkeinen wrote:
> > 
> > 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?
> 
> It isn't, the display should be on after all :-)
> 
> > 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.
> 
> imxfb_lcd_set_power isn't called during boot.
> 
> > 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?
> 
> Right. I just confirmed that with next-20160419 with this patch on top:

And now I also checked the code. The driver's lcd_ops is only used in

	lcd = devm_lcd_device_register(&pdev->dev, "imxfb-lcd", &pdev->dev, fbi, &imxfb_lcd_ops);

(i.e. the last argument). The only thing that happes with the lcd_ops is
that it is assigned to a newly allocated struct lcd_device's .ops. There
imxfb_lcd_set_power is only used for some sysfs attributes (for the lcd)
and the function fb_blank which is in turn not called automatically,
only by some sysfs attributes and ioctls.

So as of now regulator_init_complete disables the (unused) power
regulator before userspace is up.

To reach my goal to keep the display on (showing whatever the bootloader
initialized the display with) until the application starts caring for
the display some driver must enable the regulator. The only candidates
are the lcd driver (i.e. the patch under discussion) or some mechanism
in the fb core. As there doesn't seem to be code in the core (and the
core is dead?) what do you suggest me to do?

Best regards
Uwe

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

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

[Lothar Waßmann]

Hi,

On Tue, 5 Jul 2016 12:09:02 +0200 Uwe Kleine-König wrote:
> Hello Tomi, hello Jean-Christophe,
> 
> On Wed, Apr 20, 2016 at 09:17:27PM +0200, Uwe Kleine-König wrote:
> > On Fri, Mar 11, 2016 at 01:22:40PM +0200, Tomi Valkeinen wrote:
> > > 
> > > 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?
> > 
> > It isn't, the display should be on after all :-)
> > 
> > > 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.
> > 
> > imxfb_lcd_set_power isn't called during boot.
> > 
> > > 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?
> > 
> > Right. I just confirmed that with next-20160419 with this patch on top:
> 
> And now I also checked the code. The driver's lcd_ops is only used in
> 
> 	lcd = devm_lcd_device_register(&pdev->dev, "imxfb-lcd", &pdev->dev, fbi, &imxfb_lcd_ops);
> 
> (i.e. the last argument). The only thing that happes with the lcd_ops is
> that it is assigned to a newly allocated struct lcd_device's .ops. There
> imxfb_lcd_set_power is only used for some sysfs attributes (for the lcd)
> and the function fb_blank which is in turn not called automatically,
> only by some sysfs attributes and ioctls.
> 
> So as of now regulator_init_complete disables the (unused) power
> regulator before userspace is up.
> 
> To reach my goal to keep the display on (showing whatever the bootloader
> initialized the display with) until the application starts caring for
> the display some driver must enable the regulator. The only candidates
> are the lcd driver (i.e. the patch under discussion) or some mechanism
> in the fb core. As there doesn't seem to be code in the core (and the
> core is dead?) what do you suggest me to do?
> 
The simplefb driver (CONFIG_FB_SIMPLE) is designed for exactly this
purpose. If you rely on the bootloader's framebuffer being preserved
until your application starts without any driver that reserves the
corresponding memory, the framebuffer contents may be corrupted by the
booting kernel.


Lothar Waßmann

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

[Uwe Kleine-König]

Hello Lothar,

On Tue, Jul 05, 2016 at 02:08:38PM +0200, Lothar Waßmann wrote:
> On Tue, 5 Jul 2016 12:09:02 +0200 Uwe Kleine-König wrote:
> > To reach my goal to keep the display on (showing whatever the bootloader
> > initialized the display with) until the application starts caring for
> > the display some driver must enable the regulator. The only candidates
> > are the lcd driver (i.e. the patch under discussion) or some mechanism
> > in the fb core. As there doesn't seem to be code in the core (and the
> > core is dead?) what do you suggest me to do?
> > 
> The simplefb driver (CONFIG_FB_SIMPLE) is designed for exactly this
> purpose. If you rely on the bootloader's framebuffer being preserved
> until your application starts without any driver that reserves the
> corresponding memory, the framebuffer contents may be corrupted by the
> booting kernel.

I don't want to use the simplefb driver because the application makes
use of planes which isn't supported by simplefb. To prevent the
framebuffer being overwritten a chunk of memory at the end of RAM is
reserved (similar to how simplefb is supposed to work).

And even without my usecase of a persistent splash screen, if the fb is
used as console, it should not be disabled either.

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello,

On Tue, Jul 05, 2016 at 12:09:02PM +0200, Uwe Kleine-König wrote:
> To reach my goal to keep the display on (showing whatever the bootloader
> initialized the display with) until the application starts caring for
> the display some driver must enable the regulator. The only candidates
> are the lcd driver (i.e. the patch under discussion) or some mechanism
> in the fb core. As there doesn't seem to be code in the core (and the
> core is dead?) what do you suggest me to do?

Unfortunately I didn't get an answer here. Do you still have this mail
on your radar?

Best regards
Uwe

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

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

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 777 bytes --]



On 27/07/16 22:57, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jul 05, 2016 at 12:09:02PM +0200, Uwe Kleine-König wrote:
>> To reach my goal to keep the display on (showing whatever the bootloader
>> initialized the display with) until the application starts caring for
>> the display some driver must enable the regulator. The only candidates
>> are the lcd driver (i.e. the patch under discussion) or some mechanism
>> in the fb core. As there doesn't seem to be code in the core (and the
>> core is dead?) what do you suggest me to do?
> 
> Unfortunately I didn't get an answer here. Do you still have this mail
> on your radar?

Can you resend the series? And I think I had comments about the third
patch, to which I don't see any reply.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Uwe Kleine-König]

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

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 3dd2824e6773..671b3719db56 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -473,8 +473,9 @@ 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;
@@ -496,10 +497,27 @@ 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) {
+		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;
+	}
+
 	fbi->enabled = true;
+	return 0;
 }
 
 static void imxfb_disable_controller(struct imxfb_info *fbi)
@@ -510,8 +528,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);
@@ -520,6 +538,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
 static int imxfb_blank(int blank, struct fb_info *info)
 {
 	struct imxfb_info *fbi = info->par;
+	int ret = 0;;
 
 	pr_debug("imxfb_blank: blank=%d\n", blank);
 
@@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)
 		break;
 
 	case FB_BLANK_UNBLANK:
-		imxfb_enable_controller(fbi);
+		ret = imxfb_enable_controller(fbi);
 		break;
 	}
-	return 0;
+	return ret;
 }
 
 static struct fb_ops imxfb_ops = {
-- 
2.7.0

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

[Philipp Zabel]

Am Montag, den 07.03.2016, 20:53 +0100 schrieb Uwe Kleine-König:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 3dd2824e6773..671b3719db56 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -473,8 +473,9 @@ 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;
> @@ -496,10 +497,27 @@ 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) {
> +		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;
> +	}
> +
>  	fbi->enabled = true;
> +	return 0;
>  }
>  
>  static void imxfb_disable_controller(struct imxfb_info *fbi)
> @@ -510,8 +528,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);
> @@ -520,6 +538,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  static int imxfb_blank(int blank, struct fb_info *info)
>  {
>  	struct imxfb_info *fbi = info->par;
> +	int ret = 0;;

s/;;/;/

>  
>  	pr_debug("imxfb_blank: blank=%d\n", blank);
>  
> @@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)
>  		break;
>  
>  	case FB_BLANK_UNBLANK:
> -		imxfb_enable_controller(fbi);
> +		ret = imxfb_enable_controller(fbi);
>  		break;

or this could be simplified to:
		return imxfb_enable_controller(fbi);

>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  static struct fb_ops imxfb_ops = {

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

regards
Philipp

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

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2904 bytes --]


On 07/03/16 21:53, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Always have a description in a patch. In trivial cases it can be more or
less the same as the subject.

> ---
>  drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 3dd2824e6773..671b3719db56 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -473,8 +473,9 @@ 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;
> @@ -496,10 +497,27 @@ 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) {
> +		clk_disable_unprepare(fbi->clk_ahb);
> +err_enable_ahb:
> +		clk_disable_unprepare(fbi->clk_ipg);
> +err_enable_ipg:
> +		writel(0, fbi->regs + LCDC_RMCR);

Please don't do that =). If you use goto, have the labels at the end of
the function, not in the middle inside a if() block...

> +
> +		return ret;
> +	}
> +
>  	fbi->enabled = true;
> +	return 0;
>  }
>  
>  static void imxfb_disable_controller(struct imxfb_info *fbi)
> @@ -510,8 +528,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);

This is not error handling. I don't mind it being in the same patch, but
this change is something to mention in the description.

>  	fbi->enabled = false;
>  
>  	writel(0, fbi->regs + LCDC_RMCR);
> @@ -520,6 +538,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  static int imxfb_blank(int blank, struct fb_info *info)
>  {
>  	struct imxfb_info *fbi = info->par;
> +	int ret = 0;;

Extra ;.

>  
>  	pr_debug("imxfb_blank: blank=%d\n", blank);
>  
> @@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)
>  		break;
>  
>  	case FB_BLANK_UNBLANK:
> -		imxfb_enable_controller(fbi);
> +		ret = imxfb_enable_controller(fbi);
>  		break;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  static struct fb_ops imxfb_ops = {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Fabio Estevam]

Hi Uwe,

On Mon, Mar 7, 2016 at 4:53 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> it seems the imxfb driver stopped working quite some time ago. Here come

Yes, I noticed it has been broken for a long time.

After this fix I managed to get it working again:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/video/fbdev/imxfb.c?id¸2fe6ddd782f847332aeabf8cab980852f61629

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

[Uwe Kleine-König]

On Mon, Mar 07, 2016 at 05:03:53PM -0300, Fabio Estevam wrote:
> Hi Uwe,
> 
> On Mon, Mar 7, 2016 at 4:53 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hello,
> >
> > it seems the imxfb driver stopped working quite some time ago. Here come
> 
> Yes, I noticed it has been broken for a long time.
> 
> After this fix I managed to get it working again:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/video/fbdev/imxfb.c?id¸2fe6ddd782f847332aeabf8cab980852f61629

I missed this one, but it works for me also without this patch (maybe
because my bootloader is aware of the display?).

My problem was that the handling for the optional regulator was
wrong/nonexistent. imx25-pdk.dts doesn't use it and so doesn't suffer
the corresponding problems.

Best regards
Uwe

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