On Thu, Jul 12, 2012 at 11:37:33AM +0200, Simon Glass wrote:

Hi Alex,

On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot [off-list ref] wrote:

quoted

Signed-off-by: Alexandre Courbot <redacted>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi        |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..c67d9e1 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts

@@ -317,6 +317,37 @@
                bus-width = <8>;
        };

I would like to do something similar in U-Boot for Tegra, although
perhaps not right away. For now I will go with something considerably
simpler! But if this is merged into the kernel we will move to it in
U-Boot. Anyway here are my comments:

quoted

+       backlight {
+               compatible = "pwm-backlight";
+               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;

We seem to have a lot of these - should we move to a range and step size?

These actually seem to be a little bogus. The reason for introducing the
levels was to allow calibration of these values because they in fact
usually do not scale linearly. Instead, a linear brightness increase
usually maps to something like a logarithmic (at best) increase of the
duty cycle.

quoted

+               default-brightness-level = <12>;
+
+               pwms = <&pwm 2 5000000>;
+               pwm-names = "backlight";
+               power-supply = <&backlight_reg>;
+               enable-gpios = <&gpio 28 0>;
+
+               power-on-sequence = "REGULATOR", "power", <1>,
+                                   "DELAY", <10>,
+                                   "PWM", "backlight", <1>,
+                                   "GPIO", "enable", <1>;

So the names REGULATOR, DELAY, etc. here are looked up through some
existing mechanism? In general I am not a big fan of mixing strings
and numbers in a property. Maybe something like:

power_on_sequence {
   step@0 {
      phandle = <&backlight_reg>;
      enable = <1>;
      post-delay = <10>;
   }
   step@1 {
      phandle = <&pwm 2 5000000>;
   }
   step@2 {
      phandle = <&gpio 28 0>;
      enable = <1>;
   }

I actually like that a lot. What's missing from your example is a way to
specify the type of the phandle because it cannot be safely inferred.
Perhaps this could be done by just the property name:

	power_on_sequence {
		step@0 {
			power-supply = <&backlight_reg>;
			enable = <1>;
			post-delay = <10>;
		};
		step@1 {
			pwm = <&pwm 2 5000000>;
		};
		step@2 {
			enable-gpios = <&gpio 28 0>;
			enable = <1>;
		};
	};

However I think one of the reasons for Alex to choose his particular
representation is that people want this to be representable in the
platform data as well, which unfortunately can't deal very well with
this kind of situation.

Usually in the kernel you have one API to obtain a resource (pwm_get(),
regulator_get(), ...) which takes as an argument a device handle and a
name. For device tree, that API will lookup the resource by phandle,
while the non-DT code path typically uses a static lookup table.

It's a shame that we have to keep this kind of backwards compatibility
because it prevents these things from being represented in a logic way
in the DT sense.

Thierry