Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752605AbdCOKZ6 (ORCPT ); Wed, 15 Mar 2017 06:25:58 -0400 Received: from regular1.263xmail.com ([211.150.99.133]:45291 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765AbdCOKZ4 (ORCPT ); Wed, 15 Mar 2017 06:25:56 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: a.hajda@samsung.com X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <39d328721596fef5d71ad1fe67a0d0e1> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <58C9167F.5030905@rock-chips.com> Date: Wed, 15 Mar 2017 18:25:03 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Andrzej Hajda , Mark Yao CC: Douglas Anderson , Guenter Roeck , Brian Norris , Sean Paul , Tomasz Figa , Heiko Stuebner , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, David Airlie , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2] drm/rockchip: Refactor the component match logic. References: <1489488349-3053-1-git-send-email-jeffy.chen@rock-chips.com> <9352c2e4-5394-4725-f247-ff98318cd3d4@samsung.com> In-Reply-To: <9352c2e4-5394-4725-f247-ff98318cd3d4@samsung.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9228 Lines: 324 Hi Andrzej, On 03/14/2017 08:05 PM, Andrzej Hajda wrote: > Hi Jeffy, > > On 14.03.2017 11:45, Jeffy Chen wrote: >> Currently we are adding all components from the dts, if one of their >> drivers been disabled, we would not be able to bring up others. >> >> Refactor component match logic, follow exynos drm. >> >> Signed-off-by: Jeffy Chen > > Reviewed-by: Andrzej Hajda > > Below few nitpicks. > > (...) > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> index b360e62..b6bccfc 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> @@ -356,34 +356,52 @@ static const struct dev_pm_ops rockchip_drm_pm_ops = { >> rockchip_drm_sys_resume) >> }; >> >> -static int compare_of(struct device *dev, void *data) >> -{ >> - struct device_node *np = data; >> +static struct platform_driver *rockchip_drm_comp_drvs[] = { >> + &vop_platform_driver, >> +#ifdef CONFIG_ROCKCHIP_ANALOGIX_DP >> + &rockchip_dp_driver, >> +#endif >> +#ifdef CONFIG_ROCKCHIP_CDN_DP >> + &cdn_dp_driver, >> +#endif >> +#ifdef CONFIG_ROCKCHIP_DW_HDMI >> + &dw_hdmi_rockchip_pltfm_driver, >> +#endif >> +#ifdef CONFIG_ROCKCHIP_DW_MIPI_DSI >> + &dw_mipi_dsi_driver, >> +#endif >> +#ifdef CONFIG_ROCKCHIP_INNO_HDMI >> + &inno_hdmi_driver, >> +#endif >> +}; > > To avoid #ifdefs, you can use DRV_PTR [1], but it depends what do you > hate more? ifdefs or macro magic :) > > [1]: > http://lxr.free-electrons.com/source/drivers/gpu/drm/exynos/exynos_drm_drv.c#L472 > Done. >> >> - return dev->of_node == np; >> +static int compare_dev(struct device *dev, void *data) >> +{ >> + return dev == (struct device *)data; >> } >> >> -static void rockchip_add_endpoints(struct device *dev, >> - struct component_match **match, >> - struct device_node *port) >> +static struct component_match *rockchip_drm_match_add(struct device *dev) >> { >> - struct device_node *ep, *remote; >> + struct component_match *match = NULL; >> + int i; >> >> - for_each_child_of_node(port, ep) { >> - remote = of_graph_get_remote_port_parent(ep); >> - if (!remote || !of_device_is_available(remote)) { >> - of_node_put(remote); >> - continue; >> - } else if (!of_device_is_available(remote->parent)) { >> - dev_warn(dev, "parent device of %s is not available\n", >> - remote->full_name); >> - of_node_put(remote); >> - continue; >> - } >> + for (i = 0; i < ARRAY_SIZE(rockchip_drm_comp_drvs); i++) { >> + struct platform_driver *drv = rockchip_drm_comp_drvs[i]; >> + struct device *p = NULL, *d; >> >> - drm_of_component_match_add(dev, match, compare_of, remote); >> - of_node_put(remote); >> + do { >> + d = bus_find_device(&platform_bus_type, p, &drv->driver, >> + (void *)platform_bus_type.match); >> + put_device(p); >> + p = d; >> + >> + if (!d) >> + break; >> + component_match_add(dev, &match, compare_dev, d); >> + } while (true); >> } >> + >> + return match ?: ERR_PTR(-ENODEV); >> } >> >> static const struct component_master_ops rockchip_drm_ops = { >> @@ -391,21 +409,16 @@ static const struct component_master_ops rockchip_drm_ops = { >> .unbind = rockchip_drm_unbind, >> }; >> >> -static int rockchip_drm_platform_probe(struct platform_device *pdev) >> +static int rockchip_drm_platform_of_probe(struct device *dev) >> { >> - struct device *dev = &pdev->dev; >> - struct component_match *match = NULL; >> struct device_node *np = dev->of_node; >> struct device_node *port; >> + bool found = false; >> int i; >> >> if (!np) >> return -ENODEV; >> - /* >> - * Bind the crtc ports first, so that >> - * drm_of_find_possible_crtcs called from encoder .bind callbacks >> - * works as expected. >> - */ >> + >> for (i = 0;; i++) { >> struct device_node *iommu; >> >> @@ -429,9 +442,9 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev) >> is_support_iommu = false; >> } >> >> + found = true; >> + >> of_node_put(iommu); >> - drm_of_component_match_add(dev, &match, compare_of, >> - port->parent); >> of_node_put(port); >> } >> >> @@ -440,27 +453,27 @@ static int rockchip_drm_platform_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - if (!match) { >> + if (!found) { >> dev_err(dev, "No available vop found for display-subsystem.\n"); >> return -ENODEV; >> } >> - /* >> - * For each bound crtc, bind the encoders attached to its >> - * remote endpoint. >> - */ >> - for (i = 0;; i++) { >> - port = of_parse_phandle(np, "ports", i); >> - if (!port) >> - break; >> >> - if (!of_device_is_available(port->parent)) { >> - of_node_put(port); >> - continue; >> - } >> + return 0; >> +} >> >> - rockchip_add_endpoints(dev, &match, port); >> - of_node_put(port); >> - } >> +static int rockchip_drm_platform_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct component_match *match = NULL; >> + int ret; >> + >> + ret = rockchip_drm_platform_of_probe(dev); >> + if (ret) >> + return ret; >> + >> + match = rockchip_drm_match_add(dev); >> + if (IS_ERR(match)) >> + return PTR_ERR(match); >> >> return component_master_add_with_match(dev, &rockchip_drm_ops, match); >> } >> @@ -488,7 +501,60 @@ static struct platform_driver rockchip_drm_platform_driver = { >> }, >> }; >> >> -module_platform_driver(rockchip_drm_platform_driver); >> +static void rockchip_drm_unregister_drivers(void) >> +{ >> + int i; >> + >> + for (i = ARRAY_SIZE(rockchip_drm_comp_drvs) - 1; i >= 0; i--) >> + platform_driver_unregister(rockchip_drm_comp_drvs[i]); >> +} >> + >> +static int rockchip_drm_register_drivers(void) >> +{ >> + int i, ret; >> + >> + for (i = 0; i < ARRAY_SIZE(rockchip_drm_comp_drvs); i++) { >> + ret = platform_driver_register(rockchip_drm_comp_drvs[i]); >> + if (ret) >> + goto err_unreg; >> + } >> + >> + return 0; >> + >> +err_unreg: >> + for (; i--; ) >> + platform_driver_unregister(rockchip_drm_comp_drvs[i]); > > Strange placement of decremental in for, maybe sth like this would be > better: > while (i-- > 0) > platform_driver_unregister(rockchip_drm_comp_drvs[i]); > Yes, this looks better. > >> + return ret; >> +} >> + >> +static int __init rockchip_drm_init(void) >> +{ >> + int ret; >> + >> + ret = rockchip_drm_register_drivers(); >> + if (ret) >> + return ret; >> + >> + ret = platform_driver_register(&rockchip_drm_platform_driver); >> + if (ret) >> + goto err_unreg_drivers; >> + >> + return 0; >> + >> +err_unreg_drivers: >> + rockchip_drm_unregister_drivers(); >> + return ret; >> +} >> + >> +static void __exit rockchip_drm_fini(void) >> +{ >> + platform_driver_unregister(&rockchip_drm_platform_driver); >> + >> + rockchip_drm_unregister_drivers(); >> +} >> + >> +module_init(rockchip_drm_init); >> +module_exit(rockchip_drm_fini); >> >> MODULE_AUTHOR("Mark Yao "); >> MODULE_DESCRIPTION("ROCKCHIP DRM Driver"); >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> index adc3930..91812bd 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> @@ -79,4 +79,20 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, >> int rockchip_drm_wait_line_flag(struct drm_crtc *crtc, unsigned int line_num, >> unsigned int mstimeout); >> >> +#ifdef CONFIG_ROCKCHIP_CDN_DP >> +extern struct platform_driver cdn_dp_driver; >> +#endif >> +#ifdef CONFIG_ROCKCHIP_DW_HDMI >> +extern struct platform_driver dw_hdmi_rockchip_pltfm_driver; >> +#endif >> +#ifdef CONFIG_ROCKCHIP_DW_MIPI_DSI >> +extern struct platform_driver dw_mipi_dsi_driver; >> +#endif >> +#ifdef CONFIG_ROCKCHIP_INNO_HDMI >> +extern struct platform_driver inno_hdmi_driver; >> +#endif >> +#ifdef CONFIG_ROCKCHIP_ANALOGIX_DP >> +extern struct platform_driver rockchip_dp_driver; >> +#endif >> +extern struct platform_driver vop_platform_driver; >> #endif /* _ROCKCHIP_DRM_DRV_H_ */ > > I think gcc should be fine if you remove ifdefs. Unused extern > declarations are ignored, unless you use some paranoic static checker, > aren't they? But this is also matter of taste. Ok. > > > Regards > Andrzej > > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c >> index 91fbc7b..0da4444 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c >> @@ -404,7 +404,7 @@ static int vop_remove(struct platform_device *pdev) >> return 0; >> } >> >> -static struct platform_driver vop_platform_driver = { >> +struct platform_driver vop_platform_driver = { >> .probe = vop_probe, >> .remove = vop_remove, >> .driver = { >> @@ -412,9 +412,3 @@ static struct platform_driver vop_platform_driver = { >> .of_match_table = of_match_ptr(vop_driver_dt_match), >> }, >> }; >> - >> -module_platform_driver(vop_platform_driver); >> - >> -MODULE_AUTHOR("Mark Yao "); >> -MODULE_DESCRIPTION("ROCKCHIP VOP Driver"); >> -MODULE_LICENSE("GPL v2"); >> > > > > Thanx for your comments~