Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932194AbbHJQYS (ORCPT ); Mon, 10 Aug 2015 12:24:18 -0400 Received: from regular1.263xmail.com ([211.150.99.139]:41141 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754041AbbHJQYP (ORCPT ); Mon, 10 Aug 2015 12:24:15 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: nel@lists.infradead.org X-SENDER-IP: 220.249.180.47 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: <3a976037db7b41d7067a09feae250014> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver To: Thierry Reding References: <1438943674-18191-1-git-send-email-ykk@rock-chips.com> <1438944380-18897-1-git-send-email-ykk@rock-chips.com> <1730542.s7otqjtiXD@diego> <55C57D7E.6080800@rock-chips.com> <20150810100055.GB7850@ulmo.nvidia.com> <55C8A040.8090405@rock-chips.com> <20150810131709.GA7302@ulmo.nvidia.com> Cc: Thierry Reding , =?UTF-8?Q?Heiko_St=c3=bcbner?= , Russell King , Fabio Estevam , Jingoo Han , Inki Dae , djkurtz@google.com, dianders@google.com, seanpaul@google.com, joe@perches.com, Takashi Iwai , Andrzej Hajda , Philipp Zabel , David Airlie , Gustavo Padovan , Seung-Woo Kim , Kyungmin Park , Krzysztof Kozlowski , Kukjin Kim , Ajay Kumar , Joonyoung Shim , Vincent Palatin , Mark Yao , Andy Yan , 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 From: Yakir Yang Message-ID: <55C8D01A.7010508@rock-chips.com> Date: Tue, 11 Aug 2015 00:23:54 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20150810131709.GA7302@ulmo.nvidia.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4360 Lines: 99 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@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. >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/