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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
