Skip to contentSkip to conversation

ARM: dts: mbimxsd25: cmo-qvga: Fix lcd regulator

PATCH In review Denis Carikli 3 participants 5 messages Jun 17, 2014 – Jun 21, 2014

Subsystems (4)
Device tree bindings Arm i.MX and Layerscape Framebuffer layer Freescale i.MX / MXC framebuffer driver
lore
Share & tools
lore

Denis Carikli

author
#0 lore raw

PATCH 1/3 ARM: dts: mbimxsd25: cmo-qvga: Fix lcd regulator

The fixed-regulator's enable-active-high property
is needed to indicate that the GPIO regulator is
active high.

Before that the regulator state was inverted.

Signed-off-by Denis Carikli <[email protected]>
---
 .../imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts  |    1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts +1 -0
index 9797280..68d0834 100644
@@ -52,6 +52,7 @@
 			regulator-min-microvolt = <3300000>;
 			regulator-max-microvolt = <3300000>;
 			gpio = <&gpio1 26 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
 		};
 	};
 };
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 2/3 video: imxfb: Fix unbalanced regulators

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 <[email protected]>
---
 drivers/video/fbdev/imxfb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/video/fbdev/imxfb.c +2 -2
index f6e6216..121cbd7 100644
@@ -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);
 	}
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 3/3 video: imxfb: Fix lcd_ops .set_power on/off inversion

In reply to 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.

Signed-off-by Denis Carikli <[email protected]>
---
 drivers/video/fbdev/imxfb.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
drivers/video/fbdev/imxfb.c +15 -5
index 121cbd7..2ffbd5e 100644
@@ -764,15 +764,25 @@ static int imxfb_lcd_get_power(struct lcd_device *lcddev)
 	return 1;
 }
 
-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;
Signature · 1 line
1.7.9.5
#3 lore raw

Re: [PATCH 3/3] video: imxfb: Fix lcd_ops .set_power on/off inversion

Tue, 17 Jun 2014 12:46:39 +0200 ?? Denis Carikli <[email protected]>:

Quoted 40 lines

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

Signed-off-by Denis Carikli <[email protected]>
---
 drivers/video/fbdev/imxfb.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
drivers/video/fbdev/imxfb.c +8 -5
index 121cbd7..2ffbd5e 100644
@@ -764,15 +764,25 @@ static int imxfb_lcd_get_power(struct lcd_device *lcddev)
 	return 1;
 }
 
-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);

drivers/video/fbdev/imxfb.c static int imxfb_lcd_get_power(struct lcd_device *lcddev) · line 788

Quoted 10 lines
drivers/video/fbdev/imxfb.c +7 -0
+			break;
+
+		case FB_BLANK_UNBLANK:
+			if (!regulator_is_enabled(fbi->lcd_pwr))
+				return regulator_enable(fbi->lcd_pwr);
+			break;
+		}
 	}
 
 	return 0;

I think, the patch is correct, but we should update imxfb_lcd_get_power() in the same way.

Like as:
if (!IS_ERR_OR_NULL(fbi->lcd_pwr))
if (!regulator_is_enabled(fbi->lcd_pwr))

    return FB_BLANK_NORMAL;

return FB_BLANK_UNBLANK;

---

Applied
#4 lore raw

PATCH 1/3 ARM: dts: mbimxsd25: cmo-qvga: Fix lcd regulator

In reply to Denis Carikli
Quoted 7 lines · Denis Carikli

The fixed-regulator's enable-active-high property
is needed to indicate that the GPIO regulator is
active high.

Before that the regulator state was inverted.

Signed-off-by Denis Carikli <[email protected]>

Applied this one, thanks.