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
.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>
---
drivers/video/fbdev/imxfb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
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>
---
drivers/video/fbdev/imxfb.c | 9 +++++++++
1 file changed, 9 insertions(+)
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/ |
@@ -759,9 +759,9 @@ static int imxfb_lcd_get_power(struct lcd_device *lcddev)structimxfb_info*fbi=dev_get_drvdata(&lcddev->dev);if(!IS_ERR(fbi->lcd_pwr))-returnregulator_is_enabled(fbi->lcd_pwr);+return!regulator_is_enabled(fbi->lcd_pwr);-return1;+return0;
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;
quoted hunk
}
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)
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:
quoted
.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/ |
From: Tomi Valkeinen <hidden> Date: 2016-03-11 11:22:40
On 07/03/16 21:53, Uwe Kleine-König wrote:
quoted hunk
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>
---
drivers/video/fbdev/imxfb.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -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)+gotofailed_regulator;+}+return0;+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
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:
quoted
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>
---
drivers/video/fbdev/imxfb.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -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)+gotofailed_regulator;+}+return0;+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:
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:
quoted
On 07/03/16 21:53, Uwe Kleine-König wrote:
quoted
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>
---
drivers/video/fbdev/imxfb.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -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)+gotofailed_regulator;+}+return0;+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 :-)
quoted
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.
quoted
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/ |
Hello Tomi, hello Jean-Christophe,
On Wed, Apr 20, 2016 at 09:17:27PM +0200, Uwe Kleine-König wrote:
quoted
On Fri, Mar 11, 2016 at 01:22:40PM +0200, Tomi Valkeinen wrote:
quoted
On 07/03/16 21:53, Uwe Kleine-König wrote:
quoted
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>
---
drivers/video/fbdev/imxfb.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -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)+gotofailed_regulator;+}+return0;+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 :-)
quoted
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.
quoted
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
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:
quoted
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/ |
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/ |
From: Tomi Valkeinen <hidden> Date: 2016-08-10 10:29:32
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:
quoted
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