[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

When using the board files the LCD works, but not with the DT.
This adds enables the original da850-evm to work with the same
LCD in device tree mode.

The EVM has a gpio for the regulator and a gpio enable.  The LCD and
the vpif display pins are mutually exclusive, so if using the LCD,
do not load the vpif driver.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
     backlight which better matches the schematic.  Updated the description to explain
     that it cannot be used at the same time as the vpif driver.

V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO  

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 2e817da37fdb..3f1c8be07efe 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -27,6 +27,50 @@
 		spi0 = &spi1;
 	};
 
+	backlight {
+		compatible = "gpio-backlight";
+		enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
+	};
+
+	panel {
+		compatible = "ti,tilcdc,panel";
+		pinctrl-names = "default";
+		pinctrl-0 = <&lcd_pins>;
+		status = "okay";
+		enable-gpios = <&gpio 40 GPIO_ACTIVE_HIGH>; /* GP2[8] */
+
+		panel-info {
+			ac-bias		= <255>;
+			ac-bias-intrpt	= <0>;
+			dma-burst-sz	= <16>;
+			bpp		= <16>;
+			fdd		= <0x80>;
+			sync-edge	= <0>;
+			sync-ctrl	= <1>;
+			raster-order	= <0>;
+			fifo-th		= <0>;
+		};
+
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: 480x272 {
+				clock-frequency = <9000000>;
+				hactive = <480>;
+				vactive = <272>;
+				hfront-porch = <3>;
+				hback-porch = <2>;
+				hsync-len = <42>;
+				vback-porch = <3>;
+				vfront-porch = <4>;
+				vsync-len = <11>;
+				hsync-active = <0>;
+				vsync-active = <0>;
+				de-active = <1>;
+				pixelclk-active = <1>;
+			};
+		};
+	};
+
 	vbat: fixedregulator0 {
 		compatible = "regulator-fixed";
 		regulator-name = "vbat";
@@ -35,6 +79,16 @@
 		regulator-boot-on;
 	};
 
+	backlight_reg: backlight-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "lcd_backlight_pwr";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
+		regulator-always-on;
+		enable-active-high;
+	};
+
 	sound {
 		compatible = "simple-audio-card";
 		simple-audio-card,name = "DA850/OMAP-L138 EVM";
@@ -109,6 +163,10 @@
 	status = "okay";
 };
 
+&lcdc {
+	status = "okay";
+};
+
 &i2c0 {
 	status = "okay";
 	clock-frequency = <100000>;
-- 
2.17.0

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

When using the board files the LCD works, but not with the DT.
This adds enables the original da850-evm to work with the same
LCD in device tree mode.

The EVM has a gpio for the regulator and a gpio enable.  The LCD and
the vpif display pins are mutually exclusive, so if using the LCD,
do not load the vpif driver.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
     backlight which better matches the schematic.  Updated the description to explain
     that it cannot be used at the same time as the vpif driver.

V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO  

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 2e817da37fdb..3f1c8be07efe 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -27,6 +27,50 @@
 		spi0 = &spi1;
 	};
 
+	backlight {
+		compatible = "gpio-backlight";
+		enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
+	};
+
+	panel {
+		compatible = "ti,tilcdc,panel";
+		pinctrl-names = "default";
+		pinctrl-0 = <&lcd_pins>;
+		status = "okay";
+		enable-gpios = <&gpio 40 GPIO_ACTIVE_HIGH>; /* GP2[8] */
+
+		panel-info {
+			ac-bias		= <255>;
+			ac-bias-intrpt	= <0>;
+			dma-burst-sz	= <16>;
+			bpp		= <16>;
+			fdd		= <0x80>;
+			sync-edge	= <0>;
+			sync-ctrl	= <1>;
+			raster-order	= <0>;
+			fifo-th		= <0>;
+		};
+
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: 480x272 {
+				clock-frequency = <9000000>;
+				hactive = <480>;
+				vactive = <272>;
+				hfront-porch = <3>;
+				hback-porch = <2>;
+				hsync-len = <42>;
+				vback-porch = <3>;
+				vfront-porch = <4>;
+				vsync-len = <11>;
+				hsync-active = <0>;
+				vsync-active = <0>;
+				de-active = <1>;
+				pixelclk-active = <1>;
+			};
+		};
+	};
+
 	vbat: fixedregulator0 {
 		compatible = "regulator-fixed";
 		regulator-name = "vbat";
@@ -35,6 +79,16 @@
 		regulator-boot-on;
 	};
 
+	backlight_reg: backlight-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "lcd_backlight_pwr";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
+		regulator-always-on;
+		enable-active-high;
+	};
+
 	sound {
 		compatible = "simple-audio-card";
 		simple-audio-card,name = "DA850/OMAP-L138 EVM";
@@ -109,6 +163,10 @@
 	status = "okay";
 };
 
+&lcdc {
+	status = "okay";
+};
+
 &i2c0 {
 	status = "okay";
 	clock-frequency = <100000>;
-- 
2.17.0

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

Hi Adam,

On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
> When using the board files the LCD works, but not with the DT.
> This adds enables the original da850-evm to work with the same
> LCD in device tree mode.
> 
> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
> the vpif display pins are mutually exclusive, so if using the LCD,
> do not load the vpif driver.

Its not sufficient just note this in patch description.

a) Disable (status = "disabled") the VPIF node which clashes for pins
with LCD.
b) Add a comment on top of the status = "disabled" giving information on
how user can enable it (disable lcdc node and then change to status =
"okay").

> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>      backlight which better matches the schematic.  Updated the description to explain
>      that it cannot be used at the same time as the vpif driver.
> 
> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO  
> 
> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
> index 2e817da37fdb..3f1c8be07efe 100644
> --- a/arch/arm/boot/dts/da850-evm.dts
> +++ b/arch/arm/boot/dts/da850-evm.dts
> @@ -27,6 +27,50 @@
>  		spi0 = &spi1;
>  	};
>  
> +	backlight {
> +		compatible = "gpio-backlight";
> +		enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */

The gpio-backlight binding does not describe a property called
enable-gpios. It should just be gpios.

a) Are you using gpio-backlight because you are not able to get the PWM
to work?

b) What is GP0[7] connected to in the schematic you have? In the
schematic I have I see LCD_PWM0 is connected to
SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.

c) The /* GP0[7] */ comment is not really useful on its own as it can be
computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
Same for other places like this below.

> @@ -35,6 +79,16 @@
>  		regulator-boot-on;
>  	};
>  
> +	backlight_reg: backlight-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "lcd_backlight_pwr";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
> +		regulator-always-on;

Why should this regulator never be disabled?

Thanks,
Sekhar

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

Hi Adam,

On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
> When using the board files the LCD works, but not with the DT.
> This adds enables the original da850-evm to work with the same
> LCD in device tree mode.
> 
> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
> the vpif display pins are mutually exclusive, so if using the LCD,
> do not load the vpif driver.

Its not sufficient just note this in patch description.

a) Disable (status = "disabled") the VPIF node which clashes for pins
with LCD.
b) Add a comment on top of the status = "disabled" giving information on
how user can enable it (disable lcdc node and then change to status =
"okay").

> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>      backlight which better matches the schematic.  Updated the description to explain
>      that it cannot be used at the same time as the vpif driver.
> 
> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO  
> 
> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
> index 2e817da37fdb..3f1c8be07efe 100644
> --- a/arch/arm/boot/dts/da850-evm.dts
> +++ b/arch/arm/boot/dts/da850-evm.dts
> @@ -27,6 +27,50 @@
>  		spi0 = &spi1;
>  	};
>  
> +	backlight {
> +		compatible = "gpio-backlight";
> +		enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */

The gpio-backlight binding does not describe a property called
enable-gpios. It should just be gpios.

a) Are you using gpio-backlight because you are not able to get the PWM
to work?

b) What is GP0[7] connected to in the schematic you have? In the
schematic I have I see LCD_PWM0 is connected to
SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.

c) The /* GP0[7] */ comment is not really useful on its own as it can be
computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
Same for other places like this below.

> @@ -35,6 +79,16 @@
>  		regulator-boot-on;
>  	};
>  
> +	backlight_reg: backlight-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "lcd_backlight_pwr";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
> +		regulator-always-on;

Why should this regulator never be disabled?

Thanks,
Sekhar

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> Hi Adam,
>
> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>> When using the board files the LCD works, but not with the DT.
>> This adds enables the original da850-evm to work with the same
>> LCD in device tree mode.
>>
>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>> the vpif display pins are mutually exclusive, so if using the LCD,
>> do not load the vpif driver.
>
> Its not sufficient just note this in patch description.
>
> a) Disable (status = "disabled") the VPIF node which clashes for pins
> with LCD.
> b) Add a comment on top of the status = "disabled" giving information on
> how user can enable it (disable lcdc node and then change to status =
> "okay").
>
>>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>> ---
>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>      backlight which better matches the schematic.  Updated the description to explain
>>      that it cannot be used at the same time as the vpif driver.
>>
>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>
>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>> index 2e817da37fdb..3f1c8be07efe 100644
>> --- a/arch/arm/boot/dts/da850-evm.dts
>> +++ b/arch/arm/boot/dts/da850-evm.dts
>> @@ -27,6 +27,50 @@
>>               spi0 = &spi1;
>>       };
>>
>> +     backlight {
>> +             compatible = "gpio-backlight";
>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>
> The gpio-backlight binding does not describe a property called
> enable-gpios. It should just be gpios.

I will fix that.

>
> a) Are you using gpio-backlight because you are not able to get the PWM
> to work?
>
Yes,  You told me not to worry about doing a PWM backlight because the
legacy board does not PWM either.

> b) What is GP0[7] connected to in the schematic you have? In the
> schematic I have I see LCD_PWM0 is connected to
> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.

I have schematic 1016572 dated Wednesday, August 18, 2010.  According
to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
pre-release versions.

>
> c) The /* GP0[7] */ comment is not really useful on its own as it can be
> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
> Same for other places like this below.

I can do that.
>
>> @@ -35,6 +79,16 @@
>>               regulator-boot-on;
>>       };
>>
>> +     backlight_reg: backlight-regulator {
>> +             compatible = "regulator-fixed";
>> +             regulator-name = "lcd_backlight_pwr";
>> +             regulator-min-microvolt = <3300000>;
>> +             regulator-max-microvolt = <3300000>;
>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>> +             regulator-always-on;
>
> Why should this regulator never be disabled?

The gpio-backlight does not have a way that I can see to associate the
regulator to it.  I read through the bindings, but I didn't see an
option to associate a regulator it.  I use this regulator to drive
lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
this option, the system disables lcd_backlight_pwr and the screen is
blank


adam
>
> Thanks,
> Sekhar

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> Hi Adam,
>
> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>> When using the board files the LCD works, but not with the DT.
>> This adds enables the original da850-evm to work with the same
>> LCD in device tree mode.
>>
>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>> the vpif display pins are mutually exclusive, so if using the LCD,
>> do not load the vpif driver.
>
> Its not sufficient just note this in patch description.
>
> a) Disable (status = "disabled") the VPIF node which clashes for pins
> with LCD.
> b) Add a comment on top of the status = "disabled" giving information on
> how user can enable it (disable lcdc node and then change to status =
> "okay").
>
>>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>> ---
>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>      backlight which better matches the schematic.  Updated the description to explain
>>      that it cannot be used at the same time as the vpif driver.
>>
>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>
>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>> index 2e817da37fdb..3f1c8be07efe 100644
>> --- a/arch/arm/boot/dts/da850-evm.dts
>> +++ b/arch/arm/boot/dts/da850-evm.dts
>> @@ -27,6 +27,50 @@
>>               spi0 = &spi1;
>>       };
>>
>> +     backlight {
>> +             compatible = "gpio-backlight";
>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>
> The gpio-backlight binding does not describe a property called
> enable-gpios. It should just be gpios.

I will fix that.

>
> a) Are you using gpio-backlight because you are not able to get the PWM
> to work?
>
Yes,  You told me not to worry about doing a PWM backlight because the
legacy board does not PWM either.

> b) What is GP0[7] connected to in the schematic you have? In the
> schematic I have I see LCD_PWM0 is connected to
> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.

I have schematic 1016572 dated Wednesday, August 18, 2010.  According
to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
pre-release versions.

>
> c) The /* GP0[7] */ comment is not really useful on its own as it can be
> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
> Same for other places like this below.

I can do that.
>
>> @@ -35,6 +79,16 @@
>>               regulator-boot-on;
>>       };
>>
>> +     backlight_reg: backlight-regulator {
>> +             compatible = "regulator-fixed";
>> +             regulator-name = "lcd_backlight_pwr";
>> +             regulator-min-microvolt = <3300000>;
>> +             regulator-max-microvolt = <3300000>;
>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>> +             regulator-always-on;
>
> Why should this regulator never be disabled?

The gpio-backlight does not have a way that I can see to associate the
regulator to it.  I read through the bindings, but I didn't see an
option to associate a regulator it.  I use this regulator to drive
lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
this option, the system disables lcd_backlight_pwr and the screen is
blank


adam
>
> Thanks,
> Sekhar

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> Hi Adam,
>>
>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>> When using the board files the LCD works, but not with the DT.
>>> This adds enables the original da850-evm to work with the same
>>> LCD in device tree mode.
>>>
>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>> do not load the vpif driver.
>>
>> Its not sufficient just note this in patch description.
>>
>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>> with LCD.
>> b) Add a comment on top of the status = "disabled" giving information on
>> how user can enable it (disable lcdc node and then change to status =
>> "okay").
>>
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> ---
>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>      backlight which better matches the schematic.  Updated the description to explain
>>>      that it cannot be used at the same time as the vpif driver.
>>>
>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>
>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>> index 2e817da37fdb..3f1c8be07efe 100644
>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>> @@ -27,6 +27,50 @@
>>>               spi0 = &spi1;
>>>       };
>>>
>>> +     backlight {
>>> +             compatible = "gpio-backlight";
>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>
>> The gpio-backlight binding does not describe a property called
>> enable-gpios. It should just be gpios.
> 
> I will fix that.
> 
>>
>> a) Are you using gpio-backlight because you are not able to get the PWM
>> to work?
>>
> Yes,  You told me not to worry about doing a PWM backlight because the
> legacy board does not PWM either.

Yeah, I meant not to add backlight control till the time we are able to
get it working using PWM. Is this needed for the basic LCD functionality
to work? I would like to avoid the churn of adding it using GPIO now and
changing to PWM later, if possible.

> 
>> b) What is GP0[7] connected to in the schematic you have? In the
>> schematic I have I see LCD_PWM0 is connected to
>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
> 
> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
> pre-release versions.

Ah, okay. In your schematic, is GP2[14] connected to anything?

> 
>>
>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>> Same for other places like this below.
> 
> I can do that.
>>
>>> @@ -35,6 +79,16 @@
>>>               regulator-boot-on;
>>>       };
>>>
>>> +     backlight_reg: backlight-regulator {
>>> +             compatible = "regulator-fixed";
>>> +             regulator-name = "lcd_backlight_pwr";
>>> +             regulator-min-microvolt = <3300000>;
>>> +             regulator-max-microvolt = <3300000>;
>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>> +             regulator-always-on;
>>
>> Why should this regulator never be disabled?
> 
> The gpio-backlight does not have a way that I can see to associate the
> regulator to it.  I read through the bindings, but I didn't see an
> option to associate a regulator it.  I use this regulator to drive
> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
> this option, the system disables lcd_backlight_pwr and the screen is
> blank

It sounds like this is a hack to enable backlight on this board. I think
either the backlight driver needs to gain functionality to enable the
GPIO. Or backlight could be treated as part of the panel and enabled
using enable-gpios property in the panel. TBH, I will be okay either
way. Can you check with Jyri, Tomi and rest of the DRM folks on what
should be right way of dealing with this?

Thanks,
Sekhar

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> Hi Adam,
>>
>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>> When using the board files the LCD works, but not with the DT.
>>> This adds enables the original da850-evm to work with the same
>>> LCD in device tree mode.
>>>
>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>> do not load the vpif driver.
>>
>> Its not sufficient just note this in patch description.
>>
>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>> with LCD.
>> b) Add a comment on top of the status = "disabled" giving information on
>> how user can enable it (disable lcdc node and then change to status =
>> "okay").
>>
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> ---
>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>      backlight which better matches the schematic.  Updated the description to explain
>>>      that it cannot be used at the same time as the vpif driver.
>>>
>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>
>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>> index 2e817da37fdb..3f1c8be07efe 100644
>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>> @@ -27,6 +27,50 @@
>>>               spi0 = &spi1;
>>>       };
>>>
>>> +     backlight {
>>> +             compatible = "gpio-backlight";
>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>
>> The gpio-backlight binding does not describe a property called
>> enable-gpios. It should just be gpios.
> 
> I will fix that.
> 
>>
>> a) Are you using gpio-backlight because you are not able to get the PWM
>> to work?
>>
> Yes,  You told me not to worry about doing a PWM backlight because the
> legacy board does not PWM either.

Yeah, I meant not to add backlight control till the time we are able to
get it working using PWM. Is this needed for the basic LCD functionality
to work? I would like to avoid the churn of adding it using GPIO now and
changing to PWM later, if possible.

> 
>> b) What is GP0[7] connected to in the schematic you have? In the
>> schematic I have I see LCD_PWM0 is connected to
>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
> 
> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
> pre-release versions.

Ah, okay. In your schematic, is GP2[14] connected to anything?

> 
>>
>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>> Same for other places like this below.
> 
> I can do that.
>>
>>> @@ -35,6 +79,16 @@
>>>               regulator-boot-on;
>>>       };
>>>
>>> +     backlight_reg: backlight-regulator {
>>> +             compatible = "regulator-fixed";
>>> +             regulator-name = "lcd_backlight_pwr";
>>> +             regulator-min-microvolt = <3300000>;
>>> +             regulator-max-microvolt = <3300000>;
>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>> +             regulator-always-on;
>>
>> Why should this regulator never be disabled?
> 
> The gpio-backlight does not have a way that I can see to associate the
> regulator to it.  I read through the bindings, but I didn't see an
> option to associate a regulator it.  I use this regulator to drive
> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
> this option, the system disables lcd_backlight_pwr and the screen is
> blank

It sounds like this is a hack to enable backlight on this board. I think
either the backlight driver needs to gain functionality to enable the
GPIO. Or backlight could be treated as part of the panel and enabled
using enable-gpios property in the panel. TBH, I will be okay either
way. Can you check with Jyri, Tomi and rest of the DRM folks on what
should be right way of dealing with this?

Thanks,
Sekhar

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
>> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> Hi Adam,

Added Tomi, Laurent, and Jyri for feedback.

>>>
>>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>>> When using the board files the LCD works, but not with the DT.
>>>> This adds enables the original da850-evm to work with the same
>>>> LCD in device tree mode.
>>>>
>>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>>> do not load the vpif driver.
>>>
>>> Its not sufficient just note this in patch description.
>>>
>>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>>> with LCD.
>>> b) Add a comment on top of the status = "disabled" giving information on
>>> how user can enable it (disable lcdc node and then change to status =
>>> "okay").
>>>
>>>>
>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>> ---
>>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>>      backlight which better matches the schematic.  Updated the description to explain
>>>>      that it cannot be used at the same time as the vpif driver.
>>>>
>>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>>
>>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>>> index 2e817da37fdb..3f1c8be07efe 100644
>>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>>> @@ -27,6 +27,50 @@
>>>>               spi0 = &spi1;
>>>>       };
>>>>
>>>> +     backlight {
>>>> +             compatible = "gpio-backlight";
>>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>>
>>> The gpio-backlight binding does not describe a property called
>>> enable-gpios. It should just be gpios.
>>
>> I will fix that.
>>
>>>
>>> a) Are you using gpio-backlight because you are not able to get the PWM
>>> to work?
>>>
>> Yes,  You told me not to worry about doing a PWM backlight because the
>> legacy board does not PWM either.
>
> Yeah, I meant not to add backlight control till the time we are able to
> get it working using PWM. Is this needed for the basic LCD functionality
> to work? I would like to avoid the churn of adding it using GPIO now and
> changing to PWM later, if possible.
>
>>
>>> b) What is GP0[7] connected to in the schematic you have? In the
>>> schematic I have I see LCD_PWM0 is connected to
>>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
>>
>> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
>> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
>> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
>> pre-release versions.
>
> Ah, okay. In your schematic, is GP2[14] connected to anything?
>
>>
>>>
>>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>>> Same for other places like this below.
>>
>> I can do that.
>>>
>>>> @@ -35,6 +79,16 @@
>>>>               regulator-boot-on;
>>>>       };
>>>>
>>>> +     backlight_reg: backlight-regulator {
>>>> +             compatible = "regulator-fixed";
>>>> +             regulator-name = "lcd_backlight_pwr";
>>>> +             regulator-min-microvolt = <3300000>;
>>>> +             regulator-max-microvolt = <3300000>;
>>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>>> +             regulator-always-on;
>>>
>>> Why should this regulator never be disabled?
>>
>> The gpio-backlight does not have a way that I can see to associate the
>> regulator to it.  I read through the bindings, but I didn't see an
>> option to associate a regulator it.  I use this regulator to drive
>> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
>> this option, the system disables lcd_backlight_pwr and the screen is
>> blank
>
> It sounds like this is a hack to enable backlight on this board. I think
> either the backlight driver needs to gain functionality to enable the
> GPIO. Or backlight could be treated as part of the panel and enabled
> using enable-gpios property in the panel. TBH, I will be okay either
> way. Can you check with Jyri, Tomi and rest of the DRM folks on what
> should be right way of dealing with this?

Per your request I added them into this thread.  I added Tomi, Jyri,
and Laurent to this as Laurent's name is associated with the gpio
backlight driver.

I am not sure why you think it's a hack.  I pulled up the schematic
for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
controls the power-on sequence of the back-light controller.  Without
this, there is no power, so it seems to me that the 'regulator-fixed'
device is the correct way to do it.

The separate pin associated to the gpio is used to tell the backlight
IC to actually turn on/off the back-light.  Ideally it seems like it
would nice to have the gpio-backlight driver be able to specify the
regulator, so when the backlight is in use, it would power the
regulator, but until that's available, the it seems like
'regulator-always-on' is the way to make it stay on.

Laurent, Jyri, Tomi, do you have any thoughts on this matter?

Thanks,

adam
>
> Thanks,
> Sekhar

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Adam Ford]

On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
>> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> Hi Adam,

Added Tomi, Laurent, and Jyri for feedback.

>>>
>>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>>> When using the board files the LCD works, but not with the DT.
>>>> This adds enables the original da850-evm to work with the same
>>>> LCD in device tree mode.
>>>>
>>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>>> do not load the vpif driver.
>>>
>>> Its not sufficient just note this in patch description.
>>>
>>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>>> with LCD.
>>> b) Add a comment on top of the status = "disabled" giving information on
>>> how user can enable it (disable lcdc node and then change to status =
>>> "okay").
>>>
>>>>
>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>> ---
>>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>>      backlight which better matches the schematic.  Updated the description to explain
>>>>      that it cannot be used at the same time as the vpif driver.
>>>>
>>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>>
>>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>>> index 2e817da37fdb..3f1c8be07efe 100644
>>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>>> @@ -27,6 +27,50 @@
>>>>               spi0 = &spi1;
>>>>       };
>>>>
>>>> +     backlight {
>>>> +             compatible = "gpio-backlight";
>>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>>
>>> The gpio-backlight binding does not describe a property called
>>> enable-gpios. It should just be gpios.
>>
>> I will fix that.
>>
>>>
>>> a) Are you using gpio-backlight because you are not able to get the PWM
>>> to work?
>>>
>> Yes,  You told me not to worry about doing a PWM backlight because the
>> legacy board does not PWM either.
>
> Yeah, I meant not to add backlight control till the time we are able to
> get it working using PWM. Is this needed for the basic LCD functionality
> to work? I would like to avoid the churn of adding it using GPIO now and
> changing to PWM later, if possible.
>
>>
>>> b) What is GP0[7] connected to in the schematic you have? In the
>>> schematic I have I see LCD_PWM0 is connected to
>>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
>>
>> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
>> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
>> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
>> pre-release versions.
>
> Ah, okay. In your schematic, is GP2[14] connected to anything?
>
>>
>>>
>>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>>> Same for other places like this below.
>>
>> I can do that.
>>>
>>>> @@ -35,6 +79,16 @@
>>>>               regulator-boot-on;
>>>>       };
>>>>
>>>> +     backlight_reg: backlight-regulator {
>>>> +             compatible = "regulator-fixed";
>>>> +             regulator-name = "lcd_backlight_pwr";
>>>> +             regulator-min-microvolt = <3300000>;
>>>> +             regulator-max-microvolt = <3300000>;
>>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>>> +             regulator-always-on;
>>>
>>> Why should this regulator never be disabled?
>>
>> The gpio-backlight does not have a way that I can see to associate the
>> regulator to it.  I read through the bindings, but I didn't see an
>> option to associate a regulator it.  I use this regulator to drive
>> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
>> this option, the system disables lcd_backlight_pwr and the screen is
>> blank
>
> It sounds like this is a hack to enable backlight on this board. I think
> either the backlight driver needs to gain functionality to enable the
> GPIO. Or backlight could be treated as part of the panel and enabled
> using enable-gpios property in the panel. TBH, I will be okay either
> way. Can you check with Jyri, Tomi and rest of the DRM folks on what
> should be right way of dealing with this?

Per your request I added them into this thread.  I added Tomi, Jyri,
and Laurent to this as Laurent's name is associated with the gpio
backlight driver.

I am not sure why you think it's a hack.  I pulled up the schematic
for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
controls the power-on sequence of the back-light controller.  Without
this, there is no power, so it seems to me that the 'regulator-fixed'
device is the correct way to do it.

The separate pin associated to the gpio is used to tell the backlight
IC to actually turn on/off the back-light.  Ideally it seems like it
would nice to have the gpio-backlight driver be able to specify the
regulator, so when the backlight is in use, it would power the
regulator, but until that's available, the it seems like
'regulator-always-on' is the way to make it stay on.

Laurent, Jyri, Tomi, do you have any thoughts on this matter?

Thanks,

adam
>
> Thanks,
> Sekhar

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

On Monday 14 May 2018 11:34 PM, Adam Ford wrote:
> On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
>>> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>>> Hi Adam,
> 
> Added Tomi, Laurent, and Jyri for feedback.
> 
>>>>
>>>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>>>> When using the board files the LCD works, but not with the DT.
>>>>> This adds enables the original da850-evm to work with the same
>>>>> LCD in device tree mode.
>>>>>
>>>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>>>> do not load the vpif driver.
>>>>
>>>> Its not sufficient just note this in patch description.
>>>>
>>>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>>>> with LCD.
>>>> b) Add a comment on top of the status = "disabled" giving information on
>>>> how user can enable it (disable lcdc node and then change to status =
>>>> "okay").
>>>>
>>>>>
>>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>>> ---
>>>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>>>      backlight which better matches the schematic.  Updated the description to explain
>>>>>      that it cannot be used at the same time as the vpif driver.
>>>>>
>>>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>>>> index 2e817da37fdb..3f1c8be07efe 100644
>>>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>>>> @@ -27,6 +27,50 @@
>>>>>               spi0 = &spi1;
>>>>>       };
>>>>>
>>>>> +     backlight {
>>>>> +             compatible = "gpio-backlight";
>>>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>>>
>>>> The gpio-backlight binding does not describe a property called
>>>> enable-gpios. It should just be gpios.
>>>
>>> I will fix that.
>>>
>>>>
>>>> a) Are you using gpio-backlight because you are not able to get the PWM
>>>> to work?
>>>>
>>> Yes,  You told me not to worry about doing a PWM backlight because the
>>> legacy board does not PWM either.
>>
>> Yeah, I meant not to add backlight control till the time we are able to
>> get it working using PWM. Is this needed for the basic LCD functionality
>> to work? I would like to avoid the churn of adding it using GPIO now and
>> changing to PWM later, if possible.
>>
>>>
>>>> b) What is GP0[7] connected to in the schematic you have? In the
>>>> schematic I have I see LCD_PWM0 is connected to
>>>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
>>>
>>> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
>>> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
>>> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
>>> pre-release versions.
>>
>> Ah, okay. In your schematic, is GP2[14] connected to anything?
>>
>>>
>>>>
>>>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>>>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>>>> Same for other places like this below.
>>>
>>> I can do that.
>>>>
>>>>> @@ -35,6 +79,16 @@
>>>>>               regulator-boot-on;
>>>>>       };
>>>>>
>>>>> +     backlight_reg: backlight-regulator {
>>>>> +             compatible = "regulator-fixed";
>>>>> +             regulator-name = "lcd_backlight_pwr";
>>>>> +             regulator-min-microvolt = <3300000>;
>>>>> +             regulator-max-microvolt = <3300000>;
>>>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>>>> +             regulator-always-on;
>>>>
>>>> Why should this regulator never be disabled?
>>>
>>> The gpio-backlight does not have a way that I can see to associate the
>>> regulator to it.  I read through the bindings, but I didn't see an
>>> option to associate a regulator it.  I use this regulator to drive
>>> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
>>> this option, the system disables lcd_backlight_pwr and the screen is
>>> blank
>>
>> It sounds like this is a hack to enable backlight on this board. I think
>> either the backlight driver needs to gain functionality to enable the
>> GPIO. Or backlight could be treated as part of the panel and enabled
>> using enable-gpios property in the panel. TBH, I will be okay either
>> way. Can you check with Jyri, Tomi and rest of the DRM folks on what
>> should be right way of dealing with this?
> 
> Per your request I added them into this thread.  I added Tomi, Jyri,
> and Laurent to this as Laurent's name is associated with the gpio
> backlight driver.

Okay. The reason I did not loop them in myself is because I thought a
fresh thread with background will be better. But okay.

> I am not sure why you think it's a hack.  I pulled up the schematic
> for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
> controls the power-on sequence of the back-light controller.  Without
> this, there is no power, so it seems to me that the 'regulator-fixed'
> device is the correct way to do it.

Not questioning modeling the GPIO as a regulator.

> 
> The separate pin associated to the gpio is used to tell the backlight
> IC to actually turn on/off the back-light.  Ideally it seems like it
> would nice to have the gpio-backlight driver be able to specify the
> regulator, so when the backlight is in use, it would power the
> regulator, but until that's available, the it seems like
> 'regulator-always-on' is the way to make it stay on.

We need to add support for this in backlight driver. Using
regulator-always-on to paper over this lack of support in backlight
driver is what I am calling a hack. 'regulator-always-on' means the
regulator cannot be turned off. Thats certainly not the case as you have
pointed out.

Thanks,
Sekhar

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Sekhar Nori]

On Monday 14 May 2018 11:34 PM, Adam Ford wrote:
> On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
>>> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>>> Hi Adam,
> 
> Added Tomi, Laurent, and Jyri for feedback.
> 
>>>>
>>>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
>>>>> When using the board files the LCD works, but not with the DT.
>>>>> This adds enables the original da850-evm to work with the same
>>>>> LCD in device tree mode.
>>>>>
>>>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
>>>>> the vpif display pins are mutually exclusive, so if using the LCD,
>>>>> do not load the vpif driver.
>>>>
>>>> Its not sufficient just note this in patch description.
>>>>
>>>> a) Disable (status = "disabled") the VPIF node which clashes for pins
>>>> with LCD.
>>>> b) Add a comment on top of the status = "disabled" giving information on
>>>> how user can enable it (disable lcdc node and then change to status =
>>>> "okay").
>>>>
>>>>>
>>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>>> ---
>>>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
>>>>>      backlight which better matches the schematic.  Updated the description to explain
>>>>>      that it cannot be used at the same time as the vpif driver.
>>>>>
>>>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
>>>>> index 2e817da37fdb..3f1c8be07efe 100644
>>>>> --- a/arch/arm/boot/dts/da850-evm.dts
>>>>> +++ b/arch/arm/boot/dts/da850-evm.dts
>>>>> @@ -27,6 +27,50 @@
>>>>>               spi0 = &spi1;
>>>>>       };
>>>>>
>>>>> +     backlight {
>>>>> +             compatible = "gpio-backlight";
>>>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
>>>>
>>>> The gpio-backlight binding does not describe a property called
>>>> enable-gpios. It should just be gpios.
>>>
>>> I will fix that.
>>>
>>>>
>>>> a) Are you using gpio-backlight because you are not able to get the PWM
>>>> to work?
>>>>
>>> Yes,  You told me not to worry about doing a PWM backlight because the
>>> legacy board does not PWM either.
>>
>> Yeah, I meant not to add backlight control till the time we are able to
>> get it working using PWM. Is this needed for the basic LCD functionality
>> to work? I would like to avoid the churn of adding it using GPIO now and
>> changing to PWM later, if possible.
>>
>>>
>>>> b) What is GP0[7] connected to in the schematic you have? In the
>>>> schematic I have I see LCD_PWM0 is connected to
>>>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
>>>
>>> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
>>> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
>>> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
>>> pre-release versions.
>>
>> Ah, okay. In your schematic, is GP2[14] connected to anything?
>>
>>>
>>>>
>>>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
>>>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
>>>> Same for other places like this below.
>>>
>>> I can do that.
>>>>
>>>>> @@ -35,6 +79,16 @@
>>>>>               regulator-boot-on;
>>>>>       };
>>>>>
>>>>> +     backlight_reg: backlight-regulator {
>>>>> +             compatible = "regulator-fixed";
>>>>> +             regulator-name = "lcd_backlight_pwr";
>>>>> +             regulator-min-microvolt = <3300000>;
>>>>> +             regulator-max-microvolt = <3300000>;
>>>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
>>>>> +             regulator-always-on;
>>>>
>>>> Why should this regulator never be disabled?
>>>
>>> The gpio-backlight does not have a way that I can see to associate the
>>> regulator to it.  I read through the bindings, but I didn't see an
>>> option to associate a regulator it.  I use this regulator to drive
>>> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
>>> this option, the system disables lcd_backlight_pwr and the screen is
>>> blank
>>
>> It sounds like this is a hack to enable backlight on this board. I think
>> either the backlight driver needs to gain functionality to enable the
>> GPIO. Or backlight could be treated as part of the panel and enabled
>> using enable-gpios property in the panel. TBH, I will be okay either
>> way. Can you check with Jyri, Tomi and rest of the DRM folks on what
>> should be right way of dealing with this?
> 
> Per your request I added them into this thread.  I added Tomi, Jyri,
> and Laurent to this as Laurent's name is associated with the gpio
> backlight driver.

Okay. The reason I did not loop them in myself is because I thought a
fresh thread with background will be better. But okay.

> I am not sure why you think it's a hack.  I pulled up the schematic
> for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
> controls the power-on sequence of the back-light controller.  Without
> this, there is no power, so it seems to me that the 'regulator-fixed'
> device is the correct way to do it.

Not questioning modeling the GPIO as a regulator.

> 
> The separate pin associated to the gpio is used to tell the backlight
> IC to actually turn on/off the back-light.  Ideally it seems like it
> would nice to have the gpio-backlight driver be able to specify the
> regulator, so when the backlight is in use, it would power the
> regulator, but until that's available, the it seems like
> 'regulator-always-on' is the way to make it stay on.

We need to add support for this in backlight driver. Using
regulator-always-on to paper over this lack of support in backlight
driver is what I am calling a hack. 'regulator-always-on' means the
regulator cannot be turned off. Thats certainly not the case as you have
pointed out.

Thanks,
Sekhar

Re: [PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Laurent Pinchart]

Hi Adam,

On Monday, 14 May 2018 21:04:07 EEST Adam Ford wrote:
> On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> > On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
> >> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> >>> Hi Adam,
> 
> Added Tomi, Laurent, and Jyri for feedback.
> 
> >>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
> >>>> When using the board files the LCD works, but not with the DT.
> >>>> This adds enables the original da850-evm to work with the same
> >>>> LCD in device tree mode.
> >>>> 
> >>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
> >>>> the vpif display pins are mutually exclusive, so if using the LCD,
> >>>> do not load the vpif driver.
> >>> 
> >>> Its not sufficient just note this in patch description.
> >>> 
> >>> a) Disable (status = "disabled") the VPIF node which clashes for pins
> >>> with LCD.
> >>> b) Add a comment on top of the status = "disabled" giving information on
> >>> how user can enable it (disable lcdc node and then change to status =
> >>> "okay").
> >>> 
> >>>> Signed-off-by: Adam Ford <aford173@gmail.com>
> >>>> ---
> >>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be
> >>>>      more explict to backlight which better matches the schematic. 
> >>>>      Updated the description to explain that it cannot be used at the
> >>>>      same time as the vpif driver.
> >>>> 
> >>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and
> >>>>      replace with GPIO
> >>>> 
> >>>> diff --git a/arch/arm/boot/dts/da850-evm.dts
> >>>> b/arch/arm/boot/dts/da850-evm.dts index 2e817da37fdb..3f1c8be07efe
> >>>> 100644
> >>>> --- a/arch/arm/boot/dts/da850-evm.dts
> >>>> +++ b/arch/arm/boot/dts/da850-evm.dts
> >>>> @@ -27,6 +27,50 @@
> >>>>               spi0 = &spi1;
> >>>>       };
> >>>> 
> >>>> +     backlight {
> >>>> +             compatible = "gpio-backlight";
> >>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
> >>> 
> >>> The gpio-backlight binding does not describe a property called
> >>> enable-gpios. It should just be gpios.
> >> 
> >> I will fix that.
> >> 
> >>> a) Are you using gpio-backlight because you are not able to get the PWM
> >>> to work?
> >> 
> >> Yes,  You told me not to worry about doing a PWM backlight because the
> >> legacy board does not PWM either.
> > 
> > Yeah, I meant not to add backlight control till the time we are able to
> > get it working using PWM. Is this needed for the basic LCD functionality
> > to work? I would like to avoid the churn of adding it using GPIO now and
> > changing to PWM later, if possible.
> > 
> >>> b) What is GP0[7] connected to in the schematic you have? In the
> >>> schematic I have I see LCD_PWM0 is connected to
> >>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
> >> 
> >> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
> >> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
> >> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
> >> pre-release versions.
> > 
> > Ah, okay. In your schematic, is GP2[14] connected to anything?
> > 
> >>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
> >>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
> >>> Same for other places like this below.
> >> 
> >> I can do that.
> >> 
> >>>> @@ -35,6 +79,16 @@
> >>>>               regulator-boot-on;
> >>>>       };
> >>>> 
> >>>> +     backlight_reg: backlight-regulator {
> >>>> +             compatible = "regulator-fixed";
> >>>> +             regulator-name = "lcd_backlight_pwr";
> >>>> +             regulator-min-microvolt = <3300000>;
> >>>> +             regulator-max-microvolt = <3300000>;
> >>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
> >>>> +             regulator-always-on;
> >>> 
> >>> Why should this regulator never be disabled?
> >> 
> >> The gpio-backlight does not have a way that I can see to associate the
> >> regulator to it.  I read through the bindings, but I didn't see an
> >> option to associate a regulator it.  I use this regulator to drive
> >> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
> >> this option, the system disables lcd_backlight_pwr and the screen is
> >> blank
> > 
> > It sounds like this is a hack to enable backlight on this board. I think
> > either the backlight driver needs to gain functionality to enable the
> > GPIO. Or backlight could be treated as part of the panel and enabled
> > using enable-gpios property in the panel. TBH, I will be okay either
> > way. Can you check with Jyri, Tomi and rest of the DRM folks on what
> > should be right way of dealing with this?
> 
> Per your request I added them into this thread.  I added Tomi, Jyri,
> and Laurent to this as Laurent's name is associated with the gpio
> backlight driver.
> 
> I am not sure why you think it's a hack.  I pulled up the schematic
> for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
> controls the power-on sequence of the back-light controller.  Without
> this, there is no power, so it seems to me that the 'regulator-fixed'
> device is the correct way to do it.

It's a hack because keeping the backlight power on when the display is off 
will consume power unnecessarily. The backlight power should be turned off in 
that case.

> The separate pin associated to the gpio is used to tell the backlight
> IC to actually turn on/off the back-light.  Ideally it seems like it
> would nice to have the gpio-backlight driver be able to specify the
> regulator, so when the backlight is in use, it would power the
> regulator, but until that's available, the it seems like
> 'regulator-always-on' is the way to make it stay on.
> 
> Laurent, Jyri, Tomi, do you have any thoughts on this matter?

This has been attempted 6 years ago, and got rejected. See https://lwn.net/
Articles/525422/.

The problem is that the number of power supplies, the order in which they need 
to be sequenced, and the related timings (as in delays required between 
turning power supplies on/off) is highly hardware-specific. We can't hardcode 
a specific sequence and specific timings in the gpio-backlight (or pwm-
backlight) driver, and specifying the sequence in DT was considered to be 
over-engineered.

You thus have two options, either creating a custom backlight driver that will 
handle the power supplies specific to your backlight controller, or 
integrating backlight power supply control in your panel driver. If the power 
supplies also power the panel and not just the backlight, or if their 
sequencing depends in any way on the panel model in which the backlight 
controller is integrated, I'd go for the latter, otherwise you can pick any of 
the two methods.

-- 
Regards,

Laurent Pinchart

[PATCH V3] ARM: dts: da850-evm: Enable LCD and Backlight

[Laurent Pinchart]

Hi Adam,

On Monday, 14 May 2018 21:04:07 EEST Adam Ford wrote:
> On Mon, May 14, 2018 at 7:35 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> > On Monday 14 May 2018 04:22 PM, Adam Ford wrote:
> >> On Mon, May 14, 2018 at 12:29 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> >>> Hi Adam,
> 
> Added Tomi, Laurent, and Jyri for feedback.
> 
> >>> On Monday 14 May 2018 04:50 AM, Adam Ford wrote:
> >>>> When using the board files the LCD works, but not with the DT.
> >>>> This adds enables the original da850-evm to work with the same
> >>>> LCD in device tree mode.
> >>>> 
> >>>> The EVM has a gpio for the regulator and a gpio enable.  The LCD and
> >>>> the vpif display pins are mutually exclusive, so if using the LCD,
> >>>> do not load the vpif driver.
> >>> 
> >>> Its not sufficient just note this in patch description.
> >>> 
> >>> a) Disable (status = "disabled") the VPIF node which clashes for pins
> >>> with LCD.
> >>> b) Add a comment on top of the status = "disabled" giving information on
> >>> how user can enable it (disable lcdc node and then change to status =
> >>> "okay").
> >>> 
> >>>> Signed-off-by: Adam Ford <aford173@gmail.com>
> >>>> ---
> >>>> V3:  Fix errant GPIO, label GPIO pins, and rename the regulator to be
> >>>>      more explict to backlight which better matches the schematic. 
> >>>>      Updated the description to explain that it cannot be used at the
> >>>>      same time as the vpif driver.
> >>>> 
> >>>> V2:  Add regulator and GPIO enable pins. Remove PWM backlight and
> >>>>      replace with GPIO
> >>>> 
> >>>> diff --git a/arch/arm/boot/dts/da850-evm.dts
> >>>> b/arch/arm/boot/dts/da850-evm.dts index 2e817da37fdb..3f1c8be07efe
> >>>> 100644
> >>>> --- a/arch/arm/boot/dts/da850-evm.dts
> >>>> +++ b/arch/arm/boot/dts/da850-evm.dts
> >>>> @@ -27,6 +27,50 @@
> >>>>               spi0 = &spi1;
> >>>>       };
> >>>> 
> >>>> +     backlight {
> >>>> +             compatible = "gpio-backlight";
> >>>> +             enable-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* GP0[7] */
> >>> 
> >>> The gpio-backlight binding does not describe a property called
> >>> enable-gpios. It should just be gpios.
> >> 
> >> I will fix that.
> >> 
> >>> a) Are you using gpio-backlight because you are not able to get the PWM
> >>> to work?
> >> 
> >> Yes,  You told me not to worry about doing a PWM backlight because the
> >> legacy board does not PWM either.
> > 
> > Yeah, I meant not to add backlight control till the time we are able to
> > get it working using PWM. Is this needed for the basic LCD functionality
> > to work? I would like to avoid the churn of adding it using GPIO now and
> > changing to PWM later, if possible.
> > 
> >>> b) What is GP0[7] connected to in the schematic you have? In the
> >>> schematic I have I see LCD_PWM0 is connected to
> >>> SPI1_SCS[0]/EPWM1B/GP2[14]/TM64P3_IN12.
> >> 
> >> I have schematic 1016572 dated Wednesday, August 18, 2010.  According
> >> to it, AXR15 / EPWMN0_TZ[0] / ECAP2_APWM2 / GPIO0[7] connects to U25,
> >> Pin 46 to generate M_LCD_PWM0.  You might have one of the early,
> >> pre-release versions.
> > 
> > Ah, okay. In your schematic, is GP2[14] connected to anything?
> > 
> >>> c) The /* GP0[7] */ comment is not really useful on its own as it can be
> >>> computed. What I wanted to see is the schematic symbol like "LCD_PWM0".
> >>> Same for other places like this below.
> >> 
> >> I can do that.
> >> 
> >>>> @@ -35,6 +79,16 @@
> >>>>               regulator-boot-on;
> >>>>       };
> >>>> 
> >>>> +     backlight_reg: backlight-regulator {
> >>>> +             compatible = "regulator-fixed";
> >>>> +             regulator-name = "lcd_backlight_pwr";
> >>>> +             regulator-min-microvolt = <3300000>;
> >>>> +             regulator-max-microvolt = <3300000>;
> >>>> +             gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* GP2[15] */
> >>>> +             regulator-always-on;
> >>> 
> >>> Why should this regulator never be disabled?
> >> 
> >> The gpio-backlight does not have a way that I can see to associate the
> >> regulator to it.  I read through the bindings, but I didn't see an
> >> option to associate a regulator it.  I use this regulator to drive
> >> lcd_backlight_pwr and the backlight driver to write lcd_pwm0.  Without
> >> this option, the system disables lcd_backlight_pwr and the screen is
> >> blank
> > 
> > It sounds like this is a hack to enable backlight on this board. I think
> > either the backlight driver needs to gain functionality to enable the
> > GPIO. Or backlight could be treated as part of the panel and enabled
> > using enable-gpios property in the panel. TBH, I will be okay either
> > way. Can you check with Jyri, Tomi and rest of the DRM folks on what
> > should be right way of dealing with this?
> 
> Per your request I added them into this thread.  I added Tomi, Jyri,
> and Laurent to this as Laurent's name is associated with the gpio
> backlight driver.
> 
> I am not sure why you think it's a hack.  I pulled up the schematic
> for the LCD to see what it's doing, and the  lcd_backlight_pwr pin
> controls the power-on sequence of the back-light controller.  Without
> this, there is no power, so it seems to me that the 'regulator-fixed'
> device is the correct way to do it.

It's a hack because keeping the backlight power on when the display is off 
will consume power unnecessarily. The backlight power should be turned off in 
that case.

> The separate pin associated to the gpio is used to tell the backlight
> IC to actually turn on/off the back-light.  Ideally it seems like it
> would nice to have the gpio-backlight driver be able to specify the
> regulator, so when the backlight is in use, it would power the
> regulator, but until that's available, the it seems like
> 'regulator-always-on' is the way to make it stay on.
> 
> Laurent, Jyri, Tomi, do you have any thoughts on this matter?

This has been attempted 6 years ago, and got rejected. See https://lwn.net/
Articles/525422/.

The problem is that the number of power supplies, the order in which they need 
to be sequenced, and the related timings (as in delays required between 
turning power supplies on/off) is highly hardware-specific. We can't hardcode 
a specific sequence and specific timings in the gpio-backlight (or pwm-
backlight) driver, and specifying the sequence in DT was considered to be 
over-engineered.

You thus have two options, either creating a custom backlight driver that will 
handle the power supplies specific to your backlight controller, or 
integrating backlight power supply control in your panel driver. If the power 
supplies also power the panel and not just the backlight, or if their 
sequencing depends in any way on the panel model in which the backlight 
controller is integrated, I'd go for the latter, otherwise you can pick any of 
the two methods.

-- 
Regards,

Laurent Pinchart