Messages in this thread | | | Date | Sat, 30 Jun 2012 20:37:42 +0200 | From | Thierry Reding <> | Subject | Re: [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] | |