Hi Olof,
On 6 January 2012 12:16, Olof Johansson <olof@xxxxxxxxx> 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@xxxxxxxxxx> 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 - <®ulator7>;
>> + };
>
>
> [...]
>> +
>> +#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@xxxxxxxxxxxx>
>> + *
>> + * 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
