[PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Andrew Chew]

The backlight enable regulator is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
Changed the name of the property from en-supply to enable-supply.

Made enable-supply a mandatory property.  Changed the example in the
bindings documentation accordingly.

Moved devm_regulator_get() to the probe function.  Regulator is now
requested unconditionally.

Changed IS_ERR_OR_NULL to IS_ERR per Alex's recommendation.


 .../bindings/video/backlight/pwm-backlight.txt     |   14 ++++++
 drivers/video/backlight/pwm_bl.c                   |   52 ++++++++++++++++----
 2 files changed, 56 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 069983c..ff98cdd 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;
+	struct regulator	*en_supply;
+	bool			en_supply_enabled;
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
@@ -35,6 +38,34 @@ 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);
+
+	pwm_enable(pb->pwm);
+
+	if (pb->en_supply && !pb->en_supply_enabled) {
+		if (regulator_enable(pb->en_supply) != 0)
+			dev_warn(&bl->dev, "Failed to enable regulator");
+		else
+			pb->en_supply_enabled = true;
+	}
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+
+	if (pb->en_supply && pb->en_supply_enabled) {
+		if (regulator_disable(pb->en_supply) != 0)
+			dev_warn(&bl->dev, "Failed to disable regulator");
+		else
+			pb->en_supply_enabled = false;
+	}
+
+	pwm_disable(pb->pwm);
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -52,7 +83,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;
 
@@ -66,7 +97,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)
@@ -145,12 +176,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;
 }
 
@@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 
+	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
+	if (IS_ERR(pb->en_supply)) {
+		ret = PTR_ERR(pb->en_supply);
+		pb->en_supply = NULL;
+		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 +300,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 +315,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 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Thierry Reding]

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

On Wed, Mar 06, 2013 at 09:17:18AM -0800, Andrew Chew wrote:
[...]
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 069983c..ff98cdd 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;
> +	struct regulator	*en_supply;

Can this be renamed to enable_supply to match the DT property name?

> +	bool			en_supply_enabled;

This boolean can be dropped. As discussed in a previous thread, the
pwm-backlight driver shouldn't need to know about any other uses of the
regulator.

> +static void pwm_backlight_enable(struct backlight_device *bl)
> +{
> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> +
> +	pwm_enable(pb->pwm);
> +
> +	if (pb->en_supply && !pb->en_supply_enabled) {
> +		if (regulator_enable(pb->en_supply) != 0)
> +			dev_warn(&bl->dev, "Failed to enable regulator");
> +		else
> +			pb->en_supply_enabled = true;
> +	}
> +}
> +
> +static void pwm_backlight_disable(struct backlight_device *bl)
> +{
> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> +
> +	if (pb->en_supply && pb->en_supply_enabled) {
> +		if (regulator_disable(pb->en_supply) != 0)
> +			dev_warn(&bl->dev, "Failed to disable regulator");
> +		else
> +			pb->en_supply_enabled = false;
> +	}
> +
> +	pwm_disable(pb->pwm);
> +}

Alex already brought this up: shouldn't the sequences be reversed. That
is, when enabling the backlight, turn on the regulator first, then
enable the PWM. When disabling, disable the PWM first, then turn off the
regulator?

> @@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	pb->exit = data->exit;
>  	pb->dev = &pdev->dev;
>  
> +	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
> +	if (IS_ERR(pb->en_supply)) {
> +		ret = PTR_ERR(pb->en_supply);
> +		pb->en_supply = NULL;
> +		goto err_alloc;
> +	}

This effectively makes the regulator mandatory, so the board files that
use pwm-backlight need to be updated or otherwise will break.

Thierry

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

Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Alex Courbot]

On 03/07/2013 04:11 PM, Thierry Reding wrote:
>> +	bool			en_supply_enabled;
>
> This boolean can be dropped. As discussed in a previous thread, the
> pwm-backlight driver shouldn't need to know about any other uses of the
> regulator.

Sorry for being obstinate - but I'm still not convinced we can get rid 
of it. I checked the regulator code, and as you mentioned in the 
previous version, calls to regulator_enable() and regulator_disable() 
*must* be balanced in this driver.

Without this variable we would call regulator_enable() every time 
pwm_backlight_enable() is called (and same thing when disabling). Now 
imagine the driver is asked to set the following intensities: 5, 12, 
then 0. You would have two calls to regulator_enable() but only one to 
regulator_disable(), which would result in the enable GPIO remaining 
active even though it would be shut down. Or I missed something obvious.

The regulator must be enabled/disabled on transitions from/to 0, and 
AFAICT there is no way for this driver to detect them.

>> +static void pwm_backlight_enable(struct backlight_device *bl)
>> +{
>> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
>> +
>> +	pwm_enable(pb->pwm);
>> +
>> +	if (pb->en_supply && !pb->en_supply_enabled) {
>> +		if (regulator_enable(pb->en_supply) != 0)
>> +			dev_warn(&bl->dev, "Failed to enable regulator");
>> +		else
>> +			pb->en_supply_enabled = true;
>> +	}
>> +}
>> +
>> +static void pwm_backlight_disable(struct backlight_device *bl)
>> +{
>> +	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
>> +
>> +	if (pb->en_supply && pb->en_supply_enabled) {
>> +		if (regulator_disable(pb->en_supply) != 0)
>> +			dev_warn(&bl->dev, "Failed to disable regulator");
>> +		else
>> +			pb->en_supply_enabled = false;
>> +	}
>> +
>> +	pwm_disable(pb->pwm);
>> +}
>
> Alex already brought this up: shouldn't the sequences be reversed. That
> is, when enabling the backlight, turn on the regulator first, then
> enable the PWM. When disabling, disable the PWM first, then turn off the
> regulator?

Actually the current sequence seems to make sense - the PWM is always 
active when the enable GPIO is switched. If we do the contrary, we might 
have a short time where the backlight is enabled without receiving 
anything from the PWM. Don't think that would be serious, but the 
current behavior is similar to e.g. panels which we enable only after a 
signal is available.

>> @@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>   	pb->exit = data->exit;
>>   	pb->dev = &pdev->dev;
>>
>> +	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
>> +	if (IS_ERR(pb->en_supply)) {
>> +		ret = PTR_ERR(pb->en_supply);
>> +		pb->en_supply = NULL;
>> +		goto err_alloc;
>> +	}
>
> This effectively makes the regulator mandatory, so the board files that
> use pwm-backlight need to be updated or otherwise will break.

Yes. Btw, should such changes go into the same patch? This seems 
difficult to split without breaking things at some point.

Alex.

Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Thierry Reding]

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

On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote:
> On 03/07/2013 04:11 PM, Thierry Reding wrote:
> >>+	bool			en_supply_enabled;
> >
> >This boolean can be dropped. As discussed in a previous thread, the
> >pwm-backlight driver shouldn't need to know about any other uses of the
> >regulator.
> 
> Sorry for being obstinate - but I'm still not convinced we can get
> rid of it. I checked the regulator code, and as you mentioned in the
> previous version, calls to regulator_enable() and
> regulator_disable() *must* be balanced in this driver.
> 
> Without this variable we would call regulator_enable() every time
> pwm_backlight_enable() is called (and same thing when disabling).
> Now imagine the driver is asked to set the following intensities: 5,
> 12, then 0. You would have two calls to regulator_enable() but only
> one to regulator_disable(), which would result in the enable GPIO
> remaining active even though it would be shut down. Or I missed
> something obvious.
> 
> The regulator must be enabled/disabled on transitions from/to 0, and
> AFAICT there is no way for this driver to detect them.

Yes, that's true, but I don't think it should be solved for just this
one regulator. Instead if we need to track the enable state we might as
well track it for *any* resource so that the PWM isn't enabled/disabled
twice either.

> >>+static void pwm_backlight_enable(struct backlight_device *bl)
> >>+{
> >>+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> >>+
> >>+	pwm_enable(pb->pwm);
> >>+
> >>+	if (pb->en_supply && !pb->en_supply_enabled) {
> >>+		if (regulator_enable(pb->en_supply) != 0)
> >>+			dev_warn(&bl->dev, "Failed to enable regulator");
> >>+		else
> >>+			pb->en_supply_enabled = true;
> >>+	}
> >>+}
> >>+
> >>+static void pwm_backlight_disable(struct backlight_device *bl)
> >>+{
> >>+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> >>+
> >>+	if (pb->en_supply && pb->en_supply_enabled) {
> >>+		if (regulator_disable(pb->en_supply) != 0)
> >>+			dev_warn(&bl->dev, "Failed to disable regulator");
> >>+		else
> >>+			pb->en_supply_enabled = false;
> >>+	}
> >>+
> >>+	pwm_disable(pb->pwm);
> >>+}
> >
> >Alex already brought this up: shouldn't the sequences be reversed. That
> >is, when enabling the backlight, turn on the regulator first, then
> >enable the PWM. When disabling, disable the PWM first, then turn off the
> >regulator?
> 
> Actually the current sequence seems to make sense - the PWM is
> always active when the enable GPIO is switched. If we do the
> contrary, we might have a short time where the backlight is enabled
> without receiving anything from the PWM. Don't think that would be
> serious, but the current behavior is similar to e.g. panels which we
> enable only after a signal is available.

Okay, let's leave it like that then.

> >>@@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >>  	pb->exit = data->exit;
> >>  	pb->dev = &pdev->dev;
> >>
> >>+	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
> >>+	if (IS_ERR(pb->en_supply)) {
> >>+		ret = PTR_ERR(pb->en_supply);
> >>+		pb->en_supply = NULL;
> >>+		goto err_alloc;
> >>+	}
> >
> >This effectively makes the regulator mandatory, so the board files that
> >use pwm-backlight need to be updated or otherwise will break.
> 
> Yes. Btw, should such changes go into the same patch? This seems
> difficult to split without breaking things at some point.

I expect that if the changes are split up then the board-setup code
changes need to be done prior to the driver change. Using the lookup
tables should make this easy because they aren't tied to the platform
data and can be added independently. The patches should probably go
through the same subsystem tree to take care of the dependencies.

Keeping everything in one patch would work too, but it's certainly more
chaotic.

Thierry

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

RE: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Andrew Chew]

> From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
> Sent: Thursday, March 07, 2013 3:27 AM
> To: Alex Courbot
> Cc: Andrew Chew; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable
> regulator
> 
> * PGP Signed by an unknown key
> 
> On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote:
> > On 03/07/2013 04:11 PM, Thierry Reding wrote:
> > >>+	bool			en_supply_enabled;
> > >
> > >This boolean can be dropped. As discussed in a previous thread, the
> > >pwm-backlight driver shouldn't need to know about any other uses of
> > >the regulator.
> >
> > Sorry for being obstinate - but I'm still not convinced we can get rid
> > of it. I checked the regulator code, and as you mentioned in the
> > previous version, calls to regulator_enable() and
> > regulator_disable() *must* be balanced in this driver.
> >
> > Without this variable we would call regulator_enable() every time
> > pwm_backlight_enable() is called (and same thing when disabling).
> > Now imagine the driver is asked to set the following intensities: 5,
> > 12, then 0. You would have two calls to regulator_enable() but only
> > one to regulator_disable(), which would result in the enable GPIO
> > remaining active even though it would be shut down. Or I missed
> > something obvious.
> >
> > The regulator must be enabled/disabled on transitions from/to 0, and
> > AFAICT there is no way for this driver to detect them.
> 
> Yes, that's true, but I don't think it should be solved for just this one
> regulator. Instead if we need to track the enable state we might as well track
> it for *any* resource so that the PWM isn't enabled/disabled twice either.

That makes sense, but I'm confused due to previous comments.  The most
obvious way to do this seems to be to have a bool track the enable state.
Do you still want me to do away with this bool?  I can satisfy your very
last comment by keeping the bool (renaming it to something more generic)
and encapsulating the pwm_enable()/pwm_disable() call within.
I'll send out v5 today to show what I mean.

> > >This effectively makes the regulator mandatory, so the board files
> > >that use pwm-backlight need to be updated or otherwise will break.
> >
> > Yes. Btw, should such changes go into the same patch? This seems
> > difficult to split without breaking things at some point.
> 
> I expect that if the changes are split up then the board-setup code changes
> need to be done prior to the driver change. Using the lookup tables should
> make this easy because they aren't tied to the platform data and can be
> added independently. The patches should probably go through the same
> subsystem tree to take care of the dependencies.
> 
> Keeping everything in one patch would work too, but it's certainly more
> chaotic.

Am I supposed to handle those patches?  I'm concerned that I don't have
hardware to test properly, but I can give it a shot if it's my responsibility.

Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Alex Courbot]

On 03/08/2013 06:07 AM, Andrew Chew wrote:
>> From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
>> Sent: Thursday, March 07, 2013 3:27 AM
>> To: Alex Courbot
>> Cc: Andrew Chew; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable
>> regulator
>>
>> * PGP Signed by an unknown key
>>
>> On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote:
>>> On 03/07/2013 04:11 PM, Thierry Reding wrote:
>>>>> +	bool			en_supply_enabled;
>>>>
>>>> This boolean can be dropped. As discussed in a previous thread, the
>>>> pwm-backlight driver shouldn't need to know about any other uses of
>>>> the regulator.
>>>
>>> Sorry for being obstinate - but I'm still not convinced we can get rid
>>> of it. I checked the regulator code, and as you mentioned in the
>>> previous version, calls to regulator_enable() and
>>> regulator_disable() *must* be balanced in this driver.
>>>
>>> Without this variable we would call regulator_enable() every time
>>> pwm_backlight_enable() is called (and same thing when disabling).
>>> Now imagine the driver is asked to set the following intensities: 5,
>>> 12, then 0. You would have two calls to regulator_enable() but only
>>> one to regulator_disable(), which would result in the enable GPIO
>>> remaining active even though it would be shut down. Or I missed
>>> something obvious.
>>>
>>> The regulator must be enabled/disabled on transitions from/to 0, and
>>> AFAICT there is no way for this driver to detect them.
>>
>> Yes, that's true, but I don't think it should be solved for just this one
>> regulator. Instead if we need to track the enable state we might as well track
>> it for *any* resource so that the PWM isn't enabled/disabled twice either.
>
> That makes sense, but I'm confused due to previous comments.  The most
> obvious way to do this seems to be to have a bool track the enable state.
> Do you still want me to do away with this bool?  I can satisfy your very
> last comment by keeping the bool (renaming it to something more generic)
> and encapsulating the pwm_enable()/pwm_disable() call within.

I think that's what Thierry meant, yes.

>> I expect that if the changes are split up then the board-setup code changes
>> need to be done prior to the driver change. Using the lookup tables should
>> make this easy because they aren't tied to the platform data and can be
>> added independently. The patches should probably go through the same
>> subsystem tree to take care of the dependencies.
>>
>> Keeping everything in one patch would work too, but it's certainly more
>> chaotic.
>
> Am I supposed to handle those patches?  I'm concerned that I don't have
> hardware to test properly, but I can give it a shot if it's my responsibility.

Yes, if you introduce incompatibilities you have the burden of 
performing the transition without breaking things at any single point of 
the git history. Since this is just about adding a dummy regulator, it 
should go fine even without testing. And in the event it does not, 
that's what linux-next is for.

Make sure you also update the dts of current device tree users, as they 
will break, too.

What I don't know is if you should update all users in one big patch, or 
instead provide one patch per platform changed. Maybe Thierry can 
provide some guidance here.

Alex.

Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Thierry Reding]

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

On Fri, Mar 08, 2013 at 11:21:04AM +0900, Alex Courbot wrote:
> On 03/08/2013 06:07 AM, Andrew Chew wrote:
> >>From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
> >>Sent: Thursday, March 07, 2013 3:27 AM
> >>To: Alex Courbot
> >>Cc: Andrew Chew; linux-kernel@vger.kernel.org
> >>Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable
> >>regulator
> >>
> >>* PGP Signed by an unknown key
> >>
> >>On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote:
> >>>On 03/07/2013 04:11 PM, Thierry Reding wrote:
> >>>>>+	bool			en_supply_enabled;
> >>>>
> >>>>This boolean can be dropped. As discussed in a previous thread, the
> >>>>pwm-backlight driver shouldn't need to know about any other uses of
> >>>>the regulator.
> >>>
> >>>Sorry for being obstinate - but I'm still not convinced we can get rid
> >>>of it. I checked the regulator code, and as you mentioned in the
> >>>previous version, calls to regulator_enable() and
> >>>regulator_disable() *must* be balanced in this driver.
> >>>
> >>>Without this variable we would call regulator_enable() every time
> >>>pwm_backlight_enable() is called (and same thing when disabling).
> >>>Now imagine the driver is asked to set the following intensities: 5,
> >>>12, then 0. You would have two calls to regulator_enable() but only
> >>>one to regulator_disable(), which would result in the enable GPIO
> >>>remaining active even though it would be shut down. Or I missed
> >>>something obvious.
> >>>
> >>>The regulator must be enabled/disabled on transitions from/to 0, and
> >>>AFAICT there is no way for this driver to detect them.
> >>
> >>Yes, that's true, but I don't think it should be solved for just this one
> >>regulator. Instead if we need to track the enable state we might as well track
> >>it for *any* resource so that the PWM isn't enabled/disabled twice either.
> >
> >That makes sense, but I'm confused due to previous comments.  The most
> >obvious way to do this seems to be to have a bool track the enable state.
> >Do you still want me to do away with this bool?  I can satisfy your very
> >last comment by keeping the bool (renaming it to something more generic)
> >and encapsulating the pwm_enable()/pwm_disable() call within.
> 
> I think that's what Thierry meant, yes.

Yes, it is. =)

> >>I expect that if the changes are split up then the board-setup code changes
> >>need to be done prior to the driver change. Using the lookup tables should
> >>make this easy because they aren't tied to the platform data and can be
> >>added independently. The patches should probably go through the same
> >>subsystem tree to take care of the dependencies.
> >>
> >>Keeping everything in one patch would work too, but it's certainly more
> >>chaotic.
> >
> >Am I supposed to handle those patches?  I'm concerned that I don't have
> >hardware to test properly, but I can give it a shot if it's my responsibility.
> 
> Yes, if you introduce incompatibilities you have the burden of
> performing the transition without breaking things at any single
> point of the git history. Since this is just about adding a dummy
> regulator, it should go fine even without testing. And in the event
> it does not, that's what linux-next is for.

Right. We'll need an Acked-by from the board/machine maintainers anyway
and if something still breaks we can always fix it after somebody's
actually done the testing.

> Make sure you also update the dts of current device tree users, as
> they will break, too.
> 
> What I don't know is if you should update all users in one big
> patch, or instead provide one patch per platform changed. Maybe
> Thierry can provide some guidance here.

I think it'd be good to split them up into per-architecture and
per-machine. Per-board would probably be too much. That'll allow the
respective maintainers to ack patches that touch their machines or
boards without having them go through all other hunks too.

Thierry

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