| Message ID | 1379972467-11243-10-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/23/2013 03:41 PM, Thierry Reding wrote: > Many backlights require a power supply to work properly. This commit > uses a power-supply regulator, if available, to power up and power down > the panel. I think that all backlights require a power supply, albeit the supply may not be SW-controllable. Hence, shouldn't the regulator be mandatory in the binding, yet the driver be defensively coded such that if one isn't specified, the driver continues to work? > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device *pdev) > } > } > > + pb->power_supply = devm_regulator_get_optional(&pdev->dev, "power"); ... so I think that should be devm_regulator_get(), since the regulator isn't really optional. > + if (IS_ERR(pb->power_supply)) { > + if (PTR_ERR(pb->power_supply) != -ENODEV) { > + ret = PTR_ERR(pb->power_supply); > + goto err_gpio; > + } > + > + pb->power_supply = NULL; If devm_regulator_get_optional() returns an error value or a valid value, then I don't think that this driver should transmute error values into NULL; NULL might be a perfectly valid regulator value. Related, I think the if (pb->power_supply) tests should be replaced with if (!IS_ERR(pb->power_supply)) instead.
On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote: > On 09/23/2013 03:41 PM, Thierry Reding wrote: > > Many backlights require a power supply to work properly. This commit > > uses a power-supply regulator, if available, to power up and power down > > the panel. > > I think that all backlights require a power supply, albeit the supply > may not be SW-controllable. Hence, shouldn't the regulator be mandatory > in the binding, yet the driver be defensively coded such that if one > isn't specified, the driver continues to work? That has already changed in my local version of this patch. > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > } > > } > > > > + pb->power_supply = devm_regulator_get_optional(&pdev->dev, "power"); > > ... so I think that should be devm_regulator_get(), since the regulator > isn't really optional. > > > + if (IS_ERR(pb->power_supply)) { > > + if (PTR_ERR(pb->power_supply) != -ENODEV) { > > + ret = PTR_ERR(pb->power_supply); > > + goto err_gpio; > > + } > > + > > + pb->power_supply = NULL; > > If devm_regulator_get_optional() returns an error value or a valid > value, then I don't think that this driver should transmute error values > into NULL; NULL might be a perfectly valid regulator value. Related, I > think the if (pb->power_supply) tests should be replaced with if > (!IS_ERR(pb->power_supply)) instead. All of that is already done in my local tree. This actually turns out to work rather smoothly with the new support for optional regulators. The regulator core will give you a dummy regulator (assuming it's there physically but hasn't been wired up in software) that's always on, so the driver doesn't even have to special case it anymore. Thierry
On 10/01/2013 02:53 PM, Thierry Reding wrote: > On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote: >> On 09/23/2013 03:41 PM, Thierry Reding wrote: >>> Many backlights require a power supply to work properly. This >>> commit uses a power-supply regulator, if available, to power up >>> and power down the panel. >> >> I think that all backlights require a power supply, albeit the >> supply may not be SW-controllable. Hence, shouldn't the regulator >> be mandatory in the binding, yet the driver be defensively coded >> such that if one isn't specified, the driver continues to work? > > That has already changed in my local version of this patch. > >>> diff --git a/drivers/video/backlight/pwm_bl.c >>> b/drivers/video/backlight/pwm_bl.c >> >>> @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct >>> platform_device *pdev) } } >>> >>> + pb->power_supply = devm_regulator_get_optional(&pdev->dev, >>> "power"); >> >> ... so I think that should be devm_regulator_get(), since the >> regulator isn't really optional. >> >>> + if (IS_ERR(pb->power_supply)) { + if >>> (PTR_ERR(pb->power_supply) != -ENODEV) { + ret = >>> PTR_ERR(pb->power_supply); + goto err_gpio; + } + + >>> pb->power_supply = NULL; >> >> If devm_regulator_get_optional() returns an error value or a >> valid value, then I don't think that this driver should transmute >> error values into NULL; NULL might be a perfectly valid regulator >> value. Related, I think the if (pb->power_supply) tests should be >> replaced with if (!IS_ERR(pb->power_supply)) instead. > > All of that is already done in my local tree. This actually turns > out to work rather smoothly with the new support for optional > regulators. The regulator core will give you a dummy regulator > (assuming it's there physically but hasn't been wired up in > software) that's always on, so the driver doesn't even have to > special case it anymore. OK, hopefully it (the regulator core) complains about the missing DT property though; I assume you're using regulator_get() not regulator_get_optional(), since the supply really is not optional.
On Tue, Oct 01, 2013 at 02:59:43PM -0600, Stephen Warren wrote: > On 10/01/2013 02:53 PM, Thierry Reding wrote: > > On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote: > >> On 09/23/2013 03:41 PM, Thierry Reding wrote: > >>> Many backlights require a power supply to work properly. This > >>> commit uses a power-supply regulator, if available, to power up > >>> and power down the panel. > >> > >> I think that all backlights require a power supply, albeit the > >> supply may not be SW-controllable. Hence, shouldn't the regulator > >> be mandatory in the binding, yet the driver be defensively coded > >> such that if one isn't specified, the driver continues to work? > > > > That has already changed in my local version of this patch. > > > >>> diff --git a/drivers/video/backlight/pwm_bl.c > >>> b/drivers/video/backlight/pwm_bl.c > >> > >>> @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct > >>> platform_device *pdev) } } > >>> > >>> + pb->power_supply = devm_regulator_get_optional(&pdev->dev, > >>> "power"); > >> > >> ... so I think that should be devm_regulator_get(), since the > >> regulator isn't really optional. > >> > >>> + if (IS_ERR(pb->power_supply)) { + if > >>> (PTR_ERR(pb->power_supply) != -ENODEV) { + ret = > >>> PTR_ERR(pb->power_supply); + goto err_gpio; + } + + > >>> pb->power_supply = NULL; > >> > >> If devm_regulator_get_optional() returns an error value or a > >> valid value, then I don't think that this driver should transmute > >> error values into NULL; NULL might be a perfectly valid regulator > >> value. Related, I think the if (pb->power_supply) tests should be > >> replaced with if (!IS_ERR(pb->power_supply)) instead. > > > > All of that is already done in my local tree. This actually turns > > out to work rather smoothly with the new support for optional > > regulators. The regulator core will give you a dummy regulator > > (assuming it's there physically but hasn't been wired up in > > software) that's always on, so the driver doesn't even have to > > special case it anymore. > > OK, hopefully it (the regulator core) complains about the missing DT > property though; I assume you're using regulator_get() not > regulator_get_optional(), since the supply really is not optional. It doesn't always. There's a pr_warn() in _regulator_get(), but that's only for non-DT (since for DT, has_full_constraints is set to true in regulator_init_complete()). Actually that would mean that the regulator core will complain as long as init isn't complete. But since, like many other drivers, pwm-backlight could use deferred probing it's likely to end up being probed after init. Cc'ing Mark Brown. Thierry
On Tue, Oct 01, 2013 at 11:31:27PM +0200, Thierry Reding wrote: > On Tue, Oct 01, 2013 at 02:59:43PM -0600, Stephen Warren wrote: > > OK, hopefully it (the regulator core) complains about the missing DT > > property though; I assume you're using regulator_get() not > > regulator_get_optional(), since the supply really is not optional. > It doesn't always. There's a pr_warn() in _regulator_get(), but that's > only for non-DT (since for DT, has_full_constraints is set to true in > regulator_init_complete()). Actually that would mean that the regulator > core will complain as long as init isn't complete. But since, like many > other drivers, pwm-backlight could use deferred probing it's likely to > end up being probed after init. So, part of the thing here is that nobody ever bothers actually creating the supplies even when the device fails to load without them - everyone is much more keen to complain about needing to do the work than actually provide the hookup even though it's generally pretty quick and simple to do so. That said we probably should still moan, I'll go and do that.
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt index 052eb03..3898f26 100644 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt @@ -15,6 +15,7 @@ Optional properties: - pwm-names: a list of names for the PWM devices specified in the "pwms" property (see PWM binding[0]) - enable-gpios: a list of GPIOs to enable and disable the backlight + - power-supply: regulator for supply voltage [0]: Documentation/devicetree/bindings/pwm/pwm.txt @@ -28,4 +29,5 @@ Example: default-brightness-level = <6>; enable-gpios = <&gpio 58 0>; + power-supply = <&vdd_bl_reg>; }; diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 506810c..a2b3876 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -21,6 +21,7 @@ #include <linux/err.h> #include <linux/pwm.h> #include <linux/pwm_backlight.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> struct pwm_bl_data { @@ -29,6 +30,7 @@ struct pwm_bl_data { unsigned int period; unsigned int lth_brightness; unsigned int *levels; + struct regulator *power_supply; int enable_gpio; unsigned long enable_gpio_flags; int (*notify)(struct device *, @@ -56,6 +58,12 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness, pwm_config(pb->pwm, duty_cycle, pb->period); + if (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) gpio_set_value(pb->enable_gpio, 0); @@ -76,6 +84,9 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) else gpio_set_value(pb->enable_gpio, 0); } + + if (pb->power_supply) + regulator_disable(pb->power_supply); } static int pwm_backlight_update_status(struct backlight_device *bl) @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device *pdev) } } + pb->power_supply = devm_regulator_get_optional(&pdev->dev, "power"); + if (IS_ERR(pb->power_supply)) { + if (PTR_ERR(pb->power_supply) != -ENODEV) { + ret = PTR_ERR(pb->power_supply); + goto err_gpio; + } + + pb->power_supply = NULL; + } + pb->pwm = devm_pwm_get(&pdev->dev, NULL); if (IS_ERR(pb->pwm)) { dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
Many backlights require a power supply to work properly. This commit uses a power-supply regulator, if available, to power up and power down the panel. Signed-off-by: Thierry Reding <treding@nvidia.com> --- .../bindings/video/backlight/pwm-backlight.txt | 2 ++ drivers/video/backlight/pwm_bl.c | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+)