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 <[email protected]>
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
pgp2XRYrCq1gG.pgp
Description: PGP signature
