[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

Commit 22ceeee1 (pwm-backlight: Add power supply support) caused breakage on 
boards that do not define the backlight regulator on their dts files.

Instead of making the backlight regulator mandatory, treat it as optional 
instead, so that we do not have breakage.

This avoids the need for adding a dummy regulator on every dts file that uses 
the pwm-backlight.

Tested on a mx6qsabresd.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 .../devicetree/bindings/video/backlight/pwm-backlight.txt |  2 +-
 drivers/video/backlight/pwm_bl.c                          | 15 +++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 764db86..ed3dc3c 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,13 +10,13 @@ Required properties:
       last value in the array represents a 100% duty cycle (brightest).
   - default-brightness-level: the default brightness level (index into the
       array defined by the "brightness-levels" property)
-  - power-supply: regulator for supply voltage
 
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
                "pwms" property (see PWM binding[0])
   - enable-gpios: contains a single GPIO specifier for the GPIO which enables
                   and disables the backlight (see GPIO binding[1])
+  - power-supply: regulator for supply voltage
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index fb80d68..fb3008c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -50,9 +50,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 	if (pb->enabled)
 		return;
 
-	err = regulator_enable(pb->power_supply);
-	if (err < 0)
-		dev_err(pb->dev, "failed to enable power supply\n");
+	if (!IS_ERR(pb->power_supply)) {
+		err = regulator_enable(pb->power_supply);
+		if (err < 0)
+			dev_err(pb->dev, "failed to enable power supply\n");
+	}
 
 	if (gpio_is_valid(pb->enable_gpio)) {
 		if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
@@ -80,7 +82,8 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 			gpio_set_value(pb->enable_gpio, 0);
 	}
 
-	regulator_disable(pb->power_supply);
+	if (!IS_ERR(pb->power_supply))
+		regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
@@ -283,10 +286,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
-	if (IS_ERR(pb->power_supply)) {
-		ret = PTR_ERR(pb->power_supply);
-		goto err_gpio;
-	}
 
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm)) {
-- 
1.8.1.2

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Marek Vasut]

Dear Fabio Estevam,

> Commit 22ceeee1 (pwm-backlight: Add power supply support) caused breakage
> on boards that do not define the backlight regulator on their dts files.
> 
> Instead of making the backlight regulator mandatory, treat it as optional
> instead, so that we do not have breakage.
> 
> This avoids the need for adding a dummy regulator on every dts file that
> uses the pwm-backlight.
> 
> Tested on a mx6qsabresd.

This patch certainly makes sense, there are plenty of simple boards that have no 
actual supply for the PWM.

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Thierry Reding]

On Thu, Nov 21, 2013 at 03:43:15PM -0200, Fabio Estevam wrote:
> Commit 22ceeee1 (pwm-backlight: Add power supply support) caused breakage on 
> boards that do not define the backlight regulator on their dts files.
> 
> Instead of making the backlight regulator mandatory, treat it as optional 
> instead, so that we do not have breakage.
> 
> This avoids the need for adding a dummy regulator on every dts file that uses 
> the pwm-backlight.

That was actually part of an earlier version of this patch. It was
rejected on the grounds that regulators really aren't optional. They are
there, but in most cases just not described in DT (most likely because
they aren't controllable and always on).

But even so, the regulator core now has functionality to return a dummy
regulator if none is specified in DT, specifically so that users don't
have to special case the handling of "optional" regulators. That should
be enough to keep all boards working even if no power-supply property
was given in the DTS. As a matter of fact it's a constellation that I
explicitly tested and found to work well.

Cc'ing Mark Brown, perhaps he can help figure out what's causing this.

Thierry

> 
> Tested on a mx6qsabresd.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  .../devicetree/bindings/video/backlight/pwm-backlight.txt |  2 +-
>  drivers/video/backlight/pwm_bl.c                          | 15 +++++++--------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> index 764db86..ed3dc3c 100644
> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> @@ -10,13 +10,13 @@ Required properties:
>        last value in the array represents a 100% duty cycle (brightest).
>    - default-brightness-level: the default brightness level (index into the
>        array defined by the "brightness-levels" property)
> -  - power-supply: regulator for supply voltage
>  
>  Optional properties:
>    - pwm-names: a list of names for the PWM devices specified in the
>                 "pwms" property (see PWM binding[0])
>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>                    and disables the backlight (see GPIO binding[1])
> +  - power-supply: regulator for supply voltage
>  
>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index fb80d68..fb3008c 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -50,9 +50,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>  	if (pb->enabled)
>  		return;
>  
> -	err = regulator_enable(pb->power_supply);
> -	if (err < 0)
> -		dev_err(pb->dev, "failed to enable power supply\n");
> +	if (!IS_ERR(pb->power_supply)) {
> +		err = regulator_enable(pb->power_supply);
> +		if (err < 0)
> +			dev_err(pb->dev, "failed to enable power supply\n");
> +	}
>  
>  	if (gpio_is_valid(pb->enable_gpio)) {
>  		if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
> @@ -80,7 +82,8 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  			gpio_set_value(pb->enable_gpio, 0);
>  	}
>  
> -	regulator_disable(pb->power_supply);
> +	if (!IS_ERR(pb->power_supply))
> +		regulator_disable(pb->power_supply);
>  	pb->enabled = false;
>  }
>  
> @@ -283,10 +286,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	}
>  
>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> -	if (IS_ERR(pb->power_supply)) {
> -		ret = PTR_ERR(pb->power_supply);
> -		goto err_gpio;
> -	}
>  
>  	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>  	if (IS_ERR(pb->pwm)) {
> -- 
> 1.8.1.2
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131122/b1a2ee3c/attachment.sig>

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Mark Brown]

On Fri, Nov 22, 2013 at 03:22:32PM +0100, Thierry Reding wrote:

> Cc'ing Mark Brown, perhaps he can help figure out what's causing this.

The obvious question is what kernel this is being tested on...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131122/12ee085f/attachment.sig>

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

On Fri, Nov 22, 2013 at 12:52 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Nov 22, 2013 at 03:22:32PM +0100, Thierry Reding wrote:
>
>> Cc'ing Mark Brown, perhaps he can help figure out what's causing this.
>
> The obvious question is what kernel this is being tested on...

Tested linux-next 20131120 originally and display was blank.

Now on 20131122 the display works fine.

Regards,

Fabio Estevam

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Shawn Guo]

On Fri, Nov 22, 2013 at 02:52:24PM +0000, Mark Brown wrote:
> On Fri, Nov 22, 2013 at 03:22:32PM +0100, Thierry Reding wrote:
> 
> > Cc'ing Mark Brown, perhaps he can help figure out what's causing this.
> 
> The obvious question is what kernel this is being tested on...

Hmm, I'm seeing this breakage on v3.13-rc1 now with the following line
in the kernel log.

  pwm-backlight backlight.18: dummy supplies not allowed

What's missing here?

Shawn

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

Hi Shawn,

On Mon, Nov 25, 2013 at 1:10 AM, Shawn Guo <shawn.guo@linaro.org> wrote:

> Hmm, I'm seeing this breakage on v3.13-rc1 now with the following line
> in the kernel log.
>
>   pwm-backlight backlight.18: dummy supplies not allowed
>
> What's missing here?

Yes, this is weird.

linux-next 20131120: backlight fails
linux-next 20131122: backlight works
linux-next 20131125: backlight fails

Looks like we need to apply this one to make sure backlight works:
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/281826

Regards,

Fabio Estevam

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Shawn Guo]

On Mon, Nov 25, 2013 at 11:26:53AM -0200, Fabio Estevam wrote:
> Looks like we need to apply this one to make sure backlight works:
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/281826

But per Thierry's comment below, this is the exactly the case that we
should avoid - patch individual board DTS file to add a dummy/fake
regulator to get it through.

| But even so, the regulator core now has functionality to return a dummy
| regulator if none is specified in DT, specifically so that users don't
| have to special case the handling of "optional" regulators. That should
| be enough to keep all boards working even if no power-supply property
| was given in the DTS. As a matter of fact it's a constellation that I
| explicitly tested and found to work well.

Shawn

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

Mark/Thierry,

On Mon, Nov 25, 2013 at 11:37 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Mon, Nov 25, 2013 at 11:26:53AM -0200, Fabio Estevam wrote:
>> Looks like we need to apply this one to make sure backlight works:
>> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/281826
>
> But per Thierry's comment below, this is the exactly the case that we
> should avoid - patch individual board DTS file to add a dummy/fake
> regulator to get it through.
>
> | But even so, the regulator core now has functionality to return a dummy
> | regulator if none is specified in DT, specifically so that users don't

What commit provides this regulator core functionality?

Looks like it has not reached 3.13-rc1.

Regards,

Fabio Estevam

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Mark Brown]

On Mon, Nov 25, 2013 at 12:43:58PM -0200, Fabio Estevam wrote:

> What commit provides this regulator core functionality?

regulator: core: Provide a dummy regulator with full constraints

> Looks like it has not reached 3.13-rc1.

Yes, it did.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/650b4eb8/attachment.sig>

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

On Mon, Nov 25, 2013 at 3:29 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Nov 25, 2013 at 12:43:58PM -0200, Fabio Estevam wrote:
>
>> What commit provides this regulator core functionality?
>
> regulator: core: Provide a dummy regulator with full constraints
>
>> Looks like it has not reached 3.13-rc1.
>
> Yes, it did.

Ok, I see.

Another way to have LCD working is on a 3.13-rc1 (without passing the
fake regulator node in dt):

--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -284,7 +284,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)

        pb->power_supply = devm_regulator_get(&pdev->dev, "power");
        if (IS_ERR(pb->power_supply)) {
-               ret = PTR_ERR(pb->power_supply);
+               ret = -EPROBE_DEFER;
                goto err_gpio;
        }

At least mx28evk and mx6qsabresd have no LCD output in 3.13-rc1.

Not sure what is the proper fix though.

Regards,

Fabio Estevam

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Fabio Estevam]

On Mon, Nov 25, 2013 at 6:37 PM, Fabio Estevam <festevam@gmail.com> wrote:

> At least mx28evk and mx6qsabresd have no LCD output in 3.13-rc1.
>
> Not sure what is the proper fix though.

What about this?

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3850,4 +3850,4 @@ unlock:

        return 0;
 }
-late_initcall(regulator_init_complete);
+core_initcall(regulator_init_complete);

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Mark Brown]

On Mon, Nov 25, 2013 at 06:58:30PM -0200, Fabio Estevam wrote:

> -late_initcall(regulator_init_complete);
> +core_initcall(regulator_init_complete);

No, like I said previously fiddling about with the initcall order is not
going to help.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131126/ed2c619b/attachment.sig>

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Mark Brown]

On Mon, Nov 25, 2013 at 11:26:53AM -0200, Fabio Estevam wrote:

> linux-next 20131120: backlight fails
> linux-next 20131122: backlight works
> linux-next 20131125: backlight fails

What are the differences between those kernels?  I'm not aware of any
regulator core changes at all there and diff isn't showing me any - are
you sure you used a consistent kernel configuration?  Have you tried to
look at what happens when we should provide the dummy regulator, and do
the logs say anything?

> Looks like we need to apply this one to make sure backlight works:
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/281826

Please always provide plain text versions of links and SHA1s...  that
link is to a patch adding a fixed regulator which you should do but
ought not to need to do.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/dd9cf2a7/attachment-0001.sig>

[PATCH] backlight: pwm_bl: Do not make the regulator mandatory

[Mark Brown]

On Mon, Nov 25, 2013 at 11:10:32AM +0800, Shawn Guo wrote:

> Hmm, I'm seeing this breakage on v3.13-rc1 now with the following line
> in the kernel log.

>   pwm-backlight backlight.18: dummy supplies not allowed

> What's missing here?

Are you racing with the late initcall to flag that DT is present?  Wei
Ni was looking at hooking this into DT so that when we pass the DT over
we flag that we have full constraints (or provide a notifier to let
subsystems do that) but that ground to a halt.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/729043dc/attachment.sig>