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

Hi Krzysztof,

在 08/23/2015 07:43 PM, Krzysztof Kozlowski 写道:

2015-08-24 8:23 GMT+09:00 Rob Herring robherring2@gmail.com:

...

On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang ykk@rock-chips.com 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@rock-chips.com

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@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

---

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