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

From: Alexandre Courbot
Date: Sun Jul 01 2012 - 23:34:03 EST

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
&gpio 28 0>;
power-off = <&gpio 28 0
&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.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at