[PATCH] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset

Add a lcd panel driver for simple raster-type lcd's which uses a gpio
controlled panel reset. The driver controls the nRESET line of the panel
using a gpio connected from the host system. The Vcc supply to the panel
is (optionally) controlled using a voltage regulator. This driver excludes
support for lcd panels that use a serial command interface or direct
memory mapped IO interface.

Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Thomas Abraham <redacted>
---
 .../devicetree/bindings/lcd/lcd-pwrctrl.txt        |   39 ++++
 drivers/video/backlight/Kconfig                    |    7 +
 drivers/video/backlight/Makefile                   |    1 +
 drivers/video/backlight/lcd_pwrctrl.c              |  231 ++++++++++++++++++++
 include/video/lcd_pwrctrl.h                        |   30 +++
 5 files changed, 308 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
 create mode 100644 drivers/video/backlight/lcd_pwrctrl.c
 create mode 100644 include/video/lcd_pwrctrl.h

diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
new file mode 100644
index 0000000..941e2ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt

@@ -0,0 +1,39 @@
+* Power controller for simple lcd panels
+
+Some LCD panels provide a simple control interface for the host system. The
+control mechanism would include a nRESET line connected to a gpio of the host
+system and a Vcc supply line which the host can optionally be controlled using
+a voltage regulator. Such simple panels do not support serial command
+interface (such as i2c or spi) or memory-mapped-io interface.
+
+Required properties:
+- compatible: should be 'lcd,powerctrl'
+- gpios: The GPIO number of the host system used to control the nRESET line.
+  The format of the gpio specifier depends on the gpio controller of the
+  host system.
+
+Optional properties:
+- lcd,pwrctrl-nreset-gpio-invert: When the nRESET line is asserted low, the
+  lcd panel is reset and stays in reset mode as long as the nRESET line is
+  asserted low. This is the default behaviour of most lcd panels. If a lcd
+  panel requires the nRESET line to be asserted high for panel reset, then
+  this property is used.
+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the minimum voltage the regulator should supply.
+  The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the maximum voltage the regulator should limit to
+  on the Vcc line. The value of this property should in in micro-volts.
+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+  the lcd panel.
+
+Example:
+
+	lcd_pwrctrl {
+		compatible = "lcd,powerctrl";
+		gpios = <&gpe0 4 1 0 0>;
+		lcd,pwrctrl-nreset-gpio-invert;
+		lcd,pwrctrl-min-uV = <2500000>;
+		lcd,pwrctrl-max-uV = <3300000>;
+		lcd-vcc-supply - <&regulator7>;
+	};

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 278aeaa..fc1dc17 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig

@@ -86,6 +86,13 @@ config LCD_PLATFORM
 	  This driver provides a platform-device registered LCD power
 	  control interface.
 
+config LCD_PWRCTRL
+	tristate "LCD panel power control"
+	help
+	  Say y here, if you have a lcd panel that allows reset and vcc to be
+	  controlled by the host system, and which does not use a serial command
+	  interface (such as i2c or spi) or memory-mapped-io interface.
+
 config LCD_TOSA
 	tristate "Sharp SL-6000 LCD Driver"
 	depends on SPI && MACH_TOSA

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index fdd1fc4..944482e 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile

@@ -8,6 +8,7 @@ obj-$(CONFIG_LCD_LMS283GF05)	   += lms283gf05.o
 obj-$(CONFIG_LCD_LTV350QV)	   += ltv350qv.o
 obj-$(CONFIG_LCD_ILI9320)	   += ili9320.o
 obj-$(CONFIG_LCD_PLATFORM)	   += platform_lcd.o
+obj-$(CONFIG_LCD_PWRCTRL) 	   += lcd_pwrctrl.o
 obj-$(CONFIG_LCD_VGG2432A4)	   += vgg2432a4.o
 obj-$(CONFIG_LCD_TDO24M)	   += tdo24m.o
 obj-$(CONFIG_LCD_TOSA)		   += tosa_lcd.o

diff --git a/drivers/video/backlight/lcd_pwrctrl.c b/drivers/video/backlight/lcd_pwrctrl.c
new file mode 100644
index 0000000..6f3110b
--- /dev/null
+++ b/drivers/video/backlight/lcd_pwrctrl.c

@@ -0,0 +1,231 @@
+/*
+ * Simple lcd panel power control driver.
+ *
+ * Copyright (c) 2011-2012 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2011-2012 Linaro Ltd.
+ *
+ * This driver is for controlling power for raster type lcd panels that requires
+ * its nRESET interface line to be connected and controlled by a GPIO of the
+ * host system and the Vcc line controlled by a voltage regulator.  This
+ * excludes support for lcd panels that use a serial command interface or direct
+ * memory mapped IO interface.
+ *
+ * The nRESET interface line of the panel should be connected to a gpio of the
+ * host system. The Vcc pin is controlled using a external volatage regulator.
+ * Panel backlight is not controlled by this driver.
+ *
+ * This driver is derived from platform-lcd.c which was written by
+ * Ben Dooks <ben@simtec.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+*/
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/fb.h>
+#include <linux/lcd.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
+#include <video/lcd_pwrctrl.h>
+
+struct lcd_pwrctrl {
+	struct device		*dev;
+	struct lcd_device	*lcd;
+	struct lcd_pwrctrl_data	*pdata;
+	struct regulator	*regulator;
+	unsigned int		power;
+	unsigned int		suspended:1;
+};
+
+static int lcd_pwrctrl_get_power(struct lcd_device *lcd)
+{
+	struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+	return lp->power;
+}
+
+static int lcd_pwrctrl_set_power(struct lcd_device *lcd, int power)
+{
+	struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+	struct lcd_pwrctrl_data *pd = lp->pdata;
+	int lcd_enable, lcd_reset;
+
+	lcd_enable = (power == FB_BLANK_POWERDOWN || lp->suspended) ? 0 : 1;
+	lcd_reset = (pd->invert) ? !lcd_enable : lcd_enable;
+
+	if (IS_ERR(lp->regulator))
+		goto no_regulator;
+
+	if (lcd_enable) {
+		if ((pd->min_uV || pd->max_uV) &&
+			regulator_set_voltage(lp->regulator,
+						pd->min_uV, pd->max_uV))
+				dev_info(lp->dev,
+					"regulator voltage set failed\n");
+		if (regulator_enable(lp->regulator))
+			dev_info(lp->dev, "failed to enable regulator\n");
+	} else {
+		regulator_disable(lp->regulator);
+	}
+
+ no_regulator:
+	gpio_direction_output(lp->pdata->gpio, lcd_reset);
+	lp->power = power;
+	return 0;
+}
+
+static int lcd_pwrctrl_check_fb(struct lcd_device *lcd, struct fb_info *info)
+{
+	struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+	return lp->dev->parent == info->device;
+}
+
+static struct lcd_ops lcd_pwrctrl_ops = {
+	.get_power	= lcd_pwrctrl_get_power,
+	.set_power	= lcd_pwrctrl_set_power,
+	.check_fb	= lcd_pwrctrl_check_fb,
+};
+
+#ifdef CONFIG_OF
+static void __devinit lcd_pwrctrl_parse_dt(struct device *dev,
+					struct lcd_pwrctrl_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+
+	pdata->gpio = of_get_gpio(np, 0);
+	if (of_get_property(np, "lcd,pwrctrl-nreset-gpio-invert", NULL))
+		pdata->invert = true;
+	of_property_read_u32(np, "lcd,pwrctrl-min-uV", &pdata->min_uV);
+	of_property_read_u32(np, "lcd,pwrctrl-max-uV", &pdata->max_uV);
+}
+#endif
+
+static int __devinit lcd_pwrctrl_probe(struct platform_device *pdev)
+{
+	struct lcd_pwrctrl *lp;
+	struct lcd_pwrctrl_data *pdata = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	int err;
+
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			dev_err(dev, "memory allocation for pdata failed\n");
+			return -ENOMEM;
+		}
+		lcd_pwrctrl_parse_dt(dev, pdata);
+	}
+#endif
+
+	if (!pdata) {
+		dev_err(dev, "platform data not available\n");
+		return -EINVAL;
+	}
+
+	err = gpio_request(pdata->gpio, "LCD-nRESET");
+	if (err) {
+		dev_err(dev, "gpio [%d] request failed\n", pdata->gpio);
+		return err;
+	}
+
+	lp = devm_kzalloc(dev, sizeof(struct lcd_pwrctrl), GFP_KERNEL);
+	if (!lp) {
+		dev_err(dev, "memory allocation failed for private data\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * If power to lcd and/or lcd interface is controlled using a regulator,
+	 * get the handle to the regulator for later use during power switching.
+	 */
+	lp->regulator = regulator_get(dev, "vcc-lcd");
+	if (IS_ERR(lp->regulator))
+		dev_info(dev, "could not find regulator\n");
+
+	lp->dev = dev;
+	lp->pdata = pdata;
+	lp->lcd = lcd_device_register(dev_name(dev), dev, lp, &lcd_pwrctrl_ops);
+	if (IS_ERR(lp->lcd)) {
+		dev_err(dev, "cannot register lcd device\n");
+		regulator_put(lp->regulator);
+		return PTR_ERR(lp->lcd);
+	}
+
+	platform_set_drvdata(pdev, lp);
+	lcd_pwrctrl_set_power(lp->lcd, FB_BLANK_NORMAL);
+	return 0;
+}
+
+static int __devexit lcd_pwrctrl_remove(struct platform_device *pdev)
+{
+	struct lcd_pwrctrl *lp = platform_get_drvdata(pdev);
+	lcd_device_unregister(lp->lcd);
+	gpio_free(lp->pdata->gpio);
+	if (!IS_ERR(lp->regulator))
+		regulator_put(lp->regulator);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int lcd_pwrctrl_suspend(struct platform_device *pdev, pm_message_t st)
+{
+	struct lcd_pwrctrl *lp = platform_get_drvdata(pdev);
+
+	lp->suspended = 1;
+	lcd_pwrctrl_set_power(lp->lcd, FB_BLANK_POWERDOWN);
+	return 0;
+}
+
+static int lcd_pwrctrl_resume(struct platform_device *pdev)
+{
+	struct lcd_pwrctrl *lp = platform_get_drvdata(pdev);
+
+	lp->suspended = 0;
+	lcd_pwrctrl_set_power(lp->lcd, FB_BLANK_UNBLANK);
+	return 0;
+}
+#else
+#define lcd_pwrctrl_suspend NULL
+#define lcd_pwrctrl_resume NULL
+#endif /* CONFIG_PM */
+
+#ifdef CONFIG_OF
+static const struct of_device_id lcd_pwrctrl_match[] = {
+	{ .compatible = "lcd,powerctrl", },
+	{},
+};
+#endif
+
+static struct platform_driver lcd_pwrctrl_driver = {
+	.driver		= {
+		.name	= "lcd-pwrctrl",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(lcd_pwrctrl_match),
+	},
+	.probe		= lcd_pwrctrl_probe,
+	.remove		= lcd_pwrctrl_remove,
+	.suspend	= lcd_pwrctrl_suspend,
+	.resume		= lcd_pwrctrl_resume,
+};
+
+static int __init lcd_pwrctrl_init(void)
+{
+	return platform_driver_register(&lcd_pwrctrl_driver);
+}
+
+static void __exit lcd_pwrctrl_cleanup(void)
+{
+	platform_driver_unregister(&lcd_pwrctrl_driver);
+}
+
+module_init(lcd_pwrctrl_init);
+module_exit(lcd_pwrctrl_cleanup);
+
+MODULE_AUTHOR("Thomas Abraham <thomas.ab@samsung.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:lcd-pwrctrl");

diff --git a/include/video/lcd_pwrctrl.h b/include/video/lcd_pwrctrl.h
new file mode 100644
index 0000000..75b6ce2
--- /dev/null
+++ b/include/video/lcd_pwrctrl.h

@@ -0,0 +1,30 @@
+/*
+ * Simple lcd panel power control driver.
+ *
+ * Copyright (c) 2011-2012 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2011-2012 Linaro Ltd.
+ *
+ * This driver is derived from platform-lcd.h which was written by
+ * Ben Dooks <ben@simtec.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+*/
+
+/**
+ * struct lcd_pwrctrl_data - platform data for lcd_pwrctrl driver.
+ * @gpio: GPIO number of the host system that connects to nRESET line.
+ * @invert: True, if output of gpio connected to nRESET should be inverted.
+ * @min_uV: Minimum required voltage output from the regulator. The voltage
+ *   should be specified in micro-volts.
+ * @max_uV: Maximum acceptable voltage output from the regulator. The voltage
+ *   should be specified in micro-volts.
+ */
+struct lcd_pwrctrl_data {
+	int		gpio;
+	bool		invert;
+	int		min_uV;
+	int		max_uV;
+};

-- 
1.6.6.rc2

Maybe use OF_GPIO_ACTIVE_LOW here instead. That would make active high the
default but be a bit more consistent.

+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the minimum voltage the regulator should supply.
+  The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the maximum voltage the regulator should limit to
+  on the Vcc line. The value of this property should in in micro-volts.

The min and max voltage should rather be specified through the regulator
constraints.

+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+  the lcd panel.
+

[...]

I wouldn't use a goto here.

+
+	if (lcd_enable) {
+		if ((pd->min_uV || pd->max_uV) &&
+			regulator_set_voltage(lp->regulator,
+						pd->min_uV, pd->max_uV))
+				dev_info(lp->dev,
+					"regulator voltage set failed\n");
+		if (regulator_enable(lp->regulator))
+			dev_info(lp->dev, "failed to enable regulator\n");
+	} else {
+		regulator_disable(lp->regulator);
+	}

I think you have to make sure that the regulator enable and disable calls are
balanced.

+
+ no_regulator:
+	gpio_direction_output(lp->pdata->gpio, lcd_reset);
+	lp->power = power;
+	return 0;
+}
+

[...]

+
+#ifdef CONFIG_OF

I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself out.

+static void __devinit lcd_pwrctrl_parse_dt(struct device *dev,
+					struct lcd_pwrctrl_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+
+	pdata->gpio = of_get_gpio(np, 0);
+	if (of_get_property(np, "lcd,pwrctrl-nreset-gpio-invert", NULL))
+		pdata->invert = true;
+	of_property_read_u32(np, "lcd,pwrctrl-min-uV", &pdata->min_uV);
+	of_property_read_u32(np, "lcd,pwrctrl-max-uV", &pdata->max_uV);
+}
+#endif
+
+static int __devinit lcd_pwrctrl_probe(struct platform_device *pdev)
+{
+	struct lcd_pwrctrl *lp;
+	struct lcd_pwrctrl_data *pdata = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	int err;
+
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			dev_err(dev, "memory allocation for pdata failed\n");
+			return -ENOMEM;
+		}
+		lcd_pwrctrl_parse_dt(dev, pdata);
+	}
+#endif
+
+	if (!pdata) {
+		dev_err(dev, "platform data not available\n");
+		return -EINVAL;
+	}
+
+	err = gpio_request(pdata->gpio, "LCD-nRESET");
+	if (err) {
+		dev_err(dev, "gpio [%d] request failed\n", pdata->gpio);
+		return err;
+	}
+
+	lp = devm_kzalloc(dev, sizeof(struct lcd_pwrctrl), GFP_KERNEL);
+	if (!lp) {
+		dev_err(dev, "memory allocation failed for private data\n");
+		return -ENOMEM;

You are leaking the gpio here.

+	}
+
+	/*
+	 * If power to lcd and/or lcd interface is controlled using a regulator,
+	 * get the handle to the regulator for later use during power switching.
+	 */
+	lp->regulator = regulator_get(dev, "vcc-lcd");
+	if (IS_ERR(lp->regulator))
+		dev_info(dev, "could not find regulator\n");
+
+	lp->dev = dev;
+	lp->pdata = pdata;
+	lp->lcd = lcd_device_register(dev_name(dev), dev, lp, &lcd_pwrctrl_ops);
+	if (IS_ERR(lp->lcd)) {
+		dev_err(dev, "cannot register lcd device\n");
+		regulator_put(lp->regulator);

And here.

+		return PTR_ERR(lp->lcd);
+	}
+
+	platform_set_drvdata(pdev, lp);
+	lcd_pwrctrl_set_power(lp->lcd, FB_BLANK_NORMAL);
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id lcd_pwrctrl_match[] = {
+	{ .compatible = "lcd,powerctrl", },
+	{},
+};

MODULE_DEVICE_TABLE(...)

+#endif

+static struct platform_driver lcd_pwrctrl_driver = {
+	.driver		= {
+		.name	= "lcd-pwrctrl",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(lcd_pwrctrl_match),
+	},
+	.probe		= lcd_pwrctrl_probe,
+	.remove		= lcd_pwrctrl_remove,
+	.suspend	= lcd_pwrctrl_suspend,
+	.resume		= lcd_pwrctrl_resume,

please use dev_pm_ops instead of the legacy callbacks

+};
+
+static int __init lcd_pwrctrl_init(void)
+{
+	return platform_driver_register(&lcd_pwrctrl_driver);
+}
+
+static void __exit lcd_pwrctrl_cleanup(void)
+{
+	platform_driver_unregister(&lcd_pwrctrl_driver);
+}
+
+module_init(lcd_pwrctrl_init);
+module_exit(lcd_pwrctrl_cleanup);

module_platform_driver(&lcd_pwrctrl_driver);

+
+MODULE_AUTHOR("Thomas Abraham [off-list ref]");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:lcd-pwrctrl");

Hi, Thomas.

Can you remove unnecessary space?
Please use <tab><space><space><space> instead of <space><tab><space><space><space>
between PWRCTRL) and += lcd_.

[...]
+static struct platform_driver lcd_pwrctrl_driver = {
+	.driver		= {
+		.name	= "lcd-pwrctrl",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(lcd_pwrctrl_match),
+	},
+	.probe		= lcd_pwrctrl_probe,
+	.remove		= lcd_pwrctrl_remove,
+	.suspend	= lcd_pwrctrl_suspend,
+	.resume		= lcd_pwrctrl_resume,

Please use 'struct dev_pm_ops'.

+};
+
+static int __init lcd_pwrctrl_init(void)
+{
+	return platform_driver_register(&lcd_pwrctrl_driver);
+}
+
+static void __exit lcd_pwrctrl_cleanup(void)
+{
+	platform_driver_unregister(&lcd_pwrctrl_driver);
+}
+
+module_init(lcd_pwrctrl_init);
+module_exit(lcd_pwrctrl_cleanup);

Use module_platform_driver(lcd_pwrctrl_driver).
It can make the code simpler.

[...]

Hi,

This looks much better than the previous approach. Some comments on
the binding below.

On Thu, Jan 5, 2012 at 7:42 AM, Thomas Abraham
[off-list ref] wrote:

The convention for names is "vendor,product", so it would be better to
name this something like "lcd-powercontrol"

+- gpios: The GPIO number of the host system used to control the nRESET line.
+ ?The format of the gpio specifier depends on the gpio controller of the
+ ?host system.
+
+Optional properties:
+- lcd,pwrctrl-nreset-gpio-invert: When the nRESET line is asserted low, the
+ ?lcd panel is reset and stays in reset mode as long as the nRESET line is
+ ?asserted low. This is the default behaviour of most lcd panels. If a lcd
+ ?panel requires the nRESET line to be asserted high for panel reset, then
+ ?this property is used.
+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the minimum voltage the regulator should supply.
+ ?The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the maximum voltage the regulator should limit to
+ ?on the Vcc line. The value of this property should in in micro-volts.
+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ ?the lcd panel.

The above names are somewhat inconsistent. Why abbreviate powercontrol
in different ways between compatible and properties, for example.

Also, since there's no vendor to prefix with, it might just be easier
to avoid the prefix alltogether, or use a <word>-<property> prefix
instead, such as:

lcd-reset-gpios
lcd-reset-active-low   (some platforms can specify polarity in the
gpio specifier, so I'm not sure if this is needed?
lcd-power-min-uV
lcd-power-max-uV
lcd-power-supply

+
+Example:
+
+ ? ? ? lcd_pwrctrl {
+ ? ? ? ? ? ? ? compatible = "lcd,powerctrl";
+ ? ? ? ? ? ? ? gpios = <&gpe0 4 1 0 0>;
+ ? ? ? ? ? ? ? lcd,pwrctrl-nreset-gpio-invert;
+ ? ? ? ? ? ? ? lcd,pwrctrl-min-uV = <2500000>;
+ ? ? ? ? ? ? ? lcd,pwrctrl-max-uV = <3300000>;
+ ? ? ? ? ? ? ? lcd-vcc-supply - <&regulator7>;
+ ? ? ? };

[...]

+
+#ifdef CONFIG_OF
+static void __devinit lcd_pwrctrl_parse_dt(struct device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct lcd_pwrctrl_data *pdata)
+{
+ ? ? ? struct device_node *np = dev->of_node;
+
+ ? ? ? pdata->gpio = of_get_gpio(np, 0);
+ ? ? ? if (of_get_property(np, "lcd,pwrctrl-nreset-gpio-invert", NULL))
+ ? ? ? ? ? ? ? pdata->invert = true;
+ ? ? ? of_property_read_u32(np, "lcd,pwrctrl-min-uV", &pdata->min_uV);
+ ? ? ? of_property_read_u32(np, "lcd,pwrctrl-max-uV", &pdata->max_uV);
+}
+#endif
+
+static int __devinit lcd_pwrctrl_probe(struct platform_device *pdev)
+{
+ ? ? ? struct lcd_pwrctrl *lp;
+ ? ? ? struct lcd_pwrctrl_data *pdata = pdev->dev.platform_data;
+ ? ? ? struct device *dev = &pdev->dev;
+ ? ? ? int err;
+
+#ifdef CONFIG_OF
+ ? ? ? if (dev->of_node) {
+ ? ? ? ? ? ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ ? ? ? ? ? ? ? if (!pdata) {
+ ? ? ? ? ? ? ? ? ? ? ? dev_err(dev, "memory allocation for pdata failed\n");
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? lcd_pwrctrl_parse_dt(dev, pdata);
+ ? ? ? }
+#endif

The usual way of handling this is by checking if pdata is NULL, and if
so, call lcd_pwrctrl_fill_pdata() that returns an allocated pdata
structure (and check pdata for NULL again). That can also be done by
doing a stub that returns NULL and not use ifdef in the C code.

[...]

If this is a pure open firmware driver, then there is no need to
export this, you can just keep it internal to the C file.

On Thu, Jan 05, 2012 at 08:07:53PM +0100, Lars-Peter Clausen wrote:

quoted

+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the minimum voltage the regulator should supply.
+  The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+  this property specifies the maximum voltage the regulator should limit to
+  on the Vcc line. The value of this property should in in micro-volts.

The min and max voltage should rather be specified through the regulator
constraints.

In principle it should be specified in both places to account for shared
supplies though for all practical purposes for an LCD panel I can't see
multiple users sharing the same regulator and varying the voltage at
runtime.

quoted

+	if (lcd_enable) {
+		if ((pd->min_uV || pd->max_uV) &&
+			regulator_set_voltage(lp->regulator,
+						pd->min_uV, pd->max_uV))
+				dev_info(lp->dev,
+					"regulator voltage set failed\n");
+		if (regulator_enable(lp->regulator))
+			dev_info(lp->dev, "failed to enable regulator\n");
+	} else {
+		regulator_disable(lp->regulator);
+	}

I think you have to make sure that the regulator enable and disable calls are
balanced.

Yes.

quoted

+#ifdef CONFIG_OF

I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself out.

It's reasonably idiomatic to do this if the parsing code is in a
separate function.

Hi Lars,

On 6 January 2012 00:37, Lars-Peter Clausen [off-list ref] wrote:

quoted

[...]

diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt

[...]

quoted

+Optional properties:
+- lcd,pwrctrl-nreset-gpio-invert: When the nRESET line is asserted low, the
+ ?lcd panel is reset and stays in reset mode as long as the nRESET line is
+ ?asserted low. This is the default behaviour of most lcd panels. If a lcd
+ ?panel requires the nRESET line to be asserted high for panel reset, then
+ ?this property is used.

Maybe use OF_GPIO_ACTIVE_LOW here instead. That would make active high the
default but be a bit more consistent.

Thanks for pointing this out. But most of these panels have active low
RESET line. So OF_GPIO_ACTIVE_LOW will need to be added for every lcd
node in dts file. How about adding a new 'OF_GPIO_ACTIVE_HIGH' instead
and keeping active-low the default?

quoted

+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the minimum voltage the regulator should supply.
+ ?The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the maximum voltage the regulator should limit to
+ ?on the Vcc line. The value of this property should in in micro-volts.

The min and max voltage should rather be specified through the regulator
constraints.

The min and max voltage is a panel specific property which is used to
configure the regulator. The regulator might be capable of a larger
range but these value is used to setup the regulator to output a
voltage suitable for the panel.

quoted

+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ ?the lcd panel.
+

[...]

quoted

diff --git a/drivers/video/backlight/lcd_pwrctrl.c b/drivers/video/backlight/lcd_pwrctrl.c
new file mode 100644
index 0000000..6f3110b
--- /dev/null
+++ b/drivers/video/backlight/lcd_pwrctrl.c

@@ -0,0 +1,231 @@

[...]
+static int lcd_pwrctrl_set_power(struct lcd_device *lcd, int power)
+{
+ ? ? struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+ ? ? struct lcd_pwrctrl_data *pd = lp->pdata;
+ ? ? int lcd_enable, lcd_reset;
+
+ ? ? lcd_enable = (power == FB_BLANK_POWERDOWN || lp->suspended) ? 0 : 1;
+ ? ? lcd_reset = (pd->invert) ? !lcd_enable : lcd_enable;
+
+ ? ? if (IS_ERR(lp->regulator))
+ ? ? ? ? ? ? goto no_regulator;

I wouldn't use a goto here.

Ok. I was not happy either while writing it. I will re-work this.

quoted

+
+ ? ? if (lcd_enable) {
+ ? ? ? ? ? ? if ((pd->min_uV || pd->max_uV) &&
+ ? ? ? ? ? ? ? ? ? ? regulator_set_voltage(lp->regulator,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pd->min_uV, pd->max_uV))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_info(lp->dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "regulator voltage set failed\n");
+ ? ? ? ? ? ? if (regulator_enable(lp->regulator))
+ ? ? ? ? ? ? ? ? ? ? dev_info(lp->dev, "failed to enable regulator\n");
+ ? ? } else {
+ ? ? ? ? ? ? regulator_disable(lp->regulator);
+ ? ? }

I think you have to make sure that the regulator enable and disable calls are
balanced.

Ok. I was check on this and do required changes.

quoted

+
+ no_regulator:
+ ? ? gpio_direction_output(lp->pdata->gpio, lcd_reset);
+ ? ? lp->power = power;
+ ? ? return 0;
+}
+

[...]

quoted

+
+#ifdef CONFIG_OF

I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself out.

This #ifdef is to keep the OF code out in case of non-OF only compilation.

[...]

quoted

+ ? ? err = gpio_request(pdata->gpio, "LCD-nRESET");
+ ? ? if (err) {
+ ? ? ? ? ? ? dev_err(dev, "gpio [%d] request failed\n", pdata->gpio);
+ ? ? ? ? ? ? return err;
+ ? ? }
+
+ ? ? lp = devm_kzalloc(dev, sizeof(struct lcd_pwrctrl), GFP_KERNEL);
+ ? ? if (!lp) {
+ ? ? ? ? ? ? dev_err(dev, "memory allocation failed for private data\n");
+ ? ? ? ? ? ? return -ENOMEM;

You are leaking the gpio here.

Ok. I will fix this.

quoted

+ ? ? }
+
+ ? ? /*
+ ? ? ?* If power to lcd and/or lcd interface is controlled using a regulator,
+ ? ? ?* get the handle to the regulator for later use during power switching.
+ ? ? ?*/
+ ? ? lp->regulator = regulator_get(dev, "vcc-lcd");
+ ? ? if (IS_ERR(lp->regulator))
+ ? ? ? ? ? ? dev_info(dev, "could not find regulator\n");
+
+ ? ? lp->dev = dev;
+ ? ? lp->pdata = pdata;
+ ? ? lp->lcd = lcd_device_register(dev_name(dev), dev, lp, &lcd_pwrctrl_ops);
+ ? ? if (IS_ERR(lp->lcd)) {
+ ? ? ? ? ? ? dev_err(dev, "cannot register lcd device\n");
+ ? ? ? ? ? ? regulator_put(lp->regulator);

And here.

Ok. I will fix this.

quoted

+ ? ? ? ? ? ? return PTR_ERR(lp->lcd);
+ ? ? }
+
+ ? ? platform_set_drvdata(pdev, lp);
+ ? ? lcd_pwrctrl_set_power(lp->lcd, FB_BLANK_NORMAL);
+ ? ? return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id lcd_pwrctrl_match[] = {
+ ? ? { .compatible = "lcd,powerctrl", },
+ ? ? {},
+};

MODULE_DEVICE_TABLE(...)

Right. I missed that.

quoted

+#endif

quoted

+static struct platform_driver lcd_pwrctrl_driver = {
+ ? ? .driver ? ? ? ? = {
+ ? ? ? ? ? ? .name ? = "lcd-pwrctrl",
+ ? ? ? ? ? ? .owner ?= THIS_MODULE,
+ ? ? ? ? ? ? .of_match_table = of_match_ptr(lcd_pwrctrl_match),
+ ? ? },
+ ? ? .probe ? ? ? ? ?= lcd_pwrctrl_probe,
+ ? ? .remove ? ? ? ? = lcd_pwrctrl_remove,
+ ? ? .suspend ? ? ? ?= lcd_pwrctrl_suspend,
+ ? ? .resume ? ? ? ? = lcd_pwrctrl_resume,

please use dev_pm_ops instead of the legacy callbacks

Ok. I will use dev_pm_ops here.

[...]

Thanks for your review Lars.

Regards,
Thomas.

Dear Mr. Han,

On 6 January 2012 07:46, Jingoo Han [off-list ref] wrote:

Hi, Thomas.

[...]

quoted

?obj-$(CONFIG_LCD_PLATFORM) ? ? ?+= platform_lcd.o
+obj-$(CONFIG_LCD_PWRCTRL) ? ? ? += lcd_pwrctrl.o

Can you remove unnecessary space?
Please use <tab><space><space><space> instead of <space><tab><space><space><space>
between PWRCTRL) and += lcd_.

Ok. I will fix that.

quoted

[...]
+static struct platform_driver lcd_pwrctrl_driver = {
+ ? ? .driver ? ? ? ? = {
+ ? ? ? ? ? ? .name ? = "lcd-pwrctrl",
+ ? ? ? ? ? ? .owner ?= THIS_MODULE,
+ ? ? ? ? ? ? .of_match_table = of_match_ptr(lcd_pwrctrl_match),
+ ? ? },
+ ? ? .probe ? ? ? ? ?= lcd_pwrctrl_probe,
+ ? ? .remove ? ? ? ? = lcd_pwrctrl_remove,
+ ? ? .suspend ? ? ? ?= lcd_pwrctrl_suspend,
+ ? ? .resume ? ? ? ? = lcd_pwrctrl_resume,

Please use 'struct dev_pm_ops'.

Ok.

quoted

+};
+
+static int __init lcd_pwrctrl_init(void)
+{
+ ? ? return platform_driver_register(&lcd_pwrctrl_driver);
+}
+
+static void __exit lcd_pwrctrl_cleanup(void)
+{
+ ? ? platform_driver_unregister(&lcd_pwrctrl_driver);
+}
+
+module_init(lcd_pwrctrl_init);
+module_exit(lcd_pwrctrl_cleanup);

Use module_platform_driver(lcd_pwrctrl_driver).
It can make the code simpler.

Ok. Thanks for your review Mr. Han. I will do the required changes.

Regards,
Thomas.

Hi Olof,

On 6 January 2012 12:16, Olof Johansson [off-list ref] wrote:

Hi,

This looks much better than the previous approach. Some comments on
the binding below.

On Thu, Jan 5, 2012 at 7:42 AM, Thomas Abraham
[off-list ref] wrote:

quoted

diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
new file mode 100644
index 0000000..941e2ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt

@@ -0,0 +1,39 @@
+* Power controller for simple lcd panels
+
+Some LCD panels provide a simple control interface for the host system. The
+control mechanism would include a nRESET line connected to a gpio of the host
+system and a Vcc supply line which the host can optionally be controlled using
+a voltage regulator. Such simple panels do not support serial command
+interface (such as i2c or spi) or memory-mapped-io interface.
+
+Required properties:
+- compatible: should be 'lcd,powerctrl'

The convention for names is "vendor,product", so it would be better to
name this something like "lcd-powercontrol"

Ok. I will change this.

quoted

+- gpios: The GPIO number of the host system used to control the nRESET line.
+ ?The format of the gpio specifier depends on the gpio controller of the
+ ?host system.
+
+Optional properties:
+- lcd,pwrctrl-nreset-gpio-invert: When the nRESET line is asserted low, the
+ ?lcd panel is reset and stays in reset mode as long as the nRESET line is
+ ?asserted low. This is the default behaviour of most lcd panels. If a lcd
+ ?panel requires the nRESET line to be asserted high for panel reset, then
+ ?this property is used.
+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the minimum voltage the regulator should supply.
+ ?The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the maximum voltage the regulator should limit to
+ ?on the Vcc line. The value of this property should in in micro-volts.
+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ ?the lcd panel.

The above names are somewhat inconsistent. Why abbreviate powercontrol
in different ways between compatible and properties, for example.

Also, since there's no vendor to prefix with, it might just be easier
to avoid the prefix alltogether, or use a <word>-<property> prefix
instead, such as:

lcd-reset-gpios
lcd-reset-active-low ? (some platforms can specify polarity in the
gpio specifier, so I'm not sure if this is needed?
lcd-power-min-uV
lcd-power-max-uV
lcd-power-supply

Ok. This seems better. I will redo the code.

quoted

+
+Example:
+
+ ? ? ? lcd_pwrctrl {
+ ? ? ? ? ? ? ? compatible = "lcd,powerctrl";
+ ? ? ? ? ? ? ? gpios = <&gpe0 4 1 0 0>;
+ ? ? ? ? ? ? ? lcd,pwrctrl-nreset-gpio-invert;
+ ? ? ? ? ? ? ? lcd,pwrctrl-min-uV = <2500000>;
+ ? ? ? ? ? ? ? lcd,pwrctrl-max-uV = <3300000>;
+ ? ? ? ? ? ? ? lcd-vcc-supply - <&regulator7>;
+ ? ? ? };

[...]

quoted

+
+#ifdef CONFIG_OF
+static void __devinit lcd_pwrctrl_parse_dt(struct device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct lcd_pwrctrl_data *pdata)
+{
+ ? ? ? struct device_node *np = dev->of_node;
+
+ ? ? ? pdata->gpio = of_get_gpio(np, 0);
+ ? ? ? if (of_get_property(np, "lcd,pwrctrl-nreset-gpio-invert", NULL))
+ ? ? ? ? ? ? ? pdata->invert = true;
+ ? ? ? of_property_read_u32(np, "lcd,pwrctrl-min-uV", &pdata->min_uV);
+ ? ? ? of_property_read_u32(np, "lcd,pwrctrl-max-uV", &pdata->max_uV);
+}
+#endif
+
+static int __devinit lcd_pwrctrl_probe(struct platform_device *pdev)
+{
+ ? ? ? struct lcd_pwrctrl *lp;
+ ? ? ? struct lcd_pwrctrl_data *pdata = pdev->dev.platform_data;
+ ? ? ? struct device *dev = &pdev->dev;
+ ? ? ? int err;
+
+#ifdef CONFIG_OF
+ ? ? ? if (dev->of_node) {
+ ? ? ? ? ? ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ ? ? ? ? ? ? ? if (!pdata) {
+ ? ? ? ? ? ? ? ? ? ? ? dev_err(dev, "memory allocation for pdata failed\n");
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? lcd_pwrctrl_parse_dt(dev, pdata);
+ ? ? ? }
+#endif

The usual way of handling this is by checking if pdata is NULL, and if
so, call lcd_pwrctrl_fill_pdata() that returns an allocated pdata
structure (and check pdata for NULL again). That can also be done by
doing a stub that returns NULL and not use ifdef in the C code.

Ok. In case of kernel image that has support for both dt and non-dt
platforms, the check of pdata == NULL would not be a sufficient
condition to start parsing dt. So the dev->of_node is checked. The
#ifdef is used here to keep this portion of code out if kernel is
compiled only for non-dt platforms.

[...]

quoted

diff --git a/include/video/lcd_pwrctrl.h b/include/video/lcd_pwrctrl.h
new file mode 100644
index 0000000..75b6ce2
--- /dev/null
+++ b/include/video/lcd_pwrctrl.h

@@ -0,0 +1,30 @@
+/*
+ * Simple lcd panel power control driver.
+ *
+ * Copyright (c) 2011-2012 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2011-2012 Linaro Ltd.
+ *
+ * This driver is derived from platform-lcd.h which was written by
+ * Ben Dooks <ben@simtec.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+*/
+
+/**
+ * struct lcd_pwrctrl_data - platform data for lcd_pwrctrl driver.
+ * @gpio: GPIO number of the host system that connects to nRESET line.
+ * @invert: True, if output of gpio connected to nRESET should be inverted.
+ * @min_uV: Minimum required voltage output from the regulator. The voltage
+ * ? should be specified in micro-volts.
+ * @max_uV: Maximum acceptable voltage output from the regulator. The voltage
+ * ? should be specified in micro-volts.
+ */
+struct lcd_pwrctrl_data {
+ ? ? ? int ? ? ? ? ? ? gpio;
+ ? ? ? bool ? ? ? ? ? ?invert;
+ ? ? ? int ? ? ? ? ? ? min_uV;
+ ? ? ? int ? ? ? ? ? ? max_uV;
+};

If this is a pure open firmware driver, then there is no need to
export this, you can just keep it internal to the C file.

This driver does support non-dt platforms as well and platform code
can supply the platform data using this structure.

Thanks Olof for your review.

Regards,
Thomas.

Hi Mark,

On 6 January 2012 12:19, Mark Brown [off-list ref] wrote:

On Thu, Jan 05, 2012 at 08:07:53PM +0100, Lars-Peter Clausen wrote:

quoted

quoted

+- lcd,pwrctrl-min-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the minimum voltage the regulator should supply.
+ ?The value of this property should in in micro-volts.
+- lcd,pwrctrl-max-uV: If a regulator controls the Vcc voltage of the lcd panel,
+ ?this property specifies the maximum voltage the regulator should limit to
+ ?on the Vcc line. The value of this property should in in micro-volts.

quoted

The min and max voltage should rather be specified through the regulator
constraints.

In principle it should be specified in both places to account for shared
supplies though for all practical purposes for an LCD panel I can't see
multiple users sharing the same regulator and varying the voltage at
runtime.

It is assumed here that the boot loader as not set the output voltage
of the regulator that supports variable voltage ranges. So these
values help in setting up the regulator for LCD display.

quoted

quoted

+ ? if (lcd_enable) {
+ ? ? ? ? ? if ((pd->min_uV || pd->max_uV) &&
+ ? ? ? ? ? ? ? ? ? regulator_set_voltage(lp->regulator,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pd->min_uV, pd->max_uV))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? dev_info(lp->dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "regulator voltage set failed\n");
+ ? ? ? ? ? if (regulator_enable(lp->regulator))
+ ? ? ? ? ? ? ? ? ? dev_info(lp->dev, "failed to enable regulator\n");
+ ? } else {
+ ? ? ? ? ? regulator_disable(lp->regulator);
+ ? }

quoted

I think you have to make sure that the regulator enable and disable calls are
balanced.

Yes.

Ok. I will fix this.

quoted

quoted

+#ifdef CONFIG_OF

quoted

I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself out.

It's reasonably idiomatic to do this if the parsing code is in a
separate function.

Thanks for your review Mark.

Regards,
Thomas.

On Sat, Jan 07, 2012 at 04:34:34PM +0530, Thomas Abraham wrote:

On 6 January 2012 12:19, Mark Brown [off-list ref] wrote:

quoted

In principle it should be specified in both places to account for shared
supplies though for all practical purposes for an LCD panel I can't see
multiple users sharing the same regulator and varying the voltage at
runtime.

It is assumed here that the boot loader as not set the output voltage
of the regulator that supports variable voltage ranges. So these
values help in setting up the regulator for LCD display.

Remember that the regulator API won't allow consumers to configure
voltages unless there are constraints granting permission.  Users will
need to set both the LCD vales and the regulator constraints values to
actually allow configuration to happen.

On Thu, Jan 05, 2012 at 09:12:26PM +0530, Thomas Abraham wrote:

Add a lcd panel driver for simple raster-type lcd's which uses a gpio
controlled panel reset. The driver controls the nRESET line of the panel
using a gpio connected from the host system. The Vcc supply to the panel
is (optionally) controlled using a voltage regulator. This driver excludes
support for lcd panels that use a serial command interface or direct
memory mapped IO interface.

I'm trying to work out what kind of LCD panel this is for.  I assume
not the panels which would be connected to a SoC, which have a parallel
interface to a frame buffer device (LCD controller)?

If this is for these kinds of LCD panels, how are you handling the
timing required for active panels - some of which must not be powered
up without the LCD controller first being setup and enabled, and must
be powered down before the LCD controller is disabled.

I've seen this requirement with panels connected to ARM Ltd's development
boards, and also some SoCs.

On 01/07/2012 07:23 PM, Russell King - ARM Linux wrote:

On Thu, Jan 05, 2012 at 09:12:26PM +0530, Thomas Abraham wrote:

quoted

Add a lcd panel driver for simple raster-type lcd's which uses a gpio
controlled panel reset. The driver controls the nRESET line of the panel
using a gpio connected from the host system. The Vcc supply to the panel
is (optionally) controlled using a voltage regulator. This driver excludes
support for lcd panels that use a serial command interface or direct
memory mapped IO interface.

I'm trying to work out what kind of LCD panel this is for.  I assume
not the panels which would be connected to a SoC, which have a parallel
interface to a frame buffer device (LCD controller)?

If this is for these kinds of LCD panels, how are you handling the
timing required for active panels - some of which must not be powered
up without the LCD controller first being setup and enabled, and must
be powered down before the LCD controller is disabled.

I've seen this requirement with panels connected to ARM Ltd's development
boards, and also some SoCs.

This is handled by the LCD framework. Or at least should be, see this patchset
http://www.spinics.net/lists/linux-fbdev/msg04503.html

- Lars

Hi Mark,

On 7 January 2012 22:58, Mark Brown [off-list ref] wrote:

On Sat, Jan 07, 2012 at 04:34:34PM +0530, Thomas Abraham wrote:

quoted

On 6 January 2012 12:19, Mark Brown [off-list ref] wrote:

quoted

+       if (lcd_enable) {
+               if ((pd->min_uV || pd->max_uV) &&
+                       regulator_set_voltage(lp->regulator,
+                                               pd->min_uV, pd->max_uV))
+                               dev_info(lp->dev,
+                                       "regulator voltage set failed\n");
+               if (regulator_enable(lp->regulator))
+                       dev_info(lp->dev, "failed to enable regulator\n");
+       } else {
+               regulator_disable(lp->regulator);
+       }

quoted

quoted

In principle it should be specified in both places to account for shared
supplies though for all practical purposes for an LCD panel I can't see
multiple users sharing the same regulator and varying the voltage at
runtime.

quoted

It is assumed here that the boot loader as not set the output voltage
of the regulator that supports variable voltage ranges. So these
values help in setting up the regulator for LCD display.

Remember that the regulator API won't allow consumers to configure
voltages unless there are constraints granting permission. ?Users will
need to set both the LCD vales and the regulator constraints values to
actually allow configuration to happen.

In the case of Exynos4 based Origen board, buck7 of max8997 supplies
power to the lcd panel. buck7 is capable of supplying power at
different voltage levels but for Origen board, it is required to
supply only 3.3V. The constraints for buck7 can specify 'apply_uV' and
this will ensure that buck7 is configured to supply 3.3V.

But 'apply_uV' seems to be not ideal for regulators that supply power
to lcd panels. It would be better to power lcd panels only when video
data has to be displayed.

If 'apply_uV' is not used, then regulator_enable() does not set the
correct output voltage. So regulator_set_voltage() call is required.
If the regulator does not support multiple voltage ranges, then in the
above code min_uV and max_uV should be zero which then bypasses
regulator_set_voltage().

Hence, the call to regulator_set_voltage() is required in the above
code fragment.

Thanks,
Thomas.

Hi Russell,

On 7 January 2012 23:53, Russell King - ARM Linux
[off-list ref] wrote:

On Thu, Jan 05, 2012 at 09:12:26PM +0530, Thomas Abraham wrote:

quoted

Add a lcd panel driver for simple raster-type lcd's which uses a gpio
controlled panel reset. The driver controls the nRESET line of the panel
using a gpio connected from the host system. The Vcc supply to the panel
is (optionally) controlled using a voltage regulator. This driver excludes
support for lcd panels that use a serial command interface or direct
memory mapped IO interface.

I'm trying to work out what kind of LCD panel this is for. ?I assume
not the panels which would be connected to a SoC, which have a parallel
interface to a frame buffer device (LCD controller)?

If this is for these kinds of LCD panels, how are you handling the
timing required for active panels - some of which must not be powered
up without the LCD controller first being setup and enabled, and must
be powered down before the LCD controller is disabled.

I've seen this requirement with panels connected to ARM Ltd's development
boards, and also some SoCs.

This patch is for passive panels that use a nRESET line controlled by
a SoC GPIO and the panel do not use serial command interface (i2c or
spi) or memory-mapped-io for panel control. Panels that just need a
nRESET to be controlled can be handled by this driver.

Thanks,
Thomas.

On Wed, Jan 11, 2012 at 04:21:57PM +0530, Thomas Abraham wrote:

In the case of Exynos4 based Origen board, buck7 of max8997 supplies
power to the lcd panel. buck7 is capable of supplying power at
different voltage levels but for Origen board, it is required to
supply only 3.3V. The constraints for buck7 can specify 'apply_uV' and
this will ensure that buck7 is configured to supply 3.3V.

But 'apply_uV' seems to be not ideal for regulators that supply power
to lcd panels. It would be better to power lcd panels only when video
data has to be displayed.

What makes you say this?  Enabling and disabling a regulator are
entirely orthogonal to setting the regulator voltage.

On 11 January 2012 23:15, Mark Brown
[off-list ref] wrote:

On Wed, Jan 11, 2012 at 04:21:57PM +0530, Thomas Abraham wrote:

quoted

In the case of Exynos4 based Origen board, buck7 of max8997 supplies
power to the lcd panel. buck7 is capable of supplying power at
different voltage levels but for Origen board, it is required to
supply only 3.3V. The constraints for buck7 can specify 'apply_uV' and
this will ensure that buck7 is configured to supply 3.3V.

quoted

But 'apply_uV' seems to be not ideal for regulators that supply power
to lcd panels. It would be better to power lcd panels only when video
data has to be displayed.

What makes you say this? ?Enabling and disabling a regulator are
entirely orthogonal to setting the regulator voltage.

Sorry, I got this wrong. I will remove the regulator_set_voltage()
call from the driver.

Thanks,
Thomas.