* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
[not found] ` <1348515385-22332-1-git-send-email-ajaykumar.rs-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2012-09-27 13:44 ` Sylwester Nawrocki
2012-09-28 0:11 ` Jingoo Han
0 siblings, 1 reply; 7+ messages in thread
From: Sylwester Nawrocki @ 2012-09-27 13:44 UTC (permalink / raw)
To: Ajay Kumar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
FlorianSchandinat-Mmb7MZpHnFY, devicetree-discuss,
jg1.han-Sze3O3UU22JBDgjK7y7TUQ, thomas.ab-Sze3O3UU22JBDgjK7y7TUQ
Hi,
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
On 09/24/2012 09:36 PM, Ajay Kumar wrote:
> This patch enables device tree based discovery support for DP driver.
> The driver is modified to handle platform data in both the cases:
> with DT and non-DT.
> Documentation is also added for the DT bindings.
>
> DP-PHY should be regarded as a seperate device node while
> being passed from device tree list, and device node for
> DP should contain DP-PHY as child node with property name "dp-phy"
> associated with it.
>
> Signed-off-by: Ajay Kumar<ajaykumar.rs-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> .../devicetree/bindings/video/exynos_dp.txt | 83 ++++++++++
> drivers/video/exynos/exynos_dp_core.c | 168 ++++++++++++++++++--
> drivers/video/exynos/exynos_dp_core.h | 2 +
> 3 files changed, 239 insertions(+), 14 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..c27f892
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -0,0 +1,83 @@
> +Exynos Displayport driver should configure the displayport interface
Don't we need a whitespace between 'display' and 'port' ?
> +based on the type of panel connected to it.
> +
> +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".
> + -samsung,dptx_phy_reg:
> + Base address of DP PHY register.
Couldn't just 'reg' be used for this one ?
> + -samsung,enable_bit:
> + The bit used to enable/disable DP PHY.
Is this the bit mask or the bit index ? In the code it's used as
a bitmask. But from description it is not clear whether it is
an index or a mask. Is it different across various SoCs ?
Perhaps it's better to name it samsung,enable_mask (in case some
SoC need more than one bit) ?
> +
> +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:
> + Internet combiner values.
what? :)
> + -interrupt-parent:
> + Address of Interrupt combiner node.
> + -dp_phy:
> + Address of dptx_phy node.
"A phandle to dptx_phy node" ?
> + -samsung,color_space:
> + input video data format.
> + COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
Can this be changed at run time ?
> + -samsung,dynamic_range:
> + dynamic range for input video data.
> + VESA = 0, CEA = 1
Why is it in the device tree ? Shouldn't it be configurable at runtime ?
My apologies if this an obvious question, I don't have much experience
with DP.
> + -samsung,ycbcr_coeff:
> + YCbCr co-efficients for input video.
> + COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
> + -samsung,color_depth:
> + Bit per color component.
"Number of bits per colour component" ? Also same remark as above.
> + COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> + -samsung,link_rate:
> + link rates supportd by the panel.
typo: supportd -> supported
> + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
Is this really a property of a panel ? Why it is in the PHY node ?
Also I can see this is just a single property, so "link rates" is a bit
misleading.
> + -samsung,lane_count:
> + number of lanes supported by the panel.
> + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
What do these symbolic names are needed for ? Is lane_count a number or a
mask, is this really a _maximum_ number of lanes ? What are the valid values,
1, 2 and 4 ? Or maybe 0x3 is also valid which would indicate that we can
use 1 or 2 data lanes ?
> + -samsung,interlaced:
> + Interlace scan mode.
> + Progressive if defined, Interlaced if not defined
Why do we need this in the device tree ? Is this really a default scan mode ?
Can it be the changed at runtime ?
> + -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
> +
> +Example:
> +
> +SOC specific portion:
> + dptx_phy: dptx_phy@0x10040720 {
> + compatible = "samsung,dp-phy";
> + samsung,dptx_phy_reg =<0x10040720>;
> + samsung,enable_bit =<1>;
> + };
> +
> + display-port-controller {
> + compatible = "samsung,exynos5-dp";
> + reg =<0x145B0000 0x10000>;
> + interrupts =<10 3>;
> + interrupt-parent =<&combiner>;
> + dp_phy =<&dptx_phy>;
Shouldn't it be "samsung,dp_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>;
> + };
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-09-27 13:44 ` [PATCH V3] video: exynos_dp: Add device tree support to DP driver Sylwester Nawrocki
@ 2012-09-28 0:11 ` Jingoo Han
2012-09-28 6:48 ` Tomasz Figa
2012-10-01 5:40 ` Ajay kumar
0 siblings, 2 replies; 7+ messages in thread
From: Jingoo Han @ 2012-09-28 0:11 UTC (permalink / raw)
To: 'Sylwester Nawrocki', 'Ajay Kumar'
Cc: linux-samsung-soc, linux-fbdev, FlorianSchandinat, thomas.ab,
'devicetree-discuss', 'Jingoo Han'
On Thursday, September 27, 2012 10:45 PM Sylwester Nawrocki wrote
>
> Hi,
>
> Cc: devicetree-discuss@lists.ozlabs.org
>
> On 09/24/2012 09:36 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> > Documentation is also added for the DT bindings.
> >
> > DP-PHY should be regarded as a seperate device node while
> > being passed from device tree list, and device node for
> > DP should contain DP-PHY as child node with property name "dp-phy"
> > associated with it.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > ---
> > .../devicetree/bindings/video/exynos_dp.txt | 83 ++++++++++
> > drivers/video/exynos/exynos_dp_core.c | 168 ++++++++++++++++++--
> > drivers/video/exynos/exynos_dp_core.h | 2 +
> > 3 files changed, 239 insertions(+), 14 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..c27f892
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> > @@ -0,0 +1,83 @@
> > +Exynos Displayport driver should configure the displayport interface
>
> Don't we need a whitespace between 'display' and 'port' ?
Either 'display port' or 'DisplayPort' is commonly used.
For clarity, display port would be better.
>
> > +based on the type of panel connected to it.
> > +
> > +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".
> > + -samsung,dptx_phy_reg:
> > + Base address of DP PHY register.
>
> Couldn't just 'reg' be used for this one ?
>
> > + -samsung,enable_bit:
> > + The bit used to enable/disable DP PHY.
>
> Is this the bit mask or the bit index ? In the code it's used as
> a bitmask. But from description it is not clear whether it is
> an index or a mask. Is it different across various SoCs ?
>
> Perhaps it's better to name it samsung,enable_mask (in case some
> SoC need more than one bit) ?
It's the bit mask.
It is different across various SoCs.
OK, enable_mask would be better.
>
> > +
> > +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:
> > + Internet combiner values.
>
> what? :)
Ajay: Please, fix it.
>
> > + -interrupt-parent:
> > + Address of Interrupt combiner node.
> > + -dp_phy:
> > + Address of dptx_phy node.
>
> "A phandle to dptx_phy node" ?
Yes, you're right.
Ajay: Please, replace 'Address of dptx_phy node'
with 'phandle of dptx_phy node'.
>
> > + -samsung,color_space:
> > + input video data format.
> > + COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
>
> Can this be changed at run time ?
No, it is not changeable.
It's a default setting according to eDP panel.
>
> > + -samsung,dynamic_range:
> > + dynamic range for input video data.
> > + VESA = 0, CEA = 1
>
> Why is it in the device tree ? Shouldn't it be configurable at runtime ?
> My apologies if this an obvious question, I don't have much experience
> with DP.
Same above.
>
> > + -samsung,ycbcr_coeff:
> > + YCbCr co-efficients for input video.
> > + COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
> > + -samsung,color_depth:
> > + Bit per color component.
>
> "Number of bits per colour component" ? Also same remark as above.
Same above.
>
> > + COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> > + -samsung,link_rate:
> > + link rates supportd by the panel.
>
> typo: supportd -> supported
Ajay: Please fix it.
>
> > + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
>
> Is this really a property of a panel ? Why it is in the PHY node ?
> Also I can see this is just a single property, so "link rates" is a bit
> misleading.
>
Yes, It's a property of LCD panel.
It is not in the 'PHY' node, but 'display-port-controller' node.
Ajay: Please replace 'link rates' with 'link rate'.
> > + -samsung,lane_count:
> > + number of lanes supported by the panel.
> > + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
>
> What do these symbolic names are needed for ? Is lane_count a number or a
> mask, is this really a _maximum_ number of lanes ? What are the valid values,
> 1, 2 and 4 ? Or maybe 0x3 is also valid which would indicate that we can
> use 1 or 2 data lanes ?
>
These symbolic names are defined in './include/video/exynos_dp.h'.
It seems that Ajay used these definitions.
'lane_count' is a number of used lanes of main link which transfer data
streams as video. The number of lanes of Main Link is 1, 2, or 4 lanes.
It is dependent on eDP panel.
> > + -samsung,interlaced:
> > + Interlace scan mode.
> > + Progressive if defined, Interlaced if not defined
>
> Why do we need this in the device tree ? Is this really a default scan mode ?
> Can it be the changed at runtime ?
No, it is not changeable.
It's a default setting according to eDP panel.
>
> > + -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
> > +
> > +Example:
> > +
> > +SOC specific portion:
> > + dptx_phy: dptx_phy@0x10040720 {
> > + compatible = "samsung,dp-phy";
> > + samsung,dptx_phy_reg =<0x10040720>;
> > + samsung,enable_bit =<1>;
> > + };
> > +
> > + display-port-controller {
> > + compatible = "samsung,exynos5-dp";
> > + reg =<0x145B0000 0x10000>;
> > + interrupts =<10 3>;
> > + interrupt-parent =<&combiner>;
> > + dp_phy =<&dptx_phy>;
>
> Shouldn't it be "samsung,dp_phy" ?
Do you mean this ? It is not working.
+ dp_phy = "samsung,dp_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>;
> > + };
>
> --
>
> Regards,
> Sylwester
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-09-28 0:11 ` Jingoo Han
@ 2012-09-28 6:48 ` Tomasz Figa
2012-09-28 8:09 ` Jingoo Han
2012-09-28 8:25 ` Sylwester Nawrocki
2012-10-01 5:40 ` Ajay kumar
1 sibling, 2 replies; 7+ messages in thread
From: Tomasz Figa @ 2012-09-28 6:48 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Sylwester Nawrocki', 'Ajay Kumar',
linux-samsung-soc, linux-fbdev, FlorianSchandinat, thomas.ab,
'devicetree-discuss'
Hi,
On Friday 28 of September 2012 09:11:09 Jingoo Han wrote:
> On Thursday, September 27, 2012 10:45 PM Sylwester Nawrocki wrote
> > > +Example:
> > > +
> > > +SOC specific portion:
> > > + dptx_phy: dptx_phy@0x10040720 {
> > > + compatible = "samsung,dp-phy";
> > > + samsung,dptx_phy_reg =<0x10040720>;
> > > + samsung,enable_bit =<1>;
> > > + };
> > > +
> > > + display-port-controller {
> > > + compatible = "samsung,exynos5-dp";
> > > + reg =<0x145B0000 0x10000>;
> > > + interrupts =<10 3>;
> > > + interrupt-parent =<&combiner>;
> > > + dp_phy =<&dptx_phy>;
> >
> > Shouldn't it be "samsung,dp_phy" ?
>
> Do you mean this ? It is not working.
> + dp_phy = "samsung,dp_phy";
I believe he meant:
-+ dp_phy =<&dptx_phy>;
++ samsung,dp_phy =<&dptx_phy>;
to mark that this is a Samsung-specific property.
Best regards,
Tomasz Figa
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-09-28 6:48 ` Tomasz Figa
@ 2012-09-28 8:09 ` Jingoo Han
2012-09-28 8:25 ` Sylwester Nawrocki
1 sibling, 0 replies; 7+ messages in thread
From: Jingoo Han @ 2012-09-28 8:09 UTC (permalink / raw)
To: 'Tomasz Figa'
Cc: 'Sylwester Nawrocki', 'Ajay Kumar',
linux-samsung-soc, linux-fbdev, FlorianSchandinat, thomas.ab,
'devicetree-discuss', 'Jingoo Han'
On Friday, September 28, 2012 3:48 PM Tomasz Figa wrote
>
> Hi,
>
> On Friday 28 of September 2012 09:11:09 Jingoo Han wrote:
> > On Thursday, September 27, 2012 10:45 PM Sylwester Nawrocki wrote
> > > > +Example:
> > > > +
> > > > +SOC specific portion:
> > > > + dptx_phy: dptx_phy@0x10040720 {
> > > > + compatible = "samsung,dp-phy";
> > > > + samsung,dptx_phy_reg =<0x10040720>;
> > > > + samsung,enable_bit =<1>;
> > > > + };
> > > > +
> > > > + display-port-controller {
> > > > + compatible = "samsung,exynos5-dp";
> > > > + reg =<0x145B0000 0x10000>;
> > > > + interrupts =<10 3>;
> > > > + interrupt-parent =<&combiner>;
> > > > + dp_phy =<&dptx_phy>;
> > >
> > > Shouldn't it be "samsung,dp_phy" ?
> >
> > Do you mean this ? It is not working.
> > + dp_phy = "samsung,dp_phy";
>
> I believe he meant:
>
> -+ dp_phy =<&dptx_phy>;
> ++ samsung,dp_phy =<&dptx_phy>;
>
> to mark that this is a Samsung-specific property.
Oh, I see.
Yes, "samsung,dp_phy =<&dptx_phy>" is right.
As you mentioned, this is a Samsung-specific property.
Thank you.
Best regards,
Jingoo Han
>
> Best regards,
> Tomasz Figa
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-09-28 6:48 ` Tomasz Figa
2012-09-28 8:09 ` Jingoo Han
@ 2012-09-28 8:25 ` Sylwester Nawrocki
1 sibling, 0 replies; 7+ messages in thread
From: Sylwester Nawrocki @ 2012-09-28 8:25 UTC (permalink / raw)
To: Jingoo Han
Cc: Tomasz Figa, 'Ajay Kumar', linux-samsung-soc, linux-fbdev,
FlorianSchandinat, thomas.ab, 'devicetree-discuss'
On 09/28/2012 08:48 AM, Tomasz Figa wrote:
>>>> + display-port-controller {
>>>> + compatible = "samsung,exynos5-dp";
>>>> + reg =<0x145B0000 0x10000>;
>>>> + interrupts =<10 3>;
>>>> + interrupt-parent =<&combiner>;
>>>> + dp_phy =<&dptx_phy>;
>>>
>>> Shouldn't it be "samsung,dp_phy" ?
>>
>> Do you mean this ? It is not working.
>> + dp_phy = "samsung,dp_phy";
>
> I believe he meant:
>
> -+ dp_phy =<&dptx_phy>;
> ++ samsung,dp_phy =<&dptx_phy>;
>
> to mark that this is a Samsung-specific property.
Yes, that's what I meant. Sorry for imprecise wording.
Regards,
Sylwester
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-09-28 0:11 ` Jingoo Han
2012-09-28 6:48 ` Tomasz Figa
@ 2012-10-01 5:40 ` Ajay kumar
2012-10-04 1:50 ` Jingoo Han
1 sibling, 1 reply; 7+ messages in thread
From: Ajay kumar @ 2012-10-01 5:40 UTC (permalink / raw)
To: Jingoo Han, linux-fbdev
Cc: Tomasz Figa, linux-samsung-soc, FlorianSchandinat, thomas.ab,
devicetree-discuss
On Fri, Sep 28, 2012 at 5:41 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Thursday, September 27, 2012 10:45 PM Sylwester Nawrocki wrote
>>
>> Hi,
>>
>> Cc: devicetree-discuss@lists.ozlabs.org
>>
>> On 09/24/2012 09:36 PM, Ajay Kumar wrote:
>> > This patch enables device tree based discovery support for DP driver.
>> > The driver is modified to handle platform data in both the cases:
>> > with DT and non-DT.
>> > Documentation is also added for the DT bindings.
>> >
>> > DP-PHY should be regarded as a seperate device node while
>> > being passed from device tree list, and device node for
>> > DP should contain DP-PHY as child node with property name "dp-phy"
>> > associated with it.
>> >
>> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>> > ---
>> > .../devicetree/bindings/video/exynos_dp.txt | 83 ++++++++++
>> > drivers/video/exynos/exynos_dp_core.c | 168 ++++++++++++++++++--
>> > drivers/video/exynos/exynos_dp_core.h | 2 +
>> > 3 files changed, 239 insertions(+), 14 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..c27f892
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
>> > @@ -0,0 +1,83 @@
>> > +Exynos Displayport driver should configure the displayport interface
>>
>> Don't we need a whitespace between 'display' and 'port' ?
>
> Either 'display port' or 'DisplayPort' is commonly used.
> For clarity, display port would be better.
Ok. I will change it.
>>
>> > +based on the type of panel connected to it.
>> > +
>> > +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".
>> > + -samsung,dptx_phy_reg:
>> > + Base address of DP PHY register.
>>
>> Couldn't just 'reg' be used for this one ?
>>
>> > + -samsung,enable_bit:
>> > + The bit used to enable/disable DP PHY.
>>
>> Is this the bit mask or the bit index ? In the code it's used as
>> a bitmask. But from description it is not clear whether it is
>> an index or a mask. Is it different across various SoCs ?
>>
>> Perhaps it's better to name it samsung,enable_mask (in case some
>> SoC need more than one bit) ?
>
> It's the bit mask.
> It is different across various SoCs.
> OK, enable_mask would be better.
Ok. I will change it.
>>
>> > +
>> > +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:
>> > + Internet combiner values.
>>
>> what? :)
>
> Ajay: Please, fix it.
My bad :(...I will fix it.
>>
>> > + -interrupt-parent:
>> > + Address of Interrupt combiner node.
>> > + -dp_phy:
>> > + Address of dptx_phy node.
>>
>> "A phandle to dptx_phy node" ?
>
> Yes, you're right.
>
> Ajay: Please, replace 'Address of dptx_phy node'
> with 'phandle of dptx_phy node'.
Ok.
>>
>> > + -samsung,color_space:
>> > + input video data format.
>> > + COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
>>
>> Can this be changed at run time ?
>
> No, it is not changeable.
> It's a default setting according to eDP panel.
>
>>
>> > + -samsung,dynamic_range:
>> > + dynamic range for input video data.
>> > + VESA = 0, CEA = 1
>>
>> Why is it in the device tree ? Shouldn't it be configurable at runtime ?
>> My apologies if this an obvious question, I don't have much experience
>> with DP.
>
> Same above.
>
>>
>> > + -samsung,ycbcr_coeff:
>> > + YCbCr co-efficients for input video.
>> > + COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
>> > + -samsung,color_depth:
>> > + Bit per color component.
>>
>> "Number of bits per colour component" ? Also same remark as above.
>
> Same above.
>
>>
>> > + COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
>> > + -samsung,link_rate:
>> > + link rates supportd by the panel.
>>
>> typo: supportd -> supported
> Ajay: Please fix it.
Ok.
>
>>
>> > + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
>>
>> Is this really a property of a panel ? Why it is in the PHY node ?
>> Also I can see this is just a single property, so "link rates" is a bit
>> misleading.
>>
>
> Yes, It's a property of LCD panel.
> It is not in the 'PHY' node, but 'display-port-controller' node.
>
> Ajay: Please replace 'link rates' with 'link rate'.
Ok.
>> > + -samsung,lane_count:
>> > + number of lanes supported by the panel.
>> > + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
>>
>> What do these symbolic names are needed for ? Is lane_count a number or a
>> mask, is this really a _maximum_ number of lanes ? What are the valid values,
>> 1, 2 and 4 ? Or maybe 0x3 is also valid which would indicate that we can
>> use 1 or 2 data lanes ?
>>
>
> These symbolic names are defined in './include/video/exynos_dp.h'.
> It seems that Ajay used these definitions.
Yes. I referred include/video/exynos_dp.h.
> 'lane_count' is a number of used lanes of main link which transfer data
> streams as video. The number of lanes of Main Link is 1, 2, or 4 lanes.
> It is dependent on eDP panel.
>
>
>> > + -samsung,interlaced:
>> > + Interlace scan mode.
>> > + Progressive if defined, Interlaced if not defined
>>
>> Why do we need this in the device tree ? Is this really a default scan mode ?
>> Can it be the changed at runtime ?
>
> No, it is not changeable.
> It's a default setting according to eDP panel.
>
>>
>> > + -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
>> > +
>> > +Example:
>> > +
>> > +SOC specific portion:
>> > + dptx_phy: dptx_phy@0x10040720 {
>> > + compatible = "samsung,dp-phy";
>> > + samsung,dptx_phy_reg =<0x10040720>;
>> > + samsung,enable_bit =<1>;
>> > + };
>> > +
>> > + display-port-controller {
>> > + compatible = "samsung,exynos5-dp";
>> > + reg =<0x145B0000 0x10000>;
>> > + interrupts =<10 3>;
>> > + interrupt-parent =<&combiner>;
>> > + dp_phy =<&dptx_phy>;
>>
>> Shouldn't it be "samsung,dp_phy" ?
>
> Do you mean this ? It is not working.
> + dp_phy = "samsung,dp_phy";
I din't get this.
What do I need to change here?
>>
>> > + };
>> > +
>> > +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>;
>> > + };
>>
>> --
>>
>> Regards,
>> Sylwester
>
Thanks and regards,
Ajay Kumar
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V3] video: exynos_dp: Add device tree support to DP driver
2012-10-01 5:40 ` Ajay kumar
@ 2012-10-04 1:50 ` Jingoo Han
0 siblings, 0 replies; 7+ messages in thread
From: Jingoo Han @ 2012-10-04 1:50 UTC (permalink / raw)
To: 'Ajay kumar', linux-fbdev
Cc: linux-samsung-soc, FlorianSchandinat, thomas.ab,
'devicetree-discuss', 'Sylwester Nawrocki',
'Tomasz Figa', 'Jingoo Han'
On Monday, October 01, 2012 2:40 PM Ajay kumar wrote
>
> On Fri, Sep 28, 2012 at 5:41 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Thursday, September 27, 2012 10:45 PM Sylwester Nawrocki wrote
> >> On 09/24/2012 09:36 PM, Ajay Kumar wrote:
[...]
> >> > +Example:
> >> > +
> >> > +SOC specific portion:
> >> > + dptx_phy: dptx_phy@0x10040720 {
> >> > + compatible = "samsung,dp-phy";
> >> > + samsung,dptx_phy_reg =<0x10040720>;
> >> > + samsung,enable_bit =<1>;
> >> > + };
> >> > +
> >> > + display-port-controller {
> >> > + compatible = "samsung,exynos5-dp";
> >> > + reg =<0x145B0000 0x10000>;
> >> > + interrupts =<10 3>;
> >> > + interrupt-parent =<&combiner>;
> >> > + dp_phy =<&dptx_phy>;
> >>
> >> Shouldn't it be "samsung,dp_phy" ?
> >
> > Do you mean this ? It is not working.
> > + dp_phy = "samsung,dp_phy";
> I din't get this.
> What do I need to change here?
As Tomasz Figa mentioned, it means that
-+ dp_phy =<&dptx_phy>;
++ samsung,dp_phy =<&dptx_phy>;
It is because this is a Samsung-specific property.
Best regards,
Jingoo Han
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-10-04 1:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1348515385-22332-1-git-send-email-ajaykumar.rs@samsung.com>
[not found] ` <1348515385-22332-1-git-send-email-ajaykumar.rs-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2012-09-27 13:44 ` [PATCH V3] video: exynos_dp: Add device tree support to DP driver Sylwester Nawrocki
2012-09-28 0:11 ` Jingoo Han
2012-09-28 6:48 ` Tomasz Figa
2012-09-28 8:09 ` Jingoo Han
2012-09-28 8:25 ` Sylwester Nawrocki
2012-10-01 5:40 ` Ajay kumar
2012-10-04 1:50 ` Jingoo Han
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).