[PATCH] pwm-backlight: add regulator and GPIO support

Add support for an optional power regulator and enable/disable GPIO.
This scheme is commonly used in embedded systems.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight         |  4 +
 drivers/video/backlight/pwm_bl.c                   | 86 ++++++++++++++++++----
 include/linux/pwm_backlight.h                      |  5 ++
 3 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
index 1e4fc72..85cbb7b 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight

@@ -14,6 +14,8 @@ Required properties:
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
                "pwms" property (see PWM binding[0])
+  - power-supply: a reference to a regulator used to control the backlight power
+  - enable-gpios: a reference to a GPIO used to enable/disable the backlight
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 

@@ -25,4 +27,6 @@ Example:
 
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 6 0>;
 	};

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 057389d..821e03e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -20,6 +20,8 @@
 #include <linux/pwm.h>
 #include <linux/pwm_backlight.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
 
 struct pwm_bl_data {
 	struct pwm_device	*pwm;

@@ -27,6 +29,9 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	bool			enabled;
+	struct regulator	*power_reg;
+	int			enable_gpio;
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,

@@ -35,6 +40,40 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static void pwm_backlight_off(struct pwm_bl_data *pb)
+{
+	if (!pb->enabled)
+		return;
+
+	if (gpio_is_valid(pb->enable_gpio))
+		gpio_set_value_cansleep(pb->enable_gpio, 0);
+
+	if (pb->power_reg)
+		regulator_disable(pb->power_reg);
+
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
+	pb->enabled = false;
+}
+
+static void pwm_backlight_on(struct pwm_bl_data *pb, int brightness)
+{
+	pwm_config(pb->pwm, brightness, pb->period);
+	pwm_enable(pb->pwm);
+
+	if (pb->enabled)
+		return;
+
+	if (pb->power_reg)
+		regulator_enable(pb->power_reg);
+
+	if (gpio_is_valid(pb->enable_gpio))
+		gpio_set_value_cansleep(pb->enable_gpio, 1);
+
+	pb->enabled = true;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);

@@ -51,8 +90,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		brightness = pb->notify(pb->dev, brightness);
 
 	if (brightness = 0) {
-		pwm_config(pb->pwm, 0, pb->period);
-		pwm_disable(pb->pwm);
+		pwm_backlight_off(pb);
 	} else {
 		if (pb->levels) {
 			brightness = pb->levels[brightness];

@@ -61,8 +99,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 
 		brightness = pb->lth_brightness +
 			(brightness * (pb->period - pb->lth_brightness) / max);
-		pwm_config(pb->pwm, brightness, pb->period);
-		pwm_enable(pb->pwm);
+		pwm_backlight_on(pb, brightness);
 	}
 
 	if (pb->notify_after)

@@ -141,11 +178,14 @@ 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.
-	 */
+	ret = of_get_named_gpio(node, "enable-gpios", 0);
+	if (ret >= 0) {
+		data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
+		data->use_enable_gpio = true;
+	} else if (ret != -ENOENT) {
+		/* GPIO is optional, so ENOENT is not an error here */
+		return ret;
+	}
 
 	return 0;
 }

@@ -176,7 +216,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	if (!data) {
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret = -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}

@@ -221,8 +263,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
 	/*
 	 * The DT case will set the pwm_period_ns field to 0 and store the
 	 * period, parsed from the DT, in the PWM device. For the non-DT case,

@@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (data->pwm_period_ns > 0)
 		pwm_set_period(pb->pwm, data->pwm_period_ns);
 
+
+	pb->power_reg = devm_regulator_get(&pdev->dev, "power");
+	if (IS_ERR(pb->power_reg))
+		return PTR_ERR(pb->power_reg);
+
+	pb->enable_gpio = -EINVAL;
+	if (data->use_enable_gpio) {
+		ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
+				GPIOF_OUT_INIT_HIGH, "backlight_enable");
+		if (ret)
+			dev_warn(&pdev->dev,
+				"error %d requesting control gpio\n", ret);
+		else
+			pb->enable_gpio = data->enable_gpio;
+	}
+
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 

@@ -265,8 +321,7 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
+	pwm_backlight_off(pb);
 	pwm_put(pb->pwm);
 	if (pb->exit)
 		pb->exit(&pdev->dev);

@@ -281,8 +336,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_off(pb);
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 	return 0;

diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..5ae2cd0 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h

@@ -18,6 +18,11 @@ struct platform_pwm_backlight_data {
 	void (*notify_after)(struct device *dev, int brightness);
 	void (*exit)(struct device *dev);
 	int (*check_fb)(struct device *dev, struct fb_info *info);
+	/* optional GPIO that enables/disables the backlight */
+	int enable_gpio;
+	/* 0 (default initialization value) is a valid GPIO number. Make use of
+	 * control gpio explicit to avoid bad surprises. */
+	bool use_enable_gpio;
 };
 
 #endif

-- 
1.7.11.1

On 06/29/2012 07:22 AM, Alexandre Courbot wrote:

Add support for an optional power regulator and enable/disable GPIO.
This scheme is commonly used in embedded systems.

-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-

That seems like an unrelated change?

There's an extra blank line there.

+	if (IS_ERR(pb->power_reg))
+		return PTR_ERR(pb->power_reg);
+
+	pb->enable_gpio = -EINVAL;
+	if (data->use_enable_gpio) {
+		ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
+				GPIOF_OUT_INIT_HIGH, "backlight_enable");
+		if (ret)
+			dev_warn(&pdev->dev,
+				"error %d requesting control gpio\n", ret);

Shouldn't that be a hard error? If the user specified that some GPIO be
used, and the GPIO could not be requested, shouldn't the driver fail to
initialize?

+		else
+			pb->enable_gpio = data->enable_gpio;

Aside from that, this looks good to me.

On 06/30/2012 01:04 AM, Stephen Warren wrote:
 >> -	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 >> -
 >
 > That seems like an unrelated change?

Dammit, I hoped that would have gone unnoticed. ;)

 >> +	pb->enable_gpio = -EINVAL;
 >> +	if (data->use_enable_gpio) {
 >> +		ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
 >> +				GPIOF_OUT_INIT_HIGH, "backlight_enable");
 >> +		if (ret)
 >> +			dev_warn(&pdev->dev,
 >> +				"error %d requesting control gpio\n", ret);
 >
 > Shouldn't that be a hard error? If the user specified that some GPIO be
 > used, and the GPIO could not be requested, shouldn't the driver fail to
 > initialize?

Yes, you are right.

Thanks,
Alex.

On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote:

Add support for an optional power regulator and enable/disable GPIO.
This scheme is commonly used in embedded systems.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

I've added some comments in addition to those by Stephen.

[...]

[...]

Can't you just reuse the value of ret here?

[...]

Perhaps initialize this to -1? Assigning standard error codes to a GPIO
doesn't make much sense.

[...]

It's a shame we have to add workarounds like this...

Also the canonical form to write multi-line comments would be:

	/*
	 * 0 (default ...
	 * ... surprises.
	 */

Thierry

On 07/01/2012 03:37 AM, Thierry Reding wrote:>> +	ret = 
of_get_named_gpio(node, "enable-gpios", 0);
 >> +	if (ret >= 0) {
 >> +		data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
 >
 > Can't you just reuse the value of ret here?

Yes, definitely.

 >> +	pb->enable_gpio = -EINVAL;
 >
 > Perhaps initialize this to -1? Assigning standard error codes to a GPIO
 > doesn't make much sense.

Documentation/gpio.txt states the following:

"If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL"); that will never be valid."

gpio_is_valid() seems to be happy with any negative value, but -EINVAL 
seems to be a convention here.

 >> +	/* optional GPIO that enables/disables the backlight */
 >> +	int enable_gpio;
 >> +	/* 0 (default initialization value) is a valid GPIO number. Make 
use of
 >> +	 * control gpio explicit to avoid bad surprises. */
 >> +	bool use_enable_gpio;
 >
 > It's a shame we have to add workarounds like this...

Yeah, I hate that too. :/ I see nothing better to do unfortunately.

Other remarks from Stephen made me realize that this patch has two major 
flaws:

1) The GPIO and regulator are fixed, optional entites ; this should 
cover most cases but is not very flexible.
2) Some (most?) backlight specify timings between turning power 
on/enabling PWM/enabling backlight. Even the order of things may be 
different. This patch totally ignores that.

So instead of having fixed "power-supply" and "enable-gpio" properties, 
how about having properties describing the power-on and power-off 
sequences which odd cells alternate between phandles to 
regulators/gpios/pwm and delays in microseconds before continuing the 
sequence. For example:

power-on = <&pwm 2 5000000
	    10000
	    &backlight_reg
	    0
	    &gpio 28 0>;
power-off = <&gpio 28 0
	     0
	     &backlight_reg
	     10000
	     &pwm 2 0>;

Here the power-on sequence would translate to, power on the second PWM 
with a duty-cycle of 5ms, wait 10ms, then enable the backlight regulator 
and GPIO 28 without delay. Power-off is the exact opposite. The nice 
thing with this scheme is that you can reorder the sequence at will and 
support the weirdest setups.

What I don't know (device tree newbie here!) is:
1) Is it legal to refer the same phandle several times in the same node?
2) Is it ok to mix phandles of different types with integer values? The 
DT above compiled, but can you e.g. resolve a regulator phandle in the 
middle of a property?
3) Can you "guess" the type of a phandle before parsing it? Here the 
first phandle is a GPIO, but it could as well be the regulator. Do we 
have means to know that in the driver code?

Sorry for the long explanation, but I really wonder if doing this is 
possible at all. If it is, then I think that's the way to do for 
backlight initialization.

Alex.

On Mon, Jul 02, 2012 at 12:35:12PM +0900, Alexandre Courbot wrote:

On 07/01/2012 03:37 AM, Thierry Reding wrote:>> +	ret =
of_get_named_gpio(node, "enable-gpios", 0);

quoted

quoted

+	if (ret >= 0) {
+		data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);

Can't you just reuse the value of ret here?

Yes, definitely.

quoted

quoted

+	pb->enable_gpio = -EINVAL;

Perhaps initialize this to -1? Assigning standard error codes to a GPIO
doesn't make much sense.

Documentation/gpio.txt states the following:

"If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL"); that will never be valid."

gpio_is_valid() seems to be happy with any negative value, but
-EINVAL seems to be a convention here.

I would have thought something like -1 would be good enough to represent
an invalid GPIO, but if there's already a convention then we should
follow it.

quoted

quoted

+	/* optional GPIO that enables/disables the backlight */
+	int enable_gpio;
+	/* 0 (default initialization value) is a valid GPIO number.

Make use of

quoted

quoted

+	 * control gpio explicit to avoid bad surprises. */
+	bool use_enable_gpio;

It's a shame we have to add workarounds like this...

Yeah, I hate that too. :/ I see nothing better to do unfortunately.

Other remarks from Stephen made me realize that this patch has two
major flaws:

1) The GPIO and regulator are fixed, optional entites ; this should
cover most cases but is not very flexible.
2) Some (most?) backlight specify timings between turning power
on/enabling PWM/enabling backlight. Even the order of things may be
different. This patch totally ignores that.

So instead of having fixed "power-supply" and "enable-gpio"
properties, how about having properties describing the power-on and
power-off sequences which odd cells alternate between phandles to
regulators/gpios/pwm and delays in microseconds before continuing
the sequence. For example:

power-on = <&pwm 2 5000000
	    10000
	    &backlight_reg
	    0
	    &gpio 28 0>;
power-off = <&gpio 28 0
	     0
	     &backlight_reg
	     10000
	     &pwm 2 0>;

Here the power-on sequence would translate to, power on the second
PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight
regulator and GPIO 28 without delay. Power-off is the exact
opposite. The nice thing with this scheme is that you can reorder
the sequence at will and support the weirdest setups.

I generally like the idea. I'm Cc'ing the devicetree-discuss mailing
list, let's see what people there think about it. I've also added Mitch
Bradley who already helped a lot with the initial binding.

What I don't know (device tree newbie here!) is:
1) Is it legal to refer the same phandle several times in the same node?
2) Is it ok to mix phandles of different types with integer values?
The DT above compiled, but can you e.g. resolve a regulator phandle
in the middle of a property?

I believe these shouldn't be a problem.

3) Can you "guess" the type of a phandle before parsing it? Here the
first phandle is a GPIO, but it could as well be the regulator. Do
we have means to know that in the driver code?

That isn't possible. But you could for instance use a cell to specify
the type of the following phandle.

Sorry for the long explanation, but I really wonder if doing this is
possible at all. If it is, then I think that's the way to do for
backlight initialization.

OTOH we basically end up describing a code sequence in the DT. But as
you mention above sometimes the hardware requires specific timing
parameters so this might actually be a kind of valid sequence to
describe in the DT.

Thierry

On 07/02/2012 03:46 PM, Thierry Reding wrote:

quoted

So instead of having fixed "power-supply" and "enable-gpio"
properties, how about having properties describing the power-on and
power-off sequences which odd cells alternate between phandles to
regulators/gpios/pwm and delays in microseconds before continuing
the sequence. For example:

power-on = <&pwm 2 5000000
	    10000
	    &backlight_reg
	    0
	    &gpio 28 0>;
power-off = <&gpio 28 0
	     0
	     &backlight_reg
	     10000
	     &pwm 2 0>;

Here the power-on sequence would translate to, power on the second
PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight
regulator and GPIO 28 without delay. Power-off is the exact
opposite. The nice thing with this scheme is that you can reorder
the sequence at will and support the weirdest setups.

I generally like the idea. I'm Cc'ing the devicetree-discuss mailing
list, let's see what people there think about it. I've also added Mitch
Bradley who already helped a lot with the initial binding.

Cool, thanks. I am aware that this idea probably needs to be refined, 
but ideally we would end up with something as simple as this example.

quoted

What I don't know (device tree newbie here!) is:
1) Is it legal to refer the same phandle several times in the same node?
2) Is it ok to mix phandles of different types with integer values?
The DT above compiled, but can you e.g. resolve a regulator phandle
in the middle of a property?

I believe these shouldn't be a problem.

Nice, I'll try to see how I can parse it then.

quoted

3) Can you "guess" the type of a phandle before parsing it? Here the
first phandle is a GPIO, but it could as well be the regulator. Do
we have means to know that in the driver code?

That isn't possible. But you could for instance use a cell to specify
the type of the following phandle.

That sounds reasonnable, although we would also need to bind values to 
phandle types. That would make the DT a little bit more cryptic, 
although nothing too bad I think.

quoted

Sorry for the long explanation, but I really wonder if doing this is
possible at all. If it is, then I think that's the way to do for
backlight initialization.

OTOH we basically end up describing a code sequence in the DT. But as
you mention above sometimes the hardware requires specific timing
parameters so this might actually be a kind of valid sequence to
describe in the DT.

Yes, the power on/power off sequences are part of the board/panel 
specification - they are a hardware description and thus fits within the 
purpose of the device tree IMHO. One could argue that initialization 
sequences are usually coded inside drivers, but that only works when the 
driver has a single initialization sequence to handle. With 
pwm-backlight we try to handle something much more general, and so far 
these sequences were performed by board-specific functions.

All in all, adding timings & ordering is not so different from the 
original patch which accepted one optional regulator and GPIO.

Also, if someone knows of anything else besides PWM/GPIO/regulators that 
may need to be controlled by a backlight power sequence, please let me know.

Alex.

Hi Alexandre,

On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote:

This looses several resources allocated earlier, like the enable gpio
and the pwm. This is really bad here since I have no regulator specified
and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
tries to probe the driver the driver bails out because the gpio and the
pwm is already requested.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hi Sascha,

On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> +
 >> +	pb->power_reg = devm_regulator_get(&pdev->dev, "power");
 >> +	if (IS_ERR(pb->power_reg))
 >> +		return PTR_ERR(pb->power_reg);
 >
 > This looses several resources allocated earlier, like the enable gpio
 > and the pwm. This is really bad here since I have no regulator specified
 > and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
 > tries to probe the driver the driver bails out because the gpio and the
 > pwm is already requested.

That's very bad indeed. I assumed that the kernel would free 
devm-allocated resources after probe returned -EPROBE_DEFER, so that 
probe could reallocate them the next time it is called. Apparently that 
was wrong. Do you know what would the right approach be in this case? 
Does the kernel preserve the device structure and its associated data 
between the two calls to probe? If so, I could just check whether the 
private data has already been constructed to know which state we are in 
and continue from there.

Thanks,
Alex.

On Wed, Jul 04, 2012 at 09:26:58PM +0900, Alex Courbot wrote:

On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> +

quoted

This looses several resources allocated earlier, like the enable gpio
and the pwm. This is really bad here since I have no regulator specified
and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
tries to probe the driver the driver bails out because the gpio and the
pwm is already requested.

That's very bad indeed. I assumed that the kernel would free
devm-allocated resources after probe returned -EPROBE_DEFER, so that
probe could reallocate them the next time it is called. Apparently
that was wrong. Do you know what would the right approach be in this
case? Does the kernel preserve the device structure and its
associated data between the two calls to probe? If so, I could just
check whether the private data has already been constructed to know
which state we are in and continue from there.

I'd really expect the devm_ functions to behave as you expect - if they
don't currently we should probably fix them so that they do otherwise
we're going to be in for a lot of surprises and probe deferral becomes a
lot more cumbersome to use.

On Wed, Jul 04, 2012 at 09:26:58PM +0900, Alex Courbot wrote:

Hi Sascha,

On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> +

quoted

quoted

+	pb->power_reg = devm_regulator_get(&pdev->dev, "power");
+	if (IS_ERR(pb->power_reg))
+		return PTR_ERR(pb->power_reg);

This looses several resources allocated earlier, like the enable gpio
and the pwm. This is really bad here since I have no regulator specified
and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
tries to probe the driver the driver bails out because the gpio and the
pwm is already requested.

That's very bad indeed. I assumed that the kernel would free
devm-allocated resources after probe returned -EPROBE_DEFER,

It indeed does free devm allocated resources, but neither the gpio nor
the pwm are devm allocated.

Also please be aware that using a regulator in the pwm backlight will
instantly break all existing users. That's hardly your fault though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote:

quoted

That's very bad indeed. I assumed that the kernel would free
devm-allocated resources after probe returned -EPROBE_DEFER,

It indeed does free devm allocated resources, but neither the gpio nor
the pwm are devm allocated.

As far as I can tell the gpio is allocated through devm as well:

 > +		ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
 > +				GPIOF_OUT_INIT_HIGH, "backlight_enable");

Thus if it is not reclaimed with probe returns with -EPROBE_DEFER, then 
I guess something is going wrong elsewhere. You are right that the PWM 
should be freed by the driver thought.

Also please be aware that using a regulator in the pwm backlight will
instantly break all existing users. That's hardly your fault though.

Sorry, I don't see why. Could you elaborate on this?

Thanks,
Alex.

On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote:

On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote:

quoted

Also please be aware that using a regulator in the pwm backlight will
instantly break all existing users. That's hardly your fault though.

Sorry, I don't see why. Could you elaborate on this?

All existing machines will start failing during probe as they won't be
able to find the regulator - you should ideally make sure everyone in
mainline gets an appropriate regulator set up.

On 07/05/2012 12:24 AM, Mark Brown wrote:

On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote:

quoted

On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote:

quoted

quoted

Also please be aware that using a regulator in the pwm backlight will
instantly break all existing users. That's hardly your fault though.

quoted

Sorry, I don't see why. Could you elaborate on this?

All existing machines will start failing during probe as they won't be
able to find the regulator - you should ideally make sure everyone in
mainline gets an appropriate regulator set up.

Oh, that is a mistake of mine then. Driver probe should continue if no 
regulator is declared (but should fail if some other error occured). I 
want to maintain backward compatibility with current users of the 
driver, so regulator/gpio specification should be optional.

Thanks for all the feedback people - I will come back with a new version 
that addresses the points highlighted and also allows power on/off 
sequences to be specified in the device tree.

Alex.

On Thu, Jul 05, 2012 at 11:36:48AM +0900, Alex Courbot wrote:

On 07/05/2012 12:24 AM, Mark Brown wrote:

quoted

On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote:

quoted

On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote:

quoted

quoted

Also please be aware that using a regulator in the pwm backlight will
instantly break all existing users. That's hardly your fault though.

quoted

Sorry, I don't see why. Could you elaborate on this?

All existing machines will start failing during probe as they won't be
able to find the regulator - you should ideally make sure everyone in
mainline gets an appropriate regulator set up.

Oh, that is a mistake of mine then. Driver probe should continue if
no regulator is declared (but should fail if some other error
occured). I want to maintain backward compatibility with current
users of the driver, so regulator/gpio specification should be
optional.

I think the only way doing this is to add a flag to platform_data. I
don't know if that's accepted though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On 07/05/2012 03:20 PM, Sascha Hauer wrote:

quoted

Oh, that is a mistake of mine then. Driver probe should continue if
no regulator is declared (but should fail if some other error
occured). I want to maintain backward compatibility with current
users of the driver, so regulator/gpio specification should be
optional.

I think the only way doing this is to add a flag to platform_data. I
don't know if that's accepted though.

I thought about just checking if devm_get_regulator returned -ENODEV and 
happily continue if that was the case, assuming no regulator was declared.

But anyway with the power sequences specification this problem becomes 
null, since regulators will have to be explicitly declared anyway. I 
might be flamed for putting a parser and interpreter into a backlight 
driver, but I'll take my chances. :)

Alex.

On Thu, Jul 05, 2012 at 03:25:44PM +0900, Alex Courbot wrote:

On 07/05/2012 03:20 PM, Sascha Hauer wrote:

quoted

quoted

Oh, that is a mistake of mine then. Driver probe should continue if
no regulator is declared (but should fail if some other error
occured). I want to maintain backward compatibility with current
users of the driver, so regulator/gpio specification should be
optional.

I think the only way doing this is to add a flag to platform_data. I
don't know if that's accepted though.

I thought about just checking if devm_get_regulator returned -ENODEV
and happily continue if that was the case, assuming no regulator was
declared.

And that's the problem. The get_regulator won't return -ENODEV. It will
return -EPROBE_DEFER which tells you nothing about whether a regulator
will ever be available or not.

Having a flag in platform data would be fine with me, but I know other
people think differently.

BTW in devicetree this flag implicitely exists with the power-supply
property. The regulator core could look if a power-supply property
is given and

- if it is given, a regulator is mandatory and the core either
  returns the regulator or -EPROBE_DEFER if it cannot find one.
- If it is not given, there is no regulator and the core could either
  return a special error code or a dummy regulator.

Right now the regulator core will just return -EPROBE_DEFER in both
cases. This could easily be changed in the regulator core.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On 07/05/2012 03:47 PM, Sascha Hauer wrote:

quoted

I thought about just checking if devm_get_regulator returned -ENODEV
and happily continue if that was the case, assuming no regulator was
declared.

And that's the problem. The get_regulator won't return -ENODEV. It will
return -EPROBE_DEFER which tells you nothing about whether a regulator
will ever be available or not.

Having a flag in platform data would be fine with me, but I know other
people think differently.

BTW in devicetree this flag implicitely exists with the power-supply
property.

One could actually question whether the whole regulator/gpio thing 
should be supported at all with platform data. The platform interface 
can use the function hooks in order to implement whatever behavior it 
wants when the light needs to be powered on and off. The reason for 
introducing optional regulator/gpio parameters is because the DT cannot 
use these. Since I have no plan to remove these function hooks, making 
the regulator/gpio option available in platform data might be redundant. 
Any thought about this?

Right now the regulator core will just return -EPROBE_DEFER in both
cases. This could easily be changed in the regulator core.

Could this be because the regulator core cannot make the difference 
between a not-yet-available regulator and a missing one?

Alex.

On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote:

On 07/05/2012 03:47 PM, Sascha Hauer wrote:

quoted

quoted

I thought about just checking if devm_get_regulator returned -ENODEV
and happily continue if that was the case, assuming no regulator was
declared.

And that's the problem. The get_regulator won't return -ENODEV. It will
return -EPROBE_DEFER which tells you nothing about whether a regulator
will ever be available or not.

Having a flag in platform data would be fine with me, but I know other
people think differently.

BTW in devicetree this flag implicitely exists with the power-supply
property.

One could actually question whether the whole regulator/gpio thing
should be supported at all with platform data. The platform
interface can use the function hooks in order to implement whatever
behavior it wants when the light needs to be powered on and off. The
reason for introducing optional regulator/gpio parameters is because
the DT cannot use these. Since I have no plan to remove these
function hooks, making the regulator/gpio option available in
platform data might be redundant. Any thought about this?

I agree. Non-DT platforms have always used the callbacks to execute this
kind of code. As you've said before there are situations where it isn't
just about setting a GPIO or enabling a regulator but it also requires a
specific timing. Representing this in the platform data would become
tedious.

So I think for the DT case you can parse the power-on and power-off
sequences directly and execute code based on it, while in non-DT cases
the init and exit callbacks should be used instead. I think it even
makes sense to reuse the platform data's init and exit functions in the
DT case and implement the parser/interpreter within those.

quoted

Right now the regulator core will just return -EPROBE_DEFER in both
cases. This could easily be changed in the regulator core.

Could this be because the regulator core cannot make the difference
between a not-yet-available regulator and a missing one?

I case where the regulator comes from a DT it should assume that it will
become available at some point, so -EPROBE_DEFER is correct. However if
the DT doesn't even contain the power-supply property, then EPROBE_DEFER
will never work because there's no regulator to become available.

Thierry

On 07/05/2012 04:57 PM, Thierry Reding wrote:

I agree. Non-DT platforms have always used the callbacks to execute this
kind of code. As you've said before there are situations where it isn't
just about setting a GPIO or enabling a regulator but it also requires a
specific timing. Representing this in the platform data would become
tedious.

That will settle the whole issue then.

So I think for the DT case you can parse the power-on and power-off
sequences directly and execute code based on it, while in non-DT cases
the init and exit callbacks should be used instead. I think it even
makes sense to reuse the platform data's init and exit functions in the
DT case and implement the parser/interpreter within those.

It totally makes sense indeed.

I case where the regulator comes from a DT it should assume that it will
become available at some point, so -EPROBE_DEFER is correct. However if
the DT doesn't even contain the power-supply property, then EPROBE_DEFER
will never work because there's no regulator to become available.

Indeed. And as Sascha mentionned this could easily be fixed. Guess I can 
also submit a patch for that while I am at it.

Alex.

On 07/05/2012 02:12 AM, Alex Courbot wrote:

On 07/05/2012 04:57 PM, Thierry Reding wrote:

quoted

I agree. Non-DT platforms have always used the callbacks to execute this
kind of code. As you've said before there are situations where it isn't
just about setting a GPIO or enabling a regulator but it also requires a
specific timing. Representing this in the platform data would become
tedious.

That will settle the whole issue then.

quoted

So I think for the DT case you can parse the power-on and power-off
sequences directly and execute code based on it, while in non-DT cases
the init and exit callbacks should be used instead. I think it even
makes sense to reuse the platform data's init and exit functions in the
DT case and implement the parser/interpreter within those.

It totally makes sense indeed.

I don't agree here. It'd be best if non-DT and DT cases worked as
similarly as possible. Relying on callbacks in one case and
data-parsed-from-DT in the other isn't consistent with that. After all,
in the DT case, you parse some data out of the DT and into some data
structure. In the non-DT case, you can have that data structure passed
in directly using platform data. Now, there's certainly a need to
continue to support callbacks for backwards compatibility, at the very
least temporarily before all clients are converted to the new model, but
requiring different models rather than simply allowing it seems like a
bad idea to me.

-----Original Message-----
From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-owner@vger.kernel.org] On Behalf Of Stephen
Warren
Sent: Friday, July 06, 2012 1:04 AM
To: Alex Courbot
Cc: Thierry Reding; Sascha Hauer; Mark Brown; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-fbdev@vger.kernel.org
Subject: Re: [PATCH] pwm-backlight: add regulator and GPIO support

On 07/05/2012 02:12 AM, Alex Courbot wrote:

quoted

On 07/05/2012 04:57 PM, Thierry Reding wrote:

quoted

I agree. Non-DT platforms have always used the callbacks to execute this
kind of code. As you've said before there are situations where it isn't
just about setting a GPIO or enabling a regulator but it also requires a
specific timing. Representing this in the platform data would become
tedious.

That will settle the whole issue then.

quoted

So I think for the DT case you can parse the power-on and power-off
sequences directly and execute code based on it, while in non-DT cases
the init and exit callbacks should be used instead. I think it even
makes sense to reuse the platform data's init and exit functions in the
DT case and implement the parser/interpreter within those.

It totally makes sense indeed.

I don't agree here. It'd be best if non-DT and DT cases worked as
similarly as possible. Relying on callbacks in one case and
data-parsed-from-DT in the other isn't consistent with that. After all,
in the DT case, you parse some data out of the DT and into some data
structure. In the non-DT case, you can have that data structure passed
in directly using platform data. Now, there's certainly a need to
continue to support callbacks for backwards compatibility, at the very
least temporarily before all clients are converted to the new model, but
requiring different models rather than simply allowing it seems like a
bad idea to me.

Hi Alex Courbot,

I couldn't agree with Stephen Warren more.
Could you support DT and non-DT case for backwards compatibility?

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 07/09/2012 02:19 PM, Jingoo Han wrote:

I couldn't agree with Stephen Warren more.
Could you support DT and non-DT case for backwards compatibility?

Both cases are handled in the new version I just sent. I hope all other 
concerns have also been addressed properly. If I forgot something please 
ping me.

Alex.

On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote:

On 07/05/2012 03:47 PM, Sascha Hauer wrote:

quoted

quoted

I thought about just checking if devm_get_regulator returned -ENODEV
and happily continue if that was the case, assuming no regulator was
declared.

And that's the problem. The get_regulator won't return -ENODEV. It will
return -EPROBE_DEFER which tells you nothing about whether a regulator
will ever be available or not.

Having a flag in platform data would be fine with me, but I know other
people think differently.

BTW in devicetree this flag implicitely exists with the power-supply
property.

One could actually question whether the whole regulator/gpio thing
should be supported at all with platform data. The platform
interface can use the function hooks in order to implement whatever
behavior it wants when the light needs to be powered on and off. The
reason for introducing optional regulator/gpio parameters is because
the DT cannot use these. Since I have no plan to remove these
function hooks, making the regulator/gpio option available in
platform data might be redundant. Any thought about this?

sounds good.

quoted

Right now the regulator core will just return -EPROBE_DEFER in both
cases. This could easily be changed in the regulator core.

Could this be because the regulator core cannot make the difference
between a not-yet-available regulator and a missing one?

It could. In regulator_dev_lookup we have:

		if (node) {
			...
		} else {
			/*
			 * If we couldn't even get the node then it's
			 * not just that the device didn't register
			 * yet, there's no node and we'll never
			 * succeed.
			 */
			*ret = -ENODEV;
		}

So here the regulator core knows that there is no regulator and never
will be. All that needs to be done is to make _regulator_get look at
that value.
There may be some side effects if we just return ERR_PTR(-ENODEV) when
regulator_dev_lookup returns -ENODEV. Maybe Mark has some comments to
this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Thu, Jul 05, 2012 at 10:02:34AM +0200, Sascha Hauer wrote:

So here the regulator core knows that there is no regulator and never
will be. All that needs to be done is to make _regulator_get look at
that value.

There may be some side effects if we just return ERR_PTR(-ENODEV) when
regulator_dev_lookup returns -ENODEV. Maybe Mark has some comments to
this.

I'm concerned about how this is going to interact with all the plans
people have for dynamically loading device tree fragments during
enumeration.

On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote:

One could actually question whether the whole regulator/gpio thing
should be supported at all with platform data. The platform
interface can use the function hooks in order to implement whatever
behavior it wants when the light needs to be powered on and off. The
reason for introducing optional regulator/gpio parameters is because
the DT cannot use these. Since I have no plan to remove these
function hooks, making the regulator/gpio option available in
platform data might be redundant. Any thought about this?

Well, no - it's also done because even if you're not using device tree
(as on most of the architectures we support...) it's not good to have to
cut'n'paste code everywhere.  This means that we want to be able to
provide things like GPIOs and regulators via data which means we have
exactly the same situation as we do with device tree.

quoted

Right now the regulator core will just return -EPROBE_DEFER in both
cases. This could easily be changed in the regulator core.

Could this be because the regulator core cannot make the difference
between a not-yet-available regulator and a missing one?

Yes.

On Thu, Jul 05, 2012 at 03:25:44PM +0900, Alex Courbot wrote:

On 07/05/2012 03:20 PM, Sascha Hauer wrote:

quoted

I think the only way doing this is to add a flag to platform_data. I
don't know if that's accepted though.

I thought about just checking if devm_get_regulator returned -ENODEV
and happily continue if that was the case, assuming no regulator was
declared.

No, that's really not a good idea - as I keep saying if we really want
to go down that line we should remove all error checking instead, it's
the end result.

On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote:

On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote:

quoted

quoted

That's very bad indeed. I assumed that the kernel would free
devm-allocated resources after probe returned -EPROBE_DEFER,

It indeed does free devm allocated resources, but neither the gpio nor
the pwm are devm allocated.

As far as I can tell the gpio is allocated through devm as well:

quoted

+		ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
+				GPIOF_OUT_INIT_HIGH, "backlight_enable");

You're right. For the GPIO it's ok the way it is.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |