| Message ID | 1730542.s7otqjtiXD@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Yakir, Am Samstag, 8. August 2015, 11:54:38 schrieb Yakir Yang: > >> +static int rockchip_dp_init(struct rockchip_dp_device *dp) > >> +{ > >> + struct device *dev = dp->dev; > >> + struct device_node *np = dev->of_node; > >> + int ret; > >> + > >> + dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf"); > >> + if (IS_ERR(dp->grf)) { > >> + dev_err(dev, > >> + "rk3288-dp needs rockchip,grf property\n"); > >> + return PTR_ERR(dp->grf); > >> + } > >> + > >> + dp->clk_dp = devm_clk_get(dev, "clk_dp"); > > > > I've looked at the manual, but couldn't find an actual clock-name > > used there. Is it really "clk_dp" or should it just be "dp"? > > This should be "clk_dp", not "dp". > Cause analogix_dp_core would need a clock name with "dp", so I would > rather to pasted my rockchip-dp node here before I add dt-bindings in > next version ;) The clock we name PCLK_EDP_CTRL in the clock controller is probably the clock supplying the APB interface and named pclk already in the "Figure 3-2 DP_TXclock domain" diagram on page 19 of the manual. So your "clk_dp" should actually be "pclk". So you would have "dp", "dp_24m" and "pclk" for the 3 supplying clocks. > > edp: edp@ff970000 { > compatible = "rockchip,rk3288-dp"; > reg = <0xff970000 0x4000>; > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&cru SCLK_EDP>, <&cru SCLK_EDP_24M>, <&cru > PCLK_EDP_CTRL>; > clock-names = "clk_dp", "clk_dp_24m", "dp"; > > rockchip,grf = <&grf>; > resets = <&cru 111>; > reset-names = "dp"; > power-domains = <&power RK3288_PD_VIO>; > status = "disabled"; > > hsync-active-high = <0>; > vsync-active-high = <0>; > interlaced = <0>; > samsung,color-space = <0>; > samsung,dynamic-range = <0>; > samsung,ycbcr-coeff = <0>; > samsung,color-depth = <1>; > samsung,link-rate = <0x0a>; > samsung,lane-count = <1>; Thierry already said, that these should probably be somehow auto-detected. Properties needing to stay around should probably also be "analogix,..." with a fallback to not break Samsung devicetrees, so look for "analogix,foo!, if not found try "samsung,foo" > ports { > edp_in: port { > #address-cells = <1>; > #size-cells = <0>; > edp_in_vopb: endpoint@0 { > reg = <0>; > remote-endpoint = <&vopb_out_edp>; > }; > }; > }; > >> + > >> + 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 ? As Thierry said, please don't add prefixes. > > >> + if (IS_ERR(dp->clk_24m)) { > >> + dev_err(dev, "cannot get clk_dp_24m\n"); > >> + return PTR_ERR(dp->clk_24m); > >> + } > > > > I think you're missing the pclk here (PCLK_EDP_CTRL) or is this part of > > something else? > > Whops, as I refered in commit message I leave pclk_dp to > analogix_dp_core driver ;-) > > The reason why I want to leave pclk is I thought this clock is more like > analogix dp > core driver want, like a IP controller clock (whatever analogix_dp do > need a clock > named with "dp"). Hmm, I'd think what the core (and Samsung) driver use as "dp" clock is probably the generic clock for the IP and not the pclk for the APB interface. So I think it still should be "dp" for the core and "dp_24m" + "pclk" for the rockchip part? > > >> + > >> + dp->rst = devm_reset_control_get(dev, "dp"); > >> + if (IS_ERR(dp->rst)) { > >> + dev_err(dev, "failed to get reset\n"); > >> + return PTR_ERR(dp->rst); > >> + } > >> + > >> + ret = rockchip_dp_clk_enable(dp); > >> + if (ret < 0) { > >> + dev_err(dp->dev, "cannot enable dp clk %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = rockchip_dp_pre_init(dp); > >> + if (ret < 0) { > >> + dev_err(dp->dev, "failed to pre init %d\n", ret); > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > > > > [...] > > > >> +static int rockchip_dp_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct device_node *panel_node; > >> + struct rockchip_dp_device *dp; > >> + struct drm_panel *panel; > >> + > >> + panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); > >> + if (!panel_node) { > >> + DRM_ERROR("failed to find rockchip,panel dt node\n"); > >> + return -ENODEV; > >> + } > > > > Personally I would prefer to continue with the of-graph framework to > > attach the panel instead of defining a special node. But I'm not > > authorative on this. But that way the dts could then look like [0]. > > > > I've sucessfully modified the driver currently in use in Chromeos for my > > dev-tree and have ported this change over onto your driver [1]. > > Wow! looks very nice, and really appricate for your ported code ;) > > BTW should I rebase on your patch, or can just take your code in my next > version :-) The code I currently carry, is from the ChromeOS tree, so of course nothing in mainline should be based on it. You could simply include the change into your driver. Heiko > > > Thanks a lot, > - Yakir > > > [0] > > https://github.com/mmind/linux-rockchip/blob/devel/somewhat-stable/arch/a > > rm/boot/dts/rk3288-veyron-edp.dtsi#L76 [1] > > ---------- 8< ------------- > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index e7cf9ab..24e872d > > 100644 > > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > @@ -20,6 +20,7 @@ > > > > #include <linux/component.h> > > #include <linux/clk.h> > > #include <linux/mfd/syscon.h> > > > > +#include <linux/of_graph.h> > > > > #include <linux/regmap.h> > > #include <linux/reset.h> > > > > @@ -335,14 +336,28 @@ static const struct component_ops > > rockchip_dp_component_ops = {> > > static int rockchip_dp_probe(struct platform_device *pdev) > > { > > > > struct device *dev = &pdev->dev; > > > > - struct device_node *panel_node; > > + struct device_node *panel_node, *port, *endpoint; > > > > struct rockchip_dp_device *dp; > > struct drm_panel *panel; > > > > - panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); > > + port = of_graph_get_port_by_id(dev->of_node, 1); > > + if (!port) { > > + dev_err(dev, "can't find output port\n"); > > + return -EINVAL; > > + } > > + > > + endpoint = of_get_child_by_name(port, "endpoint"); > > + of_node_put(port); > > + if (!endpoint) { > > + dev_err(dev, "no output endpoint found\n"); > > + return -EINVAL; > > + } > > + > > + panel_node = of_graph_get_remote_port_parent(endpoint); > > + of_node_put(endpoint); > > > > if (!panel_node) { > > > > - DRM_ERROR("failed to find rockchip,panel dt node\n"); > > - return -ENODEV; > > + dev_err(&pdev->dev, "no output node found\n"); > > + return -EINVAL; > > > > } > > > > panel = of_drm_find_panel(panel_node); > > > > ---------- 8< -------------
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
Hi Heiko, ? 2015/8/10 20:08, Heiko Stübner ??: > Hi Yakir, > > Am Samstag, 8. August 2015, 11:54:38 schrieb Yakir Yang: >>>> +static int rockchip_dp_init(struct rockchip_dp_device *dp) >>>> +{ >>>> + struct device *dev = dp->dev; >>>> + struct device_node *np = dev->of_node; >>>> + int ret; >>>> + >>>> + dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf"); >>>> + if (IS_ERR(dp->grf)) { >>>> + dev_err(dev, >>>> + "rk3288-dp needs rockchip,grf property\n"); >>>> + return PTR_ERR(dp->grf); >>>> + } >>>> + >>>> + dp->clk_dp = devm_clk_get(dev, "clk_dp"); >>> I've looked at the manual, but couldn't find an actual clock-name >>> used there. Is it really "clk_dp" or should it just be "dp"? >> This should be "clk_dp", not "dp". >> Cause analogix_dp_core would need a clock name with "dp", so I would >> rather to pasted my rockchip-dp node here before I add dt-bindings in >> next version ;) > The clock we name PCLK_EDP_CTRL in the clock controller is probably the clock > supplying the APB interface and named pclk already in the "Figure 3-2 > DP_TXclock domain" diagram on page 19 of the manual. So your "clk_dp" should > actually be "pclk". > > So you would have "dp", "dp_24m" and "pclk" for the 3 supplying clocks. Oh, yes, "pclk" is for APB interface, and "sclk_edp" for IP controller, and "sclk_edp_24m" for DP PHY, thanks for your explain. So for now, I would pass "sclk_edp" to "edp" in analogix_dp, and "sclk_edp_24m" to "dp-phy_24m" in phy-rockchip-dp.c, and "pclk_edp" to "pclk" in analogix_dp-rockchip.c. > >> edp: edp@ff970000 { >> compatible = "rockchip,rk3288-dp"; >> reg = <0xff970000 0x4000>; >> interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; >> >> clocks = <&cru SCLK_EDP>, <&cru SCLK_EDP_24M>, <&cru >> PCLK_EDP_CTRL>; >> clock-names = "clk_dp", "clk_dp_24m", "dp"; >> >> rockchip,grf = <&grf>; >> resets = <&cru 111>; >> reset-names = "dp"; >> power-domains = <&power RK3288_PD_VIO>; >> status = "disabled"; >> >> hsync-active-high = <0>; >> vsync-active-high = <0>; >> interlaced = <0>; >> samsung,color-space = <0>; >> samsung,dynamic-range = <0>; >> samsung,ycbcr-coeff = <0>; >> samsung,color-depth = <1>; >> samsung,link-rate = <0x0a>; >> samsung,lane-count = <1>; > Thierry already said, that these should probably be somehow auto-detected. > Properties needing to stay around should probably also be "analogix,..." with > a fallback to not break Samsung devicetrees, so > look for "analogix,foo!, if not found try "samsung,foo" Okay, it's better to rename to "analogxi...", done. > >> ports { >> edp_in: port { >> #address-cells = <1>; >> #size-cells = <0>; >> edp_in_vopb: endpoint@0 { >> reg = <0>; >> remote-endpoint = <&vopb_out_edp>; >> }; >> }; >> }; > > >>>> + >>>> + 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 ? > As Thierry said, please don't add prefixes. Okay, so is it okay to rename them to "dp", "dp-phy-24m", "pclk" ? > >>>> + if (IS_ERR(dp->clk_24m)) { >>>> + dev_err(dev, "cannot get clk_dp_24m\n"); >>>> + return PTR_ERR(dp->clk_24m); >>>> + } >>> I think you're missing the pclk here (PCLK_EDP_CTRL) or is this part of >>> something else? >> Whops, as I refered in commit message I leave pclk_dp to >> analogix_dp_core driver ;-) >> >> The reason why I want to leave pclk is I thought this clock is more like >> analogix dp >> core driver want, like a IP controller clock (whatever analogix_dp do >> need a clock >> named with "dp"). > Hmm, I'd think what the core (and Samsung) driver use as "dp" clock is > probably the generic clock for the IP and not the pclk for the APB interface. > > So I think it still should be "dp" for the core and "dp_24m" + "pclk" for the > rockchip part? Yes, I think you are right, thanks ;) > >>>> + >>>> + dp->rst = devm_reset_control_get(dev, "dp"); >>>> + if (IS_ERR(dp->rst)) { >>>> + dev_err(dev, "failed to get reset\n"); >>>> + return PTR_ERR(dp->rst); >>>> + } >>>> + >>>> + ret = rockchip_dp_clk_enable(dp); >>>> + if (ret < 0) { >>>> + dev_err(dp->dev, "cannot enable dp clk %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = rockchip_dp_pre_init(dp); >>>> + if (ret < 0) { >>>> + dev_err(dp->dev, "failed to pre init %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>> [...] >>> >>>> +static int rockchip_dp_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct device_node *panel_node; >>>> + struct rockchip_dp_device *dp; >>>> + struct drm_panel *panel; >>>> + >>>> + panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); >>>> + if (!panel_node) { >>>> + DRM_ERROR("failed to find rockchip,panel dt node\n"); >>>> + return -ENODEV; >>>> + } >>> Personally I would prefer to continue with the of-graph framework to >>> attach the panel instead of defining a special node. But I'm not >>> authorative on this. But that way the dts could then look like [0]. >>> >>> I've sucessfully modified the driver currently in use in Chromeos for my >>> dev-tree and have ported this change over onto your driver [1]. >> Wow! looks very nice, and really appricate for your ported code ;) >> >> BTW should I rebase on your patch, or can just take your code in my next >> version :-) > The code I currently carry, is from the ChromeOS tree, so of course nothing in > mainline should be based on it. You could simply include the change into your > driver. Okay, Thanks, - Yakir > > Heiko > >> >> Thanks a lot, >> - Yakir >> >>> [0] >>> https://github.com/mmind/linux-rockchip/blob/devel/somewhat-stable/arch/a >>> rm/boot/dts/rk3288-veyron-edp.dtsi#L76 [1] >>> ---------- 8< ------------- >>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index e7cf9ab..24e872d >>> 100644 >>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> @@ -20,6 +20,7 @@ >>> >>> #include <linux/component.h> >>> #include <linux/clk.h> >>> #include <linux/mfd/syscon.h> >>> >>> +#include <linux/of_graph.h> >>> >>> #include <linux/regmap.h> >>> #include <linux/reset.h> >>> >>> @@ -335,14 +336,28 @@ static const struct component_ops >>> rockchip_dp_component_ops = {> >>> static int rockchip_dp_probe(struct platform_device *pdev) >>> { >>> >>> struct device *dev = &pdev->dev; >>> >>> - struct device_node *panel_node; >>> + struct device_node *panel_node, *port, *endpoint; >>> >>> struct rockchip_dp_device *dp; >>> struct drm_panel *panel; >>> >>> - panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); >>> + port = of_graph_get_port_by_id(dev->of_node, 1); >>> + if (!port) { >>> + dev_err(dev, "can't find output port\n"); >>> + return -EINVAL; >>> + } >>> + >>> + endpoint = of_get_child_by_name(port, "endpoint"); >>> + of_node_put(port); >>> + if (!endpoint) { >>> + dev_err(dev, "no output endpoint found\n"); >>> + return -EINVAL; >>> + } >>> + >>> + panel_node = of_graph_get_remote_port_parent(endpoint); >>> + of_node_put(endpoint); >>> >>> if (!panel_node) { >>> >>> - DRM_ERROR("failed to find rockchip,panel dt node\n"); >>> - return -ENODEV; >>> + dev_err(&pdev->dev, "no output node found\n"); >>> + return -EINVAL; >>> >>> } >>> >>> panel = of_drm_find_panel(panel_node); >>> >>> ---------- 8< ------------- > > >
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. > >>>>+ 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. Thierry
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
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index e7cf9ab..24e872d 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -20,6 +20,7 @@ #include <linux/component.h> #include <linux/clk.h> #include <linux/mfd/syscon.h> +#include <linux/of_graph.h> #include <linux/regmap.h> #include <linux/reset.h> @@ -335,14 +336,28 @@ static const struct component_ops rockchip_dp_component_ops = { static int rockchip_dp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *panel_node; + struct device_node *panel_node, *port, *endpoint; struct rockchip_dp_device *dp; struct drm_panel *panel; - panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); + port = of_graph_get_port_by_id(dev->of_node, 1); + if (!port) { + dev_err(dev, "can't find output port\n"); + return -EINVAL; + } + + endpoint = of_get_child_by_name(port, "endpoint"); + of_node_put(port); + if (!endpoint) { + dev_err(dev, "no output endpoint found\n"); + return -EINVAL; + } + + panel_node = of_graph_get_remote_port_parent(endpoint); + of_node_put(endpoint); if (!panel_node) { - DRM_ERROR("failed to find rockchip,panel dt node\n"); - return -ENODEV; + dev_err(&pdev->dev, "no output node found\n"); + return -EINVAL; } panel = of_drm_find_panel(panel_node);