lkml.org 
[lkml]   [2012]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset
    > [...]
    >
    > 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'
    > +- 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.

    Maybe use OF_GPIO_ACTIVE_LOW here instead. That would make active high the
    default but be a bit more consistent.

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


    > +- vcc-lcd-supply: phandle of the regulator that controls the vcc supply to
    > + the lcd panel.
    > +
    [...]
    > 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.

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

    > +
    > + no_regulator:
    > + gpio_direction_output(lp->pdata->gpio, lcd_reset);
    > + lp->power = power;
    > + return 0;
    > +}
    > +
    [...]
    > +
    > +#ifdef CONFIG_OF

    I think you can remove all the CONFIG_OF ifdefs, the of API should stub itself out.

    > +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
    > +
    > + if (!pdata) {
    > + dev_err(dev, "platform data not available\n");
    > + return -EINVAL;
    > + }
    > +
    > + 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.

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

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

    > +#endif


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

    > +};
    > +
    > +static int __init lcd_pwrctrl_init(void)
    > +{
    > + return platform_driver_register(&lcd_pwrctrl_driver);
    > +}
    > +
    > +static void __exit lcd_pwrctrl_cleanup(void)
    > +{
    > + platform_driver_unregister(&lcd_pwrctrl_driver);
    > +}
    > +
    > +module_init(lcd_pwrctrl_init);
    > +module_exit(lcd_pwrctrl_cleanup);

    module_platform_driver(&lcd_pwrctrl_driver);

    > +
    > +MODULE_AUTHOR("Thomas Abraham <thomas.ab@samsung.com>");
    > +MODULE_LICENSE("GPL v2");
    > +MODULE_ALIAS("platform:lcd-pwrctrl");



    \
     
     \ /
      Last update: 2012-01-05 20:11    [W:0.040 / U:119.808 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site