Message-ID: <55C8D01A.7010508@rock-chips.com>
Date:	Tue, 11 Aug 2015 00:23:54 +0800
From:	Yakir Yang <ykk@...k-chips.com>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Thierry Reding <treding@...dia.com>,
	Heiko Stübner <heiko@...ech.de>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Fabio Estevam <fabio.estevam@...escale.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Inki Dae <inki.dae@...sung.com>, djkurtz@...gle.com,
	dianders@...gle.com, seanpaul@...gle.com, joe@...ches.com,
	Takashi Iwai <tiwai@...e.de>,
	Andrzej Hajda <a.hajda@...sung.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	David Airlie <airlied@...ux.ie>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	Seung-Woo Kim <sw0312.kim@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Kukjin Kim <kgene@...nel.org>,
	Ajay Kumar <ajaykumar.rs@...sung.com>,
	Joonyoung Shim <jy0922.shim@...sung.com>,
	Vincent Palatin <vpalatin@...omium.org>,
	Mark Yao <mark.yao@...k-chips.com>,
	Andy Yan <andy.yan@...k-chips.com>, ajaynumb@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, linux-arm-ker@...L.NULL,
	nel@...ts.infradead.org
Subject: Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

Hi Thierry,

在 2015/8/10 21:17, Thierry Reding 写道:
> On Mon, Aug 10, 2015 at 08:59:44PM +0800, Yakir Yang wrote:
>> Hi Thierry,
>>
>> 在 2015/8/10 18:00, Thierry Reding 写道:
>>> On Sat, Aug 08, 2015 at 11:54:38AM +0800, Yakir Yang wrote:
>>> [...]
>>>>          edp: edp@...70000 {
>>> [...]
>>>>                  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.
>> I do think so, those numbers can parse from struct drm_mode. But I haven't
>> send those changes yet, cause I want to merge the split analogix_dp first,
>> and
>> then send some patches to improve it. If you think it's better to imptoved
>> those
>> now, I would like to do it , please let me know ;)
>>
>>>>                  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.
>> Same to previous reply
>>
>>>>                  samsung,color-depth = <1>;
>>> This is probably drm_display_info.bpc.
>> Same to previous reply
>>
>>>>                  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?
>> Yes, I do meet the problem that my eDP screen need lane-count to 4, but my
>> DP TV need lane-count to 1. Just like previous reply, if you think I should
>> improved
>> them in this series, I would rather to do it.
> The problem with these is that if you keep them in for your initial
> submission, you can never (or only under extreme pain) remove them.
> Anything in DTB needs to be effectively supported forever.
>
> Also since these don't make sense to hard-code, just improve the code
> and get rid of the need for these DT properties. Mind you that you still
> need to keep the code to parse them, because presumably Exynos relies on
> them. But depending on how you split up the driver you might be able to
> restrict these compatibility hacks to Exynos and not carry them forward
> into your new driver.

Okay, thanks for your remind ;)

>>>>>> +	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.
>> The sclk_dp & sclk_dp_24m is not IP common ask, it's only exist in RK3288
>> SoC (Like exynos
>> only got one "dp" clock), and actually I add this to rockchip platform dp
>> driver not analogix
>> dp driver. So I think it's okay to add some platform some common prefixes.
>>
>> And I got a better idea for those clock. "sclk_dp" & "sclk_dp_24m" is
>> provided for the eDP phy,
>> and I just take Heiko suggest that add an new phy-rockchip-dp.c driver, so
>> it's better to move
>> those clock to phy driver, and rename them to "dp-phy" && "dp-phy-24m".
> I agree that dealing with these in a PHY driver sounds like the better
> option. However, I still think that the dp-phy prefix is redundant. The
> names are in a per-driver scope, so "dp-phy" is implied by the device
> tree binding and driver already. You could simply use shorter names such
> as "phy" and "24m" for example.
>
> Also note that the clock provider will already have the proper names for
> these, so the clock tree will end up showing the provider names. The
> names in the binding are merely the "consumer" names.

Agree, thanks

- Yakir
> Thierry


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

Powered by blists - more mailing lists