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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPnjgZ2=Ga2P-40trA_=UABm=i7Uy4NTAM=ehea11mE22VfDew@mail.gmail.com>
Date:	Thu, 12 Jul 2012 16:27:25 +0200
From:	Simon Glass <sjg@...omium.org>
To:	Alex Courbot <acourbot@...dia.com>
Cc:	Thierry Reding <thierry.reding@...onic-design.de>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>
Subject: Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes

Hi Alex,

On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot@...dia.com> wrote:
> Hi Simon,
>
>
> On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
>>
>> 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.
>
>
> Cool, I'd love to see that used in U-Boot as well.
>
>
>>> +               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.
>
>
> Yes, these are strings we are looking up to know the type of the next
> element to power on/off in the sequence. I don't like these, honestly, and
> would rather have them replaced by constants - however there is no way to
> define constants in the DT AFAIK (but I have heard some other persons are
> interested in having them too), and this is the only way I have found to
> keep the sequence readable.

That might be the 100th time that I have heard this. I have brought a
patch from Stephen Warren into our tree to solve this locally, but it
is not merged upstream.

>
>
>> 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 see a few problems with this: first, how do you know the types of your
> phandles? At step 0, we should get a regulator, step 1 is a PWM and step 2
> is a GPIO. This is why I have used strings to identify the type of the
> phandle and the number (and type) of additional arguments to parse for a
> step.

Well it's similar to giving them names - you would need to look up the
phandle and see its compatible string or which driver type owns it.
Maybe too complicated, and no such infrastructure exists, so:

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

>
> Second, I am afraid the representation in memory would be significantly
> bigger since the properties names would have to be stored as well (I am no
> DT expert, please correct me if I am wrong). Lastly, the additional freedom
> of having extra properties might make the parser more complicated.

The property names are only stored one, in the string table. I am not
sure about parsing complexity, but it might actually be easier.

>
> I agree the type strings are a problem in the current form - if we could get
> constants in the device tree, that would be much better. Your way of
> representing the sequences is interesting though, if we can solve the type
> issue (and also evaluate its cost in terms of memory footprint), it would be
> interesting to consider it as well.

At a guess:

>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;

About 106 bytes I think

>>     step@0 { 16
            type = "regulator"; 24
>>        phandle = <&backlight_reg>; 16
>>        enable = <1>; 16
>>        post-delay = <10>; 16
>>     }
>>     step@1 { 16
            type = "pwm"; 16
>>        phandle = <&pwm 2 5000000>; 24
>>     }
>>     step@2 { 16
            type = "gpio"; 20
>>        phandle = <&gpio 28 0>; 24
>>        enable = <1>; 16
>>     }

220?

>From my understanding mixing strings and numbers in a property is
frowned on though.

>
> Alex.

Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/