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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120630183742.GE23990@avionic-0098.adnet.avionic-design.de>
Date:	Sat, 30 Jun 2012 20:37:42 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Alexandre Courbot <acourbot@...dia.com>
Cc:	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fbdev@...r.kernel.org
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@...dia.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

Content of type "application/pgp-signature" skipped