linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv13][ 1/4] video: imxfb: Introduce regulator support.
@ 2013-12-05 15:43 Denis Carikli
  2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Denis Carikli @ 2013-12-05 15:43 UTC (permalink / raw)
  To: linux-arm-kernel

This commit is based on the following commit by Fabio Estevam:
  4344429 video: mxsfb: Introduce regulator support

Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev at vger.kernel.org
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v9->v10:
- Added a return 0; at the end of imxfb_disable_controller.

ChangeLog v8->v9:
- return an error if regulator_{enable,disable} fails in
  imxfb_{enable,disable}_controller, and use it.
---
 drivers/video/imxfb.c |   55 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 44ee678..57d3b81 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -28,6 +28,7 @@
 #include <linux/cpufreq.h>
 #include <linux/clk.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/math64.h>
@@ -145,6 +146,7 @@ struct imxfb_info {
 	struct clk		*clk_ipg;
 	struct clk		*clk_ahb;
 	struct clk		*clk_per;
+	struct regulator	*reg_lcd;
 	enum imxfb_type		devtype;
 	bool			enabled;
 
@@ -561,14 +563,25 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
 }
 #endif
 
-static void imxfb_enable_controller(struct imxfb_info *fbi)
+static int imxfb_enable_controller(struct imxfb_info *fbi)
 {
+	int ret;
 
 	if (fbi->enabled)
-		return;
+		return 0;
 
 	pr_debug("Enabling LCD controller\n");
 
+	if (fbi->reg_lcd) {
+		ret = regulator_enable(fbi->reg_lcd);
+		if (ret) {
+			dev_err(&fbi->pdev->dev,
+				"lcd regulator enable failed with error: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
 
 	/* panning offset 0 (0 pixel offset)        */
@@ -593,12 +606,16 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
 		fbi->backlight_power(1);
 	if (fbi->lcd_power)
 		fbi->lcd_power(1);
+
+	return 0;
 }
 
-static void imxfb_disable_controller(struct imxfb_info *fbi)
+static int imxfb_disable_controller(struct imxfb_info *fbi)
 {
+	int ret;
+
 	if (!fbi->enabled)
-		return;
+		return 0;
 
 	pr_debug("Disabling LCD controller\n");
 
@@ -613,6 +630,17 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
 	fbi->enabled = false;
 
 	writel(0, fbi->regs + LCDC_RMCR);
+
+	if (fbi->reg_lcd) {
+		ret = regulator_disable(fbi->reg_lcd);
+		if (ret)
+			dev_err(&fbi->pdev->dev,
+				"lcd regulator disable failed with error: %d\n",
+				ret);
+			return ret;
+	}
+
+	return 0;
 }
 
 static int imxfb_blank(int blank, struct fb_info *info)
@@ -626,13 +654,12 @@ static int imxfb_blank(int blank, struct fb_info *info)
 	case FB_BLANK_VSYNC_SUSPEND:
 	case FB_BLANK_HSYNC_SUSPEND:
 	case FB_BLANK_NORMAL:
-		imxfb_disable_controller(fbi);
-		break;
+		return imxfb_disable_controller(fbi);
 
 	case FB_BLANK_UNBLANK:
-		imxfb_enable_controller(fbi);
-		break;
+		return imxfb_enable_controller(fbi);
 	}
+
 	return 0;
 }
 
@@ -734,8 +761,7 @@ static int imxfb_suspend(struct platform_device *dev, pm_message_t state)
 
 	pr_debug("%s\n", __func__);
 
-	imxfb_disable_controller(fbi);
-	return 0;
+	return imxfb_disable_controller(fbi);
 }
 
 static int imxfb_resume(struct platform_device *dev)
@@ -745,8 +771,7 @@ static int imxfb_resume(struct platform_device *dev)
 
 	pr_debug("%s\n", __func__);
 
-	imxfb_enable_controller(fbi);
-	return 0;
+	return imxfb_enable_controller(fbi);
 }
 #else
 #define imxfb_suspend	NULL
@@ -1020,6 +1045,12 @@ static int imxfb_probe(struct platform_device *pdev)
 		goto failed_register;
 	}
 
+	fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
+	if (IS_ERR(fbi->reg_lcd)) {
+		dev_info(&pdev->dev, "No lcd regulator used.\n");
+		fbi->reg_lcd = NULL;
+	}
+
 	imxfb_enable_controller(fbi);
 	fbi->pdev = pdev;
 #ifdef PWMR_BACKLIGHT_AVAILABLE
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-05 15:43 [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Denis Carikli
@ 2013-12-05 15:43 ` Denis Carikli
  2013-12-05 15:53   ` Alexander Shiyan
  2013-12-06  7:05   ` Sascha Hauer
  2013-12-05 15:43 ` [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver Denis Carikli
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Denis Carikli @ 2013-12-05 15:43 UTC (permalink / raw)
  To: linux-arm-kernel

pwmr has to be set to get the imxfb backlight work,
though pwmr was only configurable trough the platform data.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree at vger.kernel.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev at vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
---
 .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
 drivers/video/imxfb.c                              |    2 ++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..ac457ae 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -17,6 +17,9 @@ Required nodes:
 Optional properties:
 - fsl,dmacr: DMA Control Register value. This is optional. By default, the
 	register is not modified as recommended by the datasheet.
+- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
+	optional, but defining it is necessary to get the backlight working. If that
+	property is ommited, the register is zeroed.
 - fsl,lscr1: LCDC Sharp Configuration Register value.
 
 Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 57d3b81..17a46a7 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -835,6 +835,8 @@ static int imxfb_init_fbinfo(struct platform_device *pdev)
 
 		of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
 
+		of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
+
 		/* These two function pointers could be used by some specific
 		 * platforms. */
 		fbi->lcd_power = NULL;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver.
  2013-12-05 15:43 [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Denis Carikli
  2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
@ 2013-12-05 15:43 ` Denis Carikli
  2013-12-06  7:06   ` Sascha Hauer
  2013-12-05 15:43 ` [PATCHv13][ 4/4] ARM: dts: imx25: mbimxsd25: Add displays support Denis Carikli
  2013-12-06  7:12 ` [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Sascha Hauer
  3 siblings, 1 reply; 13+ messages in thread
From: Denis Carikli @ 2013-12-05 15:43 UTC (permalink / raw)
  To: linux-arm-kernel

Without that patch, a user can't select the imxfb driver when the i.MX25 and/or
  the i.MX27 device tree board are selected and that no boards that selects
  IMX_HAVE_PLATFORM_IMX_FB are compiled in.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree at vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev at vger.kernel.org
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
ChangeLog v10->v11:
- moved my signed-off-by.

ChangeLog v8->v9:
- Added Jean-Christophe PLAGNIOL-VILLARD's ACK.
---
 drivers/video/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 4f2e1b3..22adaee 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -363,7 +363,7 @@ config FB_SA1100
 
 config FB_IMX
 	tristate "Freescale i.MX1/21/25/27 LCD support"
-	depends on FB && IMX_HAVE_PLATFORM_IMX_FB
+	depends on FB && ARCH_MXC
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv13][ 4/4] ARM: dts: imx25: mbimxsd25: Add displays support.
  2013-12-05 15:43 [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Denis Carikli
  2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
  2013-12-05 15:43 ` [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver Denis Carikli
@ 2013-12-05 15:43 ` Denis Carikli
  2013-12-06  7:12 ` [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Sascha Hauer
  3 siblings, 0 replies; 13+ messages in thread
From: Denis Carikli @ 2013-12-05 15:43 UTC (permalink / raw)
  To: linux-arm-kernel

The CMO-QVGA(With backlight), DVI-VGA and DVI-SVGA displays
were added.

Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree at vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel at lists.infradead.org
Cc: Eric B?nard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v10->v13:
- This patch is the display part splitted out from the patch adding
  support for the cpuimx25(and its baseboard).
- Shawn Guo was added to the Cc list.
- The regulator part was updated to match the current style.
- The new GPIO defines are now used in the dts(i).
---
 .../imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts  |   72 ++++++++++++++++++++
 .../imx25-eukrea-mbimxsd25-baseboard-dvi-svga.dts  |   45 ++++++++++++
 .../imx25-eukrea-mbimxsd25-baseboard-dvi-vga.dts   |   45 ++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts
 create mode 100644 arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-svga.dts
 create mode 100644 arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-vga.dts

diff --git a/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts
new file mode 100644
index 0000000..0df7e9e
--- /dev/null
+++ b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-cmo-qvga.dts
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2013 Eukr?a Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include "imx25-eukrea-mbimxsd25-baseboard.dts"
+
+/ {
+	model = "Eukrea MBIMXSD25 with the CMO-QVGA Display";
+	compatible = "eukrea,mbimxsd25-baseboard-cmo-qvga", "eukrea,mbimxsd25-baseboard", "eukrea,cpuimx25", "fsl,imx25";
+
+	cmo_qvga: display {
+		model = "CMO-QVGA";
+		bits-per-pixel = <16>;
+		fsl,pcr = <0xcad08b80>;
+		bus-width = <18>;
+		native-mode = <&qvga_timings>;
+		display-timings {
+			qvga_timings: 320x240 {
+				clock-frequency = <6500000>;
+				hactive = <320>;
+				vactive = <240>;
+				hback-porch = <30>;
+				hfront-porch = <38>;
+				vback-porch = <20>;
+				vfront-porch = <3>;
+				hsync-len = <15>;
+				vsync-len = <4>;
+			};
+		};
+	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg_lcd_3v3: regulator at 0 {
+			compatible = "regulator-fixed";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_reg_lcd_3v3>;
+			regulator-name = "lcd-3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio1 26 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+		};
+	};
+};
+
+&iomuxc {
+	imx25-eukrea-mbimxsd25-baseboard-cmo-qvga {
+		pinctrl_reg_lcd_3v3: reg_lcd_3v3 {
+			fsl,pins = <MX25_PAD_PWM__GPIO_1_26 0x80000000>;
+		};
+	};
+};
+
+&lcdc {
+	display = <&cmo_qvga>;
+	fsl,pwmr = <0x00a903ff>;
+	lcd-supply = <&reg_lcd_3v3>;
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-svga.dts b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-svga.dts
new file mode 100644
index 0000000..8eee2f6
--- /dev/null
+++ b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-svga.dts
@@ -0,0 +1,45 @@
+/*
+ * Copyright 2013 Eukr?a Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include "imx25-eukrea-mbimxsd25-baseboard.dts"
+
+/ {
+	model = "Eukrea MBIMXSD25 with the DVI-SVGA Display";
+	compatible = "eukrea,mbimxsd25-baseboard-dvi-svga", "eukrea,mbimxsd25-baseboard", "eukrea,cpuimx25", "fsl,imx25";
+
+	dvi_svga: display {
+		model = "DVI-SVGA";
+		bits-per-pixel = <16>;
+		fsl,pcr = <0xfa208b80>;
+		bus-width = <18>;
+		native-mode = <&dvi_svga_timings>;
+		display-timings {
+			dvi_svga_timings: 800x600 {
+				clock-frequency = <40000000>;
+				hactive = <800>;
+				vactive = <600>;
+				hback-porch = <75>;
+				hfront-porch = <75>;
+				vback-porch = <7>;
+				vfront-porch = <75>;
+				hsync-len = <7>;
+				vsync-len = <7>;
+			};
+		};
+	};
+};
+
+&lcdc {
+	display = <&dvi_svga>;
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-vga.dts b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-vga.dts
new file mode 100644
index 0000000..447da62
--- /dev/null
+++ b/arch/arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard-dvi-vga.dts
@@ -0,0 +1,45 @@
+/*
+ * Copyright 2013 Eukr?a Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include "imx25-eukrea-mbimxsd25-baseboard.dts"
+
+/ {
+	model = "Eukrea MBIMXSD25 with the DVI-VGA Display";
+	compatible = "eukrea,mbimxsd25-baseboard-dvi-vga", "eukrea,mbimxsd25-baseboard", "eukrea,cpuimx25", "fsl,imx25";
+
+	dvi_vga: display {
+		model = "DVI-VGA";
+		bits-per-pixel = <16>;
+		fsl,pcr = <0xfa208b80>;
+		bus-width = <18>;
+		native-mode = <&dvi_vga_timings>;
+		display-timings {
+			dvi_vga_timings: 640x480 {
+				clock-frequency = <31250000>;
+				hactive = <640>;
+				vactive = <480>;
+				hback-porch = <100>;
+				hfront-porch = <100>;
+				vback-porch = <7>;
+				vfront-porch = <100>;
+				hsync-len = <7>;
+				vsync-len = <7>;
+			};
+		};
+	};
+};
+
+&lcdc {
+	display = <&dvi_vga>;
+	status = "okay";
+};
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
@ 2013-12-05 15:53   ` Alexander Shiyan
  2013-12-06  7:05   ` Sascha Hauer
  1 sibling, 0 replies; 13+ messages in thread
From: Alexander Shiyan @ 2013-12-05 15:53 UTC (permalink / raw)
  To: linux-arm-kernel

> pwmr has to be set to get the imxfb backlight work,
> though pwmr was only configurable trough the platform data.

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185022.html

...
---

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
  2013-12-05 15:53   ` Alexander Shiyan
@ 2013-12-06  7:05   ` Sascha Hauer
  2013-12-06  8:03     ` Alexander Shiyan
  1 sibling, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-12-06  7:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 05, 2013 at 04:43:37PM +0100, Denis Carikli wrote:
> pwmr has to be set to get the imxfb backlight work,
> though pwmr was only configurable trough the platform data.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree at vger.kernel.org
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev at vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Eric B?nard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> ---
>  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
>  drivers/video/imxfb.c                              |    2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> index 46da08d..ac457ae 100644
> --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> @@ -17,6 +17,9 @@ Required nodes:
>  Optional properties:
>  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
>  	register is not modified as recommended by the datasheet.
> +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> +	optional, but defining it is necessary to get the backlight working. If that
> +	property is ommited, the register is zeroed.

Why isn't this implemented as a backlight driver? Static devicetree
provided values is very limiting.

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 |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver.
  2013-12-05 15:43 ` [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver Denis Carikli
@ 2013-12-06  7:06   ` Sascha Hauer
  0 siblings, 0 replies; 13+ messages in thread
From: Sascha Hauer @ 2013-12-06  7:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 05, 2013 at 04:43:38PM +0100, Denis Carikli wrote:
> Without that patch, a user can't select the imxfb driver when the i.MX25 and/or
>   the i.MX27 device tree board are selected and that no boards that selects
>   IMX_HAVE_PLATFORM_IMX_FB are compiled in.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree at vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev at vger.kernel.org
> Cc: Eric B?nard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>


Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> ---
> ChangeLog v10->v11:
> - moved my signed-off-by.
> 
> ChangeLog v8->v9:
> - Added Jean-Christophe PLAGNIOL-VILLARD's ACK.
> ---
>  drivers/video/Kconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 4f2e1b3..22adaee 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -363,7 +363,7 @@ config FB_SA1100
>  
>  config FB_IMX
>  	tristate "Freescale i.MX1/21/25/27 LCD support"
> -	depends on FB && IMX_HAVE_PLATFORM_IMX_FB
> +	depends on FB && ARCH_MXC
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> -- 
> 1.7.9.5
> 
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv13][ 1/4] video: imxfb: Introduce regulator support.
  2013-12-05 15:43 [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Denis Carikli
                   ` (2 preceding siblings ...)
  2013-12-05 15:43 ` [PATCHv13][ 4/4] ARM: dts: imx25: mbimxsd25: Add displays support Denis Carikli
@ 2013-12-06  7:12 ` Sascha Hauer
  3 siblings, 0 replies; 13+ messages in thread
From: Sascha Hauer @ 2013-12-06  7:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Dec 05, 2013 at 04:43:36PM +0100, Denis Carikli wrote:
> This commit is based on the following commit by Fabio Estevam:
> @@ -1020,6 +1045,12 @@ static int imxfb_probe(struct platform_device *pdev)
>  		goto failed_register;
>  	}
>  
> +	fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
> +	if (IS_ERR(fbi->reg_lcd)) {
> +		dev_info(&pdev->dev, "No lcd regulator used.\n");

Please remove this log spam or at least reduce it to dev_dbg. That's the
typical message to which our customers keep asking us whether that's
something they have to care about.

Also, you should check for -EPROBE_DEFER here and return it in this
case.

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 |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-06  7:05   ` Sascha Hauer
@ 2013-12-06  8:03     ` Alexander Shiyan
  2013-12-06  8:16       ` Sascha Hauer
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Shiyan @ 2013-12-06  8:03 UTC (permalink / raw)
  To: linux-arm-kernel

> On Thu, Dec 05, 2013 at 04:43:37PM +0100, Denis Carikli wrote:
> > pwmr has to be set to get the imxfb backlight work,
> > though pwmr was only configurable trough the platform data.
> > 
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree at vger.kernel.org
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: linux-fbdev at vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel at lists.infradead.org
> > Cc: Eric B?nard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > ---
> >  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
> >  drivers/video/imxfb.c                              |    2 ++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > index 46da08d..ac457ae 100644
> > --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > @@ -17,6 +17,9 @@ Required nodes:
> >  Optional properties:
> >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> >  	register is not modified as recommended by the datasheet.
> > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > +	optional, but defining it is necessary to get the backlight working. If that
> > +	property is ommited, the register is zeroed.
> 
> Why isn't this implemented as a backlight driver? Static devicetree
> provided values is very limiting.

Let's understand the terminology.
This register should be renamed according to the datasheet, i.e. LPCCR.
As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
Yes, it works as PWM, but nothing do with the backlight subsystem.
Yes, we can make a driver for this PWM, but how are we going to control it?
I misunderstood something?

---

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-06  8:03     ` Alexander Shiyan
@ 2013-12-06  8:16       ` Sascha Hauer
  2013-12-06  8:35         ` Alexander Shiyan
  0 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-12-06  8:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> > >  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
> > >  drivers/video/imxfb.c                              |    2 ++
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > index 46da08d..ac457ae 100644
> > > --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > @@ -17,6 +17,9 @@ Required nodes:
> > >  Optional properties:
> > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > >  	register is not modified as recommended by the datasheet.
> > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > +	optional, but defining it is necessary to get the backlight working. If that
> > > +	property is ommited, the register is zeroed.
> > 
> > Why isn't this implemented as a backlight driver? Static devicetree
> > provided values is very limiting.
> 
> Let's understand the terminology.
> This register should be renamed according to the datasheet, i.e. LPCCR.
> As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> Yes, it works as PWM, but nothing do with the backlight subsystem.
> Yes, we can make a driver for this PWM, but how are we going to control it?
> I misunderstood something?

I stumbled upon 'get the backlight working' which implied for me that it
should be a backlight driver. But you're right and now I remember we
talked about this already.

I still think this should be something adjustable, not static data.
Maybe we could change the wording to something like "This property
provides the default value for the contrast control register" since even
if we add driver support for controlling the contrast we still want
to have a sane default.

BTW the contrast could be controlled with a lcd_device (see
lcd_device_register) which seems to be very easy to implement.

SaschaMaybe we could change the wording to something like "This property
provides the default value for the contrast control register" since even
if we add driver support for controlling the contrast we still want
to have a sane default.

BTW the contrast could be controlled with a lcd_device (see
lcd_device_register) which seems to be very easy to implement.

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 |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-06  8:16       ` Sascha Hauer
@ 2013-12-06  8:35         ` Alexander Shiyan
  2013-12-06  9:30           ` Sascha Hauer
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Shiyan @ 2013-12-06  8:35 UTC (permalink / raw)
  To: linux-arm-kernel

> On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
...
> > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > >  	register is not modified as recommended by the datasheet.
> > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > +	property is ommited, the register is zeroed.
> > > 
> > > Why isn't this implemented as a backlight driver? Static devicetree
> > > provided values is very limiting.
> > 
> > Let's understand the terminology.
> > This register should be renamed according to the datasheet, i.e. LPCCR.
> > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > Yes, we can make a driver for this PWM, but how are we going to control it?
> > I misunderstood something?
> 
> I stumbled upon 'get the backlight working' which implied for me that it
> should be a backlight driver. But you're right and now I remember we
> talked about this already.

Hallelujah.

> I still think this should be something adjustable, not static data.
> Maybe we could change the wording to something like "This property
> provides the default value for the contrast control register" since even
> if we add driver support for controlling the contrast we still want
> to have a sane default.

Sounds good.

> BTW the contrast could be controlled with a lcd_device (see
> lcd_device_register) which seems to be very easy to implement.

Address of register is placed within LCD area, so we cannot use this
memory region, I think is no so easy as you say....

---

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-06  8:35         ` Alexander Shiyan
@ 2013-12-06  9:30           ` Sascha Hauer
  2013-12-06  9:40             ` Alexander Shiyan
  0 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-12-06  9:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 06, 2013 at 12:35:10PM +0400, Alexander Shiyan wrote:
> > On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> ...
> > > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > > >  	register is not modified as recommended by the datasheet.
> > > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > > +	property is ommited, the register is zeroed.
> > > > 
> > > > Why isn't this implemented as a backlight driver? Static devicetree
> > > > provided values is very limiting.
> > > 
> > > Let's understand the terminology.
> > > This register should be renamed according to the datasheet, i.e. LPCCR.
> > > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > > Yes, we can make a driver for this PWM, but how are we going to control it?
> > > I misunderstood something?
> > 
> > I stumbled upon 'get the backlight working' which implied for me that it
> > should be a backlight driver. But you're right and now I remember we
> > talked about this already.
> 
> Hallelujah.
> 
> > I still think this should be something adjustable, not static data.
> > Maybe we could change the wording to something like "This property
> > provides the default value for the contrast control register" since even
> > if we add driver support for controlling the contrast we still want
> > to have a sane default.
> 
> Sounds good.
> 
> > BTW the contrast could be controlled with a lcd_device (see
> > lcd_device_register) which seems to be very easy to implement.
> 
> Address of register is placed within LCD area, so we cannot use this
> memory region, I think is no so easy as you say....

We do not need a separate driver for this. Look for example at
drivers/video/bf537-lq035.c, it just calls lcd_device_register()
in its probe function.

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 |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree.
  2013-12-06  9:30           ` Sascha Hauer
@ 2013-12-06  9:40             ` Alexander Shiyan
  0 siblings, 0 replies; 13+ messages in thread
From: Alexander Shiyan @ 2013-12-06  9:40 UTC (permalink / raw)
  To: linux-arm-kernel

> > > On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> > ...
> > > > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > > > >  	register is not modified as recommended by the datasheet.
> > > > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > > > +	property is ommited, the register is zeroed.
> > > > > 
> > > > > Why isn't this implemented as a backlight driver? Static devicetree
> > > > > provided values is very limiting.
> > > > 
> > > > Let's understand the terminology.
> > > > This register should be renamed according to the datasheet, i.e. LPCCR.
> > > > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > > > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > > > Yes, we can make a driver for this PWM, but how are we going to control it?
> > > > I misunderstood something?
> > > 
> > > I stumbled upon 'get the backlight working' which implied for me that it
> > > should be a backlight driver. But you're right and now I remember we
> > > talked about this already.
> > 
> > Hallelujah.
> > 
> > > I still think this should be something adjustable, not static data.
> > > Maybe we could change the wording to something like "This property
> > > provides the default value for the contrast control register" since even
> > > if we add driver support for controlling the contrast we still want
> > > to have a sane default.
> > 
> > Sounds good.
> > 
> > > BTW the contrast could be controlled with a lcd_device (see
> > > lcd_device_register) which seems to be very easy to implement.
> > 
> > Address of register is placed within LCD area, so we cannot use this
> > memory region, I think is no so easy as you say....
> 
> We do not need a separate driver for this. Look for example at
> drivers/video/bf537-lq035.c, it just calls lcd_device_register()
> in its probe function.

Nice. Seems this example even can handle LCD power
regulator from "[1/4] video: imxfb: Introduce regulator support."

---

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-12-06  9:40 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-05 15:43 [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Denis Carikli
2013-12-05 15:43 ` [PATCHv13][ 2/4] video: imxfb: Also add pwmr for the device tree Denis Carikli
2013-12-05 15:53   ` Alexander Shiyan
2013-12-06  7:05   ` Sascha Hauer
2013-12-06  8:03     ` Alexander Shiyan
2013-12-06  8:16       ` Sascha Hauer
2013-12-06  8:35         ` Alexander Shiyan
2013-12-06  9:30           ` Sascha Hauer
2013-12-06  9:40             ` Alexander Shiyan
2013-12-05 15:43 ` [PATCHv13][ 3/4] video: Kconfig: Allow more broad selection of the imxfb framebuffer driver Denis Carikli
2013-12-06  7:06   ` Sascha Hauer
2013-12-05 15:43 ` [PATCHv13][ 4/4] ARM: dts: imx25: mbimxsd25: Add displays support Denis Carikli
2013-12-06  7:12 ` [PATCHv13][ 1/4] video: imxfb: Introduce regulator support Sascha Hauer

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