From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752053AbdBFRqB (ORCPT ); Mon, 6 Feb 2017 12:46:01 -0500 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:37427 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751001AbdBFRp7 (ORCPT ); Mon, 6 Feb 2017 12:45:59 -0500 Message-ID: <1486403134.3005.67.camel@pengutronix.de> Subject: Re: [PATCH 2/5] drm: of: introduce drm_of_find_panel_or_bridge From: Philipp Zabel To: Rob Herring Cc: Liviu Dudau , 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 , 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 Date: Mon, 06 Feb 2017 18:45:34 +0100 In-Reply-To: <20170206165323.255ltzlzmpfx4vl7@rob-hp-laptop> References: <20170204033635.10250-1-robh@kernel.org> <20170204033635.10250-3-robh@kernel.org> <1486377768.3005.34.camel@pengutronix.de> <20170206165323.255ltzlzmpfx4vl7@rob-hp-laptop> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2017-02-06 at 10:53 -0600, Rob Herring wrote: > On Mon, Feb 06, 2017 at 11:42:48AM +0100, Philipp Zabel wrote: > > On Fri, 2017-02-03 at 21:36 -0600, Rob Herring wrote: > > > Many drivers have a common pattern of searching the OF graph for either an > > > attached panel or bridge and then finding the DRM struct for the panel > > > or bridge. Also, most drivers need to handle deferred probing when the > > > DRM device is not yet instantiated. Create a common function, > > > drm_of_find_panel_or_bridge, to find the connected node and the > > > associated DRM panel or bridge device. > > [...] > > > > +int drm_of_find_panel_or_bridge(const struct device_node *np, > > > + int port, int endpoint, > > > + struct drm_panel **panel, > > > + struct drm_bridge **bridge) > > > +{ > > > + int ret = -ENODEV; > > > > This is only returned if !panel && !bridge. I'd consider this invalid > > usage of this function, so maybe use -EINVAL? > > Yes. > > > > + struct device_node *remote; > > > + > > > + remote = of_graph_get_remote_node(np, port, endpoint); > > > + if (!remote) > > > + return -ENODEV; > > > + > > > + if (bridge) > > > + *bridge = NULL; > > > > I would move this ^ ... > > > > > + if (panel) { > > > + *panel = of_drm_find_panel(remote); > > > + if (*panel) { > > > > ... here. > > Okay. > > > > + ret = 0; > > > + goto out_put; > > > + } > > > + ret = -EPROBE_DEFER; > > > + } > > > + > > > + if (bridge) { > > > + *bridge = of_drm_find_bridge(remote); > > > + if (*bridge) > > > + ret = 0; > > > + else > > > + ret = -EPROBE_DEFER; > > > + } > > > +out_put: > > > + of_node_put(remote); > > > + return ret; > > > +} > > I've ended up re-writing things a bit getting rid of the goto and the > result looks like this: Looks good to me. > int drm_of_find_panel_or_bridge(const struct device_node *np, > int port, int endpoint, > struct drm_panel **panel, > struct drm_bridge **bridge) > { > int ret = -EPROBE_DEFER; > struct device_node *remote; > > if (!panel && !bridge) > return -EINVAL; > > remote = of_graph_get_remote_node(np, port, endpoint); > if (!remote) > return -ENODEV; > > if (panel) { > *panel = of_drm_find_panel(remote); > if (*panel) { > if (bridge) > *bridge = NULL; With the goto out_put gone, I'm conflicted whether I find this clearer here, or ... > ret = 0; > } > } > > /* No panel found yet, check for a bridge next. */ > if (ret && bridge) { > *bridge = of_drm_find_bridge(remote); > if (*bridge) > ret = 0; > } ... even down here: if (bridge) { if (ret) { /* No panel found yet, check for a bridge next. */ *bridge = of_drm_find_bridge(remote) if (*bridge) ret = 0; } else { *bridge = NULL; } } That way bridge doesn't have to be checked twice and all the modification of *bridge is in the same block. > > of_node_put(remote); > return ret; > } Either way, Acked-by: Philipp Zabel regards Philipp