Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes

[Thierry Reding]

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 <acourbot@xxxxxxxxxx> wrote:
> > Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> > ---
> >  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:
> 
> > +       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.

> > +               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

Attachment: pgp00000.pgp
Description: PGP signature