From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752011AbdBFRJy (ORCPT ); Mon, 6 Feb 2017 12:09:54 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:33309 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751395AbdBFRJw (ORCPT ); Mon, 6 Feb 2017 12:09:52 -0500 Date: Mon, 6 Feb 2017 11:09:49 -0600 From: Rob Herring To: Liviu Dudau Cc: David Airlie , Daniel Vetter , Sean Paul , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Frank Rowand , Boris Brezillon , Archit Taneja , Jingoo Han , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , Javier Martinez Canillas , Stefan Agner , Alison Wang , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , Philipp Zabel , CK Hu , Matthias Brugger , Marek Vasut , Mark Yao , Heiko Stuebner , Maxime Ripard , Chen-Yu Tsai , Mali DP Maintainers , Neil Armstrong , Carlo Caione , Kevin Hilman , Rob Clark , Jyri Sarha , Tomi Valkeinen , Eric Anholt , Russell King Subject: Re: [PATCH 3/5] drm: convert drivers to use of_graph_get_remote_node Message-ID: <20170206170949.tmm4m2lm2rybm5dh@rob-hp-laptop> References: <20170204033635.10250-1-robh@kernel.org> <20170204033635.10250-4-robh@kernel.org> <20170206102933.GV3140@e110455-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170206102933.GV3140@e110455-lin.cambridge.arm.com> User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 06, 2017 at 10:29:33AM +0000, Liviu Dudau wrote: > On Fri, Feb 03, 2017 at 09:36:33PM -0600, Rob Herring wrote: > > Convert drivers to use the new of_graph_get_remote_node() helper > > instead of parsing the endpoint node and then getting the remote device > > node. Now drivers can just specify the device node and which > > port/endpoint and get back the connected remote device node. The details > > of the graph binding are nicely abstracted into the core OF graph code. > > > > This changes some error messages to debug messages (in the graph core). > > Graph connections are often "no connects" depending on the particular > > board, so we want to avoid spurious messages. Plus the kernel is not a > > DT validator. > > > > Signed-off-by: Rob Herring > > --- > > drivers/gpu/drm/arm/hdlcd_drv.c | 22 ++------- > > drivers/gpu/drm/arm/malidp_drv.c | 29 ++--------- > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 12 +---- > > drivers/gpu/drm/bridge/dumb-vga-dac.c | 15 ++---- > > drivers/gpu/drm/bridge/ti-tfp410.c | 15 ++---- > > drivers/gpu/drm/exynos/exynos_drm_dpi.c | 16 +----- > > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++--- > > drivers/gpu/drm/exynos/exynos_drm_mic.c | 27 +--------- > > drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 26 ++-------- > > drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 30 +---------- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 12 ++--- > > drivers/gpu/drm/mediatek/mtk_hdmi.c | 26 ++-------- > > drivers/gpu/drm/meson/meson_drv.c | 12 ++--- > > drivers/gpu/drm/meson/meson_venc_cvbs.c | 19 ++----- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +- > > drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 28 +---------- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 18 +++---- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 11 +---- > > drivers/gpu/drm/tilcdc/tilcdc_external.c | 66 +++---------------------- > > drivers/gpu/drm/vc4/vc4_dpi.c | 15 ++---- > > 20 files changed, 64 insertions(+), 351 deletions(-) > > > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c > > index e5f4f4a6546d..0f70f5fe9970 100644 > > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > > @@ -430,29 +430,13 @@ static int compare_dev(struct device *dev, void *data) > > > > static int hdlcd_probe(struct platform_device *pdev) > > { > > - struct device_node *port, *ep; > > + struct device_node *port; > > struct component_match *match = NULL; > > > > - if (!pdev->dev.of_node) > > - return -ENODEV; > > - > > /* there is only one output port inside each device, find it */ > > - ep = of_graph_get_next_endpoint(pdev->dev.of_node, NULL); > > - if (!ep) > > - return -ENODEV; > > - > > - if (!of_device_is_available(ep)) { > > - of_node_put(ep); > > + port = of_graph_get_remote_node(pdev->dev.of_node, 0, 0); > > + if (!port) > > return -ENODEV; > > - } > > - > > - /* add the remote encoder port as component */ > > - port = of_graph_get_remote_port_parent(ep); > > - of_node_put(ep); > > - if (!port || !of_device_is_available(port)) { > > - of_node_put(port); > > - return -EAGAIN; > > The HDLCD change looks reasonable except for this -EAGAIN business. I'll have to > test your changes on my setup to see how this affects having the encoder as a module. What are you expecting to happen with -EAGAIN? This one was a bit of an oddball. This condition would only change if you had an overlay. That's a use case that needs to be handled in a common way ('cause I don't want to clean-up every driver doing overlays in their own way latter). Just having "status" changing at runtime would have all sorts of implications in the kernel. > > > - } > > > > drm_of_component_match_add(&pdev->dev, &match, compare_dev, port); > > of_node_put(port); > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > > index 32f746e31379..bfa04be7f5de 100644 > > --- a/drivers/gpu/drm/arm/malidp_drv.c > > +++ b/drivers/gpu/drm/arm/malidp_drv.c > > @@ -262,7 +262,6 @@ static int malidp_bind(struct device *dev) > > { > > struct resource *res; > > struct drm_device *drm; > > - struct device_node *ep; > > struct malidp_drm *malidp; > > struct malidp_hw_device *hwdev; > > struct platform_device *pdev = to_platform_device(dev); > > @@ -360,12 +359,7 @@ static int malidp_bind(struct device *dev) > > goto init_fail; > > > > /* Set the CRTC's port so that the encoder component can find it */ > > - ep = of_graph_get_next_endpoint(dev->of_node, NULL); > > - if (!ep) { > > - ret = -EINVAL; > > - goto port_fail; > > - } > > - malidp->crtc.port = of_get_next_parent(ep); > > + malidp->crtc.port = of_graph_get_port_by_id(dev->of_node, 0); > > > > ret = component_bind_all(dev, drm); > > if (ret) { > > @@ -418,9 +412,7 @@ static int malidp_bind(struct device *dev) > > irq_init_fail: > > component_unbind_all(dev, drm); > > bind_fail: > > - of_node_put(malidp->crtc.port); > > Why removing this line? AFAICT this is still needed, according to of_graph_get_port_by_id() > documentation. Yes, you are right. Rob From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Herring Subject: Re: [PATCH 3/5] drm: convert drivers to use of_graph_get_remote_node Date: Mon, 6 Feb 2017 11:09:49 -0600 Message-ID: <20170206170949.tmm4m2lm2rybm5dh@rob-hp-laptop> References: <20170204033635.10250-1-robh@kernel.org> <20170204033635.10250-4-robh@kernel.org> <20170206102933.GV3140@e110455-lin.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170206102933.GV3140-A/Nd4k6kWRHZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Liviu Dudau Cc: David Airlie , Daniel Vetter , Sean Paul , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Frank Rowand , Boris Brezillon , Archit Taneja , Jingoo Han , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , Javier Martinez Canillas , Stefan Agner , Alison Wang , Xinliang Liu List-Id: devicetree@vger.kernel.org On Mon, Feb 06, 2017 at 10:29:33AM +0000, Liviu Dudau wrote: > On Fri, Feb 03, 2017 at 09:36:33PM -0600, Rob Herring wrote: > > Convert drivers to use the new of_graph_get_remote_node() helper > > instead of parsing the endpoint node and then getting the remote device > > node. Now drivers can just specify the device node and which > > port/endpoint and get back the connected remote device node. The details > > of the graph binding are nicely abstracted into the core OF graph code. > > > > This changes some error messages to debug messages (in the graph core). > > Graph connections are often "no connects" depending on the particular > > board, so we want to avoid spurious messages. Plus the kernel is not a > > DT validator. > > > > Signed-off-by: Rob Herring > > --- > > drivers/gpu/drm/arm/hdlcd_drv.c | 22 ++------- > > drivers/gpu/drm/arm/malidp_drv.c | 29 ++--------- > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 12 +---- > > drivers/gpu/drm/bridge/dumb-vga-dac.c | 15 ++---- > > drivers/gpu/drm/bridge/ti-tfp410.c | 15 ++---- > > drivers/gpu/drm/exynos/exynos_drm_dpi.c | 16 +----- > > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++--- > > drivers/gpu/drm/exynos/exynos_drm_mic.c | 27 +--------- > > drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 26 ++-------- > > drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 30 +---------- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 12 ++--- > > drivers/gpu/drm/mediatek/mtk_hdmi.c | 26 ++-------- > > drivers/gpu/drm/meson/meson_drv.c | 12 ++--- > > drivers/gpu/drm/meson/meson_venc_cvbs.c | 19 ++----- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +- > > drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 28 +---------- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 18 +++---- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 11 +---- > > drivers/gpu/drm/tilcdc/tilcdc_external.c | 66 +++---------------------- > > drivers/gpu/drm/vc4/vc4_dpi.c | 15 ++---- > > 20 files changed, 64 insertions(+), 351 deletions(-) > > > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c > > index e5f4f4a6546d..0f70f5fe9970 100644 > > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > > @@ -430,29 +430,13 @@ static int compare_dev(struct device *dev, void *data) > > > > static int hdlcd_probe(struct platform_device *pdev) > > { > > - struct device_node *port, *ep; > > + struct device_node *port; > > struct component_match *match = NULL; > > > > - if (!pdev->dev.of_node) > > - return -ENODEV; > > - > > /* there is only one output port inside each device, find it */ > > - ep = of_graph_get_next_endpoint(pdev->dev.of_node, NULL); > > - if (!ep) > > - return -ENODEV; > > - > > - if (!of_device_is_available(ep)) { > > - of_node_put(ep); > > + port = of_graph_get_remote_node(pdev->dev.of_node, 0, 0); > > + if (!port) > > return -ENODEV; > > - } > > - > > - /* add the remote encoder port as component */ > > - port = of_graph_get_remote_port_parent(ep); > > - of_node_put(ep); > > - if (!port || !of_device_is_available(port)) { > > - of_node_put(port); > > - return -EAGAIN; > > The HDLCD change looks reasonable except for this -EAGAIN business. I'll have to > test your changes on my setup to see how this affects having the encoder as a module. What are you expecting to happen with -EAGAIN? This one was a bit of an oddball. This condition would only change if you had an overlay. That's a use case that needs to be handled in a common way ('cause I don't want to clean-up every driver doing overlays in their own way latter). Just having "status" changing at runtime would have all sorts of implications in the kernel. > > > - } > > > > drm_of_component_match_add(&pdev->dev, &match, compare_dev, port); > > of_node_put(port); > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c > > index 32f746e31379..bfa04be7f5de 100644 > > --- a/drivers/gpu/drm/arm/malidp_drv.c > > +++ b/drivers/gpu/drm/arm/malidp_drv.c > > @@ -262,7 +262,6 @@ static int malidp_bind(struct device *dev) > > { > > struct resource *res; > > struct drm_device *drm; > > - struct device_node *ep; > > struct malidp_drm *malidp; > > struct malidp_hw_device *hwdev; > > struct platform_device *pdev = to_platform_device(dev); > > @@ -360,12 +359,7 @@ static int malidp_bind(struct device *dev) > > goto init_fail; > > > > /* Set the CRTC's port so that the encoder component can find it */ > > - ep = of_graph_get_next_endpoint(dev->of_node, NULL); > > - if (!ep) { > > - ret = -EINVAL; > > - goto port_fail; > > - } > > - malidp->crtc.port = of_get_next_parent(ep); > > + malidp->crtc.port = of_graph_get_port_by_id(dev->of_node, 0); > > > > ret = component_bind_all(dev, drm); > > if (ret) { > > @@ -418,9 +412,7 @@ static int malidp_bind(struct device *dev) > > irq_init_fail: > > component_unbind_all(dev, drm); > > bind_fail: > > - of_node_put(malidp->crtc.port); > > Why removing this line? AFAICT this is still needed, according to of_graph_get_port_by_id() > documentation. Yes, you are right. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html