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

[YoungJun Cho <yj44.cho@samsung.com>]

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 <yj44.cho@samsung.com>
>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>
>>>   .../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>;
>>> +		};
>>> +	};
>