Re: [PATCH v3 1/2] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset
Cc: rpur...@rpsys.net, florianschandi...@gmx.de,
devicetree-disc...@lists.ozlabs.org, linux-fb...@vger.kernel.org,
linux-samsung-soc@vger.kernel.org, grant.lik...@secretlab.ca,
rob.herr...@calxeda.com, kgene@samsung.com, jg1@samsung.com,
broo...@opensource.wolfsonmicro.com, kyungmin.p...@samsung.com,
augulis.dar...@gmail.com, ben-li...@fluff.org, l...@metafoo.de,
patc...@linaro.org
Poor me. When someone sends a patch like this, I need to go and hunt
down everyone's real names to nicely add them to the changelog's Cc:
list. I prefer that you do this ;)
You can add Cc:'s to the changelog yourself, of course. Often that
works out better than having me try to work out who might be
interested in the patch.
On Mon, 26 Mar 2012 14:16:39 +0530
Thomas Abraham thomas.abra...@linaro.org 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.
...
+struct lcd_pwrctrl {
+ struct device *dev;
+ struct lcd_device *lcd;
+ struct lcd_pwrctrl_data *pdata;
+ struct regulator*regulator;
+ unsigned intpower;
+ boolsuspended;
+ boolpwr_en;
Generally kernel code will avoid these unpronounceable abbreviations.
So we do
pwr - power
en - enable
ctrl - control
etc. It results in longer identifiers, but the code is more readable
and, more importantly, more *rememberable*.
+};
+
+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)
See, shouldn't that have been lcd_pwrctrl_set_pwr?
If we avoid the abbreviations, such issues do not arise.
+{
+ struct lcd_pwrctrl *lp = lcd_get_data(lcd);
+ struct lcd_pwrctrl_data *pd = lp-pdata;
+ bool lcd_enable;
+ int lcd_reset, ret = 0;
+
+ lcd_enable = (power == FB_BLANK_POWERDOWN || lp-suspended) ? 0 : 1;
This isn't how to use `bool'. We can use
lcd_enable = (power == FB_BLANK_POWERDOWN) || lp-suspended;
+ lcd_reset = (pd-invert) ? !lcd_enable : lcd_enable;
+
+ if (lp-pwr_en == lcd_enable)
+ return 0;
+
+ if (!IS_ERR_OR_NULL(lp-regulator)) {
+ if (lcd_enable) {
+ if (regulator_enable(lp-regulator)) {
+ dev_info(lp-dev, regulator enable failed\n);
+ ret = -EPERM;
+ }
+ } else {
+ if (regulator_disable(lp-regulator)) {
+ dev_info(lp-dev, regulator disable failed\n);
+ ret = -EPERM;
+ }
+ }
+ }
+
+ gpio_direction_output(lp-pdata-gpio, lcd_reset);
+ lp-power = power;
+ lp-pwr_en = lcd_enable;
Again, this could have been any of
lp-[power|pwr] = [power|pwr];
lp-[power|pwr]_[en|enable] = lcd_[en|enable];
zillions of combinations! If we just avoid the abbreviations, there is
only one combination.
+ return ret;
+}
+
...
+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;
+ }
+
+ lp = devm_kzalloc(dev, sizeof(struct lcd_pwrctrl), GFP_KERNEL);
Nit: I prefer sizeof(*lp) here, so I don't have to scroll back and
check the type of lp.
+ if (!lp) {
+ dev_err(dev, memory allocation failed for private data\n);
+ return -ENOMEM;
+ }
+
+ err = gpio_request(pdata-gpio, LCD-nRESET);
+ if (err) {
+ dev_err(dev, gpio [%d] request failed\n, pdata-gpio);
+ return err;
+ }
+
...
The code looks OK to me, but I do think the naming decisions should be
revisited, please.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] 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 l...@metafoo.de
Signed-off-by: Thomas Abraham thomas.abra...@linaro.org
---
.../devicetree/bindings/lcd/lcd-pwrctrl.txt| 36 +++
drivers/video/backlight/Kconfig|7 +
drivers/video/backlight/Makefile |1 +
drivers/video/backlight/lcd_pwrctrl.c | 226
include/video/lcd_pwrctrl.h| 24 ++
5 files changed, 294 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 000..22604a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
@@ -0,0 +1,36 @@
+* 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-powercontrol'
+
+- lcd-reset-gpio: 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-reset-active-high: 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.
+ Note: Some platforms might allow inverting the polarity of the gpio output
+ in the 'lcd-reset-gpio' gpio specifier. On such platforms, if the polarity
+ is used to control the output of the gpio, then this property should not be
+ used.
+
+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ the lcd panel.
+
+Example:
+
+ lcd_pwrctrl {
+ compatible = lcd-powercontrol;
+ lcd-reset-gpio = gpe0 4 1 0 0;
+ lcd-reset-active-high;
+ lcd-vcc-supply = regulator7;
+ };
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 681b369..9b52ea8 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 af5cf65..d85c8d2 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 000..917d842
--- /dev/null
+++ b/drivers/video/backlight/lcd_pwrctrl.c
@@ -0,0 +1,226 @@
+/*
+ * 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
