diff mbox

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

Message ID 1439995834-18363-1-git-send-email-ykk@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yakir Yang Aug. 19, 2015, 2:50 p.m. UTC
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.

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

Comments

Rob Herring Aug. 23, 2015, 11:23 p.m. UTC | #1
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?


> 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
Krzysztof Kozlowski Aug. 24, 2015, 12:43 a.m. UTC | #2
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.
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.
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.
3. Use proper subject for the patch changing DTS. This is not
documentation change!
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.

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
Yakir Yang Aug. 24, 2015, 2:19 a.m. UTC | #3
Hi Rob,

? 08/23/2015 06:23 PM, Rob Herring ??:
> 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?
>

Yeah, this change only start to introduce in version 3 series, so there is
no agreement or discuss before.

>
>> 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.

Okay, thanks


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

Like "link-rate-mbps = 162000", so I need of_property_read_u32() for 
this prop.

Okay, done.

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

Done

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

Hmm... I don't understand how the HDMI connector binding works, there are no
driver that name with "hdmi-connector" compatible, does it just an 
sample case
for all HDMI dts node?

But I'm okay with your suggest here, change "analogix,hpd-gpio" to 
"hpd-gpios"  ;)

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

I though I make an mistaken here, "-video interfaces" is the same as 
previous "ports".

And "ports" is an required property in rockchip case, cause we use it to 
bind "dp" driver
to crtc "vop" driver. But I thinks "ports" in not an required property 
in exynos case, they
only use when there are a dp/lvds application scenarios.

Here are example (./arch/arm/boot/dts/exynos5420-peach-pit.dts):
     &dp {
             [...]
             ports {
                     port@0 {
                             dp_out: endpoint {
                                     remote-endpoint = <&bridge_in>;
                             };
                     };
             };
     };

     ps8625: lvds-bridge@48 {
                 [...]
                 ports {
                         port@0 {
                                 bridge_out: endpoint {
                                         remote-endpoint = <&panel_in>;
                                 };
                         };

                         port@1 {
                                 bridge_in: endpoint {
                                         remote-endpoint = <&dp_out>;
                                 };
                         };
                 };

         };

So I would rather to remove "ports" to analogix_dp-rockchip.txt with 
"required" flag,
and keep "video-port" in analogox-dp.txt with "optional" flag.

>> +
>> +[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
>
>
Yakir Yang Aug. 24, 2015, 2:42 a.m. UTC | #4
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
>
>
Krzysztof Kozlowski Aug. 24, 2015, 4:20 a.m. UTC | #5
On 24.08.2015 11:42, Yakir Yang wrote:
> 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.

That is one of ways how to do this. However more important is that
driver should still support old bindings so such code:

-       if (of_property_read_u32(dp_node, "samsung,color-space",
+       if (of_property_read_u32(dp_node, "analogix,color-space",

is probably wrong. Will the driver support old DTB in the same way as it
was supporting before the change?

> Let me show same examples, make
> me understand your suggest rightly.

exynos-dp already contains deprecated properties. Other ways of doing
this would be:
Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
Documentation/devicetree/bindings/rtc/s3c-rtc.txt

It depends up to you. The "touchscreen" looks good because it organizes
old properties in a common section. In case of exynos-dp.txt you can add
at beginning of file information about new bindings and mark everything
deprecated.

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

Yes, this is 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: ".

Yes.

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

I was referring to bisectability in general. The patchset should be
fully bisectable which means that it does not introduce any issues
during "git bisect". This effectively means that at each intermediate
step (after applying each patch, one by one) every existing stuff works
the same as previously without any regression. Including booting with
old DTB.

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

Yes, you're 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.

Sure. Now I found older versions of the patchset but previously there
were no changes to the bindings. Again the prefix in subject is
important to easily filter out and find necessary emails.

BTW, I like the patchset because I like in general works which merge
code and reduce duplicate stuff.

Best regards,
Krzysztof
Han Jingoo Aug. 24, 2015, 7:40 a.m. UTC | #6
On 2015. 8. 24., at AM 9:43, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
> 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.
> 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.
> 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.
> 3. Use proper subject for the patch changing DTS. This is not
> documentation change!
> 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.

To Yakir Yang,

Please be careful when you CC people.

I don't find the reason why you CC'ed the following people. Of course, they
look to be one of the talented developers. However, they look to be
unrelated to your patchset.

 Takashi Iwai
 Vincent Palatin
 Fabio Estevam
 Philipp Zabel


Also, please add Exynos Architecture maintainers to CC list. I don't understand
why you removed them from CC list.
 Kukjin Kim
 Krzysztof Kozlowski

Without their Ack, you will not change the codes of ARM Exynos Architecture.

Best regards,
Jingoo Han

> 
> 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
Yakir Yang Aug. 24, 2015, 12:48 p.m. UTC | #7
Hi Krzysztof,

? 08/24/2015 12:20 PM, Krzysztof Kozlowski ??:
> On 24.08.2015 11:42, Yakir Yang wrote:
>> 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.
> That is one of ways how to do this. However more important is that
> driver should still support old bindings so such code:
> -       if (of_property_read_u32(dp_node, "samsung,color-space",
> +       if (of_property_read_u32(dp_node, "analogix,color-space",
>
> is probably wrong. Will the driver support old DTB in the same way as it
> was supporting before the change?

Okay, I got your means. So document is not the focus, the most important 
is that
driver should support the old dts prop. If so the new analogix dp driver 
should keep
the "samsung,color-space", rather then just mark it with [DEPRECATED] flag.

But from your follow suggest, I think you agree to update driver code, 
and just mark
old prop with deprecated flag. If so I think such code would not be wrong

-       if (of_property_read_u32(dp_node, "samsung,color-space",
+      if (of_property_read_u32(dp_node, "analogix,color-space",

And actually @Rob have suggest me to remove the prefix, just use 
"color-space" here.

>
>> Let me show same examples, make
>> me understand your suggest rightly.
> exynos-dp already contains deprecated properties. Other ways of doing
> this would be:
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> Documentation/devicetree/bindings/rtc/s3c-rtc.txt
>
> It depends up to you. The "touchscreen" looks good because it organizes
> old properties in a common section. In case of exynos-dp.txt you can add
> at beginning of file information about new bindings and mark everything
> deprecated.

Whoops, thanks for your remind, I prefer the "touchscreen" style.

>> 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 ?
> Yes, this is 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: ".
> Yes.
>
>> Honestly, I don't understand what the "bisectability" means in this case.
> I was referring to bisectability in general. The patchset should be
> fully bisectable which means that it does not introduce any issues
> during "git bisect". This effectively means that at each intermediate
> step (after applying each patch, one by one) every existing stuff works
> the same as previously without any regression. Including booting with
> old DTB.

Oh, thanks for your careful explain, so I guess your first comment is 
talking about
the "bisectability" that if I only apply the 01 - 05 patches, kernel 
could not bootup
normally, cause driver need "analogix,color-space" but DTB only have 
"samsung,color-space".

>
>>> 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 ?
> Yes, you're 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.
> Sure. Now I found older versions of the patchset but previously there
> were no changes to the bindings. Again the prefix in subject is
> important to easily filter out and find necessary emails.
>
> BTW, I like the patchset because I like in general works which merge
> code and reduce duplicate stuff.

Aha, thanks  :-D

Best regards,
- Yakir

> Best regards,
> Krzysztof
>
>
>
>
>
Yakir Yang Aug. 24, 2015, 12:55 p.m. UTC | #8
Hi Jingoo,

? 08/24/2015 03:40 PM, Jingoo Han ??:
> On 2015. 8. 24., at AM 9:43, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>> 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.
>> 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.
>> 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.
>> 3. Use proper subject for the patch changing DTS. This is not
>> documentation change!
>> 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.
> To Yakir Yang,
>
> Please be careful when you CC people.
>
> I don't find the reason why you CC'ed the following people. Of course, they
> look to be one of the talented developers. However, they look to be
> unrelated to your patchset.
>
>   Takashi Iwai
>   Vincent Palatin
>   Fabio Estevam
>   Philipp Zabel

Yeah, actually I just got those people from patman tools. Thanks for your
remind, I would pay more attention in next version :-)

>
> Also, please add Exynos Architecture maintainers to CC list. I don't understand
> why you removed them from CC list.
>   Kukjin Kim
>   Krzysztof Kozlowski
>
> Without their Ack, you will not change the codes of ARM Exynos Architecture.

Wow, thanks a lot, it's a serious mistaken  ;)

Thanks,
- Yakir

> Best regards,
> Jingoo Han
>
>> 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
>
>
Russell King - ARM Linux Aug. 24, 2015, 12:57 p.m. UTC | #9
On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> > +       -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.

Please think about this some more.  What does "color-depth" mean?  Does it
mean the number of bits per colour _component_, or does it mean the total
number of bits to represent a particular colour.  It's confusing as it
stands.

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

I'm not surprised one iota that DRM bindings are a mess.  There's no one
overlooking the adoption of DRM bindings, so surprise surprise, everyone
does their own thing.  This is exactly what happens every time in that
scenario.  It's not a new problem.

When we adopted the graph bindings for iMX DRM, I thought exactly at that
time "it would be nice if this could become the standard for binding DRM
components together" but I don't have the authority from either the DT
perspective or the DRM perspective to mandate that.  Neither does anyone
else.  That's the _real_ problem here.

I've seen several DRM bindings go by which don't use the of-graph stuff,
which means that they'll never be compatible with generic components
which do use the of-graph stuff.

Like you say, it's a mess, but it's a mess of our own making, because no
one has the authority to regulate this.
Rob Herring Aug. 24, 2015, 2:48 p.m. UTC | #10
On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> > +       -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.
>
> Please think about this some more.  What does "color-depth" mean?  Does it
> mean the number of bits per colour _component_, or does it mean the total
> number of bits to represent a particular colour.  It's confusing as it
> stands.

Then "component-color-bpp" perhaps?

>
>> > +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.
>
> I'm not surprised one iota that DRM bindings are a mess.  There's no one
> overlooking the adoption of DRM bindings, so surprise surprise, everyone
> does their own thing.  This is exactly what happens every time in that
> scenario.  It's not a new problem.

True.

> When we adopted the graph bindings for iMX DRM, I thought exactly at that
> time "it would be nice if this could become the standard for binding DRM
> components together" but I don't have the authority from either the DT
> perspective or the DRM perspective to mandate that.  Neither does anyone
> else.  That's the _real_ problem here.
>
> I've seen several DRM bindings go by which don't use the of-graph stuff,
> which means that they'll never be compatible with generic components
> which do use the of-graph stuff.

It goes beyond bindings IMO. The use of the component framework or not
has been at the whim of driver writers as well. It is either used or
private APIs are created. I'm using components and my need for it
boils down to passing the struct drm_device pointer to the encoder.
Other components like panels and bridges have different ways to attach
to the DRM driver.

> Like you say, it's a mess, but it's a mess of our own making, because no
> one has the authority to regulate this.

Certainly, and I will take some blame here. We deferred a lot of
binding review to subsystem maintainers with the directive to ask for
help when needed. The latter has not happened. We need to improve the
situation here before we end up with a bigger mess. The momentum to
use DRM on Android is growing, so the problem is only going to get
worse if we do nothing.

Rob
Heiko Stuebner Aug. 24, 2015, 4:16 p.m. UTC | #11
Am Montag, 24. August 2015, 09:48:27 schrieb Rob Herring:
> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> > When we adopted the graph bindings for iMX DRM, I thought exactly at that
> > time "it would be nice if this could become the standard for binding DRM
> > components together" but I don't have the authority from either the DT
> > perspective or the DRM perspective to mandate that.  Neither does anyone
> > else.  That's the _real_ problem here.
> > 
> > I've seen several DRM bindings go by which don't use the of-graph stuff,
> > which means that they'll never be compatible with generic components
> > which do use the of-graph stuff.
> 
> It goes beyond bindings IMO. The use of the component framework or not
> has been at the whim of driver writers as well. It is either used or
> private APIs are created. I'm using components and my need for it
> boils down to passing the struct drm_device pointer to the encoder.
> Other components like panels and bridges have different ways to attach
> to the DRM driver.

but that is then simply implementation specific. Panels and bridges can very 
well be part of and created from an of_graph description without needing to be 
a (linux-)component - see patch 7.
Krzysztof Kozlowski Aug. 24, 2015, 11:49 p.m. UTC | #12
On 24.08.2015 21:48, Yakir Yang wrote:
> Hi Krzysztof,
> 
> ? 08/24/2015 12:20 PM, Krzysztof Kozlowski ??:
>> On 24.08.2015 11:42, Yakir Yang wrote:
>>> 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.
>> That is one of ways how to do this. However more important is that
>> driver should still support old bindings so such code:
>> -       if (of_property_read_u32(dp_node, "samsung,color-space",
>> +       if (of_property_read_u32(dp_node, "analogix,color-space",
>>
>> is probably wrong. Will the driver support old DTB in the same way as it
>> was supporting before the change?
> 
> Okay, I got your means. So document is not the focus, the most important
> is that
> driver should support the old dts prop.

Right, the focus is on the driver.

> If so the new analogix dp driver
> should keep
> the "samsung,color-space", rather then just mark it with [DEPRECATED] flag.

If you are replacing a binding/property then it should be marked
deprecated. This means that the old property is still working but new
users of it should not be added.

> 
> But from your follow suggest, I think you agree to update driver code,
> and just mark
> old prop with deprecated flag. If so I think such code would not be wrong
> 
> -       if (of_property_read_u32(dp_node, "samsung,color-space",
> +      if (of_property_read_u32(dp_node, "analogix,color-space",

It looks wrong because it breaks backward compatibility with existing
DTB. As I said before:
>>> 1. Provide backward compatibility. Mark old properties
>>> as deprecated but still support them.


> And actually @Rob have suggest me to remove the prefix, just use
> "color-space" here.

For new bindings I don't mind. But please remember about existing users,
existing DTB and bisectability.

> 
>>
>>> Let me show same examples, make
>>> me understand your suggest rightly.
>> exynos-dp already contains deprecated properties. Other ways of doing
>> this would be:
>> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> Documentation/devicetree/bindings/rtc/s3c-rtc.txt
>>
>> It depends up to you. The "touchscreen" looks good because it organizes
>> old properties in a common section. In case of exynos-dp.txt you can add
>> at beginning of file information about new bindings and mark everything
>> deprecated.
> 
> Whoops, thanks for your remind, I prefer the "touchscreen" style.
> 
>>> 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 ?
>> Yes, this is 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: ".
>> Yes.
>>
>>> Honestly, I don't understand what the "bisectability" means in this
>>> case.
>> I was referring to bisectability in general. The patchset should be
>> fully bisectable which means that it does not introduce any issues
>> during "git bisect". This effectively means that at each intermediate
>> step (after applying each patch, one by one) every existing stuff works
>> the same as previously without any regression. Including booting with
>> old DTB.
> 
> Oh, thanks for your careful explain, so I guess your first comment is
> talking about
> the "bisectability" that if I only apply the 01 - 05 patches, kernel
> could not bootup
> normally, cause driver need "analogix,color-space" but DTB only have
> "samsung,color-space".

Right. In the same time please remember that kernel may be booted with
old DTB.

Best regards,
Krzysztof
Yakir Yang Aug. 25, 2015, 1:21 a.m. UTC | #13
? 2015/8/24 22:48, Rob Herring ??:
> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>> +       -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.
>> Please think about this some more.  What does "color-depth" mean?  Does it
>> mean the number of bits per colour _component_, or does it mean the total
>> number of bits to represent a particular colour.  It's confusing as it
>> stands.
> Then "component-color-bpp" perhaps?

Actually this "color-bpp" should come from crtc driver, maybe should 
come from
"struct drm_crtc {".

Like rockchip stuffs, analogix_dp-rockchip call an mode_config from 
rockchip_drm_vop
driver and set output mode to RGB[10:10:10], then vop driver just store 
the output mode
type to the private struct "vop->connecot_out_mode". do think that this 
outmode should
store into crtc, not just come from DT prop.

- Yakir
Yakir Yang Aug. 25, 2015, 1:33 a.m. UTC | #14
Hi Krzysztof,

? 2015/8/25 7:49, Krzysztof Kozlowski ??:
> On 24.08.2015 21:48, Yakir Yang wrote:
>> Hi Krzysztof,
>>
>> ? 08/24/2015 12:20 PM, Krzysztof Kozlowski ??:
>>> On 24.08.2015 11:42, Yakir Yang wrote:
>>>> 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.
>>> That is one of ways how to do this. However more important is that
>>> driver should still support old bindings so such code:
>>> -       if (of_property_read_u32(dp_node, "samsung,color-space",
>>> +       if (of_property_read_u32(dp_node, "analogix,color-space",
>>>
>>> is probably wrong. Will the driver support old DTB in the same way as it
>>> was supporting before the change?
>> Okay, I got your means. So document is not the focus, the most important
>> is that
>> driver should support the old dts prop.
> Right, the focus is on the driver.
>
>> If so the new analogix dp driver
>> should keep
>> the "samsung,color-space", rather then just mark it with [DEPRECATED] flag.
> If you are replacing a binding/property then it should be marked
> deprecated. This means that the old property is still working but new
> users of it should not be added.

Okay, so just quote Heiko's reply, such code would be need in analogix 
dp driver.

        if (of_property_read_u32(dp_node, "analogix,color-space",
                                  &dp_video_config->color_space))
	       if (of_property_read_u32(dp_node, "samsung,color-space",
                 	                 &dp_video_config->color_space)) {

                 	dev_err(dev, "failed to get color-space\n");
                 	return ERR_PTR(-EINVAL);
         	}


>> But from your follow suggest, I think you agree to update driver code,
>> and just mark
>> old prop with deprecated flag. If so I think such code would not be wrong
>>
>> -       if (of_property_read_u32(dp_node, "samsung,color-space",
>> +      if (of_property_read_u32(dp_node, "analogix,color-space",
> It looks wrong because it breaks backward compatibility with existing
> DTB. As I said before:
>>>> 1. Provide backward compatibility. Mark old properties
>>>> as deprecated but still support them.
>
>> And actually @Rob have suggest me to remove the prefix, just use
>> "color-space" here.
> For new bindings I don't mind. But please remember about existing users,
> existing DTB and bisectability.
>
>>>> Let me show same examples, make
>>>> me understand your suggest rightly.
>>> exynos-dp already contains deprecated properties. Other ways of doing
>>> this would be:
>>> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>> Documentation/devicetree/bindings/rtc/s3c-rtc.txt
>>>
>>> It depends up to you. The "touchscreen" looks good because it organizes
>>> old properties in a common section. In case of exynos-dp.txt you can add
>>> at beginning of file information about new bindings and mark everything
>>> deprecated.
>> Whoops, thanks for your remind, I prefer the "touchscreen" style.
>>
>>>> 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 ?
>>> Yes, this is 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: ".
>>> Yes.
>>>
>>>> Honestly, I don't understand what the "bisectability" means in this
>>>> case.
>>> I was referring to bisectability in general. The patchset should be
>>> fully bisectable which means that it does not introduce any issues
>>> during "git bisect". This effectively means that at each intermediate
>>> step (after applying each patch, one by one) every existing stuff works
>>> the same as previously without any regression. Including booting with
>>> old DTB.
>> Oh, thanks for your careful explain, so I guess your first comment is
>> talking about
>> the "bisectability" that if I only apply the 01 - 05 patches, kernel
>> could not bootup
>> normally, cause driver need "analogix,color-space" but DTB only have
>> "samsung,color-space".
> Right. In the same time please remember that kernel may be booted with
> old DTB.

Okay, thanks a lot  ;)

- Yakir

> Best regards,
> Krzysztof
>
>
>
Krzysztof Kozlowski Aug. 25, 2015, 1:35 a.m. UTC | #15
On 25.08.2015 10:33, Yakir Yang wrote:
> Hi Krzysztof,
> 
> ? 2015/8/25 7:49, Krzysztof Kozlowski ??:
>> On 24.08.2015 21:48, Yakir Yang wrote:
>>> Hi Krzysztof,
>>>
>>> ? 08/24/2015 12:20 PM, Krzysztof Kozlowski ??:
>>>> On 24.08.2015 11:42, Yakir Yang wrote:
>>>>> 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.
>>>> That is one of ways how to do this. However more important is that
>>>> driver should still support old bindings so such code:
>>>> -       if (of_property_read_u32(dp_node, "samsung,color-space",
>>>> +       if (of_property_read_u32(dp_node, "analogix,color-space",
>>>>
>>>> is probably wrong. Will the driver support old DTB in the same way
>>>> as it
>>>> was supporting before the change?
>>> Okay, I got your means. So document is not the focus, the most important
>>> is that
>>> driver should support the old dts prop.
>> Right, the focus is on the driver.
>>
>>> If so the new analogix dp driver
>>> should keep
>>> the "samsung,color-space", rather then just mark it with [DEPRECATED]
>>> flag.
>> If you are replacing a binding/property then it should be marked
>> deprecated. This means that the old property is still working but new
>> users of it should not be added.
> 
> Okay, so just quote Heiko's reply, such code would be need in analogix
> dp driver.
> 
>        if (of_property_read_u32(dp_node, "analogix,color-space",
>                                  &dp_video_config->color_space))
>            if (of_property_read_u32(dp_node, "samsung,color-space",
>                                      &dp_video_config->color_space)) {
> 
>                     dev_err(dev, "failed to get color-space\n");
>                     return ERR_PTR(-EINVAL);
>             }

Yes. It does not look pretty but something like this is needed.

Best regards,
Krzysztof
Thierry Reding Aug. 25, 2015, 9:12 a.m. UTC | #16
On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >> > +       -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.
> >
> > Please think about this some more.  What does "color-depth" mean?  Does it
> > mean the number of bits per colour _component_, or does it mean the total
> > number of bits to represent a particular colour.  It's confusing as it
> > stands.
> 
> Then "component-color-bpp" perhaps?

There should be no need to have this in DT at all. The BPC is a property
of the attached panel and it should come from the panel (either the
panel driver or parsed from EDID if available).

> > When we adopted the graph bindings for iMX DRM, I thought exactly at that
> > time "it would be nice if this could become the standard for binding DRM
> > components together" but I don't have the authority from either the DT
> > perspective or the DRM perspective to mandate that.  Neither does anyone
> > else.  That's the _real_ problem here.
> >
> > I've seen several DRM bindings go by which don't use the of-graph stuff,
> > which means that they'll never be compatible with generic components
> > which do use the of-graph stuff.
> 
> It goes beyond bindings IMO. The use of the component framework or not
> has been at the whim of driver writers as well. It is either used or
> private APIs are created. I'm using components and my need for it
> boils down to passing the struct drm_device pointer to the encoder.
> Other components like panels and bridges have different ways to attach
> to the DRM driver.

I certainly support unification, but it needs to be reasonable. There
are cases where a different structure for the binding work better than
another and I think this always needs to be evaluated on a case by case
basis.

Because of that I think it makes sense to make all these framework bits
opt-in, otherwise we could easily end up in a situation where drivers
have to be rearchitected (or even DT bindings altered!) in order to be
able to reuse code.

Thierry
Thierry Reding Aug. 25, 2015, 9:15 a.m. UTC | #17
On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
[...]
> > +       -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.

There is no need whatsoever to hard-code this in DT. (e)DP provides the
means to detect what rate the link supports and the specification
provides guidance on how to select an appropriate one.

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

Same as for the link rate.

Thierry
Russell King - ARM Linux Aug. 25, 2015, 9:29 a.m. UTC | #18
On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> > It goes beyond bindings IMO. The use of the component framework or not
> > has been at the whim of driver writers as well. It is either used or
> > private APIs are created. I'm using components and my need for it
> > boils down to passing the struct drm_device pointer to the encoder.
> > Other components like panels and bridges have different ways to attach
> > to the DRM driver.
> 
> I certainly support unification, but it needs to be reasonable. There
> are cases where a different structure for the binding work better than
> another and I think this always needs to be evaluated on a case by case
> basis.

It can't be a case-by-case basis.

The TDA998x encoder/connector is going to be component only.  This is
a generic chip, which can be attached to the output of any parallel
RGB+sync+clock bus.  In other words, it could appear anywhere.

Are you really saying that we need to support multiple schemes of
attaching the driver to DRM?  That's totally insane IMHO.

The problem with the drm_encoder_slave stuff is that you can't sanely
attach of-nodes to the drm-created i2c device.  Yes, you can parse
them from the DT file as a sub-node of the upper device, but that
then goes against the principle of the I2C bindings, which is to
list the I2C devices as a child below the I2C adapter node.  If you
try and put the DT node there, then the OF code will create the I2C
device for you, and the drm_encoder_slave stuff won't have the
control it needs to communicate through the wrapped i2c_driver
stuff.

So, tda998x is going component-only, as that's the _only_ sane solution
for it.

Now, what happens when some other DRM driver wants to use the tda998x
driver, and its bindings are not compatible with the component helpers?
They're pretty much stuck up the creek without a paddle.

Case by case doesn't work unless you're talking about truely isolated
hardware where no one shares anything.
Yakir Yang Aug. 25, 2015, 9:37 a.m. UTC | #19
Hi Thierry,

? 2015/8/25 17:15, Thierry Reding ??:
> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> [...]
>>> +       -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.
> There is no need whatsoever to hard-code this in DT. (e)DP provides the
> means to detect what rate the link supports and the specification
> provides guidance on how to select an appropriate one.

Hmm... could you share more about this :-)

I only find that drm_dp_link_probe() could get the panel link-rate and
num-lanes by reading the DP_DPCD_REV messag.

I don't found there are some guidance to help select the approriate one.
Beside this DT prop just indicate the max eDP controller link-rate & lanes
support, how could eDP detect them automatically?

- Yakir

>>> +       -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.
> Same as for the link rate.
>
> Thierry
Yakir Yang Aug. 25, 2015, 9:41 a.m. UTC | #20
Hi Thierry,

? 2015/8/25 17:12, Thierry Reding ??:
> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
>> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>>> +       -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.
>>> Please think about this some more.  What does "color-depth" mean?  Does it
>>> mean the number of bits per colour _component_, or does it mean the total
>>> number of bits to represent a particular colour.  It's confusing as it
>>> stands.
>> Then "component-color-bpp" perhaps?
> There should be no need to have this in DT at all. The BPC is a property
> of the attached panel and it should come from the panel (either the
> panel driver or parsed from EDID if available).

Actually I have send an email about this one to you in version 2, just 
past from that email:

"samsung,color_space" and "samsung,color-depth"

The drm_display_info's color_formats and bpc indicate the monitor 
display ability, but
the edp driver could not take it as input video format directly.

For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
support in drm_display_info.color_formats and 16bit bpc support, but 
RK3288 crtc
driver could only output RGB & ITU formats, so finally 
analogix_dp-rockchip driver
config crtc to RGBaaa 10bpc mode.

In this sutiation, the analogix_dp core driver would pazzled by the 
drm_display_info,
can't chose the right color_space and bpc.

And this is the place that confused me, wish you could give some ideas 
about this one :-)

- Yakir
>>> When we adopted the graph bindings for iMX DRM, I thought exactly at that
>>> time "it would be nice if this could become the standard for binding DRM
>>> components together" but I don't have the authority from either the DT
>>> perspective or the DRM perspective to mandate that.  Neither does anyone
>>> else.  That's the _real_ problem here.
>>>
>>> I've seen several DRM bindings go by which don't use the of-graph stuff,
>>> which means that they'll never be compatible with generic components
>>> which do use the of-graph stuff.
>> It goes beyond bindings IMO. The use of the component framework or not
>> has been at the whim of driver writers as well. It is either used or
>> private APIs are created. I'm using components and my need for it
>> boils down to passing the struct drm_device pointer to the encoder.
>> Other components like panels and bridges have different ways to attach
>> to the DRM driver.
> I certainly support unification, but it needs to be reasonable. There
> are cases where a different structure for the binding work better than
> another and I think this always needs to be evaluated on a case by case
> basis.
>
> Because of that I think it makes sense to make all these framework bits
> opt-in, otherwise we could easily end up in a situation where drivers
> have to be rearchitected (or even DT bindings altered!) in order to be
> able to reuse code.
>
> Thierry
Thierry Reding Aug. 25, 2015, 9:58 a.m. UTC | #21
On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
[...]
> +	-analogix,color-space:
> +		input video data format.
> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2

I don't think DT is an appropriate place to set this. To my knowledge
this depends on the display and/or mode, so I don't think hard-coding
it here is the right thing to do.

Thierry
Thierry Reding Aug. 25, 2015, 10:06 a.m. UTC | #22
On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> ? 2015/8/25 17:12, Thierry Reding ??:
> >On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> >>On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> >><linux@arm.linux.org.uk> wrote:
> >>>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >>>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>>>>+       -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.
> >>>Please think about this some more.  What does "color-depth" mean?  Does it
> >>>mean the number of bits per colour _component_, or does it mean the total
> >>>number of bits to represent a particular colour.  It's confusing as it
> >>>stands.
> >>Then "component-color-bpp" perhaps?
> >There should be no need to have this in DT at all. The BPC is a property
> >of the attached panel and it should come from the panel (either the
> >panel driver or parsed from EDID if available).
> 
> Actually I have send an email about this one to you in version 2, just past
> from that email:
> 
> "samsung,color_space" and "samsung,color-depth"
> 
> The drm_display_info's color_formats and bpc indicate the monitor display
> ability, but
> the edp driver could not take it as input video format directly.
> 
> For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
> support in drm_display_info.color_formats and 16bit bpc support, but RK3288
> crtc
> driver could only output RGB & ITU formats, so finally analogix_dp-rockchip
> driver
> config crtc to RGBaaa 10bpc mode.
> 
> In this sutiation, the analogix_dp core driver would pazzled by the
> drm_display_info,
> can't chose the right color_space and bpc.
> 
> And this is the place that confused me, wish you could give some ideas about
> this one :-)

Your display driver should choose whatever it is capable of outputting.
If the display reports that it can do 16 bits-per-color, but your
display driver can't do it, then it should choose a configuration that
it supports. Similarily for the color encodings. If you can't generate
YCrCb444 with your hardware, then it's the driver's job to know about
that and select the next appropriate configuration.

But hard-coding this is not the right solution because the value in DT
may end up conflicting with what the display reports.

Thierry
Thierry Reding Aug. 25, 2015, 10:40 a.m. UTC | #23
On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
> > On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> > > It goes beyond bindings IMO. The use of the component framework or not
> > > has been at the whim of driver writers as well. It is either used or
> > > private APIs are created. I'm using components and my need for it
> > > boils down to passing the struct drm_device pointer to the encoder.
> > > Other components like panels and bridges have different ways to attach
> > > to the DRM driver.
> > 
> > I certainly support unification, but it needs to be reasonable. There
> > are cases where a different structure for the binding work better than
> > another and I think this always needs to be evaluated on a case by case
> > basis.
> 
> It can't be a case-by-case basis.
> 
> The TDA998x encoder/connector is going to be component only.  This is
> a generic chip, which can be attached to the output of any parallel
> RGB+sync+clock bus.  In other words, it could appear anywhere.
> 
> Are you really saying that we need to support multiple schemes of
> attaching the driver to DRM?  That's totally insane IMHO.

No, what I'm saying is that we should have a single scheme, but one
that doesn't put any restrictions on what kind of DT binding you use or
how your driver is architected.

> The problem with the drm_encoder_slave stuff is that you can't sanely
> attach of-nodes to the drm-created i2c device.  Yes, you can parse
> them from the DT file as a sub-node of the upper device, but that
> then goes against the principle of the I2C bindings, which is to
> list the I2C devices as a child below the I2C adapter node.  If you
> try and put the DT node there, then the OF code will create the I2C
> device for you, and the drm_encoder_slave stuff won't have the
> control it needs to communicate through the wrapped i2c_driver
> stuff.
> 
> So, tda998x is going component-only, as that's the _only_ sane solution
> for it.

Has anyone ever considered turning it into a DRM bridge driver? I had
always envisioned component/master to be primarily useful to glue
together various SoC components to form one componentized device. Now
if tda998x is an I2C slave it is external to the SoC (auxiliary), so
in my opinion much better off as a bridge driver.

Bridge drivers don't come with any of the disadvantages that the
drm_encoder_slave stuff has. They are regular drivers that are probed
via their parent busses (I2C, platform, SPI, ...) and hook into DRM via
an abstract interface. The DT aspect is taken care of automatically
because they get instantiated by their parent bus like any other device.

> Now, what happens when some other DRM driver wants to use the tda998x
> driver, and its bindings are not compatible with the component helpers?
> They're pretty much stuck up the creek without a paddle.

I'm sure that will be very helpful response for whoever's going to end
up having to deal with that situation.

> Case by case doesn't work unless you're talking about truely isolated
> hardware where no one shares anything.

There are two different things here. The inter-driver interface, which,
in my opinion, it makes a lot of sense to standardize. Like I mentioned
above I think it unwise to make this interface depend upon a framework
or the firmware description such as DT in order to avoid unnecessary
restrictions. The second, orthogonal, issue, is the DT bindings. Those
I think should absolutely be designed case by case and select whatever
most accurately describes the hardware.

Thierry
Russell King - ARM Linux Aug. 25, 2015, 10:52 a.m. UTC | #24
On Tue, Aug 25, 2015 at 12:40:01PM +0200, Thierry Reding wrote:
> On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
> > Now, what happens when some other DRM driver wants to use the tda998x
> > driver, and its bindings are not compatible with the component helpers?
> > They're pretty much stuck up the creek without a paddle.
> 
> I'm sure that will be very helpful response for whoever's going to end
> up having to deal with that situation.

Thank you for that comment, it's very constructive and much appreciated.
I can see it's well worth me continuing to spend time on this thread.
Rob Herring Aug. 25, 2015, 1:27 p.m. UTC | #25
On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> [...]
>> > +       -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.
>
> There is no need whatsoever to hard-code this in DT. (e)DP provides the
> means to detect what rate the link supports and the specification
> provides guidance on how to select an appropriate one.

Good, even better.

Rob
Yakir Yang Aug. 25, 2015, 1:48 p.m. UTC | #26
Hi Thierry & Rob,

? 2015/8/25 21:27, Rob Herring ??:
> On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> [...]
>>>> +       -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.
>> There is no need whatsoever to hard-code this in DT. (e)DP provides the
>> means to detect what rate the link supports and the specification
>> provides guidance on how to select an appropriate one.
> Good, even better.

I do think we still need keep this DT prop yet.

I think drm_dp_help.c could get the "panel" max link-rate and lane-count,
but it's not enough, we still need knew the "eDP controller" max link-rate
and lane-count.

Let me show the exact example that happened in my side. When I connect
my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP 
controller link-rate
to 5.4Gbps, the DP TV just broken, do not light up normally.

This reason why TV broken is the max link-rate which support by RK3288 eDP
controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:

*??Compliant with DisplayPortTM Specification, Version 1.2.
??Compliant with eDPTM Specification, Version 1.3.
??HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
??Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
*
**


Beside I haven't found there are some registers would indicate the eDP 
controller
max link-rate and lane-count, so this is why I still instance that we 
need this DT
prop to indicata "Max rate controller support".

So, I wish you could agree with me on this point.


Thanks,
- Yakir

> Rob
>
>
>
Yakir Yang Aug. 25, 2015, 2:02 p.m. UTC | #27
Hi Thierry,

? 2015/8/25 18:06, Thierry Reding ??:
> On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote:
>> Hi Thierry,
>>
>> ? 2015/8/25 17:12, Thierry Reding ??:
>>> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
>>>> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>>>>> +       -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.
>>>>> Please think about this some more.  What does "color-depth" mean?  Does it
>>>>> mean the number of bits per colour _component_, or does it mean the total
>>>>> number of bits to represent a particular colour.  It's confusing as it
>>>>> stands.
>>>> Then "component-color-bpp" perhaps?
>>> There should be no need to have this in DT at all. The BPC is a property
>>> of the attached panel and it should come from the panel (either the
>>> panel driver or parsed from EDID if available).
>> Actually I have send an email about this one to you in version 2, just past
>> from that email:
>>
>> "samsung,color_space" and "samsung,color-depth"
>>
>> The drm_display_info's color_formats and bpc indicate the monitor display
>> ability, but
>> the edp driver could not take it as input video format directly.
>>
>> For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
>> support in drm_display_info.color_formats and 16bit bpc support, but RK3288
>> crtc
>> driver could only output RGB & ITU formats, so finally analogix_dp-rockchip
>> driver
>> config crtc to RGBaaa 10bpc mode.
>>
>> In this sutiation, the analogix_dp core driver would pazzled by the
>> drm_display_info,
>> can't chose the right color_space and bpc.
>>
>> And this is the place that confused me, wish you could give some ideas about
>> this one :-)
> Your display driver should choose whatever it is capable of outputting.
> If the display reports that it can do 16 bits-per-color, but your
> display driver can't do it, then it should choose a configuration that
> it supports. Similarily for the color encodings. If you can't generate
> YCrCb444 with your hardware, then it's the driver's job to know about
> that and select the next appropriate configuration.
>
> But hard-coding this is not the right solution because the value in DT
> may end up conflicting with what the display reports.

Yeah, thanks for your explain, you are right. It's the best way to get 
"color-depth"
and "color-space" from display driver, not to hard-code in DT prop.

But if the common analogix-dp driver want to get those values, then 
those values
should come from the common drm struct data. Personally I think "struct 
drm_crtc"
is the best place that should indicate the output ability of SoC vop/lcdc.

But I haven't find out there are some place to store those message for 
now (I don't
think it's good to modify the original color-space and color-bpc which 
parsed from
monitor edid).

So could you share sme ideas about this, and I would rather to talk with 
Mark (Author
of rockchip drm driver) to find out the better way to fix this one.

Besides, I would appreciate very much if you can share some ideas about 
how Exynos
handler with this problem ;)

Thanks,
- Yakir
> Thierry
Yakir Yang Aug. 25, 2015, 2:03 p.m. UTC | #28
Hi Thierry,

? 2015/8/25 17:58, Thierry Reding ??:
> On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
> [...]
>> +	-analogix,color-space:
>> +		input video data format.
>> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> I don't think DT is an appropriate place to set this. To my knowledge
> this depends on the display and/or mode, so I don't think hard-coding
> it here is the right thing to do.

Yeah, same question with my previous reply ;)

Thanks,
- Yakir
>
> Thierry
Thierry Reding Aug. 25, 2015, 2:16 p.m. UTC | #29
On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote:
> Hi Thierry & Rob,
> 
> ? 2015/8/25 21:27, Rob Herring ??:
> >On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
> >>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>[...]
> >>>>+       -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.
> >>There is no need whatsoever to hard-code this in DT. (e)DP provides the
> >>means to detect what rate the link supports and the specification
> >>provides guidance on how to select an appropriate one.
> >Good, even better.
> 
> I do think we still need keep this DT prop yet.
> 
> I think drm_dp_help.c could get the "panel" max link-rate and lane-count,
> but it's not enough, we still need knew the "eDP controller" max link-rate
> and lane-count.
> 
> Let me show the exact example that happened in my side. When I connect
> my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
> from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller
> link-rate
> to 5.4Gbps, the DP TV just broken, do not light up normally.
> 
> This reason why TV broken is the max link-rate which support by RK3288 eDP
> controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:
> 
> *??Compliant with DisplayPortTM Specification, Version 1.2.
> ??Compliant with eDPTM Specification, Version 1.3.
> ??HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
> ??Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
> *
> **
> 
> 
> Beside I haven't found there are some registers would indicate the eDP
> controller
> max link-rate and lane-count, so this is why I still instance that we need
> this DT
> prop to indicata "Max rate controller support".
> 
> So, I wish you could agree with me on this point.

Your driver should know what link rates it supports and restrict itself
to use those. This is implied by the compatible string and doesn't need
to be duplicated into device tree.

Thierry
Thierry Reding Aug. 25, 2015, 2:21 p.m. UTC | #30
On Tue, Aug 25, 2015 at 10:03:52PM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> ? 2015/8/25 17:58, Thierry Reding ??:
> >On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
> >[...]
> >>+	-analogix,color-space:
> >>+		input video data format.
> >>+			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> >I don't think DT is an appropriate place to set this. To my knowledge
> >this depends on the display and/or mode, so I don't think hard-coding
> >it here is the right thing to do.
> 
> Yeah, same question with my previous reply ;)

I don't have an answer for you, unfortunately. But like I said,
hard-coding isn't going to work. What if, for example, you set this to a
fixed value and then you connect a monitor that doesn't support the
specific one you set?

You cited code from dw_hdmi.c earlier, it looks like it might be correct
even though it doesn't cite a reference for why this was done. Perhaps
someone on this thread, or someone involved with dw_hdmi can answer
where that code came from.

Thierry
Yakir Yang Aug. 25, 2015, 2:23 p.m. UTC | #31
Hi Thierry,

? 2015/8/25 22:16, Thierry Reding ??:
> On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote:
>> Hi Thierry & Rob,
>>
>> ? 2015/8/25 21:27, Rob Herring ??:
>>> On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
>>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>> [...]
>>>>>> +       -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.
>>>> There is no need whatsoever to hard-code this in DT. (e)DP provides the
>>>> means to detect what rate the link supports and the specification
>>>> provides guidance on how to select an appropriate one.
>>> Good, even better.
>> I do think we still need keep this DT prop yet.
>>
>> I think drm_dp_help.c could get the "panel" max link-rate and lane-count,
>> but it's not enough, we still need knew the "eDP controller" max link-rate
>> and lane-count.
>>
>> Let me show the exact example that happened in my side. When I connect
>> my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
>> from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller
>> link-rate
>> to 5.4Gbps, the DP TV just broken, do not light up normally.
>>
>> This reason why TV broken is the max link-rate which support by RK3288 eDP
>> controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:
>>
>> *??Compliant with DisplayPortTM Specification, Version 1.2.
>> ??Compliant with eDPTM Specification, Version 1.3.
>> ??HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
>> ??Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
>> *
>> **
>>
>>
>> Beside I haven't found there are some registers would indicate the eDP
>> controller
>> max link-rate and lane-count, so this is why I still instance that we need
>> this DT
>> prop to indicata "Max rate controller support".
>>
>> So, I wish you could agree with me on this point.
> Your driver should know what link rates it supports and restrict itself
> to use those. This is implied by the compatible string and doesn't need
> to be duplicated into device tree.

Oh, yeah, good idea   :-D
Thanks for your point out.

- Yakir

> Thierry
Russell King - ARM Linux Aug. 25, 2015, 3:57 p.m. UTC | #32
On Tue, Aug 25, 2015 at 04:21:51PM +0200, Thierry Reding wrote:
> You cited code from dw_hdmi.c earlier, it looks like it might be correct
> even though it doesn't cite a reference for why this was done. Perhaps
> someone on this thread, or someone involved with dw_hdmi can answer
> where that code came from.

dw_hdmi doesn't do any format conversion - it's hard coded to RGB, 8
bits per colour component.  That's a requirement for all HDMI sinks.

The reason it's hard-coded in dw_hdmi is that (a) no one has yet decided
its worth the effort to get the dw_hdmi hardware to do the colourspace
conversion to the YUV spaces and verify that it works, and (b) I really
don't see the point when we're talking about computer like devices which
work primerily with RGB and RGB is always supported by the sink.

As far as greater colour depths go, the driver came from the Freescale
iMX6 code base, and the hardware which feeds dw_hdmi can't do more than
8 bits per component - so going to 10, 12 or 16 bits per component is
beyond what iMX6 can cope with.  Hence, no one on the iMX6 side has a
need for the deep colour stuff.

In any case, I view this as a very low priority issue - it would be nice
to have audio support on HDMI for iMX6 at some point in the next 20 years,
preferably before the hardware becomes obsolete.  I've been maintaining
patches for this for 1.5 years now... how much longer is it going to take?
My pull request to David from 15th July was ignored.  My re-send of that
after he returned was ignored.  My reminder of it has been ignored.  What's
going on in DRM land?  It would be nice to get _some_ kind of feedback so
I know why they're not being taken, so I can fix whatever the issue is.
diff mbox

Patch

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
+	-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
+	-analogix,lane-count:
+		max number of lanes supported by the eDP contoller.
+			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
+	-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
+	-video interfaces: Device node can contain video interface port
+			    nodes according to [1].
+
+[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>;
 };