[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Denis Carikli]

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);
 	}
 
-- 
1.7.9.5

[PATCH v2 2/2] video: imxfb: Fix lcd_ops .set_power and .get_power on/off inversion

[Denis Carikli]

.set_power takes the blank level as argument instead
of an on/off parameter:
fb_blank calls fb_notifier_call_chain with the blank
level encoded in its parameters, which is then sent
as-is to the set_power callback.

imxfb_lcd_set_power was expecting an on/off parameter
instead.
This resulted in the inversion of the on/off behaviour.

.get_power was also fixed in the same way.

Signed-off-by: Denis Carikli <denis@eukrea.com>
---
Changelog v1->v2:
- get_power which has the same issue, was also fixed as requested.
- The commit message was updated to reflect that.
---
 drivers/video/fbdev/imxfb.c |   27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 121cbd7..f734757 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -758,21 +758,32 @@ 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);
+	if (!IS_ERR_OR_NULL(fbi->lcd_pwr))
+		if (!regulator_is_enabled(fbi->lcd_pwr))
+			return FB_BLANK_NORMAL;
 
-	return 1;
+	return FB_BLANK_UNBLANK;
 }
 
-static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
+static int imxfb_lcd_set_power(struct lcd_device *lcddev, int blank)
 {
 	struct imxfb_info *fbi = dev_get_drvdata(&lcddev->dev);
 
 	if (!IS_ERR(fbi->lcd_pwr)) {
-		if (power && !regulator_is_enabled(fbi->lcd_pwr))
-			return regulator_enable(fbi->lcd_pwr);
-		else if (regulator_is_enabled(fbi->lcd_pwr))
-			return regulator_disable(fbi->lcd_pwr);
+		switch (blank) {
+		case FB_BLANK_VSYNC_SUSPEND:
+		case FB_BLANK_HSYNC_SUSPEND:
+		case FB_BLANK_NORMAL:
+		case FB_BLANK_POWERDOWN:
+			if (regulator_is_enabled(fbi->lcd_pwr))
+				return regulator_disable(fbi->lcd_pwr);
+			break;
+
+		case FB_BLANK_UNBLANK:
+			if (!regulator_is_enabled(fbi->lcd_pwr))
+				return regulator_enable(fbi->lcd_pwr);
+			break;
+		}
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH v2 2/2] video: imxfb: Fix lcd_ops .set_power and .get_power on/off inversion

[Sascha Hauer]

On Mon, Jun 23, 2014 at 03:07:19PM +0200, Denis Carikli wrote:
> .set_power takes the blank level as argument instead
> of an on/off parameter:
> fb_blank calls fb_notifier_call_chain with the blank
> level encoded in its parameters, which is then sent
> as-is to the set_power callback.
> 
> imxfb_lcd_set_power was expecting an on/off parameter
> instead.
> This resulted in the inversion of the on/off behaviour.
> 
> .get_power was also fixed in the same way.
> 
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> Changelog v1->v2:
> - get_power which has the same issue, was also fixed as requested.
> - The commit message was updated to reflect that.
> ---
>  drivers/video/fbdev/imxfb.c |   27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 121cbd7..f734757 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -758,21 +758,32 @@ 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);
> +	if (!IS_ERR_OR_NULL(fbi->lcd_pwr))
> +		if (!regulator_is_enabled(fbi->lcd_pwr))
> +			return FB_BLANK_NORMAL;

You use IS_ERR_OR_NULL() here. imxfb_lcd_set_power uses IS_ERR(). One of
both must be wrong.

>  
> -	return 1;
> +	return FB_BLANK_UNBLANK;
>  }
>  
> -static int imxfb_lcd_set_power(struct lcd_device *lcddev, int power)
> +static int imxfb_lcd_set_power(struct lcd_device *lcddev, int blank)
>  {
>  	struct imxfb_info *fbi = dev_get_drvdata(&lcddev->dev);
>  
>  	if (!IS_ERR(fbi->lcd_pwr)) {
> -		if (power && !regulator_is_enabled(fbi->lcd_pwr))
> -			return regulator_enable(fbi->lcd_pwr);
> -		else if (regulator_is_enabled(fbi->lcd_pwr))
> -			return regulator_disable(fbi->lcd_pwr);
> +		switch (blank) {
> +		case FB_BLANK_VSYNC_SUSPEND:
> +		case FB_BLANK_HSYNC_SUSPEND:
> +		case FB_BLANK_NORMAL:
> +		case FB_BLANK_POWERDOWN:
> +			if (regulator_is_enabled(fbi->lcd_pwr))
> +				return regulator_disable(fbi->lcd_pwr);
> +			break;
> +
> +		case FB_BLANK_UNBLANK:
> +			if (!regulator_is_enabled(fbi->lcd_pwr))
> +				return regulator_enable(fbi->lcd_pwr);
> +			break;
> +		}
>  	}

You could reduce indentation by bailing out early:

	if (IS_ERR(fbi->lcd_pwr))
		return 0;

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 2/2] video: imxfb: Fix lcd_ops .set_power and .get_power on/off inversion

[Denis Carikli]

On 06/25/2014 08:16 AM, Sascha Hauer wrote:
> You use IS_ERR_OR_NULL() here. imxfb_lcd_set_power uses IS_ERR(). One of
> both must be wrong.
Fixed by using IS_ERR_OR_NULL.

> You could reduce indentation by bailing out early:
>
> 	if (IS_ERR(fbi->lcd_pwr))
> 		return 0;
Fixed (with IS_ERR_OR_NULL).

Denis.

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Denis Carikli]

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.

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Denis Carikli]

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.

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 1/2] video: imxfb: Fix unbalanced regulators

[Shawn Guo]

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