From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4B563C43457 for ; Sun, 11 Oct 2020 23:03:28 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E5FE6205CA for ; Sun, 11 Oct 2020 23:03:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="vBzy/rrD"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Jk4XuflM" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E5FE6205CA Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=fEyYdS5yFH7XLW0SN/u9Oe6Fb3uVIMk56pvlvpl5a9E=; b=vBzy/rrDJfVk/mZA/Do8BGNTC 2kxWtM+UbxeF470r5457UeKpw0eLsR3uwZjrwpAS1MapBFrdJHjHNLnMPMjyB1MWRPOp1ksZAUPal DNedQapD5EZXOCDPGxKWdBhqmKy9AZghs2/ooRg9NWeKSYWS4YmYKzI+CLOg7G0uqs1ynUx3bCrjO BBzFC9kM1ytpGSgUpOS8XAbbEfav6y7YmnW4z4+5P/stMjaqAkIPwnowJyo6nu87YvfYcGPKm0Q5v 6klUCnk15hjNp5H3cEWBnRXcXrR/q/y3x9RcgWZ+u0zFz45HbyCiABfrKzp9G9MzJ89CZIuEuXCUF pJsuw9skg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kRkLC-0007uz-Ij; Sun, 11 Oct 2020 23:01:22 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kRkLA-0007sB-08 for linux-arm-kernel@lists.infradead.org; Sun, 11 Oct 2020 23:01:21 +0000 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A4A9308; Mon, 12 Oct 2020 01:01:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1602457275; bh=uhwaH5dk8caXUXCfjGq8FIzwI8jsBl7UFxbmORYemP4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Jk4XuflM7wT92ttPSgspLWilsQwimZAMmefxagq796kzRwpgOT7Z8E1qWE649FYev gn3eMWmV7BcwfPKp54KMQ8KtYSp7sjhLQ98wiMefKoZR8U6cUBN3b9g1GhpCc1LozR 5UAPd2L7DuODEYj5zNh5GoCjOQXu0tFs4sfqOaLY= Date: Mon, 12 Oct 2020 02:00:30 +0300 From: Laurent Pinchart To: Maxime Ripard Subject: Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Message-ID: <20201011230030.GD3944@pendragon.ideasonboard.com> References: <6169dd15782627c8415583881fa94ba39c4f5221.1601910923.git-series.maxime@cerno.tech> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6169dd15782627c8415583881fa94ba39c4f5221.1601910923.git-series.maxime@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201011_190120_314676_D3D0F3EF X-CRM114-Status: GOOD ( 33.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree@vger.kernel.org, David Airlie , Maarten Lankhorst , dri-devel@lists.freedesktop.org, Chen-Yu Tsai , Rob Herring , Thomas Zimmermann , Daniel Vetter , Frank Rowand , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Maxime, Thank you for the patch. On Mon, Oct 05, 2020 at 05:15:39PM +0200, Maxime Ripard wrote: > The drm_of_lvds_get_dual_link_pixel_order() function took so far the > device_node of the two ports used together to make up a dual-link LVDS > output. > > This assumes that a binding would use an entire port for the LVDS output. > However, some bindings have used endpoints instead and thus we need to > operate at the endpoint level. Change slightly the arguments to allow that. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/drm_of.c | 98 +++++++++++++++--------------- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 8 +-- > include/drm/drm_of.h | 16 +++-- > 3 files changed, 63 insertions(+), 59 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index b50b44e76279..2dcb49b0401b 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -291,50 +291,34 @@ static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node) > (odd_pixels ? DRM_OF_LVDS_ODD : 0); > } > > -static int drm_of_lvds_get_remote_pixels_type( > - const struct device_node *port_node) > +static int drm_of_lvds_get_remote_pixels_type(const struct device_node *endpoint) > { > - struct device_node *endpoint = NULL; > - int pixels_type = -EPIPE; > + struct device_node *remote_port; > + int pixels_type; > > - for_each_child_of_node(port_node, endpoint) { > - struct device_node *remote_port; > - int current_pt; > - > - if (!of_node_name_eq(endpoint, "endpoint")) > - continue; > - > - remote_port = of_graph_get_remote_port(endpoint); > - if (!remote_port) { > - of_node_put(remote_port); > - return -EPIPE; > - } > - > - current_pt = drm_of_lvds_get_port_pixels_type(remote_port); > + remote_port = of_graph_get_remote_port(endpoint); > + if (!remote_port) { > of_node_put(remote_port); You can drop this line. > - if (pixels_type < 0) > - pixels_type = current_pt; > - > - /* > - * Sanity check, ensure that all remote endpoints have the same > - * pixel type. We may lift this restriction later if we need to > - * support multiple sinks with different dual-link > - * configurations by passing the endpoints explicitly to > - * drm_of_lvds_get_dual_link_pixel_order(). > - */ Shouldn't we keep this check when endpoint_id is -1 in drm_of_lvds_get_dual_link_pixel_order() ? > - if (!current_pt || pixels_type != current_pt) { > - of_node_put(remote_port); > - return -EINVAL; > - } > + return -EPIPE; > } > > + pixels_type = drm_of_lvds_get_port_pixels_type(remote_port); > + of_node_put(remote_port); > + > + if (pixels_type < 0) > + pixels_type = -EPIPE; > + > return pixels_type; > } > > /** > * drm_of_lvds_get_dual_link_pixel_order - Get LVDS dual-link pixel order > - * @port1: First DT port node of the Dual-link LVDS source > - * @port2: Second DT port node of the Dual-link LVDS source > + * @dev1: First DT device node of the Dual-Link LVDS source > + * @port1_id: ID of the first DT port node of the Dual-Link LVDS source > + * @endpoint1_id: ID of the first DT port node of the Dual-Link LVDS source The port1_id and endpoint1_id parameters have the exact same documentation. Same for port2. I would shorten port1_id to port1 and endpoint1_id to endpoint1, but that's up to you. > + * @dev2: First DT device node of the Dual-Link LVDS source > + * @port2_id: ID of the first DT port node of the Dual-Link LVDS source > + * @endpoint2_id: ID of the first DT port node of the Dual-Link LVDS source > * > * An LVDS dual-link connection is made of two links, with even pixels > * transitting on one link, and odd pixels on the other link. This function > @@ -348,32 +332,48 @@ static int drm_of_lvds_get_remote_pixels_type( > * > * If either port is not connected, this function returns -EPIPE. > * > - * @port1 and @port2 are typically DT sibling nodes, but may have different > - * parents when, for instance, two separate LVDS encoders carry the even and odd > - * pixels. > + * @port1_id and @port2_id are typically DT sibling nodes, but may have > + * different parents when, for instance, two separate LVDS encoders carry the > + * even and odd pixels. > + * > + * If @port1_id, @port2_id, @endpoint1_id or @endpoint2_id are set to -1, their > + * value is going to be ignored. And what happens when they're ignored ? :-) You should document that the first endpoint / port is then used. > * > * Return: > - * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2 > - * carries odd pixels > - * * DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS - @port1 carries odd pixels and @port2 > - * carries even pixels > - * * -EINVAL - @port1 and @port2 are not connected to a dual-link LVDS sink, or > - * the sink configuration is invalid > - * * -EPIPE - when @port1 or @port2 are not connected > + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @endpoint1_id carries even pixels and > + * @endpoint2_id carries odd pixels > + * * DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS - @endpoint1_id carries odd pixels and > + * @endpoint2_id carries even pixels > + * * -EINVAL - @endpoint1_id and @endpoint2_id are not connected to a dual-link > + * LVDS sink, or the sink configuration is invalid > + * * -EPIPE - when @endpoint1_id or @endpoint2_id are not connected > */ > -int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1, > - const struct device_node *port2) > +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1, > + int port1_id, > + int endpoint1_id, > + const struct device_node *dev2, > + int port2_id, > + int endpoint2_id) > { > + struct device_node *endpoint1, *endpoint2; > int remote_p1_pt, remote_p2_pt; > > - if (!port1 || !port2) > + if (!dev1 || !dev2) > + return -EINVAL; > + > + endpoint1 = of_graph_get_endpoint_by_regs(dev1, port1_id, endpoint1_id); > + if (!endpoint1) > + return -EINVAL; > + > + endpoint2 = of_graph_get_endpoint_by_regs(dev2, port2_id, endpoint2_id); > + if (!endpoint2) > return -EINVAL; YOu're leaking a reference to endpoint1 here, and to both endpoint1 and endpoint2 in all the error paths (and the success path actually) below. > > - remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1); > + remote_p1_pt = drm_of_lvds_get_remote_pixels_type(endpoint1); > if (remote_p1_pt < 0) > return remote_p1_pt; > > - remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2); > + remote_p2_pt = drm_of_lvds_get_remote_pixels_type(endpoint2); > if (remote_p2_pt < 0) > return remote_p2_pt; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index ab0d49618cf9..02d8c4ce820e 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -715,7 +715,6 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > { > const struct of_device_id *match; > struct device_node *companion; > - struct device_node *port0, *port1; > struct rcar_lvds *companion_lvds; > struct device *dev = lvds->dev; > int dual_link; > @@ -743,11 +742,8 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > * connected to, if they are marked as expecting even pixels and > * odd pixels than we need to enable vertical stripe output. > */ > - port0 = of_graph_get_port_by_id(dev->of_node, 1); > - port1 = of_graph_get_port_by_id(companion, 1); > - dual_link = drm_of_lvds_get_dual_link_pixel_order(port0, port1); > - of_node_put(port0); > - of_node_put(port1); > + dual_link = drm_of_lvds_get_dual_link_pixel_order(dev->of_node, 1, -1, > + companion, 1, -1); > > switch (dual_link) { > case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h > index b9b093add92e..7bb1f6603beb 100644 > --- a/include/drm/drm_of.h > +++ b/include/drm/drm_of.h > @@ -47,8 +47,12 @@ 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 drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1, > - const struct device_node *port2); > +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1, > + int port1_id, > + int endpoint1_id, > + const struct device_node *dev2, > + int port2_id, > + int endpoint2_id); > #else > static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev, > struct device_node *port) > @@ -93,8 +97,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np, > } > > static inline int > -drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1, > - const struct device_node *port2) > +drm_of_lvds_get_dual_link_pixel_order(const struct device_node *dev1, > + int port1_id, > + int endpoint1_id, > + const struct device_node *dev2, > + int port2_id, > + int endpoint2_id) > { > return -EINVAL; > } -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel