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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
