| Message ID | 1341814105-20690-4-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alex, On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot@nvidia.com> wrote: > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > 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? > + 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>; } > + power-off-sequence = "GPIO", "enable", <0>, > + "PWM", "backlight", <0>, > + "DELAY", <10>, > + "REGULATOR", "power", <0>; > + }; > + > + backlight_reg: fixedregulator@176 { > + compatible = "regulator-fixed"; > + regulator-name = "backlight_regulator"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + gpio = <&gpio 176 0>; > + startup-delay-us = <0>; > + enable-active-high; > + regulator-boot-off; > + }; > + > sound { > compatible = "nvidia,tegra-audio-wm8903-ventana", > "nvidia,tegra-audio-wm8903"; > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi > index 405d167..67a6cd9 100644 > --- a/arch/arm/boot/dts/tegra20.dtsi > +++ b/arch/arm/boot/dts/tegra20.dtsi > @@ -123,7 +123,7 @@ > status = "disabled"; > }; > > - pwm { > + pwm: pwm { > compatible = "nvidia,tegra20-pwm"; > reg = <0x7000a000 0x100>; > #pwm-cells = <2>; > -- > 1.7.11.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> wrote: > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > --- > > 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
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. > 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. 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. 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. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alex, On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot@nvidia.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-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/12/2012 11:27 PM, Simon Glass wrote >> 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? I compiled both versions to try it out. Your version was just 50 bytes larger than mine (I assumed that with yours, we would be able to remove the top-level pwm/regulator/gpio definitions that are referred by the sequence). The question here is do we want to have something more DT-ish, or are we trying to save every possible byte in the DT structure? As Thierry also mentionned, we are trying to provide the same feature using the platform interface. I am not sure how we can elegantly support both ways through this. > From my understanding mixing strings and numbers in a property is > frowned on though. But doesn't it make sense in the current case? The power sequence is basically a program that is run by an interpreter. From this perspective, it makes more sense to me to have it as a binary field rather than a hierarchy of nodes and properties that will be harder to parse and will make error detection more complicated. I don't really see any practical benefit from turning the steps into sub-nodes, but then again I am not so familiar with the DT. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/12/2012 11:32 PM, Alex Courbot wrote: > On 07/12/2012 11:27 PM, Simon Glass wrote ... >> From my understanding mixing strings and numbers in a property is >> frowned on though. > > But doesn't it make sense in the current case? The power sequence is > basically a program that is run by an interpreter. From this > perspective, it makes more sense to me to have it as a binary field > rather than a hierarchy of nodes and properties that will be harder to > parse and will make error detection more complicated. I don't really see > any practical benefit from turning the steps into sub-nodes, but then > again I am not so familiar with the DT. Mixing strings and integers in a property isn't "allowed" (by convention of DT bindings reviewers - as you noticed, dtc will happily compile it); there are practical issues with attempting to do so, such as causing the integer values to be unaligned, and thus causing the current integer parsing code to fail, etc. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>; }; + backlight { + compatible = "pwm-backlight"; + brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>; + 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>; + power-off-sequence = "GPIO", "enable", <0>, + "PWM", "backlight", <0>, + "DELAY", <10>, + "REGULATOR", "power", <0>; + }; + + backlight_reg: fixedregulator@176 { + compatible = "regulator-fixed"; + regulator-name = "backlight_regulator"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + gpio = <&gpio 176 0>; + startup-delay-us = <0>; + enable-active-high; + regulator-boot-off; + }; + sound { compatible = "nvidia,tegra-audio-wm8903-ventana", "nvidia,tegra-audio-wm8903"; diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi index 405d167..67a6cd9 100644 --- a/arch/arm/boot/dts/tegra20.dtsi +++ b/arch/arm/boot/dts/tegra20.dtsi @@ -123,7 +123,7 @@ status = "disabled"; }; - pwm { + pwm: pwm { compatible = "nvidia,tegra20-pwm"; reg = <0x7000a000 0x100>; #pwm-cells = <2>;
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++ arch/arm/boot/dts/tegra20.dtsi | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-)