Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Thierry Reding <thierry.reding@avionic-design.de>]

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

On Wed, Mar 06, 2013 at 09:17:18AM -0800, Andrew Chew wrote:
[...]
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 069983c..ff98cdd 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -20,10 +20,13 @@
>  #include <linux/pwm.h>
>  #include <linux/pwm_backlight.h>
>  #include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
>  
>  struct pwm_bl_data {
>  	struct pwm_device	*pwm;
>  	struct device		*dev;
> +	struct regulator	*en_supply;

Can this be renamed to enable_supply to match the DT property name?

> +	bool			en_supply_enabled;

This boolean can be dropped. As discussed in a previous thread, the
pwm-backlight driver shouldn't need to know about any other uses of the
regulator.

> +static void pwm_backlight_enable(struct backlight_device *bl)
> +{
> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> +
> +	pwm_enable(pb->pwm);
> +
> +	if (pb->en_supply && !pb->en_supply_enabled) {
> +		if (regulator_enable(pb->en_supply) != 0)
> +			dev_warn(&bl->dev, "Failed to enable regulator");
> +		else
> +			pb->en_supply_enabled = true;
> +	}
> +}
> +
> +static void pwm_backlight_disable(struct backlight_device *bl)
> +{
> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> +
> +	if (pb->en_supply && pb->en_supply_enabled) {
> +		if (regulator_disable(pb->en_supply) != 0)
> +			dev_warn(&bl->dev, "Failed to disable regulator");
> +		else
> +			pb->en_supply_enabled = false;
> +	}
> +
> +	pwm_disable(pb->pwm);
> +}

Alex already brought this up: shouldn't the sequences be reversed. That
is, when enabling the backlight, turn on the regulator first, then
enable the PWM. When disabling, disable the PWM first, then turn off the
regulator?

> @@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	pb->exit = data->exit;
>  	pb->dev = &pdev->dev;
>  
> +	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
> +	if (IS_ERR(pb->en_supply)) {
> +		ret = PTR_ERR(pb->en_supply);
> +		pb->en_supply = NULL;
> +		goto err_alloc;
> +	}

This effectively makes the regulator mandatory, so the board files that
use pwm-backlight need to be updated or otherwise will break.

Thierry

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