Hi Sylwester,
On Wed, Oct 10, 2012 at 2:59 AM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> 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@xxxxxxxxxxx>
>> ---
>> .../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.
>> +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.
>> + -samsung,dptx_phy_reg:
>
> I think it's fine to use just 'reg' instead of this vendor specific name.
Ok.
>> + 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!
>> +
>> +Example:
>> +
>> +SOC specific portion:
>> + dptx_phy: dptx_phy@0x10040720 {
>> + compatible = "samsung,dp-phy";
>> + samsung,dptx_phy_reg =<0x10040720>;
>
> reg = <0x10040720>;
Ok.
>> + samsung,enable_mask =<1>;
>> + };
>> +
>> + display-port-controller {
>> + compatible = "samsung,exynos5-dp";
>> + reg =<0x145B0000 0x10000>;
>
> I think lower case is preferred.
Ok. Will change it.
>> + 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@xxxxxxxxxxxxxxx/msg52743.html
> [2] http://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg53323.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Regards,
Ajay
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
