Re: [PATCH] pwm-backlight: add regulator and GPIO support

Thierry Reding
2012-06-30T18:37:55Z

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]