Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

From: Thierry Reding <treding@nvidia.com>
To: Yakir Yang <ykk@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>,
	"Russell King" <rmk+kernel@arm.linux.org.uk>,
	"Fabio Estevam" <fabio.estevam@freescale.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Inki Dae" <inki.dae@samsung.com>,
	djkurtz@google.com, dianders@google.com, seanpaul@google.com,
	joe@perches.com, "Takashi Iwai" <tiwai@suse.de>,
	"Andrzej Hajda" <a.hajda@samsung.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"David Airlie" <airlied@linux.ie>,
	"Gustavo Padovan" <gustavo.padovan@collabora.co.uk>,
	"Seung-Woo Kim" <sw0312.kim@samsung.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Krzysztof Kozlowski" <k.kozlowski@samsung.com>,
	"Kukjin Kim" <kgene@kernel.org>,
	"Ajay Kumar" <ajaykumar.rs@samsung.com>,
	"Joonyoung Shim" <jy0922.shim@samsung.com>,
	"Vincent Palatin" <vpalatin@chromium.org>,
	"Mark Yao" <mark.yao@rock-chips.com>,
	"Andy Yan" <andy.yan@rock-chips.com>,
	ajaynumb@gmail.com, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
	linux-rockchip@lists.infradead.org, linux-arm-ker@NULL.NULL,
	nel@lists.infradead.org
Subject: Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver
Date: Mon, 10 Aug 2015 12:00:57 +0200	[thread overview]
Message-ID: <20150810100055.GB7850@ulmo.nvidia.com> (raw)
In-Reply-To: <55C57D7E.6080800@rock-chips.com>

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On Sat, Aug 08, 2015 at 11:54:38AM +0800, Yakir Yang wrote:
[...]
>         edp: edp@ff970000 {
[...]
>                 hsync-active-high = <0>;
>                 vsync-active-high = <0>;
>                 interlaced = <0>;

These look like they should come from the display mode definition (EDID)
rather than device tree.

>                 samsung,color-space = <0>;
>                 samsung,dynamic-range = <0>;
>                 samsung,ycbcr-coeff = <0>;

I think these should also come from EDID, though I'm not sure if we
store this in internal data structures yet.

>                 samsung,color-depth = <1>;

This is probably drm_display_info.bpc.

>                 samsung,link-rate = <0x0a>;
>                 samsung,lane-count = <1>;

And these should really be derived from values in the DPCD and adjusted
(if necessary) during link training.

Why would you ever want to hard-code the above?

> >>+	dp->clk_24m = devm_clk_get(dev, "clk_dp_24m");
> >Same here, maybe "dp_24m".
> Like my previous reply. And actually as those two clocks all have
> a common prefix "SCLK" in rk3288 clock tree, I thinkt we can name
> them to "sclk_dp" & "sclk_dp_24m", is it okay ?

I don't think there's a need for these common prefixes. The names here
are identifiers in the context of the IP block, so any SoC-specific
prefixes are irrelevant. Also they do appear, in DT and in code, in the
context of clocks already, so "sclk_" or "clk_" is completely redundant
in these names.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]