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 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...@vg
[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
Signed-off-by: Thomas Abraham
---
.../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 = <®ulator7>;
+ };
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 to a gpio of the
+ * host syste
