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

Hi Sylwester,

On Wed, Oct 10, 2012 at 2:59 AM, Sylwester Nawrocki
[off-list ref] wrote:

Hi Ajay,

On 10/10/2012 01:08 AM, Ajay Kumar wrote:

quoted

Add documentation for the DT bindings in exynos display port driver.

Signed-off-by: Ajay Kumar<redacted>
---
  .../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."

Ok. I will change 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.

Ok. Will make use of dashes and mention optional properties.

quoted

+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().

Ok. Will change it.

quoted

+     -samsung,dptx_phy_reg:

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

Ok.

quoted

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

hsync-active-high and vsync-active-high seems to hold good in our case.
Also, are you asking us to just use only the standard names or use standard
helper functions as well? Since we use only hsync and vsync polarity and no
other LCD timing properties, I think we need not use standard helper functions
for parsing display timings!

quoted

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

                reg = <0x10040720>;

Ok.

quoted

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

I think lower case is preferred.

Ok. Will change it.

quoted

+             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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Ajay