[PATCH 0/2 v2] Add mandatory regulator for all users of pwm-backlight.

[PATCH 0/2 v2] Add mandatory regulator for all users of pwm-backlight.

[Andrew Chew]

Many backlights are enabled via GPIO.  We can generalize the GPIO to a
fixed regulator.

The enable regulator needs to be mandatory because there was no good way
to determine the difference between opting out of the regulator, and probe
deferral.

This series of patches is intended to add a dummy regulator (or a GPIO
regulator) for all users of the pwm-backlight.

The last patch in the series will always be the pwm-backlight change to add
this mandatory regulator.  Patches following up to that patch add the
mandatory regulator on a per mach family basis.  Once all users of
pwm-backlight have been patched, this series can be applied in order to
maintain bisectability.

Andrew Chew (2):
  ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight
  pwm_bl: Add mandatory backlight enable regulator

 .../bindings/video/backlight/pwm-backlight.txt     |   14 +++++
 arch/arm/mach-omap2/board-4430sdp.c                |    5 ++
 drivers/video/backlight/pwm_bl.c                   |   55 ++++++++++++++++----
 3 files changed, 64 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Andrew Chew]

The pwm-backlight driver now takes a mandatory regulator that is gotten
during driver probe.  Initialize a dummy regulator to satisfy this
requirement.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 arch/arm/mach-omap2/board-4430sdp.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
index 35f3ad0..62022c0 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -291,6 +291,10 @@ static struct platform_device sdp4430_leds_pwm = {
 	},
 };
 
+/* Dummy regulator for pwm-backlight driver */
+static struct regulator_consumer_supply backlight_supply =
+	REGULATOR_SUPPLY("enable", NULL);
+
 static struct platform_pwm_backlight_data sdp4430_backlight_data = {
 	.max_brightness = 127,
 	.dft_brightness = 127,
@@ -718,6 +722,7 @@ static void __init omap_4430sdp_init(void)
 
 	omap4_i2c_init();
 	omap_sfh7741prox_init();
+	regulator_register_always_on(-1, "bl-enable", &backlight_supply, 1, 0);
 	platform_add_devices(sdp4430_devices, ARRAY_SIZE(sdp4430_devices));
 	omap_serial_init();
 	omap_sdrc_init(NULL, NULL);
-- 
1.7.9.5

Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Peter Ujfalusi]

On 03/12/2013 11:22 PM, Andrew Chew wrote:
> The pwm-backlight driver now takes a mandatory regulator that is gotten
> during driver probe.  Initialize a dummy regulator to satisfy this
> requirement.

I can test this tomorrow, but I have one comment:

> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  arch/arm/mach-omap2/board-4430sdp.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
> index 35f3ad0..62022c0 100644
> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -291,6 +291,10 @@ static struct platform_device sdp4430_leds_pwm = {
>  	},
>  };
>  
> +/* Dummy regulator for pwm-backlight driver */
> +static struct regulator_consumer_supply backlight_supply =
> +	REGULATOR_SUPPLY("enable", NULL);

'enable' is just too generic, the device name should be also provided:
	REGULATOR_SUPPLY("enable", "pwm-backlight");

> +
>  static struct platform_pwm_backlight_data sdp4430_backlight_data = {
>  	.max_brightness = 127,
>  	.dft_brightness = 127,
> @@ -718,6 +722,7 @@ static void __init omap_4430sdp_init(void)
>  
>  	omap4_i2c_init();
>  	omap_sfh7741prox_init();
> +	regulator_register_always_on(-1, "bl-enable", &backlight_supply, 1, 0);
>  	platform_add_devices(sdp4430_devices, ARRAY_SIZE(sdp4430_devices));
>  	omap_serial_init();
>  	omap_sdrc_init(NULL, NULL);
> 


-- 
Péter
--
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

RE: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Andrew Chew]

> > +/* Dummy regulator for pwm-backlight driver */ static struct
> > +regulator_consumer_supply backlight_supply =
> > +	REGULATOR_SUPPLY("enable", NULL);
> 
> 'enable' is just too generic, the device name should be also provided:
> 	REGULATOR_SUPPLY("enable", "pwm-backlight");

You're right.  I don't like how generic it is as well.  But I don't think
"pwm-backlight" works...at least, not for me when I test it.  What
does work is "backlight.x" where x is some number (for me, it's 1).
Problem is, I don't know what it would be for you.  If only there
was a way to wildcard that...

Would it be better if we called the regulator something other than
"enable"?  Maybe "backlight-enable", or "bl-enable" for brevity?

Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

On Wed, Mar 13, 2013 at 01:38:31PM -0700, Andrew Chew wrote:
> > > +/* Dummy regulator for pwm-backlight driver */ static struct
> > > +regulator_consumer_supply backlight_supply =
> > > +	REGULATOR_SUPPLY("enable", NULL);
> > 
> > 'enable' is just too generic, the device name should be also provided:
> > 	REGULATOR_SUPPLY("enable", "pwm-backlight");
> 
> You're right.  I don't like how generic it is as well.  But I don't think
> "pwm-backlight" works...at least, not for me when I test it.  What
> does work is "backlight.x" where x is some number (for me, it's 1).
> Problem is, I don't know what it would be for you.  If only there
> was a way to wildcard that...
> 
> Would it be better if we called the regulator something other than
> "enable"?  Maybe "backlight-enable", or "bl-enable" for brevity?

The second parameter needs to match the device name. For the 4430sdp
board this should be "pwm-backlight" since the name will be generated
from the .name and .id fields of the struct platform_device. .id = -1
will result in no .<id> suffix being attached, so the device should be
named "pwm-backlight". The first parameter needs to match the name of
the supply that the driver requests, so "enable" is correct since the
call to regulator_get() uses that.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Andrew Chew]

> From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
> Sent: Wednesday, March 13, 2013 1:59 PM
> To: Andrew Chew
> Cc: Peter Ujfalusi; Alex Courbot; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator
> to pwm-backlight
> 
> * PGP Signed by an unknown key
> 
> On Wed, Mar 13, 2013 at 01:38:31PM -0700, Andrew Chew wrote:
> > > > +/* Dummy regulator for pwm-backlight driver */ static struct
> > > > +regulator_consumer_supply backlight_supply =
> > > > +	REGULATOR_SUPPLY("enable", NULL);
> > >
> > > 'enable' is just too generic, the device name should be also provided:
> > > 	REGULATOR_SUPPLY("enable", "pwm-backlight");
> >
> > You're right.  I don't like how generic it is as well.  But I don't
> > think "pwm-backlight" works...at least, not for me when I test it.
> > What does work is "backlight.x" where x is some number (for me, it's 1).
> > Problem is, I don't know what it would be for you.  If only there was
> > a way to wildcard that...
> >
> > Would it be better if we called the regulator something other than
> > "enable"?  Maybe "backlight-enable", or "bl-enable" for brevity?
> 
> The second parameter needs to match the device name. For the 4430sdp
> board this should be "pwm-backlight" since the name will be generated from
> the .name and .id fields of the struct platform_device. .id = -1 will result in no
> .<id> suffix being attached, so the device should be named "pwm-backlight".
> The first parameter needs to match the name of the supply that the driver
> requests, so "enable" is correct since the call to regulator_get() uses that.

Ah, I see.  That makes sense.  Thanks, Thierry!  "pwm-backlight" it is, then,
and I'll make sure to check for this for the other boards.

[PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Andrew Chew]

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)
+		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;
-- 
1.7.9.5

Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Peter Ujfalusi]

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


-- 
Péter
--
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

Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Andrew Chew]

> > +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?

Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Andrew Chew]

> 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.