video: fbdev: imxfb: make it work again
cover video: fbdev: imxfb: make it work again
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 at 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(-)
Signature · 1 line
2.7.0
PATCH 1/3 video: fbdev: imxfb: fix semantic of .get_power and .set_power
.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.
--- drivers/video/fbdev/imxfb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/video/fbdev/imxfb.c +3 -3
@@ -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);
Signature · 1 line
2.7.0
PATCH 1/3 video: fbdev: imxfb: fix semantic of .get_power and .set_power
Hi Uwe,
Am Montag, den 07.03.2016, 20:53 +0100 schrieb Uwe Kleine-K?nig:
Quoted 2 lines
.set_power gets passed an
FB_BLANK_XXXvalue, 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.
Quoted 10 lines
Signed-off-by Uwe Kleine-K?nig <[email protected]>--- drivers/video/fbdev/imxfb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/video/fbdev/imxfb.c +0
-0
drivers/video/fbdev/imxfb.c static int imxfb_lcd_get_power(struct lcd_device *lcddev) · line 764
Quoted 8 lines
drivers/video/fbdev/imxfb.c +2
-2@@ -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;
drivers/video/fbdev/imxfb.c static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power) · line 772
Quoted 9 lines
drivers/video/fbdev/imxfb.c +1
-1}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)
drivers/video/fbdev/imxfb.c static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power) · line 775
Quoted 3 lines
drivers/video/fbdev/imxfb.c +0
-0return regulator_enable (fbi->lcd_pwr); else return regulator_disable(fbi->lcd_pwr);
regards
Philipp
PATCH 1/3 video: fbdev: imxfb: fix semantic of .get_power and .set_power
Hello,
Quoted 8 lines · Philipp Zabel
Hi Uwe,
Am Montag, den 07.03.2016, 20:53 +0100 schrieb Uwe Kleine-K?nig:
Quoted 2 lines
.set_power gets passed an
FB_BLANK_XXXvalue, 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
Signature · 2 lines
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
In reply to Uwe Kleine-KönigThis 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.
--- drivers/video/fbdev/imxfb.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/video/fbdev/imxfb.c +9 -0
@@ -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);
Signature · 1 line
2.7.0
PATCH 2/3 video: fbdev: imxfb: enable lcd regulator in .probe
Quoted 20 lines · 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 <[email protected]>--- drivers/video/fbdev/imxfb.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/video/fbdev/imxfb.c +3
-0@@ -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)
drivers/video/fbdev/imxfb.c static int imxfb_probe(struct platform_device *pdev) · line 994
Quoted 10 lines
drivers/video/fbdev/imxfb.c +6
-0+ 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
-------------- 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/20160311/bb4f6137/attachment.sig>
PATCH 2/3 video: fbdev: imxfb: enable lcd regulator in .probe
Hello Tomi,
Quoted 36 lines · Tomi Valkeinen
Quoted 20 lines · 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 <[email protected]>--- drivers/video/fbdev/imxfb.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/video/fbdev/imxfb.c +3
-0@@ -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)drivers/video/fbdev/imxfb.c static int imxfb_probe(struct platform_device *pdev) · line 994
Quoted 10 lines
drivers/video/fbdev/imxfb.c +6
-0+ 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.
Quoted 1 line
Where is the regulator disabled which now gets enabled in probe?
It isn't, the display should be on after all :-)
Quoted 4 lines
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, thenimxfb_lcd_set_power()enables it at some point (?), so the enable-count is two then.
imxfb_lcd_set_power isn't called during boot.
Quoted 7 lines
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 causelcd_ops->set_powerto 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:
drivers/video/fbdev/imxfb.c +2 -0
@@ -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
Signature · 2 lines
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
Hello Tomi, hello Jean-Christophe,
Quoted 53 lines · Uwe Kleine-K?nig
Quoted 36 lines · Tomi Valkeinen
Quoted 20 lines · 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 <[email protected]>--- drivers/video/fbdev/imxfb.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/video/fbdev/imxfb.c +3
-0@@ -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)drivers/video/fbdev/imxfb.c static int imxfb_probe(struct platform_device *pdev) · line 994
Quoted 10 lines
drivers/video/fbdev/imxfb.c +6
-0+ 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.
Quoted 1 line
Where is the regulator disabled which now gets enabled in probe?
It isn't, the display should be on after all :-)
Quoted 4 lines
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, thenimxfb_lcd_set_power()enables it at some point (?), so the enable-count is two then.
imxfb_lcd_set_powerisn't called during boot.Quoted 5 lines
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 causelcd_ops->set_powerto get called.
Quoted 4 lines
Quoted 2 lines
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
Signature · 2 lines
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
Hi,
Quoted 74 lines · On Tue, 5 Jul 2016 12:09:02 +0200 Uwe Kleine-K?nig wrote:
Hello Tomi, hello Jean-Christophe,
Quoted 53 lines · Uwe Kleine-K?nig
Quoted 36 lines · Tomi Valkeinen
Quoted 20 lines · 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 <[email protected]>--- drivers/video/fbdev/imxfb.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/video/fbdev/imxfb.c +3
-0@@ -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)drivers/video/fbdev/imxfb.c static int imxfb_probe(struct platform_device *pdev) · line 994
Quoted 10 lines
drivers/video/fbdev/imxfb.c +6
-0+ 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.
Quoted 1 line
Where is the regulator disabled which now gets enabled in probe?
It isn't, the display should be on after all :-)
Quoted 4 lines
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, thenimxfb_lcd_set_power()enables it at some point (?), so the enable-count is two then.
imxfb_lcd_set_powerisn't called during boot.Quoted 5 lines
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 causelcd_ops->set_powerto get called.Quoted 4 lines
Quoted 2 lines
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_opsis only used inlcd = 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_opsis that it is assigned to a newly allocated struct lcd_device's .ops. Thereimxfb_lcd_set_poweris only used for some sysfs attributes (for the lcd) and the functionfb_blankwhich is in turn not called automatically, only by some sysfs attributes and ioctls.So as of now
regulator_init_completedisables the (unused) power regulator before userspace is up.
Quoted 7 lines
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
PATCH 2/3 video: fbdev: imxfb: enable lcd regulator in .probe
Hello Lothar,
Quoted 8 lines · Lothar Wa?mann
Quoted 7 lines · 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?
Quoted 5 lines
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
Signature · 2 lines
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
In reply to Uwe Kleine-KönigHello,
Quoted 6 lines · Uwe Kleine-K?nig
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
Signature · 2 lines
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
Quoted 10 lines · Uwe Kleine-K?nig
Hello,
Quoted 6 lines · Uwe Kleine-K?nig
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?
Quoted 2 lines
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
-------------- 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/20160810/c27be42b/attachment.sig>
PATCH 3/3 video: fbdev: imxfb: add some error handling
In reply to Uwe Kleine-König--- drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
drivers/video/fbdev/imxfb.c +26 -7
@@ -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 = {
Signature · 1 line
2.7.0
PATCH 3/3 video: fbdev: imxfb: add some error handling
Am Montag, den 07.03.2016, 20:53 +0100 schrieb Uwe Kleine-K?nig:
Quoted 62 lines
Signed-off-by Uwe Kleine-K?nig <[email protected]>--- drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)drivers/video/fbdev/imxfb.c +23
-5@@ -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 );
drivers/video/fbdev/imxfb.c static void imxfb_disable_controller(struct imxfb_info *fbi) · line 541
Quoted 4 lines
drivers/video/fbdev/imxfb.c +1
-0@@ -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/;;/;/
drivers/video/fbdev/imxfb.c static int imxfb_blank(int blank, struct fb_info *info) · line 555
Quoted 10 lines
drivers/video/fbdev/imxfb.c +1
-1pr_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);
drivers/video/fbdev/imxfb.c static int imxfb_blank(int blank, struct fb_info *info) · line 560
Quoted 6 lines
drivers/video/fbdev/imxfb.c +1
-1} -return 0 ; +return ret; }static struct fb_ops imxfb_ops = {
regards
Philipp
PATCH 3/3 video: fbdev: imxfb: add some error handling
In reply to Uwe Kleine-KönigQuoted 1 line · Uwe Kleine-K?nig
Signed-off-by Uwe Kleine-K?nig <[email protected]>
Always have a description in a patch. In trivial cases it can be more or less the same as the subject.
Quoted 31 lines
--- drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)drivers/video/fbdev/imxfb.c +7
-4@@ -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);
drivers/video/fbdev/imxfb.c static void imxfb_enable_controller(struct imxfb_info *fbi) · line 514
Quoted 10 lines
drivers/video/fbdev/imxfb.c +10
-0+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...
Quoted 10 lines
drivers/video/fbdev/imxfb.c +5
-0+ +return ret; + } + fbi->enabled = true; +return 0 ; }static void imxfb_disable_controller (struct imxfb_info *fbi)
drivers/video/fbdev/imxfb.c static void imxfb_disable_controller(struct imxfb_info *fbi) · line 532
Quoted 6 lines
drivers/video/fbdev/imxfb.c +1
-1@@ -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.
drivers/video/fbdev/imxfb.c static void imxfb_disable_controller(struct imxfb_info *fbi) · line 541
Quoted 8 lines
drivers/video/fbdev/imxfb.c +1
-0fbi->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 ;.
Quoted 7 lines
drivers/video/fbdev/imxfb.c +0
-0pr_debug ("imxfb_blank: blank=%d\n" , blank);@@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)break ;
drivers/video/fbdev/imxfb.c static int imxfb_blank(int blank, struct fb_info *info) · line 560
Quoted 10 lines
drivers/video/fbdev/imxfb.c +2
-2case FB_BLANK_UNBLANK : -imxfb_enable_controller (fbi); + ret =imxfb_enable_controller (fbi);break ; } -return 0 ; +return ret; }static struct fb_ops imxfb_ops = {
-------------- 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/20160311/982398a6/attachment-0001.sig>
Fabio Estevam
reviewercover video: fbdev: imxfb: make it work again
In reply to Uwe Kleine-KönigHi Uwe,
Quoted 3 lines · Uwe Kleine-K?nig
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:
cover video: fbdev: imxfb: make it work again
Quoted 6 lines · Fabio Estevam
Hi Uwe,
Quoted 2 lines · Uwe Kleine-K?nig
Hello,
Quoted 7 lines
Quoted 1 line
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:
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |