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 <thomas.abraham@linaro.org>
---
 .../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.

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.
> +
[...]
> 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.

> +
> +	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 <thomas.ab@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:lcd-pwrctrl");

Hi, Thomas.
> -----Original Message-----
> From: Thomas Abraham [mailto:thomas.abraham@linaro.org]
> Sent: Friday, January 06, 2012 12:42 AM
> Subject: [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 <thomas.abraham@linaro.org>
> ---
>  .../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
> 
> [...]
> --- 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
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
<thomas.abraham@linaro.org> wrote:

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

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

[...]

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

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

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

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

> > +#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 <lars@metafoo.de> wrote:
>> [...]
>>
>> diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
[...]

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

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

>
>
>> +- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
>> +  the lcd panel.
>> +
> [...]
>> 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.

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

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

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

[...]

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

>
>> +     }
>> +
>> +     /*
>> +      * 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.

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

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

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 <jg1.han@samsung.com> wrote:
> Hi, Thomas.
[...]

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

>> [...]
>> +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.

>> +};
>> +
>> +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 <olof@lixom.net> 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
> <thomas.abraham@linaro.org> wrote:
>
>> 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.

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

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

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.

>
> [...]
>
>> 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 <broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jan 05, 2012 at 08:07:53PM +0100, Lars-Peter Clausen wrote:
>
>> > +- 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.

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.

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

Ok. I will fix this.

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

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 <broonie@opensource.wolfsonmicro.com> wrote:

> > 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:
>> 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 <broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Jan 07, 2012 at 04:34:34PM +0530, Thomas Abraham wrote:
>> On 6 January 2012 12:19, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>>> +       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);
>>>+       }
>
>> > 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.

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
<linux@arm.linux.org.uk> wrote:
> 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.

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
<broonie@opensource.wolfsonmicro.com> wrote:
> 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.

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

Thanks,
Thomas.