[PATCH v4 0/7] Add dual-LVDS panel support to EK874

[PATCH v4 0/7] Add dual-LVDS panel support to EK874

[Fabrizio Castro]

Dear All,

this series adds support for dual-LVDS panel IDK-2121WR
from Advantech:
https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm

V4 incorporates most of the comments received from v3, making it significantly
different from v3: patch "drm: rcar-du: lvds: Add dual-LVDS panels support"
has been split in 4 patches, patches
"dt-bindings: display: Add bindings for LVDS bus-timings" and
"dt-bindings: display: Add idk-2121wr binding" have been merged together,
and a few issues have been fixed.

Thanks,
Fab

Fabrizio Castro (7):
  drm: of: Add drm_of_lvds_get_dual_link_pixel_order
  drm: rcar-du: lvds: Improve identification of panels
  drm: rcar-du: lvds: Get dual link configuration from DT
  drm: rcar-du: lvds: Allow for even and odd pixels swap
  drm: rcar-du: lvds: Fix mode for companion encoder
  dt-bindings: display: Add idk-2121wr binding
  arm64: dts: renesas: Add EK874 board with idk-2121wr display support

 .../display/panel/advantech,idk-2121wr.yaml        | 128 ++++++++++++++++++
 arch/arm64/boot/dts/renesas/Makefile               |   3 +-
 .../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 116 +++++++++++++++++
 drivers/gpu/drm/drm_of.c                           | 104 +++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_lvds.c                | 143 ++++++++++++++-------
 include/drm/drm_of.h                               |  20 +++
 6 files changed, 468 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
 create mode 100644 arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts

-- 
2.7.4

[PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order

[Fabrizio Castro]

An LVDS dual-link connection is made of two links, with even
pixels transitting on one link, and odd pixels on the other
link. The device tree can be used to fully describe dual-link
LVDS connections between encoders and bridges/panels.
The sink of an LVDS dual-link connection is made of two ports,
the corresponding OF graph port nodes can be marked
with either dual-lvds-even-pixels or dual-lvds-odd-pixels,
and that fully describes an LVDS dual-link connection,
including pixel order.

drm_of_lvds_get_dual_link_pixel_order is a new helper
added by this patch, given the source port nodes it
returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS if the source
port nodes belong to an LVDS dual-link connection, with even
pixels expected to be generated from the first port, and odd
pixels expected to be generated from the second port.
If the new helper returns DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS,
odd pixels are expected to be generated from the first port,
and even pixels from the other port.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* The patch had title "drm: Add bus timings helper" in v3
* The code has now been moved to drm_of, and has been fully
  restructured, thanks to Laurent and Daniel for the comments

v2->v3:
* new patch
---
 drivers/gpu/drm/drm_of.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     |  20 +++++++++
 2 files changed, 124 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 0ca5880..c2e9ab7 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -274,3 +274,107 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
+
+enum drm_of_lvds_pixels {
+	DRM_OF_LVDS_EVEN = BIT(0),
+	DRM_OF_LVDS_ODD = BIT(1),
+};
+
+static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
+{
+	bool even_pixels =
+		of_property_read_bool(port_node, "dual-lvds-even-pixels");
+	bool odd_pixels =
+		of_property_read_bool(port_node, "dual-lvds-odd-pixels");
+
+	return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
+	       (odd_pixels ? DRM_OF_LVDS_ODD : 0);
+}
+
+static int drm_of_lvds_get_remote_pixels_type(
+			const struct device_node *port_node)
+{
+	struct device_node *endpoint = NULL;
+	int pixels_type = -EPIPE;
+
+	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)
+			return -EPIPE;
+
+		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
+		of_node_put(remote_port);
+		if (!pixels_type)
+			pixels_type = current_pt;
+		if (!current_pt || pixels_type != current_pt)
+			return -EINVAL;
+	}
+
+	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
+ *
+ * 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
+ * returns, for two ports of an LVDS dual-link source, which port shall transmit
+ * the even and odd pixels, based on the requirements of the connected sink.
+ *
+ * The pixel order is determined from the dual-lvds-even-pixels and
+ * dual-lvds-odd-pixels properties in the sink's DT port nodes. If those
+ * properties are not present, or if their usage is not valid, this function
+ * returns -EINVAL.
+ *
+ * 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.
+ *
+ * Return:
+ * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
+ *   carries odd pixels
+ * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries odd pixels and @port1
+ *   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
+ */
+int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
+					  const struct device_node *port2)
+{
+	int remote_p1_pt, remote_p2_pt;
+
+	if (!port1 || !port2)
+		return -EINVAL;
+
+	remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1);
+	if (remote_p1_pt < 0)
+		return remote_p1_pt;
+
+	remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2);
+	if (remote_p2_pt < 0)
+		return remote_p2_pt;
+
+	/*
+	 * A valid dual-lVDS bus is found when one remote port is marked with
+	 * "dual-lvds-even-pixels", and the other remote port is marked with
+	 * "dual-lvds-odd-pixels", bail out if the markers are not right.
+	 */
+	if (remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
+		return -EINVAL;
+
+	return remote_p1_pt == DRM_OF_LVDS_EVEN ?
+		DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS :
+		DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
+}
+EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_pixel_order);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index ead34ab..8ec7ca6 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -16,6 +16,18 @@ struct drm_panel;
 struct drm_bridge;
 struct device_node;
 
+/**
+ * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection
+ * @DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: Even pixels are expected to be generated
+ *    from the first port, odd pixels from the second port
+ * @DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: Odd pixels are expected to be generated
+ *    from the first port, even pixels from the second port
+ */
+enum drm_lvds_dual_link_pixels {
+	DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 0,
+	DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 1,
+};
+
 #ifdef CONFIG_OF
 uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
 			    struct device_node *port);
@@ -35,6 +47,8 @@ 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);
 #else
 static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
 					  struct device_node *port)
@@ -77,6 +91,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
 {
 	return -EINVAL;
 }
+
+int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
+					  const struct device_node *port2)
+{
+	return -EINVAL;
+}
 #endif
 
 /*
-- 
2.7.4

[PATCH v4 2/7] drm: rcar-du: lvds: Improve identification of panels

[Fabrizio Castro]

Dual-LVDS panels are mistakenly identified as bridges, this
commit replaces the current logic with a call to
drm_of_find_panel_or_bridge to sort that out.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch extracted from patch:
  "drm: rcar-du: lvds: Add dual-LVDS panels support"
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 45 +++++++++----------------------------
 1 file changed, 10 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 8c6c172..3cb0a83 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -21,6 +21,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
 
@@ -705,10 +706,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 {
 	struct device_node *local_output = NULL;
-	struct device_node *remote_input = NULL;
 	struct device_node *remote = NULL;
-	struct device_node *node;
-	bool is_bridge = false;
 	int ret = 0;
 
 	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
@@ -736,45 +734,22 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 		goto done;
 	}
 
-	remote_input = of_graph_get_remote_endpoint(local_output);
-
-	for_each_endpoint_of_node(remote, node) {
-		if (node != remote_input) {
-			/*
-			 * We've found one endpoint other than the input, this
-			 * must be a bridge.
-			 */
-			is_bridge = true;
-			of_node_put(node);
-			break;
-		}
-	}
-
-	if (is_bridge) {
-		lvds->next_bridge = of_drm_find_bridge(remote);
-		if (!lvds->next_bridge) {
-			ret = -EPROBE_DEFER;
-			goto done;
-		}
+	ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
+					  &lvds->panel, &lvds->next_bridge);
+	if (ret)
+		goto done;
 
-		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
-			lvds->dual_link = lvds->next_bridge->timings
-					? lvds->next_bridge->timings->dual_link
-					: false;
-	} else {
-		lvds->panel = of_drm_find_panel(remote);
-		if (IS_ERR(lvds->panel)) {
-			ret = PTR_ERR(lvds->panel);
-			goto done;
-		}
-	}
+	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
+	    lvds->next_bridge)
+		lvds->dual_link = lvds->next_bridge->timings
+				? lvds->next_bridge->timings->dual_link
+				: false;
 
 	if (lvds->dual_link)
 		ret = rcar_lvds_parse_dt_companion(lvds);
 
 done:
 	of_node_put(local_output);
-	of_node_put(remote_input);
 	of_node_put(remote);
 
 	/*
-- 
2.7.4

[PATCH v4 3/7] drm: rcar-du: lvds: Get dual link configuration from DT

[Fabrizio Castro]

For dual-LVDS configurations, it is now possible to mark the
DT port nodes for the sink with boolean properties (like
dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers
know the encoders need to be configured in dual-LVDS mode.

Rework the implementation of rcar_lvds_parse_dt_companion
to make use of the DT markers while keeping backward
compatibility.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch extracted from patch:
  "drm: rcar-du: lvds: Add dual-LVDS panels support"
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3cb0a83..6c1f171 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
 static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 {
 	const struct of_device_id *match;
-	struct device_node *companion;
+	struct device_node *companion, *p0, *p1;
+	struct rcar_lvds *companion_lvds;
 	struct device *dev = lvds->dev;
+	int dual_link;
 	int ret = 0;
 
 	/* Locate the companion LVDS encoder for dual-link operation, if any. */
@@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 		goto done;
 	}
 
+	/*
+	 * We need to work out if the sink is expecting us to function in
+	 * dual-link mode. We do this by looking at the DT port nodes we are
+	 * connected to, if they are marked as expecting even pixels and
+	 * odd pixels than we need to enable vertical stripe output.
+	 */
+	p0 = of_graph_get_port_by_id(dev->of_node, 1);
+	p1 = of_graph_get_port_by_id(companion, 1);
+	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
+	of_node_put(p0);
+	of_node_put(p1);
+	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
+		lvds->dual_link = true;
+	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
+		/*
+		 * Early dual-link bridge specific implementations populate the
+		 * timings field of drm_bridge, read the dual_link flag off the
+		 * bridge directly for backward compatibility.
+		 */
+		lvds->dual_link = lvds->next_bridge->timings->dual_link;
+	}
+
+	if (!lvds->dual_link) {
+		dev_dbg(dev, "Single-link configuration detected\n");
+		goto done;
+	}
+
 	lvds->companion = of_drm_find_bridge(companion);
 	if (!lvds->companion) {
 		ret = -EPROBE_DEFER;
 		goto done;
 	}
 
-	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
+	dev_dbg(dev,
+		"Dual-link configuration detected (companion encoder %pOF)\n",
+		companion);
+
+	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
+
+	/*
+	 * FIXME: We should not be messing with the companion encoder private
+	 * data from the primary encoder, we should rather let the companion
+	 * encoder work things out on its own. However, the companion encoder
+	 * doesn't hold a reference to the primary encoder, and
+	 * drm_of_lvds_get_dual_link_pixel_order needs to be given references
+	 * to the output ports of both encoders, therefore leave it like this
+	 * for the time being.
+	 */
+	companion_lvds->dual_link = true;
 
 done:
 	of_node_put(companion);
@@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 	if (ret)
 		goto done;
 
-	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
-	    lvds->next_bridge)
-		lvds->dual_link = lvds->next_bridge->timings
-				? lvds->next_bridge->timings->dual_link
-				: false;
-
-	if (lvds->dual_link)
+	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
 		ret = rcar_lvds_parse_dt_companion(lvds);
 
 done:
-- 
2.7.4

[PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap

[Fabrizio Castro]

DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels
can be used to work out if the driver needs to swap even
and odd pixels around.

This patch makes use of the return value from function
drm_of_lvds_get_dual_link_pixel_order to determine if we
need to swap odd and even pixels around for things to work
properly.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch extracted from patch:
  "drm: rcar-du: lvds: Add dual-LVDS panels support"
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 6c1f171..cb2147c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -71,6 +71,7 @@ struct rcar_lvds {
 
 	struct drm_bridge *companion;
 	bool dual_link;
+	bool stripe_swap_data;
 };
 
 #define bridge_to_rcar_lvds(b) \
@@ -441,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
 
 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
-		/*
-		 * Configure vertical stripe based on the mode of operation of
-		 * the connected device.
-		 */
-		rcar_lvds_write(lvds, LVDSTRIPE,
-				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
+		u32 lvdstripe = 0;
+
+		if (lvds->dual_link)
+			/*
+			 * Configure vertical stripe based on the mode of
+			 * operation of the connected device.
+			 *
+			 * ST_SWAP from LVD1STRIPE is reserved, do not set
+			 * in the companion LVDS
+			 */
+			lvdstripe = LVDSTRIPE_ST_ON |
+				(lvds->companion && lvds->stripe_swap_data ?
+				 LVDSTRIPE_ST_SWAP : 0);
+		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
 	}
 
 	/*
@@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
 	of_node_put(p0);
 	of_node_put(p1);
-	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
+
+	switch (dual_link) {
+	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
+		/*
+		 * By default we generate even pixels from this encoder and odd
+		 * pixels from the companion encoder, but since p0 is connected
+		 * to the port expecting ood pixels, and p1 is connected to the
+		 * port expecting even pixels, we need to swap even and odd
+		 * pixels around.
+		 */
+		lvds->stripe_swap_data = true;
+		lvds->dual_link = true;
+		break;
+	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
 		lvds->dual_link = true;
-	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
+		break;
+	default:
 		/*
 		 * Early dual-link bridge specific implementations populate the
 		 * timings field of drm_bridge, read the dual_link flag off the
 		 * bridge directly for backward compatibility.
 		 */
-		lvds->dual_link = lvds->next_bridge->timings->dual_link;
+		if (lvds->next_bridge && lvds->next_bridge->timings)
+			lvds->dual_link = lvds->next_bridge->timings->dual_link;
 	}
 
+
 	if (!lvds->dual_link) {
 		dev_dbg(dev, "Single-link configuration detected\n");
 		goto done;
@@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 		"Dual-link configuration detected (companion encoder %pOF)\n",
 		companion);
 
+	if (lvds->stripe_swap_data)
+		dev_dbg(dev, "Data swapping required\n");
+
 	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
 
 	/*
-- 
2.7.4

[PATCH v4 5/7] drm: rcar-du: lvds: Fix mode for companion encoder

[Fabrizio Castro]

Primary and companion encoders need to set the same mode for
things to work properly.

rcar_lvds_mode_set gets called into for the primary encoder only,
therefore initialize the companion encoder mode while sorting
the primary encoder mode out.

Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch extracted from patch:
  "drm: rcar-du: lvds: Add dual-LVDS panels support"
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index cb2147c..eed5611 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -614,6 +614,18 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
 	lvds->display_mode = *adjusted_mode;
 
 	rcar_lvds_get_lvds_mode(lvds);
+	if (lvds->companion) {
+		struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
+							lvds->companion);
+
+		/*
+		 * FIXME: We should not be messing with the companion encoder
+		 * private data from the primary encoder, but since
+		 * rcar_lvds_mode_set gets called into for the primary encoder
+		 * only, we don't have much of a choice for now.
+		 */
+		companion_lvds->mode = lvds->mode;
+	}
 }
 
 static int rcar_lvds_attach(struct drm_bridge *bridge)
-- 
2.7.4

[PATCH v4 6/7] dt-bindings: display: Add idk-2121wr binding

[Fabrizio Castro]

Add binding for the idk-2121wr LVDS panel from Advantech.

Some panel-specific documentation can be found here:
https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* Absorbed patch "dt-bindings: display: Add bindings for LVDS
  bus-timings"
* Big restructuring after Rob's and Laurent's comments

v2->v3:
* new patch
---
 .../display/panel/advantech,idk-2121wr.yaml        | 128 +++++++++++++++++++++
 1 file changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
new file mode 100644
index 0000000..24cd38b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
@@ -0,0 +1,128 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/advantech,idk-2121wr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Advantech IDK-2121WR 21.5" Full-HD dual-LVDS panel
+
+maintainers:
+  - Fabrizio Castro <fabrizio.castro@bp.renesas.com>
+  - Thierry Reding <thierry.reding@gmail.com>
+
+description: |
+  The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
+  A dual-LVDS interface is a dual-link connection with even pixels traveling
+  on one link, and with odd pixels traveling on the other link.
+
+  The panel expects odd pixels on the first port, and even pixels on the
+  second port, therefore the ports must be marked accordingly (with either
+  dual-lvds-odd-pixels or dual-lvds-even-pixels).
+
+properties:
+  compatible:
+    items:
+      - const: advantech,idk-2121wr
+      - {} # panel-lvds, but not listed here to avoid false select
+
+  width-mm:
+    const: 476
+
+  height-mm:
+    const: 268
+
+  data-mapping:
+    const: vesa-24
+
+  ports:
+    type: object
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+      port@0:
+        type: object
+        description: The sink for odd pixels.
+        properties:
+          reg:
+            const: 0
+
+          dual-lvds-odd-pixels: true
+
+        required:
+          - reg
+          - dual-lvds-odd-pixels
+
+      port@1:
+        type: object
+        description: The sink for even pixels.
+        properties:
+          reg:
+            const: 1
+
+          dual-lvds-even-pixels: true
+
+        required:
+          - reg
+          - dual-lvds-even-pixels
+
+  panel-timing: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - width-mm
+  - height-mm
+  - data-mapping
+  - panel-timing
+  - ports
+
+examples:
+  - |+
+    panel-lvds {
+      compatible = "advantech,idk-2121wr", "panel-lvds";
+
+      width-mm = <476>;
+      height-mm = <268>;
+
+      data-mapping = "vesa-24";
+
+      panel-timing {
+        clock-frequency = <148500000>;
+        hactive = <1920>;
+        vactive = <1080>;
+        hsync-len = <44>;
+        hfront-porch = <88>;
+        hback-porch = <148>;
+        vfront-porch = <4>;
+        vback-porch = <36>;
+        vsync-len = <5>;
+      };
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+          dual-lvds-odd-pixels;
+          panel_in0: endpoint {
+            remote-endpoint = <&lvds0_out>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+          dual-lvds-even-pixels;
+          panel_in1: endpoint {
+            remote-endpoint = <&lvds1_out>;
+          };
+        };
+      };
+    };
+
+...
-- 
2.7.4

[PATCH v4 7/7] arm64: dts: renesas: Add EK874 board with idk-2121wr display support

[Fabrizio Castro]

The EK874 is advertised as compatible with panel IDK-2121WR from
Advantech, however the panel isn't sold alongside the board.
A new dts, adding everything that's required to get the panel to
to work with the EK874, is the most convenient way to support the
EK874 when it's connected to the IDK-2121WR.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
v3->v4:
* no change

v2->v3:
* removed renesas,swap-data property
* added dual-lvds-odd-pixels and dual-lvds-even-pixels properties

v1->v2:
* Added comment for lvds-connector-en-gpio
* Renamed &lvds0_panel_in to panel_in0
* Renamed &lvds1_panel_in to panel_in1
---
 arch/arm64/boot/dts/renesas/Makefile               |   3 +-
 .../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 116 +++++++++++++++++++++
 2 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts

diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 8fdbd22..2635799 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -3,7 +3,8 @@ dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m.dtb
 dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m-ex.dtb
 dtb-$(CONFIG_ARCH_R8A774B1) += r8a774b1-hihope-rzg2n.dtb
 dtb-$(CONFIG_ARCH_R8A774B1) += r8a774b1-hihope-rzg2n-ex.dtb
-dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb
+dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb \
+			       r8a774c0-ek874-idk-2121wr.dtb
 dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-x.dtb r8a7795-h3ulcb.dtb
 dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-h3ulcb-kf.dtb
 dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-xs.dtb
diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
new file mode 100644
index 0000000..a7b27d0
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the Silicon Linux RZ/G2E evaluation kit (EK874),
+ * connected to an Advantech IDK-2121WR 21.5" LVDS panel
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include "r8a774c0-ek874.dts"
+
+/ {
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm5 0 50000>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+
+		power-supply = <&reg_12p0v>;
+		enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	panel-lvds {
+		compatible = "advantech,idk-2121wr", "panel-lvds";
+
+		width-mm = <476>;
+		height-mm = <268>;
+
+		data-mapping = "vesa-24";
+
+		panel-timing {
+			clock-frequency = <148500000>;
+			hactive = <1920>;
+			vactive = <1080>;
+			hsync-len = <44>;
+			hfront-porch = <88>;
+			hback-porch = <148>;
+			vfront-porch = <4>;
+			vback-porch = <36>;
+			vsync-len = <5>;
+		};
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				dual-lvds-odd-pixels;
+				panel_in0: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				dual-lvds-even-pixels;
+				panel_in1: endpoint {
+					remote-endpoint = <&lvds1_out>;
+				};
+			};
+		};
+	};
+};
+
+&gpio0 {
+	/*
+	 * When GP0_17 is low LVDS[01] are connected to the LVDS connector
+	 * When GP0_17 is high LVDS[01] are connected to the LT8918L
+	 */
+	lvds-connector-en-gpio{
+		gpio-hog;
+		gpios = <17 GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "lvds-connector-en-gpio";
+	};
+};
+
+&lvds0 {
+	ports {
+		port@1 {
+			lvds0_out: endpoint {
+				remote-endpoint = <&panel_in0>;
+			};
+		};
+	};
+};
+
+&lvds1 {
+	status = "okay";
+
+	clocks = <&cpg CPG_MOD 727>, <&x13_clk>, <&extal_clk>;
+	clock-names = "fck", "dclkin.0", "extal";
+
+	ports {
+		port@1 {
+			lvds1_out: endpoint {
+				remote-endpoint = <&panel_in1>;
+			};
+		};
+	};
+};
+
+&pfc {
+	pwm5_pins: pwm5 {
+		groups = "pwm5_a";
+		function = "pwm5";
+	};
+};
+
+&pwm5 {
+	pinctrl-0 = <&pwm5_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
-- 
2.7.4

Re: [PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:48PM +0000, Fabrizio Castro wrote:
> An LVDS dual-link connection is made of two links, with even
> pixels transitting on one link, and odd pixels on the other
> link. The device tree can be used to fully describe dual-link
> LVDS connections between encoders and bridges/panels.
> The sink of an LVDS dual-link connection is made of two ports,
> the corresponding OF graph port nodes can be marked
> with either dual-lvds-even-pixels or dual-lvds-odd-pixels,
> and that fully describes an LVDS dual-link connection,
> including pixel order.
> 
> drm_of_lvds_get_dual_link_pixel_order is a new helper
> added by this patch, given the source port nodes it
> returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS if the source
> port nodes belong to an LVDS dual-link connection, with even
> pixels expected to be generated from the first port, and odd
> pixels expected to be generated from the second port.
> If the new helper returns DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS,
> odd pixels are expected to be generated from the first port,
> and even pixels from the other port.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * The patch had title "drm: Add bus timings helper" in v3
> * The code has now been moved to drm_of, and has been fully
>   restructured, thanks to Laurent and Daniel for the comments
> 
> v2->v3:
> * new patch
> ---
>  drivers/gpu/drm/drm_of.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     |  20 +++++++++
>  2 files changed, 124 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 0ca5880..c2e9ab7 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -274,3 +274,107 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> +
> +enum drm_of_lvds_pixels {
> +	DRM_OF_LVDS_EVEN = BIT(0),
> +	DRM_OF_LVDS_ODD = BIT(1),
> +};
> +
> +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> +{
> +	bool even_pixels =
> +		of_property_read_bool(port_node, "dual-lvds-even-pixels");
> +	bool odd_pixels =
> +		of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> +
> +	return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> +	       (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> +}
> +
> +static int drm_of_lvds_get_remote_pixels_type(
> +			const struct device_node *port_node)
> +{
> +	struct device_node *endpoint = NULL;
> +	int pixels_type = -EPIPE;
> +
> +	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)

You need an of_node_put(endpoint) in the code paths that exit from the
loop.

> +			return -EPIPE;
> +
> +		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> +		of_node_put(remote_port);
> +		if (!pixels_type)
> +			pixels_type = current_pt;

This will never happen as pixels_type is initialized to -EPIPE.
Replacing the condition with if (pixels_type < 0) should fix it.

> +		if (!current_pt || pixels_type != current_pt)
> +			return -EINVAL;

I would add a comment to explain this. If I understand the code
correcty, something along the lines of

		/*
		 * 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().
		 /

> +	}
> +
> +	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
> + *
> + * 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
> + * returns, for two ports of an LVDS dual-link source, which port shall transmit
> + * the even and odd pixels, based on the requirements of the connected sink.
> + *
> + * The pixel order is determined from the dual-lvds-even-pixels and
> + * dual-lvds-odd-pixels properties in the sink's DT port nodes. If those
> + * properties are not present, or if their usage is not valid, this function
> + * returns -EINVAL.
> + *
> + * 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.
> + *
> + * Return:
> + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
> + *   carries odd pixels
> + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries odd pixels and @port1

This should be DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS, and the second @port1
should be @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

s/port2/@port2/

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> +					  const struct device_node *port2)
> +{
> +	int remote_p1_pt, remote_p2_pt;
> +
> +	if (!port1 || !port2)
> +		return -EINVAL;
> +
> +	remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1);
> +	if (remote_p1_pt < 0)
> +		return remote_p1_pt;
> +
> +	remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2);
> +	if (remote_p2_pt < 0)
> +		return remote_p2_pt;
> +
> +	/*
> +	 * A valid dual-lVDS bus is found when one remote port is marked with
> +	 * "dual-lvds-even-pixels", and the other remote port is marked with
> +	 * "dual-lvds-odd-pixels", bail out if the markers are not right.
> +	 */
> +	if (remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> +		return -EINVAL;
> +
> +	return remote_p1_pt == DRM_OF_LVDS_EVEN ?
> +		DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS :
> +		DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> +}
> +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_pixel_order);
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index ead34ab..8ec7ca6 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -16,6 +16,18 @@ struct drm_panel;
>  struct drm_bridge;
>  struct device_node;
>  
> +/**
> + * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection
> + * @DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: Even pixels are expected to be generated
> + *    from the first port, odd pixels from the second port
> + * @DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: Odd pixels are expected to be generated
> + *    from the first port, even pixels from the second port
> + */
> +enum drm_lvds_dual_link_pixels {
> +	DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 0,
> +	DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 1,
> +};
> +
>  #ifdef CONFIG_OF
>  uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
>  			    struct device_node *port);
> @@ -35,6 +47,8 @@ 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);
>  #else
>  static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
>  					  struct device_node *port)
> @@ -77,6 +91,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
>  {
>  	return -EINVAL;
>  }
> +
> +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> +					  const struct device_node *port2)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
>  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/7] drm: rcar-du: lvds: Improve identification of panels

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:49PM +0000, Fabrizio Castro wrote:
> Dual-LVDS panels are mistakenly identified as bridges, this
> commit replaces the current logic with a call to
> drm_of_find_panel_or_bridge to sort that out.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch extracted from patch:
>   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 45 +++++++++----------------------------
>  1 file changed, 10 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 8c6c172..3cb0a83 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -705,10 +706,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  {
>  	struct device_node *local_output = NULL;
> -	struct device_node *remote_input = NULL;
>  	struct device_node *remote = NULL;
> -	struct device_node *node;
> -	bool is_bridge = false;
>  	int ret = 0;
>  
>  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> @@ -736,45 +734,22 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  		goto done;
>  	}

I think you can remove the calls above this too.
drm_of_find_panel_or_bridge() calls of_graph_get_remote_node(), which in
turn calls of_graph_get_endpoint_by_regs(),
of_graph_get_remote_port_parent() and checks the status of the remote
with of_device_is_available().

>  
> -	remote_input = of_graph_get_remote_endpoint(local_output);
> -
> -	for_each_endpoint_of_node(remote, node) {
> -		if (node != remote_input) {
> -			/*
> -			 * We've found one endpoint other than the input, this
> -			 * must be a bridge.
> -			 */
> -			is_bridge = true;
> -			of_node_put(node);
> -			break;
> -		}
> -	}
> -
> -	if (is_bridge) {
> -		lvds->next_bridge = of_drm_find_bridge(remote);
> -		if (!lvds->next_bridge) {
> -			ret = -EPROBE_DEFER;
> -			goto done;
> -		}
> +	ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> +					  &lvds->panel, &lvds->next_bridge);
> +	if (ret)
> +		goto done;
>  
> -		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> -			lvds->dual_link = lvds->next_bridge->timings
> -					? lvds->next_bridge->timings->dual_link
> -					: false;
> -	} else {
> -		lvds->panel = of_drm_find_panel(remote);
> -		if (IS_ERR(lvds->panel)) {
> -			ret = PTR_ERR(lvds->panel);
> -			goto done;
> -		}
> -	}
> +	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
> +	    lvds->next_bridge)
> +		lvds->dual_link = lvds->next_bridge->timings
> +				? lvds->next_bridge->timings->dual_link
> +				: false;
>  
>  	if (lvds->dual_link)
>  		ret = rcar_lvds_parse_dt_companion(lvds);
>  
>  done:
>  	of_node_put(local_output);
> -	of_node_put(remote_input);
>  	of_node_put(remote);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/7] drm: rcar-du: lvds: Get dual link configuration from DT

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:50PM +0000, Fabrizio Castro wrote:
> For dual-LVDS configurations, it is now possible to mark the
> DT port nodes for the sink with boolean properties (like
> dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers
> know the encoders need to be configured in dual-LVDS mode.
> 
> Rework the implementation of rcar_lvds_parse_dt_companion
> to make use of the DT markers while keeping backward
> compatibility.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch extracted from patch:
>   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3cb0a83..6c1f171 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
>  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  {
>  	const struct of_device_id *match;
> -	struct device_node *companion;
> +	struct device_node *companion, *p0, *p1;

Could you rename p0 and p1 to port0 and port1, and spit them to a
separate line of variable declaration ?

> +	struct rcar_lvds *companion_lvds;
>  	struct device *dev = lvds->dev;
> +	int dual_link;
>  	int ret = 0;
>  
>  	/* Locate the companion LVDS encoder for dual-link operation, if any. */
> @@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  		goto done;
>  	}
>  
> +	/*
> +	 * We need to work out if the sink is expecting us to function in
> +	 * dual-link mode. We do this by looking at the DT port nodes we are
> +	 * connected to, if they are marked as expecting even pixels and
> +	 * odd pixels than we need to enable vertical stripe output.
> +	 */
> +	p0 = of_graph_get_port_by_id(dev->of_node, 1);
> +	p1 = of_graph_get_port_by_id(companion, 1);
> +	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
> +	of_node_put(p0);
> +	of_node_put(p1);
> +	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> +		lvds->dual_link = true;
> +	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
> +		/*
> +		 * Early dual-link bridge specific implementations populate the
> +		 * timings field of drm_bridge, read the dual_link flag off the
> +		 * bridge directly for backward compatibility.
> +		 */
> +		lvds->dual_link = lvds->next_bridge->timings->dual_link;
> +	}
> +
> +	if (!lvds->dual_link) {
> +		dev_dbg(dev, "Single-link configuration detected\n");
> +		goto done;
> +	}
> +
>  	lvds->companion = of_drm_find_bridge(companion);
>  	if (!lvds->companion) {
>  		ret = -EPROBE_DEFER;
>  		goto done;
>  	}
>  
> -	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> +	dev_dbg(dev,
> +		"Dual-link configuration detected (companion encoder %pOF)\n",
> +		companion);
> +
> +	companion_lvds = bridge_to_rcar_lvds(lvds->companion);

Could you move this line after the FIXME comment ?

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	/*
> +	 * FIXME: We should not be messing with the companion encoder private
> +	 * data from the primary encoder, we should rather let the companion
> +	 * encoder work things out on its own. However, the companion encoder
> +	 * doesn't hold a reference to the primary encoder, and
> +	 * drm_of_lvds_get_dual_link_pixel_order needs to be given references
> +	 * to the output ports of both encoders, therefore leave it like this
> +	 * for the time being.
> +	 */
> +	companion_lvds->dual_link = true;
>  
>  done:
>  	of_node_put(companion);
> @@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  	if (ret)
>  		goto done;
>  
> -	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
> -	    lvds->next_bridge)
> -		lvds->dual_link = lvds->next_bridge->timings
> -				? lvds->next_bridge->timings->dual_link
> -				: false;
> -
> -	if (lvds->dual_link)
> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
>  		ret = rcar_lvds_parse_dt_companion(lvds);
>  
>  done:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:51PM +0000, Fabrizio Castro wrote:
> DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels
> can be used to work out if the driver needs to swap even
> and odd pixels around.
> 
> This patch makes use of the return value from function
> drm_of_lvds_get_dual_link_pixel_order to determine if we
> need to swap odd and even pixels around for things to work
> properly.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch extracted from patch:
>   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 6c1f171..cb2147c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -71,6 +71,7 @@ struct rcar_lvds {
>  
>  	struct drm_bridge *companion;
>  	bool dual_link;
> +	bool stripe_swap_data;

Should we merge those two fields in an int dual_link that would be set
to DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS,
DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS or a negative value (or maybe we the
value of enum drm_lvds_dual_link_pixels could be modified so that 0
could mean single link) ?

>  };
>  
>  #define bridge_to_rcar_lvds(b) \
> @@ -441,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>  
>  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> -		/*
> -		 * Configure vertical stripe based on the mode of operation of
> -		 * the connected device.
> -		 */
> -		rcar_lvds_write(lvds, LVDSTRIPE,
> -				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> +		u32 lvdstripe = 0;
> +
> +		if (lvds->dual_link)
> +			/*
> +			 * Configure vertical stripe based on the mode of
> +			 * operation of the connected device.
> +			 *
> +			 * ST_SWAP from LVD1STRIPE is reserved, do not set
> +			 * in the companion LVDS

Maybe "ST_SWAP is reserved for the companion encoder, only set it in the
primary encoder." ?

> +			 */
> +			lvdstripe = LVDSTRIPE_ST_ON |
> +				(lvds->companion && lvds->stripe_swap_data ?
> +				 LVDSTRIPE_ST_SWAP : 0);

To match the coding style of the rest of the driver,

			lvdstripe = LVDSTRIPE_ST_ON
				  | (lvds->companion && lvds->stripe_swap_data ?
				     LVDSTRIPE_ST_SWAP : 0);

Even though not strictly required, could you surround this statement
with { } as it spans quite a few lines with the comment ?

> +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
>  	}
>  
>  	/*
> @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
>  	of_node_put(p0);
>  	of_node_put(p1);
> -	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> +
> +	switch (dual_link) {
> +	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> +		/*
> +		 * By default we generate even pixels from this encoder and odd
> +		 * pixels from the companion encoder, but since p0 is connected
> +		 * to the port expecting ood pixels, and p1 is connected to the
> +		 * port expecting even pixels, we need to swap even and odd
> +		 * pixels around.
> +		 */
> +		lvds->stripe_swap_data = true;
> +		lvds->dual_link = true;
> +		break;
> +	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>  		lvds->dual_link = true;
> -	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
> +		break;
> +	default:
>  		/*
>  		 * Early dual-link bridge specific implementations populate the
>  		 * timings field of drm_bridge, read the dual_link flag off the
>  		 * bridge directly for backward compatibility.
>  		 */
> -		lvds->dual_link = lvds->next_bridge->timings->dual_link;
> +		if (lvds->next_bridge && lvds->next_bridge->timings)
> +			lvds->dual_link = lvds->next_bridge->timings->dual_link;
>  	}
>  
> +

A single blank line is enough.

>  	if (!lvds->dual_link) {
>  		dev_dbg(dev, "Single-link configuration detected\n");
>  		goto done;
> @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  		"Dual-link configuration detected (companion encoder %pOF)\n",
>  		companion);
>  
> +	if (lvds->stripe_swap_data)
> +		dev_dbg(dev, "Data swapping required\n");
> +
>  	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/7] drm: rcar-du: lvds: Fix mode for companion encoder

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:52PM +0000, Fabrizio Castro wrote:
> Primary and companion encoders need to set the same mode for
> things to work properly.
> 
> rcar_lvds_mode_set gets called into for the primary encoder only,
> therefore initialize the companion encoder mode while sorting
> the primary encoder mode out.
> 
> Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Would you mind rebasing this on top of "drm: rcar-du: lvds: Get mode
from state" ?

> ---
> v3->v4:
> * New patch extracted from patch:
>   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index cb2147c..eed5611 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -614,6 +614,18 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
>  	lvds->display_mode = *adjusted_mode;
>  
>  	rcar_lvds_get_lvds_mode(lvds);
> +	if (lvds->companion) {
> +		struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> +							lvds->companion);
> +
> +		/*
> +		 * FIXME: We should not be messing with the companion encoder
> +		 * private data from the primary encoder, but since
> +		 * rcar_lvds_mode_set gets called into for the primary encoder
> +		 * only, we don't have much of a choice for now.
> +		 */
> +		companion_lvds->mode = lvds->mode;
> +	}
>  }
>  
>  static int rcar_lvds_attach(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 6/7] dt-bindings: display: Add idk-2121wr binding

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Fri, Dec 06, 2019 at 04:32:53PM +0000, Fabrizio Castro wrote:
> Add binding for the idk-2121wr LVDS panel from Advantech.
> 
> Some panel-specific documentation can be found here:
> https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * Absorbed patch "dt-bindings: display: Add bindings for LVDS
>   bus-timings"
> * Big restructuring after Rob's and Laurent's comments
> 
> v2->v3:
> * new patch
> ---
>  .../display/panel/advantech,idk-2121wr.yaml        | 128 +++++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> new file mode 100644
> index 0000000..24cd38b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/advantech,idk-2121wr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Advantech IDK-2121WR 21.5" Full-HD dual-LVDS panel
> +
> +maintainers:
> +  - Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> +  - Thierry Reding <thierry.reding@gmail.com>
> +
> +description: |
> +  The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
> +  A dual-LVDS interface is a dual-link connection with even pixels traveling
> +  on one link, and with odd pixels traveling on the other link.
> +
> +  The panel expects odd pixels on the first port, and even pixels on the
> +  second port, therefore the ports must be marked accordingly (with either
> +  dual-lvds-odd-pixels or dual-lvds-even-pixels).
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: advantech,idk-2121wr
> +      - {} # panel-lvds, but not listed here to avoid false select
> +
> +  width-mm:
> +    const: 476
> +
> +  height-mm:
> +    const: 268
> +
> +  data-mapping:
> +    const: vesa-24
> +
> +  ports:
> +    type: object
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      port@0:
> +        type: object
> +        description: The sink for odd pixels.
> +        properties:
> +          reg:
> +            const: 0
> +
> +          dual-lvds-odd-pixels: true
> +
> +        required:
> +          - reg
> +          - dual-lvds-odd-pixels
> +
> +      port@1:
> +        type: object
> +        description: The sink for even pixels.
> +        properties:
> +          reg:
> +            const: 1
> +
> +          dual-lvds-even-pixels: true
> +
> +        required:
> +          - reg
> +          - dual-lvds-even-pixels
> +
> +  panel-timing: true
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - width-mm
> +  - height-mm
> +  - data-mapping
> +  - panel-timing
> +  - ports
> +
> +examples:
> +  - |+
> +    panel-lvds {
> +      compatible = "advantech,idk-2121wr", "panel-lvds";
> +
> +      width-mm = <476>;
> +      height-mm = <268>;
> +
> +      data-mapping = "vesa-24";
> +
> +      panel-timing {
> +        clock-frequency = <148500000>;
> +        hactive = <1920>;
> +        vactive = <1080>;
> +        hsync-len = <44>;
> +        hfront-porch = <88>;
> +        hback-porch = <148>;
> +        vfront-porch = <4>;
> +        vback-porch = <36>;
> +        vsync-len = <5>;
> +      };
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          dual-lvds-odd-pixels;
> +          panel_in0: endpoint {
> +            remote-endpoint = <&lvds0_out>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +          dual-lvds-even-pixels;
> +          panel_in1: endpoint {
> +            remote-endpoint = <&lvds1_out>;
> +          };
> +        };
> +      };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 21:06
> Subject: Re: [PATCH v4 1/7] drm: of: Add drm_of_lvds_get_dual_link_pixel_order
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:48PM +0000, Fabrizio Castro wrote:
> > An LVDS dual-link connection is made of two links, with even
> > pixels transitting on one link, and odd pixels on the other
> > link. The device tree can be used to fully describe dual-link
> > LVDS connections between encoders and bridges/panels.
> > The sink of an LVDS dual-link connection is made of two ports,
> > the corresponding OF graph port nodes can be marked
> > with either dual-lvds-even-pixels or dual-lvds-odd-pixels,
> > and that fully describes an LVDS dual-link connection,
> > including pixel order.
> >
> > drm_of_lvds_get_dual_link_pixel_order is a new helper
> > added by this patch, given the source port nodes it
> > returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS if the source
> > port nodes belong to an LVDS dual-link connection, with even
> > pixels expected to be generated from the first port, and odd
> > pixels expected to be generated from the second port.
> > If the new helper returns DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS,
> > odd pixels are expected to be generated from the first port,
> > and even pixels from the other port.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * The patch had title "drm: Add bus timings helper" in v3
> > * The code has now been moved to drm_of, and has been fully
> >   restructured, thanks to Laurent and Daniel for the comments
> >
> > v2->v3:
> > * new patch
> > ---
> >  drivers/gpu/drm/drm_of.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     |  20 +++++++++
> >  2 files changed, 124 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 0ca5880..c2e9ab7 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -274,3 +274,107 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
> > +
> > +enum drm_of_lvds_pixels {
> > +	DRM_OF_LVDS_EVEN = BIT(0),
> > +	DRM_OF_LVDS_ODD = BIT(1),
> > +};
> > +
> > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > +{
> > +	bool even_pixels =
> > +		of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > +	bool odd_pixels =
> > +		of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > +
> > +	return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> > +	       (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> > +}
> > +
> > +static int drm_of_lvds_get_remote_pixels_type(
> > +			const struct device_node *port_node)
> > +{
> > +	struct device_node *endpoint = NULL;
> > +	int pixels_type = -EPIPE;
> > +
> > +	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)
> 
> You need an of_node_put(endpoint) in the code paths that exit from the
> loop.

Right, thank you for spotting this!

> 
> > +			return -EPIPE;
> > +
> > +		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> > +		of_node_put(remote_port);
> > +		if (!pixels_type)
> > +			pixels_type = current_pt;
> 
> This will never happen as pixels_type is initialized to -EPIPE.
> Replacing the condition with if (pixels_type < 0) should fix it.

I agree

> 
> > +		if (!current_pt || pixels_type != current_pt)
> > +			return -EINVAL;
> 
> I would add a comment to explain this. If I understand the code
> correcty, something along the lines of
> 
> 		/*
> 		 * 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().
> 		 /

I think this will work. Thank you for the suggestion

> 
> > +	}
> > +
> > +	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
> > + *
> > + * 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
> > + * returns, for two ports of an LVDS dual-link source, which port shall transmit
> > + * the even and odd pixels, based on the requirements of the connected sink.
> > + *
> > + * The pixel order is determined from the dual-lvds-even-pixels and
> > + * dual-lvds-odd-pixels properties in the sink's DT port nodes. If those
> > + * properties are not present, or if their usage is not valid, this function
> > + * returns -EINVAL.
> > + *
> > + * 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.
> > + *
> > + * Return:
> > + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and @port2
> > + *   carries odd pixels
> > + * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries odd pixels and @port1
> 
> This should be DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS, and the second @port1
> should be @port2.

And I thought I double checked those... :)

> 
> > + *   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
> 
> s/port2/@port2/

Cheers

Will fix the highlighted issues in v5.

Thanks,
Fab

> 
> With those small issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > + */
> > +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> > +					  const struct device_node *port2)
> > +{
> > +	int remote_p1_pt, remote_p2_pt;
> > +
> > +	if (!port1 || !port2)
> > +		return -EINVAL;
> > +
> > +	remote_p1_pt = drm_of_lvds_get_remote_pixels_type(port1);
> > +	if (remote_p1_pt < 0)
> > +		return remote_p1_pt;
> > +
> > +	remote_p2_pt = drm_of_lvds_get_remote_pixels_type(port2);
> > +	if (remote_p2_pt < 0)
> > +		return remote_p2_pt;
> > +
> > +	/*
> > +	 * A valid dual-lVDS bus is found when one remote port is marked with
> > +	 * "dual-lvds-even-pixels", and the other remote port is marked with
> > +	 * "dual-lvds-odd-pixels", bail out if the markers are not right.
> > +	 */
> > +	if (remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > +		return -EINVAL;
> > +
> > +	return remote_p1_pt == DRM_OF_LVDS_EVEN ?
> > +		DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS :
> > +		DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_pixel_order);
> > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > index ead34ab..8ec7ca6 100644
> > --- a/include/drm/drm_of.h
> > +++ b/include/drm/drm_of.h
> > @@ -16,6 +16,18 @@ struct drm_panel;
> >  struct drm_bridge;
> >  struct device_node;
> >
> > +/**
> > + * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection
> > + * @DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: Even pixels are expected to be generated
> > + *    from the first port, odd pixels from the second port
> > + * @DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: Odd pixels are expected to be generated
> > + *    from the first port, even pixels from the second port
> > + */
> > +enum drm_lvds_dual_link_pixels {
> > +	DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 0,
> > +	DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 1,
> > +};
> > +
> >  #ifdef CONFIG_OF
> >  uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
> >  			    struct device_node *port);
> > @@ -35,6 +47,8 @@ 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);
> >  #else
> >  static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev,
> >  					  struct device_node *port)
> > @@ -77,6 +91,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
> >  {
> >  	return -EINVAL;
> >  }
> > +
> > +int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
> > +					  const struct device_node *port2)
> > +{
> > +	return -EINVAL;
> > +}
> >  #endif
> >
> >  /*
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v4 2/7] drm: rcar-du: lvds: Improve identification of panels

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 December 2019 21:22
> Subject: Re: [PATCH v4 2/7] drm: rcar-du: lvds: Improve identification of panels
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:49PM +0000, Fabrizio Castro wrote:
> > Dual-LVDS panels are mistakenly identified as bridges, this
> > commit replaces the current logic with a call to
> > drm_of_find_panel_or_bridge to sort that out.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * New patch extracted from patch:
> >   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 45 +++++++++----------------------------
> >  1 file changed, 10 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 8c6c172..3cb0a83 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -21,6 +21,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_probe_helper.h>
> >
> > @@ -705,10 +706,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  {
> >  	struct device_node *local_output = NULL;
> > -	struct device_node *remote_input = NULL;
> >  	struct device_node *remote = NULL;
> > -	struct device_node *node;
> > -	bool is_bridge = false;
> >  	int ret = 0;
> >
> >  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > @@ -736,45 +734,22 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  		goto done;
> >  	}
> 
> I think you can remove the calls above this too.
> drm_of_find_panel_or_bridge() calls of_graph_get_remote_node(), which in
> turn calls of_graph_get_endpoint_by_regs(),
> of_graph_get_remote_port_parent() and checks the status of the remote
> with of_device_is_available().

Will take those out in v5

Thanks,
Fab

> 
> >
> > -	remote_input = of_graph_get_remote_endpoint(local_output);
> > -
> > -	for_each_endpoint_of_node(remote, node) {
> > -		if (node != remote_input) {
> > -			/*
> > -			 * We've found one endpoint other than the input, this
> > -			 * must be a bridge.
> > -			 */
> > -			is_bridge = true;
> > -			of_node_put(node);
> > -			break;
> > -		}
> > -	}
> > -
> > -	if (is_bridge) {
> > -		lvds->next_bridge = of_drm_find_bridge(remote);
> > -		if (!lvds->next_bridge) {
> > -			ret = -EPROBE_DEFER;
> > -			goto done;
> > -		}
> > +	ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> > +					  &lvds->panel, &lvds->next_bridge);
> > +	if (ret)
> > +		goto done;
> >
> > -		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > -			lvds->dual_link = lvds->next_bridge->timings
> > -					? lvds->next_bridge->timings->dual_link
> > -					: false;
> > -	} else {
> > -		lvds->panel = of_drm_find_panel(remote);
> > -		if (IS_ERR(lvds->panel)) {
> > -			ret = PTR_ERR(lvds->panel);
> > -			goto done;
> > -		}
> > -	}
> > +	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
> > +	    lvds->next_bridge)
> > +		lvds->dual_link = lvds->next_bridge->timings
> > +				? lvds->next_bridge->timings->dual_link
> > +				: false;
> >
> >  	if (lvds->dual_link)
> >  		ret = rcar_lvds_parse_dt_companion(lvds);
> >
> >  done:
> >  	of_node_put(local_output);
> > -	of_node_put(remote_input);
> >  	of_node_put(remote);
> >
> >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v4 3/7] drm: rcar-du: lvds: Get dual link configuration from DT

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 December 2019 21:30
> Subject: Re: [PATCH v4 3/7] drm: rcar-du: lvds: Get dual link configuration from DT
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:50PM +0000, Fabrizio Castro wrote:
> > For dual-LVDS configurations, it is now possible to mark the
> > DT port nodes for the sink with boolean properties (like
> > dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers
> > know the encoders need to be configured in dual-LVDS mode.
> >
> > Rework the implementation of rcar_lvds_parse_dt_companion
> > to make use of the DT markers while keeping backward
> > compatibility.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * New patch extracted from patch:
> >   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------
> >  1 file changed, 47 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3cb0a83..6c1f171 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> >  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  {
> >  	const struct of_device_id *match;
> > -	struct device_node *companion;
> > +	struct device_node *companion, *p0, *p1;
> 
> Could you rename p0 and p1 to port0 and port1, and spit them to a
> separate line of variable declaration ?

sure

> 
> > +	struct rcar_lvds *companion_lvds;
> >  	struct device *dev = lvds->dev;
> > +	int dual_link;
> >  	int ret = 0;
> >
> >  	/* Locate the companion LVDS encoder for dual-link operation, if any. */
> > @@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  		goto done;
> >  	}
> >
> > +	/*
> > +	 * We need to work out if the sink is expecting us to function in
> > +	 * dual-link mode. We do this by looking at the DT port nodes we are
> > +	 * connected to, if they are marked as expecting even pixels and
> > +	 * odd pixels than we need to enable vertical stripe output.
> > +	 */
> > +	p0 = of_graph_get_port_by_id(dev->of_node, 1);
> > +	p1 = of_graph_get_port_by_id(companion, 1);
> > +	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
> > +	of_node_put(p0);
> > +	of_node_put(p1);
> > +	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> > +		lvds->dual_link = true;
> > +	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
> > +		/*
> > +		 * Early dual-link bridge specific implementations populate the
> > +		 * timings field of drm_bridge, read the dual_link flag off the
> > +		 * bridge directly for backward compatibility.
> > +		 */
> > +		lvds->dual_link = lvds->next_bridge->timings->dual_link;
> > +	}
> > +
> > +	if (!lvds->dual_link) {
> > +		dev_dbg(dev, "Single-link configuration detected\n");
> > +		goto done;
> > +	}
> > +
> >  	lvds->companion = of_drm_find_bridge(companion);
> >  	if (!lvds->companion) {
> >  		ret = -EPROBE_DEFER;
> >  		goto done;
> >  	}
> >
> > -	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> > +	dev_dbg(dev,
> > +		"Dual-link configuration detected (companion encoder %pOF)\n",
> > +		companion);
> > +
> > +	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> 
> Could you move this line after the FIXME comment ?

Will do

Thanks,
Fab

> 
> With these small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> > +	/*
> > +	 * FIXME: We should not be messing with the companion encoder private
> > +	 * data from the primary encoder, we should rather let the companion
> > +	 * encoder work things out on its own. However, the companion encoder
> > +	 * doesn't hold a reference to the primary encoder, and
> > +	 * drm_of_lvds_get_dual_link_pixel_order needs to be given references
> > +	 * to the output ports of both encoders, therefore leave it like this
> > +	 * for the time being.
> > +	 */
> > +	companion_lvds->dual_link = true;
> >
> >  done:
> >  	of_node_put(companion);
> > @@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  	if (ret)
> >  		goto done;
> >
> > -	if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) &&
> > -	    lvds->next_bridge)
> > -		lvds->dual_link = lvds->next_bridge->timings
> > -				? lvds->next_bridge->timings->dual_link
> > -				: false;
> > -
> > -	if (lvds->dual_link)
> > +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> >  		ret = rcar_lvds_parse_dt_companion(lvds);
> >
> >  done:
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 21:41
> Subject: Re: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:51PM +0000, Fabrizio Castro wrote:
> > DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels
> > can be used to work out if the driver needs to swap even
> > and odd pixels around.
> >
> > This patch makes use of the return value from function
> > drm_of_lvds_get_dual_link_pixel_order to determine if we
> > need to swap odd and even pixels around for things to work
> > properly.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * New patch extracted from patch:
> >   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++--------
> >  1 file changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 6c1f171..cb2147c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -71,6 +71,7 @@ struct rcar_lvds {
> >
> >  	struct drm_bridge *companion;
> >  	bool dual_link;
> > +	bool stripe_swap_data;
> 
> Should we merge those two fields in an int dual_link that would be set
> to DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS,
> DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS or a negative value (or maybe we the
> value of enum drm_lvds_dual_link_pixels could be modified so that 0
> could mean single link) ?

I see your point, and I think there is a third option, using definitions local to the
RCAR LVDS driver (via an enum?).
The reason for thinking about a third option is that option 1 could be a bit error prone,
as negative values usually have special meaning. I like option 2, I find it neat, but if
I did that then we would need to agree again on names, definitions, interfaces, etc.,
as the meaning of things will change slightly. Also, we will probably be fine with the
helper and definitions we already agreed on.

I think option 3 will offer a little bit of decoupling between the helper and the driver,
and should have a limited effect on previous uses of dual_link.

I'll make option 3 related changes in v5, if you don't like them then I think we should
try option 2.

> 
> >  };
> >
> >  #define bridge_to_rcar_lvds(b) \
> > @@ -441,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> >  	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> >
> >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > -		/*
> > -		 * Configure vertical stripe based on the mode of operation of
> > -		 * the connected device.
> > -		 */
> > -		rcar_lvds_write(lvds, LVDSTRIPE,
> > -				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> > +		u32 lvdstripe = 0;
> > +
> > +		if (lvds->dual_link)
> > +			/*
> > +			 * Configure vertical stripe based on the mode of
> > +			 * operation of the connected device.
> > +			 *
> > +			 * ST_SWAP from LVD1STRIPE is reserved, do not set
> > +			 * in the companion LVDS
> 
> Maybe "ST_SWAP is reserved for the companion encoder, only set it in the
> primary encoder." ?

sure

> 
> > +			 */
> > +			lvdstripe = LVDSTRIPE_ST_ON |
> > +				(lvds->companion && lvds->stripe_swap_data ?
> > +				 LVDSTRIPE_ST_SWAP : 0);
> 
> To match the coding style of the rest of the driver,

ok

> 
> 			lvdstripe = LVDSTRIPE_ST_ON
> 				  | (lvds->companion && lvds->stripe_swap_data ?
> 				     LVDSTRIPE_ST_SWAP : 0);
> 
> Even though not strictly required, could you surround this statement
> with { } as it spans quite a few lines with the comment ?

Will rework this slightly anyway to make room for option 3.

> 
> > +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> >  	}
> >
> >  	/*
> > @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  	dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1);
> >  	of_node_put(p0);
> >  	of_node_put(p1);
> > -	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> > +
> > +	switch (dual_link) {
> > +	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> > +		/*
> > +		 * By default we generate even pixels from this encoder and odd
> > +		 * pixels from the companion encoder, but since p0 is connected
> > +		 * to the port expecting ood pixels, and p1 is connected to the
> > +		 * port expecting even pixels, we need to swap even and odd
> > +		 * pixels around.
> > +		 */
> > +		lvds->stripe_swap_data = true;
> > +		lvds->dual_link = true;
> > +		break;
> > +	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
> >  		lvds->dual_link = true;
> > -	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
> > +		break;
> > +	default:
> >  		/*
> >  		 * Early dual-link bridge specific implementations populate the
> >  		 * timings field of drm_bridge, read the dual_link flag off the
> >  		 * bridge directly for backward compatibility.
> >  		 */
> > -		lvds->dual_link = lvds->next_bridge->timings->dual_link;
> > +		if (lvds->next_bridge && lvds->next_bridge->timings)
> > +			lvds->dual_link = lvds->next_bridge->timings->dual_link;
> >  	}
> >
> > +
> 
> A single blank line is enough.

Oops

Thanks,
Fab

> 
> >  	if (!lvds->dual_link) {
> >  		dev_dbg(dev, "Single-link configuration detected\n");
> >  		goto done;
> > @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  		"Dual-link configuration detected (companion encoder %pOF)\n",
> >  		companion);
> >
> > +	if (lvds->stripe_swap_data)
> > +		dev_dbg(dev, "Data swapping required\n");
> > +
> >  	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> >
> >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH v4 5/7] drm: rcar-du: lvds: Fix mode for companion encoder

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 21:42
> Subject: Re: [PATCH v4 5/7] drm: rcar-du: lvds: Fix mode for companion encoder
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:52PM +0000, Fabrizio Castro wrote:
> > Primary and companion encoders need to set the same mode for
> > things to work properly.
> >
> > rcar_lvds_mode_set gets called into for the primary encoder only,
> > therefore initialize the companion encoder mode while sorting
> > the primary encoder mode out.
> >
> > Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> Would you mind rebasing this on top of "drm: rcar-du: lvds: Get mode
> from state" ?

It looks like dropping this patch and rebasing on top of your v2 for
"drm: rcar-du: lvds: Get mode from state" does the trick now.

Will put a dependency to your patch in v5.

Cheers,
Fab

> 
> > ---
> > v3->v4:
> > * New patch extracted from patch:
> >   "drm: rcar-du: lvds: Add dual-LVDS panels support"
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index cb2147c..eed5611 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -614,6 +614,18 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> >  	lvds->display_mode = *adjusted_mode;
> >
> >  	rcar_lvds_get_lvds_mode(lvds);
> > +	if (lvds->companion) {
> > +		struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> > +							lvds->companion);
> > +
> > +		/*
> > +		 * FIXME: We should not be messing with the companion encoder
> > +		 * private data from the primary encoder, but since
> > +		 * rcar_lvds_mode_set gets called into for the primary encoder
> > +		 * only, we don't have much of a choice for now.
> > +		 */
> > +		companion_lvds->mode = lvds->mode;
> > +	}
> >  }
> >
> >  static int rcar_lvds_attach(struct drm_bridge *bridge)
> 
> --
> Regards,
> 
> Laurent Pinchart