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

Hi Lars,

On 6 January 2012 00:37, Lars-Peter Clausen [off-list ref] wrote:

quoted

[...]

diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt

[...]

quoted

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

quoted

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

quoted

+- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
+ ?the lcd panel.
+

[...]

quoted

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.

quoted

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

quoted

+
+ no_regulator:
+ ? ? gpio_direction_output(lp->pdata->gpio, lcd_reset);
+ ? ? lp->power = power;
+ ? ? return 0;
+}
+

[...]

quoted

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

[...]

quoted

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

quoted

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

quoted

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

quoted

+#endif

quoted

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