From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] video: fbdev: imxfb: enable lcd regulator in .probe
Date: Wed, 20 Apr 2016 19:17:27 +0000 [thread overview]
Message-ID: <20160420191727.GS29108@pengutronix.de> (raw)
In-Reply-To: <56E2AA80.4050908@ti.com>
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/ |
next prev parent reply other threads:[~2016-04-20 19:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-07 19:53 [PATCH 0/3] video: fbdev: imxfb: make it work again Uwe Kleine-König
2016-03-07 19:53 ` [PATCH 1/3] video: fbdev: imxfb: fix semantic of .get_power and .set_power Uwe Kleine-König
2016-03-08 7:55 ` Philipp Zabel
2016-03-08 8:30 ` Uwe Kleine-König
2016-03-07 19:53 ` [PATCH 2/3] video: fbdev: imxfb: enable lcd regulator in .probe Uwe Kleine-König
2016-03-11 11:22 ` Tomi Valkeinen
2016-04-20 19:17 ` Uwe Kleine-König [this message]
2016-07-05 10:09 ` Uwe Kleine-König
2016-07-05 12:08 ` Lothar Waßmann
2016-07-05 12:22 ` Uwe Kleine-König
2016-07-27 19:57 ` Uwe Kleine-König
2016-08-10 10:29 ` Tomi Valkeinen
2016-03-07 19:53 ` [PATCH 3/3] video: fbdev: imxfb: add some error handling Uwe Kleine-König
2016-03-08 8:00 ` Philipp Zabel
2016-03-11 11:26 ` Tomi Valkeinen
2016-03-07 20:03 ` [PATCH 0/3] video: fbdev: imxfb: make it work again Fabio Estevam
2016-03-07 20:10 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160420191727.GS29108@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).