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

[Thomas Abraham]

Hi Olof,

On 6 January 2012 12:16, Olof Johansson <olof at 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 at 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 at 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.