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

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