linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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/  |

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