| Message ID | 1403528839-31901-1-git-send-email-denis@eukrea.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 23, 2014 at 03:07:18PM +0200, Denis Carikli wrote: > Both regulator_enable and regulator_disable's comments > say that they must be balanced with its counterpart > enable/disable function. > > Not doing it result in kernel warnings each time > the lcd is powered off twice, for instance trough sysfs. > > Signed-off-by: Denis Carikli <denis@eukrea.com> > --- > Changelog v1->v2: > - None > --- > drivers/video/fbdev/imxfb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c > index f6e6216..121cbd7 100644 > --- a/drivers/video/fbdev/imxfb.c > +++ b/drivers/video/fbdev/imxfb.c > @@ -769,9 +769,9 @@ 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 && !regulator_is_enabled(fbi->lcd_pwr)) > return regulator_enable(fbi->lcd_pwr); > - else > + else if (regulator_is_enabled(fbi->lcd_pwr)) > return regulator_disable(fbi->lcd_pwr); It's a shame that the LCD controller doesn't do the reference counting. I really think it should be fixed there and not in the drivers. If for whatever reason this is not acceptable then it should be done in the imxfb driver instead of shifting it further to the regulator core. Sascha
On Mon, Jun 23, 2014 at 03:07:18PM +0200, Denis Carikli wrote: > Both regulator_enable and regulator_disable's comments > say that they must be balanced with its counterpart > enable/disable function. > > Not doing it result in kernel warnings each time > the lcd is powered off twice, for instance trough sysfs. > > Signed-off-by: Denis Carikli <denis@eukrea.com> > --- > Changelog v1->v2: > - None > --- > drivers/video/fbdev/imxfb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This two patches should have fbdev maintainers and list on copy. Shawn
On 06/25/2014 08:08 AM, Sascha Hauer wrote: > It's a shame that the LCD controller doesn't do the reference counting. > I really think it should be fixed there and not in the drivers.If for > whatever reason this is not acceptable then it should be done in the > imxfb driver instead of shifting it further to the regulator core. I'm not sure that doing reference counting in the LCD class would be enough for the regulator case. Still it seems to be a good thing to do. If I understood well, that assumes that the LCD driver is the only consumer and owner of that regulator. There is also the fact that the regulator core also enables or disables regulators by itself: For instance at boot it tries to disable the regulators that can be disabled, including this regulator. So I don't see any ways to handle it correctly in the LCD. Denis.
On 06/25/2014 10:51 AM, Denis Carikli wrote: > On 06/25/2014 08:08 AM, Sascha Hauer wrote: >> It's a shame that the LCD controller doesn't do the reference counting. [...] > So I don't see any ways to handle it correctly in the LCD. Unless if the LCD core, instead of doing reference counting, does the check by itself, by using get_power(). But I doubt that it would be a good solution (I fear that it would have race conditions). Denis.
On Wed, Jun 25, 2014 at 10:51:24AM +0200, Denis Carikli wrote: > On 06/25/2014 08:08 AM, Sascha Hauer wrote: > >It's a shame that the LCD controller doesn't do the reference counting. > >I really think it should be fixed there and not in the drivers.If for > >whatever reason this is not acceptable then it should be done in the > >imxfb driver instead of shifting it further to the regulator core. > I'm not sure that doing reference counting in the LCD class would be > enough for the regulator case. Still it seems to be a good thing to > do. > > If I understood well, that assumes that the LCD driver is the only > consumer and owner of that regulator. No. All consumers of a regulator must make sure the regulator enable count is balanced. Image that the regulator in the imxfb is used by multiple consumers. When you want to enable the backlight you must make sure that you take a reference to the regulator yourself. Otherwise it may happen that 'if (regulator_is_enabled())' is true because another consumer has enabled it and your code does nothing. Then the other consumer decides to disable the regulator which in this case will accidently disable the backlight. > > There is also the fact that the regulator core also enables or > disables regulators by itself: > For instance at boot it tries to disable the regulators that can be > disabled, including this regulator. > > So I don't see any ways to handle it correctly in the LCD. It's simple. Add an lcd_enable flag to struct lcd_device. Make sure you only call into the drivers when it differs from the current state. Sascha
diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c index f6e6216..121cbd7 100644 --- a/drivers/video/fbdev/imxfb.c +++ b/drivers/video/fbdev/imxfb.c @@ -769,9 +769,9 @@ 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 && !regulator_is_enabled(fbi->lcd_pwr)) return regulator_enable(fbi->lcd_pwr); - else + else if (regulator_is_enabled(fbi->lcd_pwr)) return regulator_disable(fbi->lcd_pwr); }
Both regulator_enable and regulator_disable's comments say that they must be balanced with its counterpart enable/disable function. Not doing it result in kernel warnings each time the lcd is powered off twice, for instance trough sysfs. Signed-off-by: Denis Carikli <denis@eukrea.com> --- Changelog v1->v2: - None --- drivers/video/fbdev/imxfb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)