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

[Olof Johansson]

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"

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