lkml.org 
[lkml]   [2012]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] pwm-backlight: add regulator and GPIO support
    On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote:
    > Add support for an optional power regulator and enable/disable GPIO.
    > This scheme is commonly used in embedded systems.
    >
    > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

    I've added some comments in addition to those by Stephen.

    [...]
    > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
    > index 057389d..821e03e 100644
    > --- a/drivers/video/backlight/pwm_bl.c
    > +++ b/drivers/video/backlight/pwm_bl.c
    [...]
    > @@ -141,11 +178,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
    > data->max_brightness--;
    > }
    >
    > - /*
    > - * TODO: Most users of this driver use a number of GPIOs to control
    > - * backlight power. Support for specifying these needs to be
    > - * added.
    > - */
    > + ret = of_get_named_gpio(node, "enable-gpios", 0);
    > + if (ret >= 0) {
    > + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);

    Can't you just reuse the value of ret here?

    [...]
    > @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
    > if (data->pwm_period_ns > 0)
    > pwm_set_period(pb->pwm, data->pwm_period_ns);
    >
    > +
    > + pb->power_reg = devm_regulator_get(&pdev->dev, "power");
    > + if (IS_ERR(pb->power_reg))
    > + return PTR_ERR(pb->power_reg);
    > +
    > + pb->enable_gpio = -EINVAL;

    Perhaps initialize this to -1? Assigning standard error codes to a GPIO
    doesn't make much sense.

    [...]
    > diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
    > index 56f4a86..5ae2cd0 100644
    > --- a/include/linux/pwm_backlight.h
    > +++ b/include/linux/pwm_backlight.h
    > @@ -18,6 +18,11 @@ struct platform_pwm_backlight_data {
    > void (*notify_after)(struct device *dev, int brightness);
    > void (*exit)(struct device *dev);
    > int (*check_fb)(struct device *dev, struct fb_info *info);
    > + /* optional GPIO that enables/disables the backlight */
    > + int enable_gpio;
    > + /* 0 (default initialization value) is a valid GPIO number. Make use of
    > + * control gpio explicit to avoid bad surprises. */
    > + bool use_enable_gpio;

    It's a shame we have to add workarounds like this...

    Also the canonical form to write multi-line comments would be:

    /*
    * 0 (default ...
    * ... surprises.
    */

    Thierry
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-06-30 21:21    [W:0.027 / U:97.308 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site