lkml.org 
[lkml]   [2012]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] backlight: lcd: add driver for raster-type lcd's with gpio controlled panel reset
    From
    Hi Lars,

    On 6 January 2012 00:37, Lars-Peter Clausen <lars@metafoo.de> wrote:
    >> [...]
    >>
    >> diff --git a/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt b/Documentation/devicetree/bindings/lcd/lcd-pwrctrl.txt
    [...]

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

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

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

    Ok. I was not happy either while writing it. I will re-work this.

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

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

    This #ifdef is to keep the OF code out in case of non-OF only compilation.

    [...]

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

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

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

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

    Ok. I will use dev_pm_ops here.

    [...]

    Thanks for your review Lars.

    Regards,
    Thomas.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-01-07 11:49    [W:0.037 / U:34.908 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site