[RFC,v2,v4,09/14] ARM: dts: s6e3fa0: add DT bindings

Commit Message

YoungJun Cho April 21, 2014, 12:28 p.m. UTC

This patch adds DT bindings for s6e3fa0 panel.
The bindings describes panel resources, display timings and cpu timings.

Changelog v2:
- Adds unit address (commented by Sachin Kamat)
Changelog v3:
- Removes optional delay, size properties (commented by Laurent Pinchart)
- Adds OLED detection, TE gpio properties
Changelog v4:
- Moves CPU timings relevant properties from FIMD DT
  (commeted by Laurent Pinchart, Andrzej Hajda)

Signed-off-by: YoungJun Cho <[e-mail]>
Acked-by: Inki Dae <[e-mail]>
Acked-by: Kyungmin Park <[e-mail]>
---
 .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt

Comments

Andrzej Hajda April 22, 2014, 2:02 p.m. UTC | #1

On 04/21/2014 02:28 PM, YoungJun Cho wrote:
> This patch adds DT bindings for s6e3fa0 panel.
> The bindings describes panel resources, display timings and cpu timings.
> 
> Changelog v2:
> - Adds unit address (commented by Sachin Kamat)
> Changelog v3:
> - Removes optional delay, size properties (commented by Laurent Pinchart)
> - Adds OLED detection, TE gpio properties
> Changelog v4:
> - Moves CPU timings relevant properties from FIMD DT
>   (commeted by Laurent Pinchart, Andrzej Hajda)
> 
> Signed-off-by: YoungJun Cho <[e-mail]>
> Acked-by: Inki Dae <[e-mail]>
> Acked-by: Kyungmin Park <[e-mail]>
> ---
>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> 
> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> new file mode 100644
> index 0000000..9eeb38b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> @@ -0,0 +1,63 @@
> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> +
> +Required properties:
> +  - compatible: "samsung,s6e3fa0"
> +  - reg: the virtual channel number of a DSI peripheral
> +  - vdd3-supply: core voltage supply
> +  - vci-supply: voltage supply for analog circuits
> +  - reset-gpio: a GPIO spec for the reset pin
> +  - det-gpio: a GPIO spec for the OLED detection pin
> +  - te-gpio: a GPIO spec for the TE pin

Just FYI, according to DT documentation [1] gpio spec should be in form
[name]-gpios, however there is plenty bindings with -gpio suffix, so I
am not sure if it is really enforced. On the other side it is enforced
by descriptor based gpio framework[2]. Integer-based gpio framework
used in your driver is obsolete according to [2].

[1]: Documentation/devicetree/bindings/gpio/gpio.txt
[2]: Documentation/gpio/gpio.txt

Regards
Andrzej

> +  - display-timings: timings for the connected panel as described by [1]
> +  - cpu-timings: CPU interface timings for the connected panel, and it contains
> +        following optional properties.
> +          - cs-setup: clock cycles for the active period of address signal
> +                enable until chip select is enable in CPU interface
> +          - wr-setup: clock cycles for the active period of CS signal enable
> +                until write signal is enable in CPU interface
> +          - wr-act: clock cycles for the active period of CS enable in CPU
> +                interface
> +          - wr-hold: clock cycles for the active period of CS disable until
> +                write signal is disable in CPU interface
> +
> +Optional properties:
> +
> +The device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in [2]. This
> +node should describe panel's video bus.
> +
> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +
> +	panel@0 {
> +		compatible = "samsung,s6e3fa0";
> +		reg = <0>;
> +		vdd3-supply = <&vcclcd_reg>;
> +		vci-supply = <&vlcd_reg>;
> +		reset-gpio = <&gpy7 4 0>;
> +		det-gpio = <&gpg0 6 0>;
> +		te-gpio = <&gpd1 7 0>;
> +
> +		display-timings {
> +			timing0: timing-0 {
> +				clock-frequency = <0>;
> +				hactive = <1080>;
> +				vactive = <1920>;
> +				hfront-porch = <2>;
> +				hback-porch = <2>;
> +				hsync-len = <1>;
> +				vfront-porch = <1>;
> +				vback-porch = <4>;
> +				vsync-len = <1>;
> +			};
> +		};
> +
> +		cpu-timings {
> +			cs-setup = <0>;
> +			wr-setup = <0>;
> +			wr-act = <1>;
> +			wr-hold = <0>;
> +		};
> +	};
>

YoungJun Cho April 23, 2014, 1:26 a.m. UTC | #2

Hi Andrzej

Thank you for comment.

On 04/22/2014 11:02 PM, Andrzej Hajda wrote:
> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>> This patch adds DT bindings for s6e3fa0 panel.
>> The bindings describes panel resources, display timings and cpu timings.
>>
>> Changelog v2:
>> - Adds unit address (commented by Sachin Kamat)
>> Changelog v3:
>> - Removes optional delay, size properties (commented by Laurent Pinchart)
>> - Adds OLED detection, TE gpio properties
>> Changelog v4:
>> - Moves CPU timings relevant properties from FIMD DT
>>    (commeted by Laurent Pinchart, Andrzej Hajda)
>>
>> Signed-off-by: YoungJun Cho <[e-mail]>
>> Acked-by: Inki Dae <[e-mail]>
>> Acked-by: Kyungmin Park <[e-mail]>
>> ---
>>   .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>
>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> new file mode 100644
>> index 0000000..9eeb38b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> @@ -0,0 +1,63 @@
>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>> +
>> +Required properties:
>> +  - compatible: "samsung,s6e3fa0"
>> +  - reg: the virtual channel number of a DSI peripheral
>> +  - vdd3-supply: core voltage supply
>> +  - vci-supply: voltage supply for analog circuits
>> +  - reset-gpio: a GPIO spec for the reset pin
>> +  - det-gpio: a GPIO spec for the OLED detection pin
>> +  - te-gpio: a GPIO spec for the TE pin
>
> Just FYI, according to DT documentation [1] gpio spec should be in form
> [name]-gpios, however there is plenty bindings with -gpio suffix, so I
> am not sure if it is really enforced. On the other side it is enforced
> by descriptor based gpio framework[2]. Integer-based gpio framework
> used in your driver is obsolete according to [2].

Yes, you're right. That is my mistake.
They should be attached 's'.
At first I used integer-based gpio framework and replaced to descriptor
based one, but did not updated DT bindings.

I'll update again.

Thank you.
Best regards YJ

>
> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> [2]: Documentation/gpio/gpio.txt
>
> Regards
> Andrzej
>
>> +  - display-timings: timings for the connected panel as described by [1]
>> +  - cpu-timings: CPU interface timings for the connected panel, and it contains
>> +        following optional properties.
>> +          - cs-setup: clock cycles for the active period of address signal
>> +                enable until chip select is enable in CPU interface
>> +          - wr-setup: clock cycles for the active period of CS signal enable
>> +                until write signal is enable in CPU interface
>> +          - wr-act: clock cycles for the active period of CS enable in CPU
>> +                interface
>> +          - wr-hold: clock cycles for the active period of CS disable until
>> +                write signal is disable in CPU interface
>> +
>> +Optional properties:
>> +
>> +The device node can contain one 'port' child node with one child
>> +'endpoint' node, according to the bindings defined in [2]. This
>> +node should describe panel's video bus.
>> +
>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +Example:
>> +
>> +	panel@0 {
>> +		compatible = "samsung,s6e3fa0";
>> +		reg = <0>;
>> +		vdd3-supply = <&vcclcd_reg>;
>> +		vci-supply = <&vlcd_reg>;
>> +		reset-gpio = <&gpy7 4 0>;
>> +		det-gpio = <&gpg0 6 0>;
>> +		te-gpio = <&gpd1 7 0>;
>> +
>> +		display-timings {
>> +			timing0: timing-0 {
>> +				clock-frequency = <0>;
>> +				hactive = <1080>;
>> +				vactive = <1920>;
>> +				hfront-porch = <2>;
>> +				hback-porch = <2>;
>> +				hsync-len = <1>;
>> +				vfront-porch = <1>;
>> +				vback-porch = <4>;
>> +				vsync-len = <1>;
>> +			};
>> +		};
>> +
>> +		cpu-timings {
>> +			cs-setup = <0>;
>> +			wr-setup = <0>;
>> +			wr-act = <1>;
>> +			wr-hold = <0>;
>> +		};
>> +	};
>>
>
>

Thierry Reding April 23, 2014, 7:33 a.m. UTC | #3

On Wed, Apr 23, 2014 at 10:26:20AM +0900, YoungJun Cho wrote:
> Hi Andrzej
> 
> Thank you for comment.
> 
> On 04/22/2014 11:02 PM, Andrzej Hajda wrote:
> >On 04/21/2014 02:28 PM, YoungJun Cho wrote:
> >>This patch adds DT bindings for s6e3fa0 panel.
> >>The bindings describes panel resources, display timings and cpu timings.
> >>
> >>Changelog v2:
> >>- Adds unit address (commented by Sachin Kamat)
> >>Changelog v3:
> >>- Removes optional delay, size properties (commented by Laurent Pinchart)
> >>- Adds OLED detection, TE gpio properties
> >>Changelog v4:
> >>- Moves CPU timings relevant properties from FIMD DT
> >>   (commeted by Laurent Pinchart, Andrzej Hajda)
> >>
> >>Signed-off-by: YoungJun Cho <[e-mail]>
> >>Acked-by: Inki Dae <[e-mail]>
> >>Acked-by: Kyungmin Park <[e-mail]>
> >>---
> >>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
> >>  1 file changed, 63 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> >>
> >>diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> >>new file mode 100644
> >>index 0000000..9eeb38b
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> >>@@ -0,0 +1,63 @@
> >>+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> >>+
> >>+Required properties:
> >>+  - compatible: "samsung,s6e3fa0"
> >>+  - reg: the virtual channel number of a DSI peripheral
> >>+  - vdd3-supply: core voltage supply
> >>+  - vci-supply: voltage supply for analog circuits
> >>+  - reset-gpio: a GPIO spec for the reset pin
> >>+  - det-gpio: a GPIO spec for the OLED detection pin
> >>+  - te-gpio: a GPIO spec for the TE pin
> >
> >Just FYI, according to DT documentation [1] gpio spec should be in form
> >[name]-gpios, however there is plenty bindings with -gpio suffix, so I
> >am not sure if it is really enforced. On the other side it is enforced
> >by descriptor based gpio framework[2]. Integer-based gpio framework
> >used in your driver is obsolete according to [2].
> 
> Yes, you're right. That is my mistake.
> They should be attached 's'.
> At first I used integer-based gpio framework and replaced to descriptor
> based one, but did not updated DT bindings.

I've been working on a patch to support both *-gpios and *-gpio variants
with the GPIO descriptor framework. *-gpios makes sense if there can
indeed be several, but for something like hotplug detection I don't
think there's a reason to require the plural.

Furthermore some bindings already use the singular *-gpio anyway, so if
we ever want to convert drivers using those bindings to the GPIO
descriptor API we have to support that form too.

Thierry

Andrzej Hajda April 23, 2014, 9:02 a.m. UTC | #4

On 04/21/2014 02:28 PM, YoungJun Cho wrote:
> This patch adds DT bindings for s6e3fa0 panel.
> The bindings describes panel resources, display timings and cpu timings.
> 
> Changelog v2:
> - Adds unit address (commented by Sachin Kamat)
> Changelog v3:
> - Removes optional delay, size properties (commented by Laurent Pinchart)
> - Adds OLED detection, TE gpio properties
> Changelog v4:
> - Moves CPU timings relevant properties from FIMD DT
>   (commeted by Laurent Pinchart, Andrzej Hajda)
> 
> Signed-off-by: YoungJun Cho <[e-mail]>
> Acked-by: Inki Dae <[e-mail]>
> Acked-by: Kyungmin Park <[e-mail]>
> ---
>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> 
> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> new file mode 100644
> index 0000000..9eeb38b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> @@ -0,0 +1,63 @@
> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> +
> +Required properties:
> +  - compatible: "samsung,s6e3fa0"
> +  - reg: the virtual channel number of a DSI peripheral
> +  - vdd3-supply: core voltage supply
> +  - vci-supply: voltage supply for analog circuits
> +  - reset-gpio: a GPIO spec for the reset pin
> +  - det-gpio: a GPIO spec for the OLED detection pin
> +  - te-gpio: a GPIO spec for the TE pin
> +  - display-timings: timings for the connected panel as described by [1]

Which properties of display-timings are relevant for CPU mode?
I guess width and height. Anything more?

> +  - cpu-timings: CPU interface timings for the connected panel, and it contains
> +        following optional properties.
> +          - cs-setup: clock cycles for the active period of address signal
> +                enable until chip select is enable in CPU interface
> +          - wr-setup: clock cycles for the active period of CS signal enable
> +                until write signal is enable in CPU interface
> +          - wr-act: clock cycles for the active period of CS enable in CPU
> +                interface
> +          - wr-hold: clock cycles for the active period of CS disable until
> +                write signal is disable in CPU interface

cpu-timings name does not sound to be related to display.
I wonder if it would not be better to merge cpu-timings into
display-timings but this will require more discussion I guess.

If you want to stay with separate node please consider to make it
optional as whole node or make some properties required. Making node
required with all sub-properties optional is weird.
By the way I hope all timings properties are generic for CPU mode,
if not they should be prefixed by vendor or model.

Regards
Andrzej

> +
> +Optional properties:
> +
> +The device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in [2]. This
> +node should describe panel's video bus.
> +
> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +
> +	panel@0 {
> +		compatible = "samsung,s6e3fa0";
> +		reg = <0>;
> +		vdd3-supply = <&vcclcd_reg>;
> +		vci-supply = <&vlcd_reg>;
> +		reset-gpio = <&gpy7 4 0>;
> +		det-gpio = <&gpg0 6 0>;
> +		te-gpio = <&gpd1 7 0>;
> +
> +		display-timings {
> +			timing0: timing-0 {
> +				clock-frequency = <0>;
> +				hactive = <1080>;
> +				vactive = <1920>;
> +				hfront-porch = <2>;
> +				hback-porch = <2>;
> +				hsync-len = <1>;
> +				vfront-porch = <1>;
> +				vback-porch = <4>;
> +				vsync-len = <1>;
> +			};
> +		};
> +
> +		cpu-timings {
> +			cs-setup = <0>;
> +			wr-setup = <0>;
> +			wr-act = <1>;
> +			wr-hold = <0>;
> +		};
> +	};
>

Laurent Pinchart April 23, 2014, 11:34 a.m. UTC | #5

Hi Andrzej,

On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
> > This patch adds DT bindings for s6e3fa0 panel.
> > The bindings describes panel resources, display timings and cpu timings.
> > 
> > Changelog v2:
> > - Adds unit address (commented by Sachin Kamat)
> > Changelog v3:
> > - Removes optional delay, size properties (commented by Laurent Pinchart)
> > - Adds OLED detection, TE gpio properties
> > Changelog v4:
> > - Moves CPU timings relevant properties from FIMD DT
> > 
> >   (commeted by Laurent Pinchart, Andrzej Hajda)
> > 
> > Signed-off-by: YoungJun Cho <[e-mail]>
> > Acked-by: Inki Dae <[e-mail]>
> > Acked-by: Kyungmin Park <[e-mail]>
> > ---
> > 
> >  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 +++++++++++++++
> > 1 file changed, 63 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt> 
> > diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> > b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
> > mode 100644
> > index 0000000..9eeb38b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> > @@ -0,0 +1,63 @@
> > +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> > +
> > +Required properties:
> > +  - compatible: "samsung,s6e3fa0"
> > +  - reg: the virtual channel number of a DSI peripheral
> > +  - vdd3-supply: core voltage supply
> > +  - vci-supply: voltage supply for analog circuits
> > +  - reset-gpio: a GPIO spec for the reset pin
> > +  - det-gpio: a GPIO spec for the OLED detection pin
> > +  - te-gpio: a GPIO spec for the TE pin
> > +  - display-timings: timings for the connected panel as described by [1]
> 
> Which properties of display-timings are relevant for CPU mode?
> I guess width and height. Anything more?
> 
> > +  - cpu-timings: CPU interface timings for the connected panel, and it
> > contains
> > +        following optional properties.
> > +          - cs-setup: clock cycles for the active period of address
> > signal
> > +                enable until chip select is enable in CPU interface
> > +          - wr-setup: clock cycles for the active period of CS signal
> > enable
> > +                until write signal is enable in CPU interface
> > +          - wr-act: clock cycles for the active period of CS enable in
> > CPU
> > +                interface

What about calling this property wr-active ? I had called this wr-cycle in a 
previous implementation, that could be an option as well.

> > +          - wr-hold: clock cycles for the active period of CS disable
> > until
> > +                write signal is disable in CPU interface
> 
> cpu-timings name does not sound to be related to display.
> I wonder if it would not be better to merge cpu-timings into
> display-timings but this will require more discussion I guess.

The panel has a memory-like interface with chip select, read/write and access 
strobe, address (1 bit) and data signals. The interface is defined in the MIPI 
DBI specification (a quick search turned up 
http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might be 
direct PDF downloads available), even if some panels implement a similar 
interface that predates MIPI DBI (with names such as i80 or SYS-80 probably 
due to the similarities with the 8051 external bus).

The cpu-timings properties describe the read and write access timings for 
those signals, unrelated to the display video timings. I thus believe that 
they should be separate from the display timings. We will probably need to add 
properties for the read signal as well, but that can be done later.

> If you want to stay with separate node please consider to make it
> optional as whole node or make some properties required. Making node
> required with all sub-properties optional is weird.
> By the way I hope all timings properties are generic for CPU mode,
> if not they should be prefixed by vendor or model.

The timings description should be pretty generic I believe, especially given 
that the interface is standardized by the MIPI alliance. Could you have a 
quick look at the spec and share your opinion ?

> > +
> > +Optional properties:
> > +
> > +The device node can contain one 'port' child node with one child
> > +'endpoint' node, according to the bindings defined in [2]. This
> > +node should describe panel's video bus.
> > +
> > +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> > +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +Example:
> > +
> > +	panel@0 {
> > +		compatible = "samsung,s6e3fa0";
> > +		reg = <0>;
> > +		vdd3-supply = <&vcclcd_reg>;
> > +		vci-supply = <&vlcd_reg>;
> > +		reset-gpio = <&gpy7 4 0>;
> > +		det-gpio = <&gpg0 6 0>;
> > +		te-gpio = <&gpd1 7 0>;
> > +
> > +		display-timings {
> > +			timing0: timing-0 {
> > +				clock-frequency = <0>;
> > +				hactive = <1080>;
> > +				vactive = <1920>;
> > +				hfront-porch = <2>;
> > +				hback-porch = <2>;
> > +				hsync-len = <1>;
> > +				vfront-porch = <1>;
> > +				vback-porch = <4>;
> > +				vsync-len = <1>;
> > +			};
> > +		};
> > +
> > +		cpu-timings {
> > +			cs-setup = <0>;
> > +			wr-setup = <0>;
> > +			wr-act = <1>;
> > +			wr-hold = <0>;
> > +		};
> > +	};

Andrzej Hajda April 23, 2014, 12:48 p.m. UTC | #6

On 04/23/2014 01:34 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
>> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>>> This patch adds DT bindings for s6e3fa0 panel.
>>> The bindings describes panel resources, display timings and cpu timings.
>>>
>>> Changelog v2:
>>> - Adds unit address (commented by Sachin Kamat)
>>> Changelog v3:
>>> - Removes optional delay, size properties (commented by Laurent Pinchart)
>>> - Adds OLED detection, TE gpio properties
>>> Changelog v4:
>>> - Moves CPU timings relevant properties from FIMD DT
>>>
>>>   (commeted by Laurent Pinchart, Andrzej Hajda)
>>>
>>> Signed-off-by: YoungJun Cho <[e-mail]>
>>> Acked-by: Inki Dae <[e-mail]>
>>> Acked-by: Kyungmin Park <[e-mail]>
>>> ---
>>>
>>>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 +++++++++++++++
>>> 1 file changed, 63 insertions(+)
>>>  create mode 100644
>>>  Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt> 
>>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>> b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
>>> mode 100644
>>> index 0000000..9eeb38b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>> @@ -0,0 +1,63 @@
>>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>>> +
>>> +Required properties:
>>> +  - compatible: "samsung,s6e3fa0"
>>> +  - reg: the virtual channel number of a DSI peripheral
>>> +  - vdd3-supply: core voltage supply
>>> +  - vci-supply: voltage supply for analog circuits
>>> +  - reset-gpio: a GPIO spec for the reset pin
>>> +  - det-gpio: a GPIO spec for the OLED detection pin
>>> +  - te-gpio: a GPIO spec for the TE pin
>>> +  - display-timings: timings for the connected panel as described by [1]
>> Which properties of display-timings are relevant for CPU mode?
>> I guess width and height. Anything more?
>>
>>> +  - cpu-timings: CPU interface timings for the connected panel, and it
>>> contains
>>> +        following optional properties.
>>> +          - cs-setup: clock cycles for the active period of address
>>> signal
>>> +                enable until chip select is enable in CPU interface
>>> +          - wr-setup: clock cycles for the active period of CS signal
>>> enable
>>> +                until write signal is enable in CPU interface
>>> +          - wr-act: clock cycles for the active period of CS enable in
>>> CPU
>>> +                interface
> What about calling this property wr-active ? I had called this wr-cycle in a 
> previous implementation, that could be an option as well.
>
>>> +          - wr-hold: clock cycles for the active period of CS disable
>>> until
>>> +                write signal is disable in CPU interface
>> cpu-timings name does not sound to be related to display.
>> I wonder if it would not be better to merge cpu-timings into
>> display-timings but this will require more discussion I guess.
> The panel has a memory-like interface with chip select, read/write and access 
> strobe, address (1 bit) and data signals. The interface is defined in the MIPI 
> DBI specification (a quick search turned up 
> http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might be 
> direct PDF downloads available), even if some panels implement a similar 
> interface that predates MIPI DBI (with names such as i80 or SYS-80 probably 
> due to the similarities with the 8051 external bus).
>
> The cpu-timings properties describe the read and write access timings for 
> those signals, unrelated to the display video timings. I thus believe that 
> they should be separate from the display timings. We will probably need to add 
> properties for the read signal as well, but that can be done later.

cpu-timings suggests these timings are for CPU, but they are for display
panel in CPU mode.
I though rather about something like display-cpu-timings or i80-timings,
just to avoid confusion.

Regards
Andrzej

>> If you want to stay with separate node please consider to make it
>> optional as whole node or make some properties required. Making node
>> required with all sub-properties optional is weird.
>> By the way I hope all timings properties are generic for CPU mode,
>> if not they should be prefixed by vendor or model.
> The timings description should be pretty generic I believe, especially given 
> that the interface is standardized by the MIPI alliance. Could you have a 
> quick look at the spec and share your opinion ?
>
>>> +
>>> +Optional properties:
>>> +
>>> +The device node can contain one 'port' child node with one child
>>> +'endpoint' node, according to the bindings defined in [2]. This
>>> +node should describe panel's video bus.
>>> +
>>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>> +
>>> +Example:
>>> +
>>> +	panel@0 {
>>> +		compatible = "samsung,s6e3fa0";
>>> +		reg = <0>;
>>> +		vdd3-supply = <&vcclcd_reg>;
>>> +		vci-supply = <&vlcd_reg>;
>>> +		reset-gpio = <&gpy7 4 0>;
>>> +		det-gpio = <&gpg0 6 0>;
>>> +		te-gpio = <&gpd1 7 0>;
>>> +
>>> +		display-timings {
>>> +			timing0: timing-0 {
>>> +				clock-frequency = <0>;
>>> +				hactive = <1080>;
>>> +				vactive = <1920>;
>>> +				hfront-porch = <2>;
>>> +				hback-porch = <2>;
>>> +				hsync-len = <1>;
>>> +				vfront-porch = <1>;
>>> +				vback-porch = <4>;
>>> +				vsync-len = <1>;
>>> +			};
>>> +		};
>>> +
>>> +		cpu-timings {
>>> +			cs-setup = <0>;
>>> +			wr-setup = <0>;
>>> +			wr-act = <1>;
>>> +			wr-hold = <0>;
>>> +		};
>>> +	};

Laurent Pinchart April 23, 2014, 12:55 p.m. UTC | #7

Hi Andrzej,

On Wednesday 23 April 2014 14:48:31 Andrzej Hajda wrote:
> On 04/23/2014 01:34 PM, Laurent Pinchart wrote:
> > On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
> >> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
> >>> This patch adds DT bindings for s6e3fa0 panel.
> >>> The bindings describes panel resources, display timings and cpu timings.
> >>> 
> >>> Changelog v2:
> >>> - Adds unit address (commented by Sachin Kamat)
> >>> Changelog v3:
> >>> - Removes optional delay, size properties (commented by Laurent
> >>> Pinchart)
> >>> - Adds OLED detection, TE gpio properties
> >>> Changelog v4:
> >>> - Moves CPU timings relevant properties from FIMD DT
> >>> 
> >>>   (commeted by Laurent Pinchart, Andrzej Hajda)
> >>> 
> >>> Signed-off-by: YoungJun Cho <[e-mail]>
> >>> Acked-by: Inki Dae <[e-mail]>
> >>> Acked-by: Kyungmin Park <[e-mail]>
> >>> ---
> >>> 
> >>>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63
> >>>  +++++++++++++++
> >>> 
> >>> 1 file changed, 63 insertions(+)
> >>> 
> >>>  create mode 100644
> >>>  Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt>
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> >>> b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
> >>> mode 100644
> >>> index 0000000..9eeb38b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> >>> @@ -0,0 +1,63 @@
> >>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> >>> +
> >>> +Required properties:
> >>> +  - compatible: "samsung,s6e3fa0"
> >>> +  - reg: the virtual channel number of a DSI peripheral
> >>> +  - vdd3-supply: core voltage supply
> >>> +  - vci-supply: voltage supply for analog circuits
> >>> +  - reset-gpio: a GPIO spec for the reset pin
> >>> +  - det-gpio: a GPIO spec for the OLED detection pin
> >>> +  - te-gpio: a GPIO spec for the TE pin
> >>> +  - display-timings: timings for the connected panel as described by
> >>> [1]
> >> 
> >> Which properties of display-timings are relevant for CPU mode?
> >> I guess width and height. Anything more?
> >> 
> >>> +  - cpu-timings: CPU interface timings for the connected panel, and it
> >>> contains
> >>> +        following optional properties.
> >>> +          - cs-setup: clock cycles for the active period of address
> >>> signal
> >>> +                enable until chip select is enable in CPU interface
> >>> +          - wr-setup: clock cycles for the active period of CS signal
> >>> enable
> >>> +                until write signal is enable in CPU interface
> >>> +          - wr-act: clock cycles for the active period of CS enable in
> >>> CPU
> >>> +                interface
> > 
> > What about calling this property wr-active ? I had called this wr-cycle in
> > a previous implementation, that could be an option as well.
> > 
> >>> +          - wr-hold: clock cycles for the active period of CS disable
> >>> until
> >>> +                write signal is disable in CPU interface
> >> 
> >> cpu-timings name does not sound to be related to display.
> >> I wonder if it would not be better to merge cpu-timings into
> >> display-timings but this will require more discussion I guess.
> > 
> > The panel has a memory-like interface with chip select, read/write and
> > access strobe, address (1 bit) and data signals. The interface is defined
> > in the MIPI DBI specification (a quick search turned up
> > http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might
> > be direct PDF downloads available), even if some panels implement a
> > similar interface that predates MIPI DBI (with names such as i80 or
> > SYS-80 probably due to the similarities with the 8051 external bus).
> > 
> > The cpu-timings properties describe the read and write access timings for
> > those signals, unrelated to the display video timings. I thus believe that
> > they should be separate from the display timings. We will probably need to
> > add properties for the read signal as well, but that can be done later.
>
> cpu-timings suggests these timings are for CPU, but they are for display
> panel in CPU mode.
> I though rather about something like display-cpu-timings or i80-timings,
> just to avoid confusion.

mipi-dbi-timings maybe ?

> >> If you want to stay with separate node please consider to make it
> >> optional as whole node or make some properties required. Making node
> >> required with all sub-properties optional is weird.
> >> By the way I hope all timings properties are generic for CPU mode,
> >> if not they should be prefixed by vendor or model.
> > 
> > The timings description should be pretty generic I believe, especially
> > given that the interface is standardized by the MIPI alliance. Could you
> > have a quick look at the spec and share your opinion ?
> > 
> >>> +
> >>> +Optional properties:
> >>> +
> >>> +The device node can contain one 'port' child node with one child
> >>> +'endpoint' node, according to the bindings defined in [2]. This
> >>> +node should describe panel's video bus.
> >>> +
> >>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> >>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
> >>> +
> >>> +Example:
> >>> +
> >>> +	panel@0 {
> >>> +		compatible = "samsung,s6e3fa0";
> >>> +		reg = <0>;
> >>> +		vdd3-supply = <&vcclcd_reg>;
> >>> +		vci-supply = <&vlcd_reg>;
> >>> +		reset-gpio = <&gpy7 4 0>;
> >>> +		det-gpio = <&gpg0 6 0>;
> >>> +		te-gpio = <&gpd1 7 0>;
> >>> +
> >>> +		display-timings {
> >>> +			timing0: timing-0 {
> >>> +				clock-frequency = <0>;
> >>> +				hactive = <1080>;
> >>> +				vactive = <1920>;
> >>> +				hfront-porch = <2>;
> >>> +				hback-porch = <2>;
> >>> +				hsync-len = <1>;
> >>> +				vfront-porch = <1>;
> >>> +				vback-porch = <4>;
> >>> +				vsync-len = <1>;
> >>> +			};
> >>> +		};
> >>> +
> >>> +		cpu-timings {
> >>> +			cs-setup = <0>;
> >>> +			wr-setup = <0>;
> >>> +			wr-act = <1>;
> >>> +			wr-hold = <0>;
> >>> +		};
> >>> +	};

Andrzej Hajda April 23, 2014, 1:33 p.m. UTC | #8

On 04/23/2014 02:55 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wednesday 23 April 2014 14:48:31 Andrzej Hajda wrote:
>> On 04/23/2014 01:34 PM, Laurent Pinchart wrote:
>>> On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
>>>> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>>>>> This patch adds DT bindings for s6e3fa0 panel.
>>>>> The bindings describes panel resources, display timings and cpu timings.
>>>>>
>>>>> Changelog v2:
>>>>> - Adds unit address (commented by Sachin Kamat)
>>>>> Changelog v3:
>>>>> - Removes optional delay, size properties (commented by Laurent
>>>>> Pinchart)
>>>>> - Adds OLED detection, TE gpio properties
>>>>> Changelog v4:
>>>>> - Moves CPU timings relevant properties from FIMD DT
>>>>>
>>>>>   (commeted by Laurent Pinchart, Andrzej Hajda)
>>>>>
>>>>> Signed-off-by: YoungJun Cho <[e-mail]>
>>>>> Acked-by: Inki Dae <[e-mail]>
>>>>> Acked-by: Kyungmin Park <[e-mail]>
>>>>> ---
>>>>>
>>>>>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63
>>>>>  +++++++++++++++
>>>>>
>>>>> 1 file changed, 63 insertions(+)
>>>>>
>>>>>  create mode 100644
>>>>>  Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt>
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>>>> b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
>>>>> mode 100644
>>>>> index 0000000..9eeb38b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>>>> @@ -0,0 +1,63 @@
>>>>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: "samsung,s6e3fa0"
>>>>> +  - reg: the virtual channel number of a DSI peripheral
>>>>> +  - vdd3-supply: core voltage supply
>>>>> +  - vci-supply: voltage supply for analog circuits
>>>>> +  - reset-gpio: a GPIO spec for the reset pin
>>>>> +  - det-gpio: a GPIO spec for the OLED detection pin
>>>>> +  - te-gpio: a GPIO spec for the TE pin
>>>>> +  - display-timings: timings for the connected panel as described by
>>>>> [1]
>>>> Which properties of display-timings are relevant for CPU mode?
>>>> I guess width and height. Anything more?
>>>>
>>>>> +  - cpu-timings: CPU interface timings for the connected panel, and it
>>>>> contains
>>>>> +        following optional properties.
>>>>> +          - cs-setup: clock cycles for the active period of address
>>>>> signal
>>>>> +                enable until chip select is enable in CPU interface
>>>>> +          - wr-setup: clock cycles for the active period of CS signal
>>>>> enable
>>>>> +                until write signal is enable in CPU interface
>>>>> +          - wr-act: clock cycles for the active period of CS enable in
>>>>> CPU
>>>>> +                interface
>>> What about calling this property wr-active ? I had called this wr-cycle in
>>> a previous implementation, that could be an option as well.
>>>
>>>>> +          - wr-hold: clock cycles for the active period of CS disable
>>>>> until
>>>>> +                write signal is disable in CPU interface
>>>> cpu-timings name does not sound to be related to display.
>>>> I wonder if it would not be better to merge cpu-timings into
>>>> display-timings but this will require more discussion I guess.
>>> The panel has a memory-like interface with chip select, read/write and
>>> access strobe, address (1 bit) and data signals. The interface is defined
>>> in the MIPI DBI specification (a quick search turned up
>>> http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might
>>> be direct PDF downloads available), even if some panels implement a
>>> similar interface that predates MIPI DBI (with names such as i80 or
>>> SYS-80 probably due to the similarities with the 8051 external bus).
>>>
>>> The cpu-timings properties describe the read and write access timings for
>>> those signals, unrelated to the display video timings. I thus believe that
>>> they should be separate from the display timings. We will probably need to
>>> add properties for the read signal as well, but that can be done later.
>> cpu-timings suggests these timings are for CPU, but they are for display
>> panel in CPU mode.
>> I though rather about something like display-cpu-timings or i80-timings,
>> just to avoid confusion.
> mipi-dbi-timings maybe ?

It looks OK.

I guess only hactive, and vactive props of display-timings are used,
maybe some flags?
I wonder if it would not be better to drop display-timings node
and place all props into mipi-dbi-timings or mipi-dbi-settings node.
Or rather all timings props should be put into port/endpoint node with
or without
any container node.

Regards
Andrzej

>
>>>> If you want to stay with separate node please consider to make it
>>>> optional as whole node or make some properties required. Making node
>>>> required with all sub-properties optional is weird.
>>>> By the way I hope all timings properties are generic for CPU mode,
>>>> if not they should be prefixed by vendor or model.
>>> The timings description should be pretty generic I believe, especially
>>> given that the interface is standardized by the MIPI alliance. Could you
>>> have a quick look at the spec and share your opinion ?
>>>
>>>>> +
>>>>> +Optional properties:
>>>>> +
>>>>> +The device node can contain one 'port' child node with one child
>>>>> +'endpoint' node, according to the bindings defined in [2]. This
>>>>> +node should describe panel's video bus.
>>>>> +
>>>>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>>>>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +	panel@0 {
>>>>> +		compatible = "samsung,s6e3fa0";
>>>>> +		reg = <0>;
>>>>> +		vdd3-supply = <&vcclcd_reg>;
>>>>> +		vci-supply = <&vlcd_reg>;
>>>>> +		reset-gpio = <&gpy7 4 0>;
>>>>> +		det-gpio = <&gpg0 6 0>;
>>>>> +		te-gpio = <&gpd1 7 0>;
>>>>> +
>>>>> +		display-timings {
>>>>> +			timing0: timing-0 {
>>>>> +				clock-frequency = <0>;
>>>>> +				hactive = <1080>;
>>>>> +				vactive = <1920>;
>>>>> +				hfront-porch = <2>;
>>>>> +				hback-porch = <2>;
>>>>> +				hsync-len = <1>;
>>>>> +				vfront-porch = <1>;
>>>>> +				vback-porch = <4>;
>>>>> +				vsync-len = <1>;
>>>>> +			};
>>>>> +		};
>>>>> +
>>>>> +		cpu-timings {
>>>>> +			cs-setup = <0>;
>>>>> +			wr-setup = <0>;
>>>>> +			wr-act = <1>;
>>>>> +			wr-hold = <0>;
>>>>> +		};
>>>>> +	};

YoungJun Cho April 24, 2014, 1:31 a.m. UTC | #9

Hi Andrzej,

Thank you for comments.

On 04/23/2014 06:02 PM, Andrzej Hajda wrote:
> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>> This patch adds DT bindings for s6e3fa0 panel.
>> The bindings describes panel resources, display timings and cpu timings.
>>
>> Changelog v2:
>> - Adds unit address (commented by Sachin Kamat)
>> Changelog v3:
>> - Removes optional delay, size properties (commented by Laurent Pinchart)
>> - Adds OLED detection, TE gpio properties
>> Changelog v4:
>> - Moves CPU timings relevant properties from FIMD DT
>>    (commeted by Laurent Pinchart, Andrzej Hajda)
>>
>> Signed-off-by: YoungJun Cho <[e-mail]>
>> Acked-by: Inki Dae <[e-mail]>
>> Acked-by: Kyungmin Park <[e-mail]>
>> ---
>>   .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 ++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>
>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> new file mode 100644
>> index 0000000..9eeb38b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> @@ -0,0 +1,63 @@
>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>> +
>> +Required properties:
>> +  - compatible: "samsung,s6e3fa0"
>> +  - reg: the virtual channel number of a DSI peripheral
>> +  - vdd3-supply: core voltage supply
>> +  - vci-supply: voltage supply for analog circuits
>> +  - reset-gpio: a GPIO spec for the reset pin
>> +  - det-gpio: a GPIO spec for the OLED detection pin
>> +  - te-gpio: a GPIO spec for the TE pin
>> +  - display-timings: timings for the connected panel as described by [1]
>
> Which properties of display-timings are relevant for CPU mode?
> I guess width and height. Anything more?

The CPU interface panel also requires hfront-porch, hback-porch,
hsync-len, vfront-porch, vback-porch and vsync-len.

>
>> +  - cpu-timings: CPU interface timings for the connected panel, and it contains
>> +        following optional properties.
>> +          - cs-setup: clock cycles for the active period of address signal
>> +                enable until chip select is enable in CPU interface
>> +          - wr-setup: clock cycles for the active period of CS signal enable
>> +                until write signal is enable in CPU interface
>> +          - wr-act: clock cycles for the active period of CS enable in CPU
>> +                interface
>> +          - wr-hold: clock cycles for the active period of CS disable until
>> +                write signal is disable in CPU interface
>
> cpu-timings name does not sound to be related to display.
> I wonder if it would not be better to merge cpu-timings into
> display-timings but this will require more discussion I guess.
>
> If you want to stay with separate node please consider to make it
> optional as whole node or make some properties required. Making node
> required with all sub-properties optional is weird.

Yes, I'll fix.

Thank you.
Best regards YJ

> By the way I hope all timings properties are generic for CPU mode,
> if not they should be prefixed by vendor or model.
>
> Regards
> Andrzej
>
>> +
>> +Optional properties:
>> +
>> +The device node can contain one 'port' child node with one child
>> +'endpoint' node, according to the bindings defined in [2]. This
>> +node should describe panel's video bus.
>> +
>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +Example:
>> +
>> +	panel@0 {
>> +		compatible = "samsung,s6e3fa0";
>> +		reg = <0>;
>> +		vdd3-supply = <&vcclcd_reg>;
>> +		vci-supply = <&vlcd_reg>;
>> +		reset-gpio = <&gpy7 4 0>;
>> +		det-gpio = <&gpg0 6 0>;
>> +		te-gpio = <&gpd1 7 0>;
>> +
>> +		display-timings {
>> +			timing0: timing-0 {
>> +				clock-frequency = <0>;
>> +				hactive = <1080>;
>> +				vactive = <1920>;
>> +				hfront-porch = <2>;
>> +				hback-porch = <2>;
>> +				hsync-len = <1>;
>> +				vfront-porch = <1>;
>> +				vback-porch = <4>;
>> +				vsync-len = <1>;
>> +			};
>> +		};
>> +
>> +		cpu-timings {
>> +			cs-setup = <0>;
>> +			wr-setup = <0>;
>> +			wr-act = <1>;
>> +			wr-hold = <0>;
>> +		};
>> +	};
>>
>
>

YoungJun Cho April 24, 2014, 3:15 a.m. UTC | #10

Hi Laurent,

Thank you for comments.

On 04/23/2014 08:34 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
>> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>>> This patch adds DT bindings for s6e3fa0 panel.
>>> The bindings describes panel resources, display timings and cpu timings.
>>>
>>> Changelog v2:
>>> - Adds unit address (commented by Sachin Kamat)
>>> Changelog v3:
>>> - Removes optional delay, size properties (commented by Laurent Pinchart)
>>> - Adds OLED detection, TE gpio properties
>>> Changelog v4:
>>> - Moves CPU timings relevant properties from FIMD DT
>>>
>>>    (commeted by Laurent Pinchart, Andrzej Hajda)
>>>
>>> Signed-off-by: YoungJun Cho <[e-mail]>
>>> Acked-by: Inki Dae <[e-mail]>
>>> Acked-by: Kyungmin Park <[e-mail]>
>>> ---
>>>
>>>   .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63 +++++++++++++++
>>> 1 file changed, 63 insertions(+)
>>>   create mode 100644
>>>   Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt>
>>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>> b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
>>> mode 100644
>>> index 0000000..9eeb38b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>> @@ -0,0 +1,63 @@
>>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>>> +
>>> +Required properties:
>>> +  - compatible: "samsung,s6e3fa0"
>>> +  - reg: the virtual channel number of a DSI peripheral
>>> +  - vdd3-supply: core voltage supply
>>> +  - vci-supply: voltage supply for analog circuits
>>> +  - reset-gpio: a GPIO spec for the reset pin
>>> +  - det-gpio: a GPIO spec for the OLED detection pin
>>> +  - te-gpio: a GPIO spec for the TE pin
>>> +  - display-timings: timings for the connected panel as described by [1]
>>
>> Which properties of display-timings are relevant for CPU mode?
>> I guess width and height. Anything more?
>>
>>> +  - cpu-timings: CPU interface timings for the connected panel, and it
>>> contains
>>> +        following optional properties.
>>> +          - cs-setup: clock cycles for the active period of address
>>> signal
>>> +                enable until chip select is enable in CPU interface
>>> +          - wr-setup: clock cycles for the active period of CS signal
>>> enable
>>> +                until write signal is enable in CPU interface
>>> +          - wr-act: clock cycles for the active period of CS enable in
>>> CPU
>>> +                interface
>
> What about calling this property wr-active ? I had called this wr-cycle in a
> previous implementation, that could be an option as well.

Okay, I'll use wr-active. It's better.

>
>>> +          - wr-hold: clock cycles for the active period of CS disable
>>> until
>>> +                write signal is disable in CPU interface
>>
>> cpu-timings name does not sound to be related to display.
>> I wonder if it would not be better to merge cpu-timings into
>> display-timings but this will require more discussion I guess.
>
> The panel has a memory-like interface with chip select, read/write and access
> strobe, address (1 bit) and data signals. The interface is defined in the MIPI
> DBI specification (a quick search turned up
> http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might be
> direct PDF downloads available), even if some panels implement a similar
> interface that predates MIPI DBI (with names such as i80 or SYS-80 probably
> due to the similarities with the 8051 external bus).
>
> The cpu-timings properties describe the read and write access timings for
> those signals, unrelated to the display video timings. I thus believe that
> they should be separate from the display timings. We will probably need to add
> properties for the read signal as well, but that can be done later.

Yes, as I wrote another thread before, this cpu interface timings is
additional one. The video timings is required also.

Thank you.
Best regards YJ

>
>> If you want to stay with separate node please consider to make it
>> optional as whole node or make some properties required. Making node
>> required with all sub-properties optional is weird.
>> By the way I hope all timings properties are generic for CPU mode,
>> if not they should be prefixed by vendor or model.
>
> The timings description should be pretty generic I believe, especially given
> that the interface is standardized by the MIPI alliance. Could you have a
> quick look at the spec and share your opinion ?
>
>>> +
>>> +Optional properties:
>>> +
>>> +The device node can contain one 'port' child node with one child
>>> +'endpoint' node, according to the bindings defined in [2]. This
>>> +node should describe panel's video bus.
>>> +
>>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>> +
>>> +Example:
>>> +
>>> +	panel@0 {
>>> +		compatible = "samsung,s6e3fa0";
>>> +		reg = <0>;
>>> +		vdd3-supply = <&vcclcd_reg>;
>>> +		vci-supply = <&vlcd_reg>;
>>> +		reset-gpio = <&gpy7 4 0>;
>>> +		det-gpio = <&gpg0 6 0>;
>>> +		te-gpio = <&gpd1 7 0>;
>>> +
>>> +		display-timings {
>>> +			timing0: timing-0 {
>>> +				clock-frequency = <0>;
>>> +				hactive = <1080>;
>>> +				vactive = <1920>;
>>> +				hfront-porch = <2>;
>>> +				hback-porch = <2>;
>>> +				hsync-len = <1>;
>>> +				vfront-porch = <1>;
>>> +				vback-porch = <4>;
>>> +				vsync-len = <1>;
>>> +			};
>>> +		};
>>> +
>>> +		cpu-timings {
>>> +			cs-setup = <0>;
>>> +			wr-setup = <0>;
>>> +			wr-act = <1>;
>>> +			wr-hold = <0>;
>>> +		};
>>> +	};
>

YoungJun Cho April 24, 2014, 3:34 a.m. UTC | #11

Hi Andrzej,

On 04/23/2014 10:33 PM, Andrzej Hajda wrote:
> On 04/23/2014 02:55 PM, Laurent Pinchart wrote:
>> Hi Andrzej,
>>
>> On Wednesday 23 April 2014 14:48:31 Andrzej Hajda wrote:
>>> On 04/23/2014 01:34 PM, Laurent Pinchart wrote:
>>>> On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
>>>>> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>>>>>> This patch adds DT bindings for s6e3fa0 panel.
>>>>>> The bindings describes panel resources, display timings and cpu timings.
>>>>>>
>>>>>> Changelog v2:
>>>>>> - Adds unit address (commented by Sachin Kamat)
>>>>>> Changelog v3:
>>>>>> - Removes optional delay, size properties (commented by Laurent
>>>>>> Pinchart)
>>>>>> - Adds OLED detection, TE gpio properties
>>>>>> Changelog v4:
>>>>>> - Moves CPU timings relevant properties from FIMD DT
>>>>>>
>>>>>>    (commeted by Laurent Pinchart, Andrzej Hajda)
>>>>>>
>>>>>> Signed-off-by: YoungJun Cho <[e-mail]>
>>>>>> Acked-by: Inki Dae <[e-mail]>
>>>>>> Acked-by: Kyungmin Park <[e-mail]>
>>>>>> ---
>>>>>>
>>>>>>   .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |   63
>>>>>>   +++++++++++++++
>>>>>>
>>>>>> 1 file changed, 63 insertions(+)
>>>>>>
>>>>>>   create mode 100644
>>>>>>   Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt>
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>>>>> b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
>>>>>> mode 100644
>>>>>> index 0000000..9eeb38b
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>>>>> @@ -0,0 +1,63 @@
>>>>>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>>>>>> +
>>>>>> +Required properties:
>>>>>> +  - compatible: "samsung,s6e3fa0"
>>>>>> +  - reg: the virtual channel number of a DSI peripheral
>>>>>> +  - vdd3-supply: core voltage supply
>>>>>> +  - vci-supply: voltage supply for analog circuits
>>>>>> +  - reset-gpio: a GPIO spec for the reset pin
>>>>>> +  - det-gpio: a GPIO spec for the OLED detection pin
>>>>>> +  - te-gpio: a GPIO spec for the TE pin
>>>>>> +  - display-timings: timings for the connected panel as described by
>>>>>> [1]
>>>>> Which properties of display-timings are relevant for CPU mode?
>>>>> I guess width and height. Anything more?
>>>>>
>>>>>> +  - cpu-timings: CPU interface timings for the connected panel, and it
>>>>>> contains
>>>>>> +        following optional properties.
>>>>>> +          - cs-setup: clock cycles for the active period of address
>>>>>> signal
>>>>>> +                enable until chip select is enable in CPU interface
>>>>>> +          - wr-setup: clock cycles for the active period of CS signal
>>>>>> enable
>>>>>> +                until write signal is enable in CPU interface
>>>>>> +          - wr-act: clock cycles for the active period of CS enable in
>>>>>> CPU
>>>>>> +                interface
>>>> What about calling this property wr-active ? I had called this wr-cycle in
>>>> a previous implementation, that could be an option as well.
>>>>
>>>>>> +          - wr-hold: clock cycles for the active period of CS disable
>>>>>> until
>>>>>> +                write signal is disable in CPU interface
>>>>> cpu-timings name does not sound to be related to display.
>>>>> I wonder if it would not be better to merge cpu-timings into
>>>>> display-timings but this will require more discussion I guess.
>>>> The panel has a memory-like interface with chip select, read/write and
>>>> access strobe, address (1 bit) and data signals. The interface is defined
>>>> in the MIPI DBI specification (a quick search turned up
>>>> http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might
>>>> be direct PDF downloads available), even if some panels implement a
>>>> similar interface that predates MIPI DBI (with names such as i80 or
>>>> SYS-80 probably due to the similarities with the 8051 external bus).
>>>>
>>>> The cpu-timings properties describe the read and write access timings for
>>>> those signals, unrelated to the display video timings. I thus believe that
>>>> they should be separate from the display timings. We will probably need to
>>>> add properties for the read signal as well, but that can be done later.
>>> cpu-timings suggests these timings are for CPU, but they are for display
>>> panel in CPU mode.
>>> I though rather about something like display-cpu-timings or i80-timings,
>>> just to avoid confusion.
>> mipi-dbi-timings maybe ?
>
> It looks OK.

Isn't it too generic? I prefer cpu-mode-timings.

>
> I guess only hactive, and vactive props of display-timings are used,
> maybe some flags?
> I wonder if it would not be better to drop display-timings node
> and place all props into mipi-dbi-timings or mipi-dbi-settings node.
> Or rather all timings props should be put into port/endpoint node with
> or without
> any container node.

The current 'display-timings' is for video mode interface,
but cpu mode interface requires it.

If we use mipi-dbi-timings, we should change previous video mode
relevant ones at all.

Thank you.
Best regards YJ

>
> Regards
> Andrzej
>
>>
>>>>> If you want to stay with separate node please consider to make it
>>>>> optional as whole node or make some properties required. Making node
>>>>> required with all sub-properties optional is weird.
>>>>> By the way I hope all timings properties are generic for CPU mode,
>>>>> if not they should be prefixed by vendor or model.
>>>> The timings description should be pretty generic I believe, especially
>>>> given that the interface is standardized by the MIPI alliance. Could you
>>>> have a quick look at the spec and share your opinion ?
>>>>
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +
>>>>>> +The device node can contain one 'port' child node with one child
>>>>>> +'endpoint' node, according to the bindings defined in [2]. This
>>>>>> +node should describe panel's video bus.
>>>>>> +
>>>>>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>>>>>> +[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +	panel@0 {
>>>>>> +		compatible = "samsung,s6e3fa0";
>>>>>> +		reg = <0>;
>>>>>> +		vdd3-supply = <&vcclcd_reg>;
>>>>>> +		vci-supply = <&vlcd_reg>;
>>>>>> +		reset-gpio = <&gpy7 4 0>;
>>>>>> +		det-gpio = <&gpg0 6 0>;
>>>>>> +		te-gpio = <&gpd1 7 0>;
>>>>>> +
>>>>>> +		display-timings {
>>>>>> +			timing0: timing-0 {
>>>>>> +				clock-frequency = <0>;
>>>>>> +				hactive = <1080>;
>>>>>> +				vactive = <1920>;
>>>>>> +				hfront-porch = <2>;
>>>>>> +				hback-porch = <2>;
>>>>>> +				hsync-len = <1>;
>>>>>> +				vfront-porch = <1>;
>>>>>> +				vback-porch = <4>;
>>>>>> +				vsync-len = <1>;
>>>>>> +			};
>>>>>> +		};
>>>>>> +
>>>>>> +		cpu-timings {
>>>>>> +			cs-setup = <0>;
>>>>>> +			wr-setup = <0>;
>>>>>> +			wr-act = <1>;
>>>>>> +			wr-hold = <0>;
>>>>>> +		};
>>>>>> +	};
>
>

Patch

diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
new file mode 100644
index 0000000..9eeb38b
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
@@ -0,0 +1,63 @@ 
+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
+
+Required properties:
+  - compatible: "samsung,s6e3fa0"
+  - reg: the virtual channel number of a DSI peripheral
+  - vdd3-supply: core voltage supply
+  - vci-supply: voltage supply for analog circuits
+  - reset-gpio: a GPIO spec for the reset pin
+  - det-gpio: a GPIO spec for the OLED detection pin
+  - te-gpio: a GPIO spec for the TE pin
+  - display-timings: timings for the connected panel as described by [1]
+  - cpu-timings: CPU interface timings for the connected panel, and it contains
+        following optional properties.
+          - cs-setup: clock cycles for the active period of address signal
+                enable until chip select is enable in CPU interface
+          - wr-setup: clock cycles for the active period of CS signal enable
+                until write signal is enable in CPU interface
+          - wr-act: clock cycles for the active period of CS enable in CPU
+                interface
+          - wr-hold: clock cycles for the active period of CS disable until
+                write signal is disable in CPU interface
+
+Optional properties:
+
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in [2]. This
+node should describe panel's video bus.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+	panel@0 {
+		compatible = "samsung,s6e3fa0";
+		reg = <0>;
+		vdd3-supply = <&vcclcd_reg>;
+		vci-supply = <&vlcd_reg>;
+		reset-gpio = <&gpy7 4 0>;
+		det-gpio = <&gpg0 6 0>;
+		te-gpio = <&gpd1 7 0>;
+
+		display-timings {
+			timing0: timing-0 {
+				clock-frequency = <0>;
+				hactive = <1080>;
+				vactive = <1920>;
+				hfront-porch = <2>;
+				hback-porch = <2>;
+				hsync-len = <1>;
+				vfront-porch = <1>;
+				vback-porch = <4>;
+				vsync-len = <1>;
+			};
+		};
+
+		cpu-timings {
+			cs-setup = <0>;
+			wr-setup = <0>;
+			wr-act = <1>;
+			wr-hold = <0>;
+		};
+	};