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