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

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

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 <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?

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>;
>> +		};
>> +	};
>>
>
>