Re: [PATCH V5 9/9] pwm_bl: Add mandatory backlight power regulator

2013-03-26 Thread Thierry Reding
On Mon, Mar 25, 2013 at 06:21:53PM -0700, Andrew Chew wrote:
[...]
> diff --git 
> a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
> b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> index 1e4fc72..cb2d46a 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)
> +  - power-supply: A phandle to the regulator device tree node. This
> +  regulator will be turned on and off as the pwm is enabled and disabled.

Please use "PWM" instead of "pwm" in prose.

>  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;
> + };

This uses a mix of tabs and spaces for indentation. Please stick to
tabs.

Thierry


pgpN6G5GyVy51.pgp
Description: PGP signature


[PATCH V5 9/9] pwm_bl: Add mandatory backlight power regulator

2013-03-25 Thread Andrew Chew
Many backlights need to be explicitly powered.  Typically, this is done
with a GPIO.  For flexibility, we generalize the power mechanism to a
regulator.

If a power regulator is not needed, then a dummy fixed 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 power 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 
Reviewed-by: Alexandre Courbot 
Tested-by: Peter Ujfalusi 
---
 .../bindings/video/backlight/pwm-backlight.txt | 14 +
 drivers/video/backlight/pwm_bl.c   | 59 ++
 2 files changed, 63 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..cb2d46a 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)
+  - power-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 power-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 500>;
 
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+   power-supply = <&bl_en>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1fea627..ddd66d0 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct pwm_bl_data {
struct pwm_device   *pwm;
struct device   *dev;
+   boolenabled;
+   struct regulator*power_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,42 @@ 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);
+   int ret;
+
+   /* Bail if we are already enabled. */
+   if (pb->enabled)
+   return;
+
+   pwm_enable(pb->pwm);
+
+   ret = regulator_enable(pb->power_supply);
+   if (ret)
+   dev_warn(&bl->dev, "Failed to enable regulator: %d", ret);
+
+   pb->enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+   struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+   int ret;
+
+   /* Bail if we are already disabled. */
+   if (!pb->enabled)
+   return;
+
+   ret = regulator_disable(pb->power_supply);
+   if (ret)
+   dev_warn(&bl->dev, "Failed to disable regulator: %d", ret);
+
+   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 +90,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 +104,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 +177,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
 
-   /*
-* TODO: Most users of this driver