| Message ID | 1385055795-24002-1-git-send-email-fabio.estevam@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > >
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...
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
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
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
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
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
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.
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.
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.
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)) {
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(-)