| Message ID | 1363126934-8754-3-git-send-email-achew@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/12/2013 11:22 PM, Andrew Chew wrote: > Many backlights need to be explicitly enabled. Typically, this is done > with a GPIO. For flexibility, we generalize the enable mechanism to a > regulator. > > If an enable regulator is not needed, then a dummy regulator can be given > to the backlight driver. If a GPIO is used to enable the backlight, > then a fixed regulator can be instantiated to control the GPIO. > > The backlight enable regulator can be specified in the device tree node > for the backlight, or can be done with legacy board setup code in the > usual way. > > Signed-off-by: Andrew Chew <achew@nvidia.com> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> > --- > .../bindings/video/backlight/pwm-backlight.txt | 14 +++++ > drivers/video/backlight/pwm_bl.c | 55 ++++++++++++++++---- > 2 files changed, 59 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > index 1e4fc72..7e2e089 100644 > --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > @@ -10,6 +10,11 @@ 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) > + - enable-supply: A phandle to the regulator device tree node. This > + regulator will be turned on and off as the pwm is enabled and disabled. > + Many backlights are enabled via a GPIO. In this case, we instantiate > + a fixed regulator and give that to enable-supply. If a regulator > + is not needed, then provide a dummy fixed regulator. > > Optional properties: > - pwm-names: a list of names for the PWM devices specified in the > @@ -19,10 +24,19 @@ Optional properties: > > Example: > > + bl_en: fixed-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "bl-en-supply"; > + regulator-boot-on; > + gpio = <&some_gpio>; > + enable-active-high; > + }; > + > backlight { > compatible = "pwm-backlight"; > pwms = <&pwm 0 5000000>; > > brightness-levels = <0 4 8 16 32 64 128 255>; > default-brightness-level = <6>; > + enable-supply = <&bl_en>; > }; > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 1fea627..c557c51 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; > + bool enabled; > + struct regulator *enable_supply; > unsigned int period; > unsigned int lth_brightness; > unsigned int *levels; > @@ -35,6 +38,38 @@ struct pwm_bl_data { > void (*exit)(struct device *); > }; > > +static void pwm_backlight_enable(struct backlight_device *bl) > +{ > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > + > + /* Bail if we are already enabled. */ > + if (pb->enabled) > + return; > + > + pwm_enable(pb->pwm); > + > + if (regulator_enable(pb->enable_supply) != 0) I would loose the '!= 0' > + dev_warn(&bl->dev, "Failed to enable regulator"); > + > + pb->enabled = true; > +} > + > +static void pwm_backlight_disable(struct backlight_device *bl) > +{ > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > + > + /* Bail if we are already disabled. */ > + if (!pb->enabled) > + return; > + > + if (regulator_disable(pb->enable_supply) != 0) > + dev_warn(&bl->dev, "Failed to disable regulator"); > + > + pwm_disable(pb->pwm); > + > + pb->enabled = false; > +} Would it not be better to have some locking here since the code started to use flag for state tracking? > + > static int pwm_backlight_update_status(struct backlight_device *bl) > { > struct pwm_bl_data *pb = bl_get_data(bl); > @@ -51,7 +86,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > if (brightness == 0) { > pwm_config(pb->pwm, 0, pb->period); > - pwm_disable(pb->pwm); > + pwm_backlight_disable(bl); > } else { > int duty_cycle; > > @@ -65,7 +100,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > duty_cycle = pb->lth_brightness + > (duty_cycle * (pb->period - pb->lth_brightness) / max); > pwm_config(pb->pwm, duty_cycle, pb->period); > - pwm_enable(pb->pwm); > + pwm_backlight_enable(bl); > } > > if (pb->notify_after) > @@ -138,12 +173,6 @@ 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. > - */ > - > return 0; > } > > @@ -206,6 +235,12 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->exit = data->exit; > pb->dev = &pdev->dev; > > + pb->enable_supply = devm_regulator_get(&pdev->dev, "enable"); > + if (IS_ERR(pb->enable_supply)) { > + ret = PTR_ERR(pb->enable_supply); > + goto err_alloc; > + } > + > 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"); > @@ -268,7 +303,7 @@ static int pwm_backlight_remove(struct platform_device *pdev) > > backlight_device_unregister(bl); > pwm_config(pb->pwm, 0, pb->period); > - pwm_disable(pb->pwm); > + pwm_backlight_disable(bl); > if (pb->exit) > pb->exit(&pdev->dev); > return 0; > @@ -283,7 +318,7 @@ static int pwm_backlight_suspend(struct device *dev) > if (pb->notify) > pb->notify(pb->dev, 0); > pwm_config(pb->pwm, 0, pb->period); > - pwm_disable(pb->pwm); > + pwm_backlight_disable(bl); > if (pb->notify_after) > pb->notify_after(pb->dev, 0); > return 0; >
On Wed, Mar 13, 2013 at 09:56:57AM +0100, Peter Ujfalusi wrote: > On 03/12/2013 11:22 PM, Andrew Chew wrote: [...] > > +static void pwm_backlight_disable(struct backlight_device *bl) > > +{ > > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > > + > > + /* Bail if we are already disabled. */ > > + if (!pb->enabled) > > + return; > > + > > + if (regulator_disable(pb->enable_supply) != 0) > > + dev_warn(&bl->dev, "Failed to disable regulator"); > > + > > + pwm_disable(pb->pwm); > > + > > + pb->enabled = false; > > +} > > Would it not be better to have some locking here since the code started to use > flag for state tracking? I don't think that's necessary. The backlight core already uses the ops_lock mutex to serial accesses. I just noticed that the documentation mentions that update_lock is used for this purpose, but the code doesn't use it after it is initialized. Still, the ops_lock should be enough. Thierry
> > +static void pwm_backlight_enable(struct backlight_device *bl) { > > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > > + > > + /* Bail if we are already enabled. */ > > + if (pb->enabled) > > + return; > > + > > + pwm_enable(pb->pwm); > > + > > + if (regulator_enable(pb->enable_supply) != 0) > > I would loose the '!= 0' I think I prefer the '!= 0'. Without it, it looks at first glance like regulator_enable() is following boolean semantics, so it reads kind of weird. But I'll defer to Thierry on this one. Thierry, what's your preference? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 13, 2013 at 02:10:16PM -0700, Andrew Chew wrote: > > > +static void pwm_backlight_enable(struct backlight_device *bl) { > > > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > > > + > > > + /* Bail if we are already enabled. */ > > > + if (pb->enabled) > > > + return; > > > + > > > + pwm_enable(pb->pwm); > > > + > > > + if (regulator_enable(pb->enable_supply) != 0) > > > > I would loose the '!= 0' > > I think I prefer the '!= 0'. Without it, it looks at first glance > like regulator_enable() is following boolean semantics, > so it reads kind of weird. But I'll defer to Thierry on this > one. Thierry, what's your preference? Why not assign the return value of regulator_enable() to a variable, for instance "err", and make that part of the warning message so that people will have a better chance to diagnose what's going wrong? Thierry
> On Wed, Mar 13, 2013 at 02:10:16PM -0700, Andrew Chew wrote: > > > > +static void pwm_backlight_enable(struct backlight_device *bl) { > > > > + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); > > > > + > > > > + /* Bail if we are already enabled. */ > > > > + if (pb->enabled) > > > > + return; > > > > + > > > > + pwm_enable(pb->pwm); > > > > + > > > > + if (regulator_enable(pb->enable_supply) != 0) > > > > > > I would loose the '!= 0' > > > > I think I prefer the '!= 0'. Without it, it looks at first glance > > like regulator_enable() is following boolean semantics, so it reads > > kind of weird. But I'll defer to Thierry on this one. Thierry, > > what's your preference? > > Why not assign the return value of regulator_enable() to a variable, for > instance "err", and make that part of the warning message so that people will > have a better chance to diagnose what's going wrong? That's a good idea. I'll have that modification in the next patch series that I post. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt index 1e4fc72..7e2e089 100644 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt @@ -10,6 +10,11 @@ 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) + - enable-supply: A phandle to the regulator device tree node. This + regulator will be turned on and off as the pwm is enabled and disabled. + Many backlights are enabled via a GPIO. In this case, we instantiate + a fixed regulator and give that to enable-supply. If a regulator + is not needed, then provide a dummy fixed regulator. Optional properties: - pwm-names: a list of names for the PWM devices specified in the @@ -19,10 +24,19 @@ Optional properties: Example: + bl_en: fixed-regulator { + compatible = "regulator-fixed"; + regulator-name = "bl-en-supply"; + regulator-boot-on; + gpio = <&some_gpio>; + enable-active-high; + }; + backlight { compatible = "pwm-backlight"; pwms = <&pwm 0 5000000>; brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + enable-supply = <&bl_en>; }; diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 1fea627..c557c51 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; + bool enabled; + struct regulator *enable_supply; unsigned int period; unsigned int lth_brightness; unsigned int *levels; @@ -35,6 +38,38 @@ struct pwm_bl_data { void (*exit)(struct device *); }; +static void pwm_backlight_enable(struct backlight_device *bl) +{ + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); + + /* Bail if we are already enabled. */ + if (pb->enabled) + return; + + pwm_enable(pb->pwm); + + if (regulator_enable(pb->enable_supply) != 0) + dev_warn(&bl->dev, "Failed to enable regulator"); + + pb->enabled = true; +} + +static void pwm_backlight_disable(struct backlight_device *bl) +{ + struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); + + /* Bail if we are already disabled. */ + if (!pb->enabled) + return; + + if (regulator_disable(pb->enable_supply) != 0) + dev_warn(&bl->dev, "Failed to disable regulator"); + + pwm_disable(pb->pwm); + + pb->enabled = false; +} + static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); @@ -51,7 +86,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) if (brightness == 0) { pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_disable(bl); } else { int duty_cycle; @@ -65,7 +100,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) duty_cycle = pb->lth_brightness + (duty_cycle * (pb->period - pb->lth_brightness) / max); pwm_config(pb->pwm, duty_cycle, pb->period); - pwm_enable(pb->pwm); + pwm_backlight_enable(bl); } if (pb->notify_after) @@ -138,12 +173,6 @@ 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. - */ - return 0; } @@ -206,6 +235,12 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->exit = data->exit; pb->dev = &pdev->dev; + pb->enable_supply = devm_regulator_get(&pdev->dev, "enable"); + if (IS_ERR(pb->enable_supply)) { + ret = PTR_ERR(pb->enable_supply); + goto err_alloc; + } + 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"); @@ -268,7 +303,7 @@ static int pwm_backlight_remove(struct platform_device *pdev) backlight_device_unregister(bl); pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_disable(bl); if (pb->exit) pb->exit(&pdev->dev); return 0; @@ -283,7 +318,7 @@ static int pwm_backlight_suspend(struct device *dev) if (pb->notify) pb->notify(pb->dev, 0); pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_disable(bl); if (pb->notify_after) pb->notify_after(pb->dev, 0); return 0;