Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

From: Yakir Yang
Date: Sun Aug 23 2015 - 22:43:19 EST

Next message: Doug Anderson: "Re: [BUG] arm: kgdb: patch_text() in kgdb_arch_set_breakpoint() may sleep"
Previous message: Joe Perches: "Re: [PATCH 1/3] ARM: uniphier: add outer cache support"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp"
Next in thread: Yakir Yang: "Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

å 08/23/2015 07:43 PM, Krzysztof Kozlowski åé:

2015-08-24 8:23 GMT+09:00 Rob Herring <robherring2@xxxxxxxxx>:

On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@xxxxxxxxxxxxxx> wrote:

Analogix dp driver is split from exynos dp driver, so we just
make an copy of exynos_dp.txt, and then simplify exynos_dp.txt

Beside update some exynos dtsi file with the latest change
according to the devicetree binding documents.

You can't just change the exynos bindings and break compatibility. Is
there some agreement with exynos folks to do this?

No, there is no agreement. This wasn't even sent to Exynos maintainers.

Sorry about this one, actually I have add Exynos maintainers in version 1 & version 2,
but lose some maintainers in version 3, I would fix it in bellow versions.

Additionally the patchset did not look interesting to me because of
misleading subject - Documentation instead of "ARM: dts:".

Yakir, please:
1. Provide backward compatibility. Mark old properties as deprecated
but still support them.

Do you mean that I should keep the old properties declare in exynos-dp.txt,
but just mark them as deprecated flag. Let me show same examples, make
me understand your suggest rightly.

1. "samsung,ycbcr-coeff" is abandoned in latest analogix-dp driver, absolutely
I should not carry this to analogix-dp.txt document. But I should keep this in
exynos-dp.txt document, and mark them with an little "deprecated" flag.

[Documentation/devicetree/bindings/video/exynos_dp.txt]
Required properties for dp-controller:
[...]
-samsung,ycbcr-coeff (DEPRECATED):
YCbCr co-efficients for input video.
COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1

Is it right ?

2. Separate all DTS changes to a separate patch, unless bisectability
would be hurt. Anyway you should prepare it in a such way that
separation would be possible without breaking bisectability.

So I should separate this patch into two parts, one is name "Document:",
the other is "ARM: dts: ".

Honestly, I don't understand what the "bisectability" means in this case.

3. Use proper subject for the patch changing DTS. This is not
documentation change!

Hmm... when I separate this patch into two parts, I though I can keep
"Documentation" proper subject in this patch, and the other is the "ARM: dts"
proper subject. Am I right ?

4. Please use script get_maintainers to obtain list of valid
maintainers and CC-them with at least cover letter and patches
requiring their attention.

Yeah, thanks.

Thanks a lot,
- Yakir

Best regards,
Krzysztof

Signed-off-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
---
Changes in v3:
- Take Heiko suggest, add devicetree binding documents.
- Take Thierry Reding suggest, remove sync pol & colorimetry properies
from the new analogix dp driver devicetree binding.
- Update the exist exynos dtsi file with the latest DP DT properies.

Changes in v2: None

.../devicetree/bindings/drm/bridge/analogix_dp.txt | 70 ++++++++++++++++++++++
.../devicetree/bindings/video/exynos_dp.txt | 50 ++++++----------
arch/arm/boot/dts/exynos5250-arndale.dts | 10 ++--
arch/arm/boot/dts/exynos5250-smdk5250.dts | 10 ++--
arch/arm/boot/dts/exynos5250-snow.dts | 12 ++--
arch/arm/boot/dts/exynos5250-spring.dts | 12 ++--
arch/arm/boot/dts/exynos5420-peach-pit.dts | 12 ++--
arch/arm/boot/dts/exynos5420-smdk5420.dts | 10 ++--
arch/arm/boot/dts/exynos5800-peach-pi.dts | 12 ++--
9 files changed, 119 insertions(+), 79 deletions(-)
create mode 100644 Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt

diff --git a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
new file mode 100644
index 0000000..6127018
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
@@ -0,0 +1,70 @@
+Analogix Display Port bridge bindings
+
+Required properties for dp-controller:
+ -compatible:
+ platform specific such as:
+ * "samsung,exynos5-dp"
+ * "rockchip,rk3288-dp"
+ -reg:
+ physical base address of the controller and length
+ of memory mapped region.
+ -interrupts:
+ interrupt combiner values.
+ -clocks:
+ from common clock binding: handle to dp clock.
+ -clock-names:
+ from common clock binding: Shall be "dp".
+ -interrupt-parent:
+ phandle to Interrupt combiner node.
+ -phys:
+ from general PHY binding: the phandle for the PHY device.
+ -phy-names:
+ from general PHY binding: Should be "dp".
+ -analogix,color-space:
+ input video data format.
+ COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
+ -analogix,color-depth:
+ number of bits per colour component.
+ COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3

This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
drop the vendor prefix.

+ -analogix,link-rate:
+ max link rate supported by the eDP controller.
+ LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
+ LINK_RATE_5_40GBPS = 0x14

Same here. I'd rather see something like "link-rate-mbps" and use the
actual rate.

+ -analogix,lane-count:
+ max number of lanes supported by the eDP contoller.
+ LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4

And drop the vendor prefix here.

+ -port@[X]: SoC specific port nodes with endpoint definitions as defined
+ in Documentation/devicetree/bindings/media/video-interfaces.txt,
+ please refer to the SoC specific binding document:
+ * Documentation/devicetree/bindings/video/exynos_dp.txt
+ * Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
+
+Optional properties for dp-controller:
+ -analogix,hpd-gpio:
+ Hotplug detect GPIO.
+ Indicates which GPIO should be used for hotplug
+ detection

We should align with "hpd-gpios" used by HDMI connector binding. Or do
we need a DP connector binding that this should be defined in?
Probably so.

The DRM related bindings are such a cluster f*ck with everyone picking
their own way to do things. Just grep hpd in bindings for starters.
That is just the tip.

+ -video interfaces: Device node can contain video interface port
+ nodes according to [1].

Isn't this the same as ports above? How are they optional? 0 ports
would be pretty useless.

+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+-------------------------------------------------------------------------------
+
+Example:
+
+ dp-controller {
+ compatible = "samsung,exynos5-dp";
+ reg = <0x145b0000 0x10000>;
+ interrupts = <10 3>;
+ interrupt-parent = <&combiner>;
+ clocks = <&clock 342>;
+ clock-names = "dp";
+
+ phys = <&dp_phy>;
+ phy-names = "dp";
+
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <4>;
+ };
diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
index 7a3a9cd..177506f 100644
--- a/Documentation/devicetree/bindings/video/exynos_dp.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
@@ -31,28 +31,10 @@ Required properties for dp-controller:
from general PHY binding: the phandle for the PHY device.
-phy-names:
from general PHY binding: Should be "dp".
- -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
- - display-timings: timings for the connected panel as described by
- Documentation/devicetree/bindings/video/display-timing.txt

Optional properties for dp-controller:
+ - display-timings: timings for the connected panel as described by
+ Documentation/devicetree/bindings/video/display-timing.txt
-interlaced:
interlace scan mode.
Progressive if defined, Interlaced if not defined
@@ -62,14 +44,18 @@ Optional properties for dp-controller:
-hsync-active-high:
HSYNC polarity configuration.
High if defined, Low if not defined
- -samsung,hpd-gpio:
- Hotplug detect GPIO.
- Indicates which GPIO should be used for hotplug
- detection
- -video interfaces: Device node can contain video interface port
- nodes according to [1].

-[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+For the below properties, please refer to Analogix DP binding document:
+ * Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
+ -phys (required)
+ -phy-names (required)
+ -analogix,color-space (required)
+ -analogix,color-depth (required)
+ -analogix,link-rate (required)
+ -analogix,lane-count (required)
+ -analogix,hpd-gpio (optional)
+ -video interfaces (optional)
+-------------------------------------------------------------------------------

Example:

@@ -88,12 +74,10 @@ SOC specific portion:

Board Specific portion:
dp-controller {
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <4>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <4>;

display-timings {
native-mode = <&lcd_timing>;
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index 7e728a1..e48798d 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -119,12 +119,10 @@

&dp {
status = "okay";
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <4>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <4>;
};

&fimd {
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 4fe186d..b8c6b8b 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -75,12 +75,10 @@
};

&dp {
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <4>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <4>;

pinctrl-names = "default";
pinctrl-0 = <&dp_hpd>;
diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index b7f4122..9ce2b89 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -239,13 +239,11 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&dp_hpd>;
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <2>;
- samsung,hpd-gpio = <&gpx0 7 GPIO_ACTIVE_HIGH>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <2>;
+ analogix,hpd-gpio = <&gpx0 7 GPIO_ACTIVE_HIGH>;

ports {
port@0 {
diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
index d03f9b8..9288ae6 100644
--- a/arch/arm/boot/dts/exynos5250-spring.dts
+++ b/arch/arm/boot/dts/exynos5250-spring.dts
@@ -69,13 +69,11 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&dp_hpd_gpio>;
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <1>;
- samsung,hpd-gpio = <&gpc3 0 GPIO_ACTIVE_HIGH>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <1>;
+ analogix,hpd-gpio = <&gpc3 0 GPIO_ACTIVE_HIGH>;
};

&ehci {
diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 8f4d76c..695a380 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -147,13 +147,11 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&dp_hpd_gpio>;
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x06>;
- samsung,lane-count = <2>;
- samsung,hpd-gpio = <&gpx2 6 0>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x06>;
+ analogix,lane-count = <2>;
+ analogix,hpd-gpio = <&gpx2 6 0>;

ports {
port@0 {
diff --git a/arch/arm/boot/dts/exynos5420-smdk5420.dts b/arch/arm/boot/dts/exynos5420-smdk5420.dts
index 98871f9..fd46714 100644
--- a/arch/arm/boot/dts/exynos5420-smdk5420.dts
+++ b/arch/arm/boot/dts/exynos5420-smdk5420.dts
@@ -91,12 +91,10 @@
&dp {
pinctrl-names = "default";
pinctrl-0 = <&dp_hpd>;
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <4>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <4>;
status = "okay";
};

diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index 7d5b386..54b4c63 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -141,13 +141,11 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <&dp_hpd_gpio>;
- samsung,color-space = <0>;
- samsung,dynamic-range = <0>;
- samsung,ycbcr-coeff = <0>;
- samsung,color-depth = <1>;
- samsung,link-rate = <0x0a>;
- samsung,lane-count = <2>;
- samsung,hpd-gpio = <&gpx2 6 0>;
+ analogix,color-space = <0>;
+ analogix,color-depth = <1>;
+ analogix,link-rate = <0x0a>;
+ analogix,lane-count = <2>;
+ analogix,hpd-gpio = <&gpx2 6 0>;
panel = <&panel>;
};

--
1.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Doug Anderson: "Re: [BUG] arm: kgdb: patch_text() in kgdb_arch_set_breakpoint() may sleep"
Previous message: Joe Perches: "Re: [PATCH 1/3] ARM: uniphier: add outer cache support"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp"
Next in thread: Yakir Yang: "Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]