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

[Thierry Reding <thierry.reding@avionic-design.de>]

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

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

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.

> >> +	/* 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.

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

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