2014-10-06 21:22:42

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt: bindings: tps65217: add compatible property for subnodes

On Thu, 25 Sep 2014, Johannes Pointner wrote:

> Add documentation for compatible property of subnodes.
>
> Signed-off-by: Johannes Pointner <[email protected]>
> ---
> Documentation/devicetree/bindings/regulator/tps65217.txt | 7 ++++++-
> .../devicetree/bindings/video/backlight/tps65217-backlight.txt | 9 ++++++---
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> index 4f05d20..8ae3a3b 100644
> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> @@ -14,6 +14,9 @@ Required properties:
> Optional properties:
> - ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
>
> +Required subnode properties:
> +- compatible: "ti,tps65217-pmic"
> +
> The valid names for regulators are:
> tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
>
> @@ -23,9 +26,11 @@ Example:
>
> tps: tps@24 {
> compatible = "ti,tps65217";
> + reg = <0x24>;
> ti,pmic-shutdown-controller;
>
> - regulators {
> + tps_pmic: regulators {

How is the tps_pmic label used?

> + compatible = "ti,tps65217-pmic";

I'd like to see a '\n' here.

> dcdc1_reg: dcdc1 {
> regulator-min-microvolt = <900000>;
> regulator-max-microvolt = <1800000>;
> diff --git a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> index 5fb9279..f92f154 100644
> --- a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> @@ -6,19 +6,22 @@ used to drive LEDs for use as backlights.
> Required properties:
> - compatible: "ti,tps65217"
> - reg: I2C slave address
> -- backlight: node for specifying WLED1 and WLED2 lines in TPS65217
> +- backlight: subnode for specifying WLED1 and WLED2 lines in TPS65217
> +
> +Required subnode properties:
> +- compatible: "ti,tps65217-bl"
> - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
> - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
> - default-brightness: valid values: 0-100
>
> -Each regulator is defined using the standard binding for regulators.
>
> Example:
>
> tps: tps@24 {
> reg = <0x24>;
> compatible = "ti,tps65217";

I'd like to see a '\n' here.

> - backlight {
> + tps_bl: backlight {

Same here. Is this label used?

> + compatible = "ti,tps65217-bl";
> isel = <1>; /* 1 - ISET1, 2 ISET2 */
> fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
> default-brightness = <50>;

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


2014-10-08 09:14:55

by Johannes Pointner

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt: bindings: tps65217: add compatible property for subnodes

2014-10-06 23:22 GMT+02:00 Lee Jones <[email protected]>:
> On Thu, 25 Sep 2014, Johannes Pointner wrote:
>
>> Add documentation for compatible property of subnodes.
>>
>> Signed-off-by: Johannes Pointner <[email protected]>
>> ---
>> Documentation/devicetree/bindings/regulator/tps65217.txt | 7 ++++++-
>> .../devicetree/bindings/video/backlight/tps65217-backlight.txt | 9 ++++++---
>> 2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> index 4f05d20..8ae3a3b 100644
>> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
>> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> @@ -14,6 +14,9 @@ Required properties:
>> Optional properties:
>> - ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
>>
>> +Required subnode properties:
>> +- compatible: "ti,tps65217-pmic"
>> +
>> The valid names for regulators are:
>> tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
>>
>> @@ -23,9 +26,11 @@ Example:
>>
>> tps: tps@24 {
>> compatible = "ti,tps65217";
>> + reg = <0x24>;
>> ti,pmic-shutdown-controller;
>>
>> - regulators {
>> + tps_pmic: regulators {
>
> How is the tps_pmic label used?
>
>> + compatible = "ti,tps65217-pmic";
>
> I'd like to see a '\n' here.
I will add a newline here.
>
>> dcdc1_reg: dcdc1 {
>> regulator-min-microvolt = <900000>;
>> regulator-max-microvolt = <1800000>;
>> diff --git a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> index 5fb9279..f92f154 100644
>> --- a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> +++ b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> @@ -6,19 +6,22 @@ used to drive LEDs for use as backlights.
>> Required properties:
>> - compatible: "ti,tps65217"
>> - reg: I2C slave address
>> -- backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>> +- backlight: subnode for specifying WLED1 and WLED2 lines in TPS65217
>> +
>> +Required subnode properties:
>> +- compatible: "ti,tps65217-bl"
>> - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
>> - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
>> - default-brightness: valid values: 0-100
>>
>> -Each regulator is defined using the standard binding for regulators.
>>
>> Example:
>>
>> tps: tps@24 {
>> reg = <0x24>;
>> compatible = "ti,tps65217";
>
> I'd like to see a '\n' here.
I will add a newline here.
>
>> - backlight {
>> + tps_bl: backlight {
>
> Same here. Is this label used?
>
>> + compatible = "ti,tps65217-bl";
>> isel = <1>; /* 1 - ISET1, 2 ISET2 */
>> fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> default-brightness = <50>;
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Regarding the labels: The backlight label could be used like the
following. I use it like this in my tests with the tilcdc panel driver
which has a backlight support.

panel {
backlight = <&tps_bl>;

panel-info {
ac-bias = <255>;
ac-bias-intrpt = <0>;
dma-burst-sz = <16>;
bpp = <32>;
fdd = <0x80>;
sync-edge = <0>;
sync-ctrl = <1>;
.
.
.

I'm not sure if there could be actual use for the pmic label, but I
thought if I add a label to backlight, I have to add one to the pmic
too to keep it similar.

2014-10-08 10:47:27

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt: bindings: tps65217: add compatible property for subnodes

On Wed, 08 Oct 2014, Johannes Pointner wrote:

> 2014-10-06 23:22 GMT+02:00 Lee Jones <[email protected]>:
> > On Thu, 25 Sep 2014, Johannes Pointner wrote:
> >
> >> Add documentation for compatible property of subnodes.
> >>
> >> Signed-off-by: Johannes Pointner <[email protected]>
> >> ---
> >> Documentation/devicetree/bindings/regulator/tps65217.txt | 7 ++++++-
> >> .../devicetree/bindings/video/backlight/tps65217-backlight.txt | 9 ++++++---
> >> 2 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> index 4f05d20..8ae3a3b 100644
> >> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> @@ -14,6 +14,9 @@ Required properties:
> >> Optional properties:
> >> - ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> >>
> >> +Required subnode properties:
> >> +- compatible: "ti,tps65217-pmic"
> >> +
> >> The valid names for regulators are:
> >> tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
> >>
> >> @@ -23,9 +26,11 @@ Example:
> >>
> >> tps: tps@24 {
> >> compatible = "ti,tps65217";
> >> + reg = <0x24>;
> >> ti,pmic-shutdown-controller;
> >>
> >> - regulators {
> >> + tps_pmic: regulators {
> >
> > How is the tps_pmic label used?
> >
> >> + compatible = "ti,tps65217-pmic";
> >
> > I'd like to see a '\n' here.
> I will add a newline here.
> >
> >> dcdc1_reg: dcdc1 {
> >> regulator-min-microvolt = <900000>;
> >> regulator-max-microvolt = <1800000>;
> >> diff --git a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> index 5fb9279..f92f154 100644
> >> --- a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> +++ b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> @@ -6,19 +6,22 @@ used to drive LEDs for use as backlights.
> >> Required properties:
> >> - compatible: "ti,tps65217"
> >> - reg: I2C slave address
> >> -- backlight: node for specifying WLED1 and WLED2 lines in TPS65217
> >> +- backlight: subnode for specifying WLED1 and WLED2 lines in TPS65217
> >> +
> >> +Required subnode properties:
> >> +- compatible: "ti,tps65217-bl"
> >> - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
> >> - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
> >> - default-brightness: valid values: 0-100
> >>
> >> -Each regulator is defined using the standard binding for regulators.
> >>
> >> Example:
> >>
> >> tps: tps@24 {
> >> reg = <0x24>;
> >> compatible = "ti,tps65217";
> >
> > I'd like to see a '\n' here.
> I will add a newline here.
> >
> >> - backlight {
> >> + tps_bl: backlight {
> >
> > Same here. Is this label used?
> >
> >> + compatible = "ti,tps65217-bl";
> >> isel = <1>; /* 1 - ISET1, 2 ISET2 */
> >> fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
> >> default-brightness = <50>;
> >
>
> Regarding the labels: The backlight label could be used like the
> following. I use it like this in my tests with the tilcdc panel driver
> which has a backlight support.
>
> panel {
> backlight = <&tps_bl>;
>
> panel-info {
> ac-bias = <255>;
> ac-bias-intrpt = <0>;
> dma-burst-sz = <16>;
> bpp = <32>;
> fdd = <0x80>;
> sync-edge = <0>;
> sync-ctrl = <1>;
> .
> .
> .
>
> I'm not sure if there could be actual use for the pmic label, but I
> thought if I add a label to backlight, I have to add one to the pmic
> too to keep it similar.

No, not at all. If it's not used, you should remove it.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2014-10-08 10:59:56

by Johannes Pointner

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt: bindings: tps65217: add compatible property for subnodes

2014-10-08 12:47 GMT+02:00 Lee Jones <[email protected]>:
> On Wed, 08 Oct 2014, Johannes Pointner wrote:
>
>> 2014-10-06 23:22 GMT+02:00 Lee Jones <[email protected]>:
>> > On Thu, 25 Sep 2014, Johannes Pointner wrote:
>> >
>> >> Add documentation for compatible property of subnodes.
>> >>
>> >> Signed-off-by: Johannes Pointner <[email protected]>
>> >> ---
>> >> Documentation/devicetree/bindings/regulator/tps65217.txt | 7 ++++++-
>> >> .../devicetree/bindings/video/backlight/tps65217-backlight.txt | 9 ++++++---
>> >> 2 files changed, 12 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> >> index 4f05d20..8ae3a3b 100644
>> >> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
>> >> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> >> @@ -14,6 +14,9 @@ Required properties:
>> >> Optional properties:
>> >> - ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
>> >>
>> >> +Required subnode properties:
>> >> +- compatible: "ti,tps65217-pmic"
>> >> +
>> >> The valid names for regulators are:
>> >> tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
>> >>
>> >> @@ -23,9 +26,11 @@ Example:
>> >>
>> >> tps: tps@24 {
>> >> compatible = "ti,tps65217";
>> >> + reg = <0x24>;
>> >> ti,pmic-shutdown-controller;
>> >>
>> >> - regulators {
>> >> + tps_pmic: regulators {
>> >
>> > How is the tps_pmic label used?
>> >
>> >> + compatible = "ti,tps65217-pmic";
>> >
>> > I'd like to see a '\n' here.
>> I will add a newline here.
>> >
>> >> dcdc1_reg: dcdc1 {
>> >> regulator-min-microvolt = <900000>;
>> >> regulator-max-microvolt = <1800000>;
>> >> diff --git a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> >> index 5fb9279..f92f154 100644
>> >> --- a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> >> +++ b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
>> >> @@ -6,19 +6,22 @@ used to drive LEDs for use as backlights.
>> >> Required properties:
>> >> - compatible: "ti,tps65217"
>> >> - reg: I2C slave address
>> >> -- backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>> >> +- backlight: subnode for specifying WLED1 and WLED2 lines in TPS65217
>> >> +
>> >> +Required subnode properties:
>> >> +- compatible: "ti,tps65217-bl"
>> >> - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
>> >> - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
>> >> - default-brightness: valid values: 0-100
>> >>
>> >> -Each regulator is defined using the standard binding for regulators.
>> >>
>> >> Example:
>> >>
>> >> tps: tps@24 {
>> >> reg = <0x24>;
>> >> compatible = "ti,tps65217";
>> >
>> > I'd like to see a '\n' here.
>> I will add a newline here.
>> >
>> >> - backlight {
>> >> + tps_bl: backlight {
>> >
>> > Same here. Is this label used?
>> >
>> >> + compatible = "ti,tps65217-bl";
>> >> isel = <1>; /* 1 - ISET1, 2 ISET2 */
>> >> fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> >> default-brightness = <50>;
>> >
>>
>> Regarding the labels: The backlight label could be used like the
>> following. I use it like this in my tests with the tilcdc panel driver
>> which has a backlight support.
>>
>> panel {
>> backlight = <&tps_bl>;
>>
>> panel-info {
>> ac-bias = <255>;
>> ac-bias-intrpt = <0>;
>> dma-burst-sz = <16>;
>> bpp = <32>;
>> fdd = <0x80>;
>> sync-edge = <0>;
>> sync-ctrl = <1>;
>> .
>> .
>> .
>>
>> I'm not sure if there could be actual use for the pmic label, but I
>> thought if I add a label to backlight, I have to add one to the pmic
>> too to keep it similar.
>
> No, not at all. If it's not used, you should remove it.

OK, my mistake, sorry.
I'll remove the pmic part of the patch and add the newline to the
backlight documentation and resend the patch, if this is OK for you?
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

2014-10-08 13:30:36

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt: bindings: tps65217: add compatible property for subnodes

On Wed, 08 Oct 2014, Johannes Pointner wrote:

> 2014-10-08 12:47 GMT+02:00 Lee Jones <[email protected]>:
> > On Wed, 08 Oct 2014, Johannes Pointner wrote:
> >
> >> 2014-10-06 23:22 GMT+02:00 Lee Jones <[email protected]>:
> >> > On Thu, 25 Sep 2014, Johannes Pointner wrote:
> >> >
> >> >> Add documentation for compatible property of subnodes.
> >> >>
> >> >> Signed-off-by: Johannes Pointner <[email protected]>
> >> >> ---
> >> >> Documentation/devicetree/bindings/regulator/tps65217.txt | 7 ++++++-
> >> >> .../devicetree/bindings/video/backlight/tps65217-backlight.txt | 9 ++++++---
> >> >> 2 files changed, 12 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> >> index 4f05d20..8ae3a3b 100644
> >> >> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> >> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> >> >> @@ -14,6 +14,9 @@ Required properties:
> >> >> Optional properties:
> >> >> - ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> >> >>
> >> >> +Required subnode properties:
> >> >> +- compatible: "ti,tps65217-pmic"
> >> >> +
> >> >> The valid names for regulators are:
> >> >> tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
> >> >>
> >> >> @@ -23,9 +26,11 @@ Example:
> >> >>
> >> >> tps: tps@24 {
> >> >> compatible = "ti,tps65217";
> >> >> + reg = <0x24>;
> >> >> ti,pmic-shutdown-controller;
> >> >>
> >> >> - regulators {
> >> >> + tps_pmic: regulators {
> >> >
> >> > How is the tps_pmic label used?
> >> >
> >> >> + compatible = "ti,tps65217-pmic";
> >> >
> >> > I'd like to see a '\n' here.
> >> I will add a newline here.
> >> >
> >> >> dcdc1_reg: dcdc1 {
> >> >> regulator-min-microvolt = <900000>;
> >> >> regulator-max-microvolt = <1800000>;
> >> >> diff --git a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> >> index 5fb9279..f92f154 100644
> >> >> --- a/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> >> +++ b/Documentation/devicetree/bindings/video/backlight/tps65217-backlight.txt
> >> >> @@ -6,19 +6,22 @@ used to drive LEDs for use as backlights.
> >> >> Required properties:
> >> >> - compatible: "ti,tps65217"
> >> >> - reg: I2C slave address
> >> >> -- backlight: node for specifying WLED1 and WLED2 lines in TPS65217
> >> >> +- backlight: subnode for specifying WLED1 and WLED2 lines in TPS65217
> >> >> +
> >> >> +Required subnode properties:
> >> >> +- compatible: "ti,tps65217-bl"
> >> >> - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
> >> >> - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
> >> >> - default-brightness: valid values: 0-100
> >> >>
> >> >> -Each regulator is defined using the standard binding for regulators.
> >> >>
> >> >> Example:
> >> >>
> >> >> tps: tps@24 {
> >> >> reg = <0x24>;
> >> >> compatible = "ti,tps65217";
> >> >
> >> > I'd like to see a '\n' here.
> >> I will add a newline here.
> >> >
> >> >> - backlight {
> >> >> + tps_bl: backlight {
> >> >
> >> > Same here. Is this label used?
> >> >
> >> >> + compatible = "ti,tps65217-bl";
> >> >> isel = <1>; /* 1 - ISET1, 2 ISET2 */
> >> >> fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
> >> >> default-brightness = <50>;
> >> >
> >>
> >> Regarding the labels: The backlight label could be used like the
> >> following. I use it like this in my tests with the tilcdc panel driver
> >> which has a backlight support.
> >>
> >> panel {
> >> backlight = <&tps_bl>;
> >>
> >> panel-info {
> >> ac-bias = <255>;
> >> ac-bias-intrpt = <0>;
> >> dma-burst-sz = <16>;
> >> bpp = <32>;
> >> fdd = <0x80>;
> >> sync-edge = <0>;
> >> sync-ctrl = <1>;
> >> .
> >> .
> >> .
> >>
> >> I'm not sure if there could be actual use for the pmic label, but I
> >> thought if I add a label to backlight, I have to add one to the pmic
> >> too to keep it similar.
> >
> > No, not at all. If it's not used, you should remove it.
>
> OK, my mistake, sorry.
> I'll remove the pmic part of the patch and add the newline to the
> backlight documentation and resend the patch, if this is OK for you?

Yes, fine.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog