Subject: Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver
To: Thierry Reding <treding@nvidia.com>
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>
Cc: =?UTF-8?Q?Heiko_St=c3=bcbner?= <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
From: Yakir Yang <ykk@rock-chips.com>
Message-ID: <55C8A040.8090405@rock-chips.com>
Date: Mon, 10 Aug 2015 20:59:44 +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: <20150810100055.GB7850@ulmo.nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2939
Lines: 79

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.

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

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/