Re: [PATCH V4 2/2] video: exynos_dp: device tree documentation

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Ajay,

On 10/10/2012 01:08 AM, Ajay Kumar wrote:
> Add documentation for the DT bindings in exynos display port driver.
> 
> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> ---
>   .../devicetree/bindings/video/exynos_dp.txt        |   83 ++++++++++++++++++++
>   1 files changed, 83 insertions(+), 0 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/video/exynos_dp.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
> new file mode 100644
> index 0000000..a021963
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -0,0 +1,83 @@
> +Exynos display port driver should configure the display port interface
> +based on the type of panel connected to it.

The bindings are supposed to describe devices, not drivers. So it
might be better to say:

"The Exynos display port interface should be configured based on the 
type of panel connected to it."

>From this documentation it is not clear which properties are required 
and which are optional for each node.

I think, in the property names dashes should be used, rather than 
underscores. Dashes are much more common among existing bindings.

> +We use two nodes:
> +	-dptx_phy node
> +	-display-port-controller node
> +
> +For the dp-phy initialization, we use a dptx_phy node.
> +Required properties for dptx_phy:
> +	-compatible:
> +		Should be "samsung,dp-phy".

Is there a separate dptx-phy driver that is being matched with this 
compatible property ? Is the dptx-node going to be referenced by other
nodes than display-port-controller ? If not, then you could likely drop 
the compatible property entirely and make the dptx-phy node a child 
node of display-port-controller, i.e.

	display-port-controller {
		compatible = "samsung,exynos5-dp";
		reg = <0x145b0000 0x10000>;
		interrupts =<10 3>;
		interrupt-parent =<&combiner>;

		dptx-phy {
			reg = <0x10040720>;
			samsung,enable_mask = <1>;
		};
       };

Then the driver could just look for a child node named "dptx-phy" with
e.g. of_find_node_by_name().

> +	-samsung,dptx_phy_reg:

I think it's fine to use just 'reg' instead of this vendor specific name.

> +		Base address of DP PHY register.
> +	-samsung,enable_mask:
> +		The bit-mask used to enable/disable DP PHY.
> +
> +For the Panel initialization, we read data from display-port-controller node.
> +Required properties for display-port-controller:
> +	-compatible:
> +		Should be "samsung,exynos5-dp".
> +	-reg:
> +		physical base address of the controller and length
> +		of memory mapped region.
> +	-interrupts:
> +		Interrupt combiner values.
> +	-interrupt-parent:
> +		phandle to Interrupt combiner node.
> +	-samsung,dp_phy:
> +		phandle to dptx_phy node.
> +	-samsung,color_space:
> +		input video data format.
> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> +	-samsung,dynamic_range:
> +		dynamic range for input video data.
> +			VESA = 0, CEA = 1
> +	-samsung,ycbcr_coeff:
> +		YCbCr co-efficients for input video.
> +			COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
> +	-samsung,color_depth:
> +		Number of bits per colour component.
> +			COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> +	-samsung,link_rate:
> +		link rate supported by the panel.
> +			LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
> +	-samsung,lane_count:
> +		number of lanes supported by the panel.
> +			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
> +	-samsung,interlaced:
> +		Interlace scan mode.
> +			Progressive if defined, Interlaced if not defined
> +	-samsung,v_sync_polarity:
> +		VSYNC polarity configuration.
> +			High if defined, Low if not defined
> +	-samsung,h_sync_polarity:
> +		HSYNC polarity configuration.
> +			High if defined, Low if not defined

So there is no common video bindings for things like these two ?
In V4L2 we decided to use vsync-active, hsync-active [1], the video
timings bindings [2] use hsync-active-high, hsync-active-high boolean
properties. Perhaps it is worth to pick some of those standard 
definitions and use instead of the vendor specific ones ?

> +
> +Example:
> +
> +SOC specific portion:
> +	dptx_phy: dptx_phy@0x10040720 {
> +		compatible = "samsung,dp-phy";
> +		samsung,dptx_phy_reg =<0x10040720>;

		reg = <0x10040720>;

> +		samsung,enable_mask =<1>;
> +	};
> +
> +	display-port-controller {
> +		compatible = "samsung,exynos5-dp";
> +		reg =<0x145B0000 0x10000>;

I think lower case is preferred.

> +		interrupts =<10 3>;
> +		interrupt-parent =<&combiner>;
> +		samsung,dp_phy =<&dptx_phy>;
> +        };
> +
> +Board Specific portion:
> +	display-port-controller {
> +		samsung,color_space =<0>;
> +		samsung,dynamic_range =<0>;
> +		samsung,ycbcr_coeff =<0>;
> +		samsung,color_depth =<1>;
> +		samsung,link_rate =<0x0a>;
> +		samsung,lane_count =<2>;
> +	};

Thanks,
Sylwester

[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg52743.html
[2] http://www.mail-archive.com/linux-media@vger.kernel.org/msg53323.html