dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20
@ 2020-10-05 15:15 Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Maxime Ripard
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

Hi,

This is a second attempt at supporting the LVDS dual-link output on the
Allwinner A20.

Let me know what you think,
Maxime

Changes from v2:
  - Added the DT binding description
  - Split the patch to enable the A20
  - Reworked a bit the error messages

Changes from v1:
  - Reworked the DT bindings
  - Refactored a bit the panel registration in the tcon code.

Maxime Ripard (6):
  drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
  dt-bindings: display: sun4i: Add LVDS Dual-Link property
  drm/sun4i: tcon: Refactor the LVDS and panel probing
  drm/sun4i: tcon: Support the LVDS Dual-Link
  drm/sun4i: tcon: Enable the A20 dual-link output
  [DO NOT MERGE] ARM: dts: sun7i: Enable LVDS Dual-Link on the Cubieboard

 Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml |   6 +++-
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts                             |  69 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_of.c                                                |  98 +++++++++++++++++++++----------------------
 drivers/gpu/drm/rcar-du/rcar_lvds.c                                     |   8 +---
 drivers/gpu/drm/sun4i/sun4i_tcon.c                                      | 163 +++++++++++++++++++++++++++++++++++++++++-------------------------------
 drivers/gpu/drm/sun4i/sun4i_tcon.h                                      |   4 ++-
 include/drm/drm_of.h                                                    |  16 +++++--
 7 files changed, 236 insertions(+), 128 deletions(-)

base-commit: d113dbba9a18f9ac71edb1a66ae552c9407355f4
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  2020-10-11 23:00   ` Laurent Pinchart
  2020-10-05 15:15 ` [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property Maxime Ripard
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

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 <maxime@cerno.tech>
---
 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);
-		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().
-		 */
-		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
+ * @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.
  *
  * 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;
 
-	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;
 }
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  2020-10-07 15:50   ` Rob Herring
  2020-10-05 15:15 ` [PATCH RESEND v3 3/6] drm/sun4i: tcon: Refactor the LVDS and panel probing Maxime Ripard
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

The Allwinner SoCs with two TCONs and LVDS output can use both to drive an
LVDS dual-link. Add a new property to express that link between these two
TCONs.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml b/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
index e5344c4ae226..ce407f5466a5 100644
--- a/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
+++ b/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
@@ -115,6 +115,12 @@ properties:
         - const: edp
         - const: lvds
 
+  allwinner,lvds-companion:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: >
+      Phandle to the other TCON in the system used to drive a dual-link LVDS
+      output.
+
   ports:
     type: object
     description: |
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 3/6] drm/sun4i: tcon: Refactor the LVDS and panel probing
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link Maxime Ripard
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

The current code to parse the DT, deal with the older device trees, and
register either the RGB or LVDS output has so far grown organically into
the bind function and has become quite hard to extend properly.

Let's move it into a single function that grabs all the resources it needs
and registers the proper panel output.

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 127 +++++++++++++-----------------
 1 file changed, 58 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 2a5a9903c4c6..8a21cf7a6bc1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -875,6 +875,63 @@ static int sun4i_tcon_init_regmap(struct device *dev,
 	return 0;
 }
 
+static int sun4i_tcon_register_panel(struct drm_device *drm,
+				     struct sun4i_tcon *tcon)
+{
+	struct device_node *companion;
+	struct device_node *remote;
+	struct device *dev = tcon->dev;
+	int ret;
+
+	/*
+	 * If we have an LVDS panel connected to the TCON, we should
+	 * just probe the LVDS connector. Otherwise, let's just register
+	 * an RGB panel.
+	 */
+	remote = of_graph_get_remote_node(dev->of_node, 1, 0);
+	if (!tcon->quirks->supports_lvds ||
+	    !of_device_is_compatible(remote, "panel-lvds"))
+		return sun4i_rgb_init(drm, tcon);
+
+	/*
+	 * This can only be made optional since we've had DT
+	 * nodes without the LVDS reset properties.
+	 *
+	 * If the property is missing, just disable LVDS, and
+	 * print a warning.
+	 */
+	tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
+	if (IS_ERR(tcon->lvds_rst)) {
+		dev_err(dev, "Couldn't get our reset line\n");
+		return PTR_ERR(tcon->lvds_rst);
+	} else if (!tcon->lvds_rst) {
+		dev_warn(dev, "Missing LVDS reset property, please upgrade your DT\n");
+		return -ENODEV;
+	}
+
+	reset_control_reset(tcon->lvds_rst);
+
+	/*
+	 * This can only be made optional since we've had DT
+	 * nodes without the LVDS clocks properties.
+	 *
+	 * If the property is missing, just disable LVDS, and
+	 * print a warning.
+	 */
+	if (tcon->quirks->has_lvds_alt) {
+		tcon->lvds_pll = devm_clk_get_optional(dev, "lvds-alt");
+		if (IS_ERR(tcon->lvds_pll)) {
+			dev_err(dev, "Couldn't get the LVDS PLL\n");
+			return PTR_ERR(tcon->lvds_pll);
+		} else if (!tcon->lvds_pll) {
+			dev_warn(dev, "Missing LVDS PLL clock, please upgrade your DT\n");
+			return -ENODEV;
+		}
+	}
+
+	return sun4i_lvds_init(drm, tcon);
+}
+
 /*
  * On SoCs with the old display pipeline design (Display Engine 1.0),
  * the TCON is always tied to just one backend. Hence we can traverse
@@ -1122,10 +1179,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = data;
 	struct sun4i_drv *drv = drm->dev_private;
 	struct sunxi_engine *engine;
-	struct device_node *remote;
 	struct sun4i_tcon *tcon;
 	struct reset_control *edp_rstc;
-	bool has_lvds_rst, has_lvds_alt, can_lvds;
 	int ret;
 
 	engine = sun4i_tcon_find_engine(drv, dev->of_node);
@@ -1170,58 +1225,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	if (tcon->quirks->supports_lvds) {
-		/*
-		 * This can only be made optional since we've had DT
-		 * nodes without the LVDS reset properties.
-		 *
-		 * If the property is missing, just disable LVDS, and
-		 * print a warning.
-		 */
-		tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
-		if (IS_ERR(tcon->lvds_rst)) {
-			dev_err(dev, "Couldn't get our reset line\n");
-			return PTR_ERR(tcon->lvds_rst);
-		} else if (tcon->lvds_rst) {
-			has_lvds_rst = true;
-			reset_control_reset(tcon->lvds_rst);
-		} else {
-			has_lvds_rst = false;
-		}
-
-		/*
-		 * This can only be made optional since we've had DT
-		 * nodes without the LVDS reset properties.
-		 *
-		 * If the property is missing, just disable LVDS, and
-		 * print a warning.
-		 */
-		if (tcon->quirks->has_lvds_alt) {
-			tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");
-			if (IS_ERR(tcon->lvds_pll)) {
-				if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {
-					has_lvds_alt = false;
-				} else {
-					dev_err(dev, "Couldn't get the LVDS PLL\n");
-					return PTR_ERR(tcon->lvds_pll);
-				}
-			} else {
-				has_lvds_alt = true;
-			}
-		}
-
-		if (!has_lvds_rst ||
-		    (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {
-			dev_warn(dev, "Missing LVDS properties, Please upgrade your DT\n");
-			dev_warn(dev, "LVDS output disabled\n");
-			can_lvds = false;
-		} else {
-			can_lvds = true;
-		}
-	} else {
-		can_lvds = false;
-	}
-
 	ret = sun4i_tcon_init_clocks(dev, tcon);
 	if (ret) {
 		dev_err(dev, "Couldn't init our TCON clocks\n");
@@ -1256,21 +1259,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	}
 
 	if (tcon->quirks->has_channel_0) {
-		/*
-		 * If we have an LVDS panel connected to the TCON, we should
-		 * just probe the LVDS connector. Otherwise, just probe RGB as
-		 * we used to.
-		 */
-		remote = of_graph_get_remote_node(dev->of_node, 1, 0);
-		if (of_device_is_compatible(remote, "panel-lvds"))
-			if (can_lvds)
-				ret = sun4i_lvds_init(drm, tcon);
-			else
-				ret = -EINVAL;
-		else
-			ret = sun4i_rgb_init(drm, tcon);
-		of_node_put(remote);
-
+		ret = sun4i_tcon_register_panel(drm, tcon);
 		if (ret < 0)
 			goto err_free_dotclock;
 	}
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
                   ` (2 preceding siblings ...)
  2020-10-05 15:15 ` [PATCH RESEND v3 3/6] drm/sun4i: tcon: Refactor the LVDS and panel probing Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  2020-10-09 15:23   ` Chen-Yu Tsai
  2020-10-05 15:15 ` [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output Maxime Ripard
  2020-10-05 15:15 ` [PATCH RESEND v3 6/6] [DO NOT MERGE] ARM: dts: sun7i: Enable LVDS Dual-Link on the Cubieboard Maxime Ripard
  5 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

The A20 and other SoC with two TCONs (A31, R40, etc.) can use its second
TCON as the secondary LVDS link in a dual-link setup, with the TCON0 being
the main link. Extend a bit the parsing code to leverage the DRM dual-link
code, register only the LVDS output on the primary TCON, and add the needed
bits to setup the TCON properly.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 35 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  4 ++++-
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 8a21cf7a6bc1..f497d866e835 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -487,6 +487,9 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
 	else
 		reg |= SUN4I_TCON0_LVDS_IF_DATA_POL_NORMAL;
 
+	if (tcon->lvds_dual_link)
+		reg |= SUN4I_TCON0_LVDS_IF_DUAL_LINK;
+
 	if (sun4i_tcon_get_pixel_depth(encoder) == 24)
 		reg |= SUN4I_TCON0_LVDS_IF_BITWIDTH_24BITS;
 	else
@@ -894,6 +897,16 @@ static int sun4i_tcon_register_panel(struct drm_device *drm,
 		return sun4i_rgb_init(drm, tcon);
 
 	/*
+	 * Only the TCON0 will be relevant for the LVDS output, so if
+	 * our ID is something else, let's prevent our TCON from
+	 * registering its own LVDS output
+	 */
+	if (tcon->id) {
+		dev_dbg(dev, "TCON used as an LVDS secondary link.");
+		return 0;
+	}
+
+	/*
 	 * This can only be made optional since we've had DT
 	 * nodes without the LVDS reset properties.
 	 *
@@ -929,6 +942,28 @@ static int sun4i_tcon_register_panel(struct drm_device *drm,
 		}
 	}
 
+	/*
+	 * If we don't have a second TCON, we will never be able to do
+	 * dual-link LVDS, so we don't have much more to do.
+	 */
+	companion = of_parse_phandle(dev->of_node, "allwinner,lvds-companion", 0);
+	if (!companion)
+		return sun4i_lvds_init(drm, tcon);
+
+	/*
+	 * Let's do a sanity check on the dual-link setup to make sure
+	 * everything is properly described.
+	 */
+	ret = drm_of_lvds_get_dual_link_pixel_order(dev->of_node, 1, 0,
+						    companion, 1, 0);
+	if (ret < 0) {
+		dev_err(dev, "Invalid Dual-Link Configuration.\n");
+		return ret;
+	}
+
+	dev_info(dev, "Primary TCON, enabling LVDS Dual-Link");
+	tcon->lvds_dual_link = true;
+
 	return sun4i_lvds_init(drm, tcon);
 }
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index cfbf4e6c1679..51c4e09cdd13 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -98,6 +98,7 @@
 
 #define SUN4I_TCON0_LVDS_IF_REG			0x84
 #define SUN4I_TCON0_LVDS_IF_EN				BIT(31)
+#define SUN4I_TCON0_LVDS_IF_DUAL_LINK			BIT(30)
 #define SUN4I_TCON0_LVDS_IF_BITWIDTH_MASK		BIT(26)
 #define SUN4I_TCON0_LVDS_IF_BITWIDTH_18BITS		(1 << 26)
 #define SUN4I_TCON0_LVDS_IF_BITWIDTH_24BITS		(0 << 26)
@@ -274,6 +275,9 @@ struct sun4i_tcon {
 	/* Associated crtc */
 	struct sun4i_crtc		*crtc;
 
+	/* Is the LVDS link a dual-channel link? */
+	bool				lvds_dual_link;
+
 	int				id;
 
 	/* TCON list management */
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
                   ` (3 preceding siblings ...)
  2020-10-05 15:15 ` [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  2020-10-09 15:20   ` Chen-Yu Tsai
  2020-10-05 15:15 ` [PATCH RESEND v3 6/6] [DO NOT MERGE] ARM: dts: sun7i: Enable LVDS Dual-Link on the Cubieboard Maxime Ripard
  5 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

The A20 second TCON (TCON1) can be used as a secondary output to drive a
dual-link LVDS output. Let's add it to our capabilities.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index f497d866e835..de3d1b17a499 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -1523,6 +1523,7 @@ static const struct sun4i_tcon_quirks sun7i_a20_tcon0_quirks = {
 };
 
 static const struct sun4i_tcon_quirks sun7i_a20_quirks = {
+	.supports_lvds		= true,
 	.has_channel_0		= true,
 	.has_channel_1		= true,
 	.dclk_min_div		= 4,
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH RESEND v3 6/6] [DO NOT MERGE] ARM: dts: sun7i: Enable LVDS Dual-Link on the Cubieboard
  2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
                   ` (4 preceding siblings ...)
  2020-10-05 15:15 ` [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output Maxime Ripard
@ 2020-10-05 15:15 ` Maxime Ripard
  5 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2020-10-05 15:15 UTC (permalink / raw)
  To: Chen-Yu Tsai, Maxime Ripard, dri-devel
  Cc: Mark Rutland, devicetree, David Airlie, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

For the sake of the example, let's enable an LVDS Dual-Link display on a
Cubieboard.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 69 ++++++++++++++++++++++-
 1 file changed, 69 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index b8203e4ef21c..20278a27ec16 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -85,6 +85,49 @@
 			gpios = <&pio 7 20 GPIO_ACTIVE_HIGH>;
 		};
 	};
+
+	panel: panel {
+		compatible = "panel-lvds";
+		width-mm = <153>;
+		height-mm = <90>;
+		data-mapping = "vesa-24";
+
+		panel-timing {
+			clock-frequency = <148500000>;
+			hfront-porch = <88>;
+			hactive = <1920>;
+			hback-porch = <148>;
+			hsync-len = <44>;
+
+			vfront-porch = <4>;
+			vactive = <1080>;
+			vback-porch = <36>;
+			vsync-len = <5>;
+		};
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				dual-lvds-even-pixels;
+
+				panel_input_0: endpoint {
+					remote-endpoint = <&tcon0_out_panel>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				dual-lvds-odd-pixels;
+
+				panel_input_1: endpoint {
+					remote-endpoint = <&tcon1_out_panel>;
+				};
+			};
+		};
+	};
 };
 
 &ahci {
@@ -218,6 +261,32 @@
 	status = "okay";
 };
 
+&tcon0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&lcd_lvds0_pins>;
+	allwinner,lvds-companion = <&tcon1>;
+	status = "okay";
+};
+
+&tcon0_out {
+	tcon0_out_panel: endpoint@0 {
+		remote-endpoint = <&panel_input_0>;
+	};
+};
+
+&tcon1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&lcd_lvds1_pins>;
+	allwinner,lvds-companion = <&tcon0>;
+	status = "okay";
+};
+
+&tcon1_out {
+	tcon1_out_panel: endpoint@0 {
+		remote-endpoint = <&panel_input_1>;
+	};
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property
  2020-10-05 15:15 ` [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property Maxime Ripard
@ 2020-10-07 15:50   ` Rob Herring
  0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2020-10-07 15:50 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Chen-Yu Tsai,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

On Mon, Oct 05, 2020 at 05:15:40PM +0200, Maxime Ripard wrote:
> The Allwinner SoCs with two TCONs and LVDS output can use both to drive an
> LVDS dual-link. Add a new property to express that link between these two
> TCONs.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml b/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
> index e5344c4ae226..ce407f5466a5 100644
> --- a/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
> +++ b/Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
> @@ -115,6 +115,12 @@ properties:
>          - const: edp
>          - const: lvds
>  
> +  allwinner,lvds-companion:

We already have 1 vendor property for this. How about 'link-companion' 
for something common.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output
  2020-10-05 15:15 ` [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output Maxime Ripard
@ 2020-10-09 15:20   ` Chen-Yu Tsai
  0 siblings, 0 replies; 13+ messages in thread
From: Chen-Yu Tsai @ 2020-10-09 15:20 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

On Mon, Oct 5, 2020 at 11:17 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> The A20 second TCON (TCON1) can be used as a secondary output to drive a
> dual-link LVDS output. Let's add it to our capabilities.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link
  2020-10-05 15:15 ` [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link Maxime Ripard
@ 2020-10-09 15:23   ` Chen-Yu Tsai
  0 siblings, 0 replies; 13+ messages in thread
From: Chen-Yu Tsai @ 2020-10-09 15:23 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Rob Herring,
	Laurent Pinchart, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

On Mon, Oct 5, 2020 at 11:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> The A20 and other SoC with two TCONs (A31, R40, etc.) can use its second
> TCON as the secondary LVDS link in a dual-link setup, with the TCON0 being
> the main link. Extend a bit the parsing code to leverage the DRM dual-link
> code, register only the LVDS output on the primary TCON, and add the needed
> bits to setup the TCON properly.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

But I suppose you might need to change the DT property name.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
  2020-10-05 15:15 ` [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Maxime Ripard
@ 2020-10-11 23:00   ` Laurent Pinchart
  2020-11-18 17:48     ` Maxime Ripard
  0 siblings, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2020-10-11 23:00 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Chen-Yu Tsai,
	Rob Herring, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

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 <maxime@cerno.tech>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
  2020-10-11 23:00   ` Laurent Pinchart
@ 2020-11-18 17:48     ` Maxime Ripard
  2021-01-26 14:10       ` Laurent Pinchart
  0 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2020-11-18 17:48 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Chen-Yu Tsai,
	Rob Herring, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 2493 bytes --]

Hi Laurent,

On Mon, Oct 12, 2020 at 02:00:30AM +0300, Laurent Pinchart wrote:
> > -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() ?

I tried to do that, and I'm not quite really sure how to go around it.

This scans all the endpoints in a given port.

However, now that we have the device, port id and endpoint id, we need
to use of_graph_get_port_by_id to get the port matching the device and
port id, and iterate over all its endpoint as done here.

The trouble is that of_graph_get_port_by_id expects a !const
device_node, yet drm_of_lvds_get_dual_link_pixel_order (and seems to be
doing so rightfully), so that creates a warning because we drop the
const there.

Changing the prototype to passing only the port device_node doesn't
really work either, since it would be const, and we would need to call
of_graph_get_endpoint_by_regs (so having the parent device_node, through
of_graph_get_port_parent) and of_graph_get_port_parent takes a !const
port device_node.

I guess we could drop const entirely from our function, but that doesn't
look right either..

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
  2020-11-18 17:48     ` Maxime Ripard
@ 2021-01-26 14:10       ` Laurent Pinchart
  0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2021-01-26 14:10 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mark Rutland, devicetree, David Airlie, dri-devel, Chen-Yu Tsai,
	Rob Herring, Thomas Zimmermann, Daniel Vetter, Frank Rowand,
	linux-arm-kernel

Hi Maxime,

On Wed, Nov 18, 2020 at 06:48:05PM +0100, Maxime Ripard wrote:
> On Mon, Oct 12, 2020 at 02:00:30AM +0300, Laurent Pinchart wrote:
> > > -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() ?
> 
> I tried to do that, and I'm not quite really sure how to go around it.
> 
> This scans all the endpoints in a given port.
> 
> However, now that we have the device, port id and endpoint id, we need
> to use of_graph_get_port_by_id to get the port matching the device and
> port id, and iterate over all its endpoint as done here.
> 
> The trouble is that of_graph_get_port_by_id expects a !const
> device_node, yet drm_of_lvds_get_dual_link_pixel_order (and seems to be
> doing so rightfully), so that creates a warning because we drop the
> const there.

of_graph_get_port_by_id() doesn't seem to modify the device_node passed
to it, couldn't it be modified to take a const pointer ?

> Changing the prototype to passing only the port device_node doesn't
> really work either, since it would be const, and we would need to call
> of_graph_get_endpoint_by_regs (so having the parent device_node, through
> of_graph_get_port_parent) and of_graph_get_port_parent takes a !const
> port device_node.
> 
> I guess we could drop const entirely from our function, but that doesn't
> look right either..

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-01-26 14:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 15:15 [PATCH RESEND v3 0/6] drm/sun4i: Add support for dual-link LVDS on the A20 Maxime Ripard
2020-10-05 15:15 ` [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order Maxime Ripard
2020-10-11 23:00   ` Laurent Pinchart
2020-11-18 17:48     ` Maxime Ripard
2021-01-26 14:10       ` Laurent Pinchart
2020-10-05 15:15 ` [PATCH RESEND v3 2/6] dt-bindings: display: sun4i: Add LVDS Dual-Link property Maxime Ripard
2020-10-07 15:50   ` Rob Herring
2020-10-05 15:15 ` [PATCH RESEND v3 3/6] drm/sun4i: tcon: Refactor the LVDS and panel probing Maxime Ripard
2020-10-05 15:15 ` [PATCH RESEND v3 4/6] drm/sun4i: tcon: Support the LVDS Dual-Link Maxime Ripard
2020-10-09 15:23   ` Chen-Yu Tsai
2020-10-05 15:15 ` [PATCH RESEND v3 5/6] drm/sun4i: tcon: Enable the A20 dual-link output Maxime Ripard
2020-10-09 15:20   ` Chen-Yu Tsai
2020-10-05 15:15 ` [PATCH RESEND v3 6/6] [DO NOT MERGE] ARM: dts: sun7i: Enable LVDS Dual-Link on the Cubieboard Maxime Ripard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).