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