| Message ID | 1439995834-18363-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 > > > > >
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 > >
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.
Hi Yakir, Am Montag, 24. August 2015, 20:48:01 schrieb Yakir Yang: > ? 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", In a generic driver, the property should have either none, or the analogix prefix. But DT-properties need to be backwards compatible, meaning an older Exynos devicetree should run unmodified with a newer kernel. So the common course of action is to mark the old one as deprecated but still test for both, so something like: 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); }
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
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.
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
? 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
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 > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
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 > > >
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
Hi Heiko, ? 2015/8/24 21:03, Heiko Stuebner ??: > Hi Yakir, > > Am Montag, 24. August 2015, 20:48:01 schrieb Yakir Yang: >> ? 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", > In a generic driver, the property should have either none, or the analogix > prefix. But DT-properties need to be backwards compatible, meaning an older > Exynos devicetree should run unmodified with a newer kernel. > > So the common course of action is to mark the old one as deprecated but still > test for both, so something like: > > 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); > } > Wow, thanks a lot for your explain and code, it do help me to understand this suggest rightly :-) Thanks, - Yakir > > > > >
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
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
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.
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
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
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
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.
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
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
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
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 --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>; };
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