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
V3 approaches the problem in a completely different way, we now
have two new properties to mark the ports in the DT as receiving
even pixels and odd pixels: dual-lvds-even-pixels and dual-lvds-odd-pixels,
which means device drivers should not use bridge specific or panel
specific dual_link flags. Also, in this case the DT describes the
connection fully.
In order for the solution to be generic, I have exported a new helper
(drm_of_lvds_get_dual_link_configuration) to walk the device tree,
and figure out if the connection is dual-LVDS. The same helper gives
information about the configuration of the connection. If Px is connected
to a port expecting even pixels and Py is connected to a port expecting
odd pixels, then the helper returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS
(like in the example below), otherwise it returns
DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS.
-------- dual-lvds-even-pixels --------
| |---- ----| |
| | Px |---------------------->| Pn | |
| |---- ----| |
| SOURCE | dual-lvds-odd-pixels | SINK |
| |---- ----| |
| | Py |---------------------->| Pm | |
| |---- ----| |
-------- --------
The device driver for the encoder then will work out if with the current
wiring the pixels need swapping or not.
The same solution works for both panels and bridges.
Since the DT describes the connection fully, driver
drivers/gpu/drm/panel/panel-lvds.c works out-of-the-box, no changes
required, however, this implementation opens up a problem with the
dt-bindings.
Driver drivers/gpu/drm/panel/panel-lvds.c can still be pleased by
a port node, but also by a ports node.
I have created Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
with the idea of including it from panels and bridges dt-bindings
supporting dual-LVDS (and of course the dt-bindings for the specific
devices should say which port should be marked as what), but file
Documentation/devicetree/bindings/display/panel/lvds.yaml formally
requires property "port", while with this implementation it should require
OneOf "port" and "ports", and unfortunately I can't seem to find a neat way
aroud that, other than creating a new compatible string
(e.g. "panel-dual-lvds"), a new dt-binding document for it, and of course adding
support for the new compatible string to drivers/gpu/drm/panel/panel-lvds.c.
As a result, this series is missing (at least) a patch necessary to fully
document the new implementation within
Documentation/devicetree/bindings/display/panel/lvds.yaml
Rob, do you have any suggestions? Do you think this idea works ok from a
documentation point of view? By the way, I don't really know what I am doing
with the yaml dt-bindings, I hope you won't be horrified by this series :-P
I hope I was able to deliver the concept clearly, if not please just ask.
Comments are very much appreciated.
Thanks,
Fab
Fabrizio Castro (8):
dt-bindings: display: Add bindings for LVDS bus-timings
dt-bindings: display: Add idk-2121wr binding
drm: Add bus timings helper
drm: rcar-du: lvds: Add dual-LVDS panels support
drm: bridge: thc63: Do not report input bus mode through bridge
timings
arm64: dts: renesas: Add EK874 board with idk-2121wr display support
[HACK] arm64: dts: renesas: draak: Enable LVDS
[HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation
.../bindings/display/bus-timings/lvds.yaml | 38 +++++++
.../display/panel/advantech,idk-2121wr.yaml | 90 ++++++++++++++++
arch/arm64/boot/dts/renesas/Makefile | 3 +-
.../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 116 +++++++++++++++++++++
arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 21 +++-
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 26 +++--
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/bridge/thc63lvd1024.c | 9 +-
drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++
drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++--------
include/drm/drm_bridge.h | 8 --
include/drm/drm_bus_timings.h | 21 ++++
12 files changed, 473 insertions(+), 69 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
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
create mode 100644 drivers/gpu/drm/drm_bus_timings.c
create mode 100644 include/drm/drm_bus_timings.h
--
2.7.4
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 <[email protected]>
---
v2->v3:
* new patch
---
.../display/panel/advantech,idk-2121wr.yaml | 90 ++++++++++++++++++++++
1 file changed, 90 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..b2ccdc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
@@ -0,0 +1,90 @@
+# 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 <[email protected]>
+ - Thierry Reding <[email protected]>
+
+description: |
+ The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
+
+ The panels expects odd pixels from the first port, and even pixels from
+ the second port, therefore the ports must be marked accordingly.
+
+allOf:
+ - $ref: lvds.yaml#
+ - $ref: ../bus-timings/lvds.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: advantech,idk-2121wr
+ - {} # panel-lvds, but not listed here to avoid false select
+
+ data-mapping:
+ const: vesa-24
+
+ width-mm:
+ const: 476
+
+ height-mm:
+ const: 268
+
+ panel-timing: true
+ ports: true
+
+additionalProperties: false
+
+required:
+ - compatible
+
+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
No need to report the input bus mode through bridge timings
anymore, that's now done through the DT, as specified by the
dt-bindings.
Signed-off-by: Fabrizio Castro <[email protected]>
---
v2->v3:
* new patch
---
drivers/gpu/drm/bridge/thc63lvd1024.c | 9 ++++-----
include/drm/drm_bridge.h | 8 --------
2 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 3d74129b..730f682 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -34,7 +34,7 @@ struct thc63_dev {
struct drm_bridge bridge;
struct drm_bridge *next;
- struct drm_bridge_timings timings;
+ bool dual_link;
};
static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
@@ -62,7 +62,7 @@ static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
* isn't supported by the driver yet, simply derive the limits from the
* input mode.
*/
- if (thc63->timings.dual_link) {
+ if (thc63->dual_link) {
min_freq = 40000;
max_freq = 150000;
} else {
@@ -157,13 +157,13 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
if (remote) {
if (of_device_is_available(remote))
- thc63->timings.dual_link = true;
+ thc63->dual_link = true;
of_node_put(remote);
}
}
dev_dbg(thc63->dev, "operating in %s-link mode\n",
- thc63->timings.dual_link ? "dual" : "single");
+ thc63->dual_link ? "dual" : "single");
return 0;
}
@@ -221,7 +221,6 @@ static int thc63_probe(struct platform_device *pdev)
thc63->bridge.driver_private = thc63;
thc63->bridge.of_node = pdev->dev.of_node;
thc63->bridge.funcs = &thc63_bridge_func;
- thc63->bridge.timings = &thc63->timings;
drm_bridge_add(&thc63->bridge);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7616f65..3228018 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -362,14 +362,6 @@ struct drm_bridge_timings {
* input signal after the clock edge.
*/
u32 hold_time_ps;
- /**
- * @dual_link:
- *
- * True if the bus operates in dual-link mode. The exact meaning is
- * dependent on the bus type. For LVDS buses, this indicates that even-
- * and odd-numbered pixels are received on separate links.
- */
- bool dual_link;
};
/**
--
2.7.4
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 <[email protected]>
---
v1->v2:
* Added comment for lvds-connector-en-gpio
* Renamed &lvds0_panel_in to panel_in0
* Renamed &lvds1_panel_in to panel_in1
v2->v3:
* removed renesas,swap-data property
* added dual-lvds-odd-pixels and dual-lvds-even-pixels properties
Geert,
no need to review this patch unless they like the idea behind this
series.
Thanks,
Fab
---
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 42b74c2..ce48478 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -1,7 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m.dtb
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m-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 = <®_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
Helper to provide bus timing information.
Signed-off-by: Fabrizio Castro <[email protected]>
---
v2->v3:
* new patch
---
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
include/drm/drm_bus_timings.h | 21 +++++++++
3 files changed, 120 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_bus_timings.c
create mode 100644 include/drm/drm_bus_timings.h
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9f0d2ee..a270063 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
drm_plane.o drm_color_mgmt.o drm_print.o \
drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
- drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
+ drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
+ drm_bus_timings.o
drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
new file mode 100644
index 0000000..e2ecd22
--- /dev/null
+++ b/drivers/gpu/drm/drm_bus_timings.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <drm/drm_bus_timings.h>
+#include <linux/errno.h>
+#include <linux/of_graph.h>
+#include <linux/of.h>
+#include <linux/types.h>
+
+#define DRM_OF_LVDS_ODD 1
+#define DRM_OF_LVDS_EVEN 2
+
+static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
+{
+ bool even_pixels, odd_pixels;
+
+ even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
+ odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
+ return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
+}
+
+/**
+ * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
+ * @p1: device tree node corresponding to the first port of the source
+ * @p2: device tree node corresponding to the second port of the source
+ *
+ * An LVDS dual-link bus is made of two connections, even pixels transit on one
+ * connection, and odd pixels transit on the other connection.
+ * This function walks the DT (from the source ports to the sink ports) looking
+ * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
+ * ports of the sink device(s). If such a bus is found, this function returns
+ * its configuration (either p1 connected to the even pixels port and p2
+ * connected to the odd pixels port, or p1 connected to the odd pixels port and
+ * p2 connected to the even pixels port).
+ *
+ * Return: A code describing the bus configuration when a valid dual-LVDS bus is
+ * found, or an error code when no valid dual-LVDS bus is found
+ *
+ * Possible codes for the bus configuration are:
+ *
+ * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
+ * port and p2 is connected to the odd pixels port
+ * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
+ * port and p2 is connected to the even pixels port
+ *
+ */
+int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
+ const struct device_node *p2)
+{
+ struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
+ struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
+ struct device_node *ep1 = NULL, *ep2 = NULL;
+ u32 reg_p1, reg_p2;
+ int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
+
+ if (!p1 || !p2)
+ return ret;
+ if (of_property_read_u32(p1, "reg", ®_p1) ||
+ of_property_read_u32(p2, "reg", ®_p2))
+ return ret;
+ parent_p1 = of_get_parent(p1);
+ parent_p2 = of_get_parent(p2);
+ if (!parent_p1 || !parent_p2)
+ goto done;
+ ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
+ ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
+ if (!ep1 || !ep2)
+ goto done;
+ remote_p1 = of_graph_get_remote_port(ep1);
+ remote_p2 = of_graph_get_remote_port(ep2);
+ if (!remote_p1 || !remote_p2)
+ goto done;
+ remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
+ remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
+ /*
+ * 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 ||
+ remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
+ goto done;
+ if (remote_p1_pt == DRM_OF_LVDS_EVEN)
+ /* The sink expects even pixels through the first port */
+ ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
+ else
+ /* The sink expects odd pixels through the first port */
+ ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
+
+done:
+ of_node_put(ep1);
+ of_node_put(ep2);
+ of_node_put(parent_p1);
+ of_node_put(parent_p2);
+ of_node_put(remote_p1);
+ of_node_put(remote_p2);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
new file mode 100644
index 0000000..db8a385
--- /dev/null
+++ b/include/drm/drm_bus_timings.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DRM_BUS_TIMINGS__
+#define __DRM_BUS_TIMINGS__
+
+struct device_node;
+
+#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
+#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
+
+#ifdef CONFIG_OF
+int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
+ const struct device_node *p2);
+#else
+int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
+ const struct device_node *p2)
+{
+ return -EINVAL;
+}
+#endif
+
+#endif /* __DRM_BUS_TIMINGS__ */
--
2.7.4
The driver doesn't support dual-link LVDS displays, and the way
it identifies bridges won't allow for dual-LVDS displays to be
connected. Also, it's not possible to swap even and odd pixels
around in case the wiring isn't taking advantage of the default
hardware configuration. Further more, the "mode" of the companion
encoder should be same as the mode of the primary encoder.
Rework the driver to improve all of the above, so that it can
support dual-LVDS displays.
Signed-off-by: Fabrizio Castro <[email protected]>
---
v2->v3:
* reworked to take advantange of the new dt-bindings
* squashed in the patche for fixing the companion's mode
Laurent,
unfortunately the best way to get the companion encoder to use
the same mode as the primary encoder is setting the mode directly
without calling into rcar_lvds_mode_set for the companion encoder,
as the below test fails for the companion encoder in
rcar_lvds_get_lvds_mode:
if (!info->num_bus_formats || !info->bus_formats)
Anyhow, setting the mode for the companion encoder doesn't seem
to be mandary according to the experiments I have been running,
but the HW User's Manual doesn't really say much about this,
therefore I think the safest option is still to set the mode for
the companion encoder.
Thanks,
Fab
---
drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
1 file changed, 65 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3fe0b86..dfec5e7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -20,6 +20,8 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
+#include <drm/drm_bus_timings.h>
+#include <drm/drm_of.h>
#include <drm/drm_panel.h>
#include <drm/drm_probe_helper.h>
@@ -69,6 +71,7 @@ struct rcar_lvds {
struct drm_bridge *companion;
bool dual_link;
+ bool stripe_swap_data;
};
#define bridge_to_rcar_lvds(b) \
@@ -439,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);
}
/*
@@ -603,6 +614,11 @@ 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);
+ companion_lvds->mode = lvds->mode;
+ }
}
static int rcar_lvds_attach(struct drm_bridge *bridge)
@@ -667,9 +683,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 = NULL, *p1 = NULL;
struct device *dev = lvds->dev;
- int ret = 0;
+ struct rcar_lvds *companion_lvds;
+ int ret = 0, dual_link;
/* Locate the companion LVDS encoder for dual-link operation, if any. */
companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
@@ -687,16 +704,50 @@ 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_configuration(p0, p1);
+ if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
+ dev_dbg(dev, "Dual-link configuration detected\n");
+ lvds->dual_link = true;
+ } else {
+ /* dual-link mode is not required */
+ dev_dbg(dev, "Single-link configuration detected\n");
+ goto done;
+ }
+ /*
+ * We may need to swap even and odd pixels around in case the wiring
+ * doesn't match the default configuration.
+ * By default we generate even pixels from this encoder and odd pixels
+ * from the companion encoder, but if p0 is connected to the port
+ * expecting ood pixels, and p1 is connected to the port expecting even
+ * pixels, then we need to swap even and odd pixels around
+ */
+ if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
+ dev_dbg(dev, "Data swapping required\n");
+ lvds->stripe_swap_data = true;
+ }
+
lvds->companion = of_drm_find_bridge(companion);
if (!lvds->companion) {
ret = -EPROBE_DEFER;
goto done;
}
+ companion_lvds = bridge_to_rcar_lvds(lvds->companion);
+ companion_lvds->dual_link = lvds->dual_link;
dev_dbg(dev, "Found companion encoder %pOF\n", companion);
done:
of_node_put(companion);
+ of_node_put(p0);
+ of_node_put(p1);
return ret;
}
@@ -704,10 +755,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);
@@ -735,45 +783,17 @@ 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;
- }
-
- 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;
- }
+ ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
+ &lvds->panel, &lvds->next_bridge);
+ if (ret) {
+ ret = -EPROBE_DEFER;
+ goto done;
}
-
- if (lvds->dual_link)
+ if (lvds->info->quirks & RCAR_LVDS_QUIRK_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
Enable and connect the second LVDS encoder to the second LVDS input of
the THC63LVD1024 for dual-link LVDS operation. This requires changing
the default settings of SW45 and SW47 to OFF and ON respectively.
This patch is based on Laurent's dual-LVDS work:
https://patchwork.kernel.org/patch/10965045/
Signed-off-by: Fabrizio Castro <[email protected]>
---
v2->v3:
* new patch
Geert,
no need to review this patch unless they like the idea behind this
series.
Thanks,
Fab
---
arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
index b38f9d4..38b9c5a 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
@@ -87,11 +87,20 @@
port@0 {
reg = <0>;
- thc63lvd1024_in: endpoint {
+ dual-lvds-even-pixels;
+ thc63lvd1024_in0: endpoint {
remote-endpoint = <&lvds0_out>;
};
};
+ port@1 {
+ reg = <1>;
+ dual-lvds-odd-pixels;
+ thc63lvd1024_in1: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+
port@2 {
reg = <2>;
thc63lvd1024_out: endpoint {
@@ -489,7 +498,7 @@
ports {
port@1 {
lvds0_out: endpoint {
- remote-endpoint = <&thc63lvd1024_in>;
+ remote-endpoint = <&thc63lvd1024_in0>;
};
};
};
@@ -507,6 +516,14 @@
<&x13_clk>,
<&extal_clk>;
clock-names = "fck", "dclkin.0", "extal";
+
+ ports {
+ port@1 {
+ lvds1_out: endpoint {
+ remote-endpoint = <&thc63lvd1024_in1>;
+ };
+ };
+ };
};
&ohci0 {
--
2.7.4
Enable and connect the second LVDS encoder to the second LVDS input of
the THC63LVD1024 for dual-link LVDS operation. This requires changing
the default settings of SW45 and SW47 to OFF and ON respectively.
This patch is based on Laurent's dual-LVDS work:
https://patchwork.kernel.org/patch/10965045/
Signed-off-by: Fabrizio Castro <[email protected]>
---
v2->v3:
* new patch
Geert,
no need to review this patch unless they like the idea behind this
series.
Thanks,
Fab
---
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index 67634cb..b4b8cde 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -77,11 +77,20 @@
port@0 {
reg = <0>;
- thc63lvd1024_in: endpoint {
+ dual-lvds-even-pixels;
+ thc63lvd1024_in0: endpoint {
remote-endpoint = <&lvds0_out>;
};
};
+ port@1 {
+ reg = <1>;
+ dual-lvds-odd-pixels;
+ thc63lvd1024_in1: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+
port@2 {
reg = <2>;
thc63lvd1024_out: endpoint {
@@ -368,24 +377,27 @@
ports {
port@1 {
lvds0_out: endpoint {
- remote-endpoint = <&thc63lvd1024_in>;
+ remote-endpoint = <&thc63lvd1024_in0>;
};
};
};
};
&lvds1 {
- /*
- * Even though the LVDS1 output is not connected, the encoder must be
- * enabled to supply a pixel clock to the DU for the DPAD output when
- * LVDS0 is in use.
- */
status = "okay";
clocks = <&cpg CPG_MOD 727>,
<&x12_clk>,
<&extal_clk>;
clock-names = "fck", "dclkin.0", "extal";
+
+ ports {
+ port@1 {
+ lvds1_out: endpoint {
+ remote-endpoint = <&thc63lvd1024_in1>;
+ };
+ };
+ };
};
&ohci0 {
--
2.7.4
On Wed, Aug 28, 2019 at 1:36 PM Fabrizio Castro
<[email protected]> wrote:
>
> 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
>
> V3 approaches the problem in a completely different way, we now
> have two new properties to mark the ports in the DT as receiving
> even pixels and odd pixels: dual-lvds-even-pixels and dual-lvds-odd-pixels,
> which means device drivers should not use bridge specific or panel
> specific dual_link flags. Also, in this case the DT describes the
> connection fully.
>
> In order for the solution to be generic, I have exported a new helper
> (drm_of_lvds_get_dual_link_configuration) to walk the device tree,
> and figure out if the connection is dual-LVDS. The same helper gives
> information about the configuration of the connection. If Px is connected
> to a port expecting even pixels and Py is connected to a port expecting
> odd pixels, then the helper returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS
> (like in the example below), otherwise it returns
> DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS.
>
>
> -------- dual-lvds-even-pixels --------
> | |---- ----| |
> | | Px |---------------------->| Pn | |
> | |---- ----| |
> | SOURCE | dual-lvds-odd-pixels | SINK |
> | |---- ----| |
> | | Py |---------------------->| Pm | |
> | |---- ----| |
> -------- --------
>
> The device driver for the encoder then will work out if with the current
> wiring the pixels need swapping or not.
>
> The same solution works for both panels and bridges.
>
> Since the DT describes the connection fully, driver
> drivers/gpu/drm/panel/panel-lvds.c works out-of-the-box, no changes
> required, however, this implementation opens up a problem with the
> dt-bindings.
> Driver drivers/gpu/drm/panel/panel-lvds.c can still be pleased by
> a port node, but also by a ports node.
> I have created Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
> with the idea of including it from panels and bridges dt-bindings
> supporting dual-LVDS (and of course the dt-bindings for the specific
> devices should say which port should be marked as what), but file
> Documentation/devicetree/bindings/display/panel/lvds.yaml formally
> requires property "port", while with this implementation it should require
> OneOf "port" and "ports", and unfortunately I can't seem to find a neat way
> aroud that, other than creating a new compatible string
Just add 'ports' and drop 'port' from being required in the common
binding. Then it is up to the panel specific bindings to define which
one is required. Or we just leave it to allow either form which the
graph code can handle.
We could have this in the common binding:
oneOf:
- required: [ports]
- required: [port]
Rob
Hi Rob,
Thank you for your feedback!
> From: Rob Herring <[email protected]>
> Sent: 29 August 2019 16:27
> To: Fabrizio Castro <[email protected]>
> Subject: Re: [PATCH v3 0/8] Add dual-LVDS panel support to EK874
>
> On Wed, Aug 28, 2019 at 1:36 PM Fabrizio Castro
> <[email protected]> wrote:
> >
> > 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
> >
> > V3 approaches the problem in a completely different way, we now
> > have two new properties to mark the ports in the DT as receiving
> > even pixels and odd pixels: dual-lvds-even-pixels and dual-lvds-odd-pixels,
> > which means device drivers should not use bridge specific or panel
> > specific dual_link flags. Also, in this case the DT describes the
> > connection fully.
> >
> > In order for the solution to be generic, I have exported a new helper
> > (drm_of_lvds_get_dual_link_configuration) to walk the device tree,
> > and figure out if the connection is dual-LVDS. The same helper gives
> > information about the configuration of the connection. If Px is connected
> > to a port expecting even pixels and Py is connected to a port expecting
> > odd pixels, then the helper returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS
> > (like in the example below), otherwise it returns
> > DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS.
> >
> >
> > -------- dual-lvds-even-pixels --------
> > | |---- ----| |
> > | | Px |---------------------->| Pn | |
> > | |---- ----| |
> > | SOURCE | dual-lvds-odd-pixels | SINK |
> > | |---- ----| |
> > | | Py |---------------------->| Pm | |
> > | |---- ----| |
> > -------- --------
> >
> > The device driver for the encoder then will work out if with the current
> > wiring the pixels need swapping or not.
> >
> > The same solution works for both panels and bridges.
> >
> > Since the DT describes the connection fully, driver
> > drivers/gpu/drm/panel/panel-lvds.c works out-of-the-box, no changes
> > required, however, this implementation opens up a problem with the
> > dt-bindings.
> > Driver drivers/gpu/drm/panel/panel-lvds.c can still be pleased by
> > a port node, but also by a ports node.
> > I have created Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
> > with the idea of including it from panels and bridges dt-bindings
> > supporting dual-LVDS (and of course the dt-bindings for the specific
> > devices should say which port should be marked as what), but file
> > Documentation/devicetree/bindings/display/panel/lvds.yaml formally
> > requires property "port", while with this implementation it should require
> > OneOf "port" and "ports", and unfortunately I can't seem to find a neat way
> > aroud that, other than creating a new compatible string
>
> Just add 'ports' and drop 'port' from being required in the common
> binding. Then it is up to the panel specific bindings to define which
> one is required. Or we just leave it to allow either form which the
> graph code can handle.
>
> We could have this in the common binding:
>
> oneOf:
> - required: [ports]
> - required: [port]
Thank you for Rob for looking into this. I will wait for a feedback from Laurent
on the code before sending out v4.
Thanks,
Fab
>
> Rob
Hi Laurent,
Did you have any time to look into this series?
Thanks,
Fab
> From: Fabrizio Castro <[email protected]>
> Sent: 28 August 2019 19:37
> Subject: [PATCH v3 0/8] Add dual-LVDS panel support to EK874
>
> 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
>
> V3 approaches the problem in a completely different way, we now
> have two new properties to mark the ports in the DT as receiving
> even pixels and odd pixels: dual-lvds-even-pixels and dual-lvds-odd-pixels,
> which means device drivers should not use bridge specific or panel
> specific dual_link flags. Also, in this case the DT describes the
> connection fully.
>
> In order for the solution to be generic, I have exported a new helper
> (drm_of_lvds_get_dual_link_configuration) to walk the device tree,
> and figure out if the connection is dual-LVDS. The same helper gives
> information about the configuration of the connection. If Px is connected
> to a port expecting even pixels and Py is connected to a port expecting
> odd pixels, then the helper returns DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS
> (like in the example below), otherwise it returns
> DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS.
>
>
> -------- dual-lvds-even-pixels --------
> | |---- ----| |
> | | Px |---------------------->| Pn | |
> | |---- ----| |
> | SOURCE | dual-lvds-odd-pixels | SINK |
> | |---- ----| |
> | | Py |---------------------->| Pm | |
> | |---- ----| |
> -------- --------
>
> The device driver for the encoder then will work out if with the current
> wiring the pixels need swapping or not.
>
> The same solution works for both panels and bridges.
>
> Since the DT describes the connection fully, driver
> drivers/gpu/drm/panel/panel-lvds.c works out-of-the-box, no changes
> required, however, this implementation opens up a problem with the
> dt-bindings.
> Driver drivers/gpu/drm/panel/panel-lvds.c can still be pleased by
> a port node, but also by a ports node.
> I have created Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
> with the idea of including it from panels and bridges dt-bindings
> supporting dual-LVDS (and of course the dt-bindings for the specific
> devices should say which port should be marked as what), but file
> Documentation/devicetree/bindings/display/panel/lvds.yaml formally
> requires property "port", while with this implementation it should require
> OneOf "port" and "ports", and unfortunately I can't seem to find a neat way
> aroud that, other than creating a new compatible string
> (e.g. "panel-dual-lvds"), a new dt-binding document for it, and of course adding
> support for the new compatible string to drivers/gpu/drm/panel/panel-lvds.c.
> As a result, this series is missing (at least) a patch necessary to fully
> document the new implementation within
> Documentation/devicetree/bindings/display/panel/lvds.yaml
>
> Rob, do you have any suggestions? Do you think this idea works ok from a
> documentation point of view? By the way, I don't really know what I am doing
> with the yaml dt-bindings, I hope you won't be horrified by this series :-P
>
> I hope I was able to deliver the concept clearly, if not please just ask.
>
> Comments are very much appreciated.
>
> Thanks,
> Fab
>
> Fabrizio Castro (8):
> dt-bindings: display: Add bindings for LVDS bus-timings
> dt-bindings: display: Add idk-2121wr binding
> drm: Add bus timings helper
> drm: rcar-du: lvds: Add dual-LVDS panels support
> drm: bridge: thc63: Do not report input bus mode through bridge
> timings
> arm64: dts: renesas: Add EK874 board with idk-2121wr display support
> [HACK] arm64: dts: renesas: draak: Enable LVDS
> [HACK] arm64: dts: renesas: draak: Enable LVDS dual-link operation
>
> .../bindings/display/bus-timings/lvds.yaml | 38 +++++++
> .../display/panel/advantech,idk-2121wr.yaml | 90 ++++++++++++++++
> arch/arm64/boot/dts/renesas/Makefile | 3 +-
> .../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 116 +++++++++++++++++++++
> arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 21 +++-
> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 26 +++--
> drivers/gpu/drm/Makefile | 3 +-
> drivers/gpu/drm/bridge/thc63lvd1024.c | 9 +-
> drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++--------
> include/drm/drm_bridge.h | 8 --
> include/drm/drm_bus_timings.h | 21 ++++
> 12 files changed, 473 insertions(+), 69 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bus-timings/lvds.yaml
> 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
> create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> create mode 100644 include/drm/drm_bus_timings.h
>
> --
> 2.7.4
Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:36PM +0100, 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 <[email protected]>
>
> ---
> v2->v3:
> * new patch
> ---
> .../display/panel/advantech,idk-2121wr.yaml | 90 ++++++++++++++++++++++
> 1 file changed, 90 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..b2ccdc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> @@ -0,0 +1,90 @@
> +# 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 <[email protected]>
> + - Thierry Reding <[email protected]>
> +
> +description: |
> + The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
> +
> + The panels expects odd pixels from the first port, and even pixels from
s/panels/panel/
Maybe s/from the/on the/g ?
> + the second port, therefore the ports must be marked accordingly.
> +
> +allOf:
> + - $ref: lvds.yaml#
> + - $ref: ../bus-timings/lvds.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - const: advantech,idk-2121wr
> + - {} # panel-lvds, but not listed here to avoid false select
> +
> + data-mapping:
> + const: vesa-24
> +
> + width-mm:
> + const: 476
> +
> + height-mm:
> + const: 268
> +
> + panel-timing: true
> + ports: true
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
Shouldn't data-mapping, width-mm, height-mm and ports be required too ?
As you mentioned in the cover letter, validating ports, port and the new
dual-lvds-*-pixels properties would be nice. I'm not YAML schema
specialist, so I'm fine with a best effort approach here, but as far as
I understand Rob proposed a way forward, could you try it ?
Apart from that, the bindings look sne to me, so
Reviewed-by: Laurent Pinchart <[email protected]>
once the above issues get addressed.
> +
> +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
Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> Helper to provide bus timing information.
You may want to expand this a bit. And actually fix it too, as the
helper you introduce isn't related to timings (same for the subject
line).
> Signed-off-by: Fabrizio Castro <[email protected]>
>
> ---
> v2->v3:
> * new patch
> ---
> drivers/gpu/drm/Makefile | 3 +-
> drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> include/drm/drm_bus_timings.h | 21 +++++++++
> 3 files changed, 120 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> create mode 100644 include/drm/drm_bus_timings.h
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f0d2ee..a270063 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
> drm_plane.o drm_color_mgmt.o drm_print.o \
> drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> - drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> + drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> + drm_bus_timings.o
>
> drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> new file mode 100644
> index 0000000..e2ecd22
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_bus_timings.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <drm/drm_bus_timings.h>
> +#include <linux/errno.h>
> +#include <linux/of_graph.h>
> +#include <linux/of.h>
> +#include <linux/types.h>
> +
> +#define DRM_OF_LVDS_ODD 1
> +#define DRM_OF_LVDS_EVEN 2
> +
> +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> +{
> + bool even_pixels, odd_pixels;
> +
> + even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> + odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> + return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
s/ / /
But I would make these bitflags.
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)
{
bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
(odd_pixels ? DRM_OF_LVDS_ODD : 0);
}
> +}
> +
> +/**
> + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
Should we name this drm_of_lvds_get_dual_link_pixel_order to better
reflect its purpose ?
> + * @p1: device tree node corresponding to the first port of the source
> + * @p2: device tree node corresponding to the second port of the source
Maybe port1 and port2 to make this more explicit ?
> + *
> + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> + * connection, and odd pixels transit on the other connection.
To match the DT bindings documentation, I would recommand
"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 walks the DT (from the source ports to the sink ports) looking
> + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> + * ports of the sink device(s). If such a bus is found, this function returns
> + * its configuration (either p1 connected to the even pixels port and p2
> + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> + * p2 connected to the even pixels port).
"walking the DT" sounds like the function goes through the whole graph.
How about the following ?
/**
* 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 off 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.
*
* @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
*/
We could also add -EPIPE as a return code for the case where port1 or
port2 are not connected.
> + *
> + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> + * found, or an error code when no valid dual-LVDS bus is found
> + *
> + * Possible codes for the bus configuration are:
> + *
> + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> + * port and p2 is connected to the odd pixels port
> + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> + * port and p2 is connected to the even pixels port
> + *
> + */
> +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> + const struct device_node *p2)
> +{
> + struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> + struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
There's no need to initialize those two variables.
> + struct device_node *ep1 = NULL, *ep2 = NULL;
> + u32 reg_p1, reg_p2;
> + int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
Please split this last line, as it otherwise hides the initialization of
ret in the middle.
> +
> + if (!p1 || !p2)
> + return ret;
You can return -EINVAL directly.
> + if (of_property_read_u32(p1, "reg", ®_p1) ||
> + of_property_read_u32(p2, "reg", ®_p2))
> + return ret;
Same here.
> + parent_p1 = of_get_parent(p1);
> + parent_p2 = of_get_parent(p2);
> + if (!parent_p1 || !parent_p2)
> + goto done;
> + ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> + ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> + if (!ep1 || !ep2)
> + goto done;
If you only support the first endpoint, this should be mentioned in the
documentation. Alternatively you could pass the endpoint nodes instead
of the port nodes, or you could pass the endpoint number.
It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
you already have the port nodes. How about adding the following helper
function ?
struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
{
struct device_node *endpoint = NULL;
for_each_child_of_node(port, endpoint) {
u32 id;
if (!of_node_name_eq(endpoint, "endpoint") ||
continue;
if (reg == -1)
return endpoint;
if (of_property_read_u32(node, "reg", &id) < 0)
continue;
if (reg == id)
return endpoint;
}
return NULL;
}
If you're concerned that adding a core helper would delay this patch
series, you could add it as a local helper, and move it to of_graph.h in
a second step.
> + remote_p1 = of_graph_get_remote_port(ep1);
> + remote_p2 = of_graph_get_remote_port(ep2);
> + if (!remote_p1 || !remote_p2)
> + goto done;
> + remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> + remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> + /*
> + * 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 ||
> + remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> + goto done;
> + if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> + /* The sink expects even pixels through the first port */
> + ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> + else
> + /* The sink expects odd pixels through the first port */
> + ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> +
> +done:
> + of_node_put(ep1);
> + of_node_put(ep2);
> + of_node_put(parent_p1);
> + of_node_put(parent_p2);
> + of_node_put(remote_p1);
> + of_node_put(remote_p2);
> + return ret;
This is heavy, I would add blank lines to make the code easier to read.
> +}
> +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> new file mode 100644
> index 0000000..db8a385
> --- /dev/null
> +++ b/include/drm/drm_bus_timings.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __DRM_BUS_TIMINGS__
> +#define __DRM_BUS_TIMINGS__
> +
> +struct device_node;
> +
> +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
> +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
These should be documented with kerneldoc. How about also turning them
into an enum ?
> +
> +#ifdef CONFIG_OF
> +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> + const struct device_node *p2);
> +#else
> +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> + const struct device_node *p2)
> +{
> + return -EINVAL;
> +}
> +#endif
> +
> +#endif /* __DRM_BUS_TIMINGS__ */
--
Regards,
Laurent Pinchart
On Thu, Nov 07, 2019 at 09:26:21PM +0200, Laurent Pinchart wrote:
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> > Helper to provide bus timing information.
>
> You may want to expand this a bit. And actually fix it too, as the
> helper you introduce isn't related to timings (same for the subject
> line).
Also the kerneldoc needs to be pulled into the templates under
Documentation/gpu. And since it's just one function, why not put this into
drm_of.c? Gets rid of a pile of overhead.
>
> > Signed-off-by: Fabrizio Castro <[email protected]>
> >
> > ---
> > v2->v3:
> > * new patch
> > ---
> > drivers/gpu/drm/Makefile | 3 +-
> > drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_bus_timings.h | 21 +++++++++
> > 3 files changed, 120 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> > create mode 100644 include/drm/drm_bus_timings.h
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 9f0d2ee..a270063 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > - drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > + drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> > + drm_bus_timings.o
> >
> > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> > new file mode 100644
> > index 0000000..e2ecd22
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_bus_timings.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
DRM core is supposed to be MIT.
-Daniel
> > +#include <drm/drm_bus_timings.h>
> > +#include <linux/errno.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +
> > +#define DRM_OF_LVDS_ODD 1
> > +#define DRM_OF_LVDS_EVEN 2
> > +
> > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > +{
> > + bool even_pixels, odd_pixels;
> > +
> > + even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > + odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > + return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
>
> s/ / /
>
> But I would make these bitflags.
>
> 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)
> {
> bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
> bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
>
> return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> }
>
> > +}
> > +
> > +/**
> > + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
>
> Should we name this drm_of_lvds_get_dual_link_pixel_order to better
> reflect its purpose ?
>
> > + * @p1: device tree node corresponding to the first port of the source
> > + * @p2: device tree node corresponding to the second port of the source
>
> Maybe port1 and port2 to make this more explicit ?
>
> > + *
> > + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> > + * connection, and odd pixels transit on the other connection.
>
> To match the DT bindings documentation, I would recommand
>
> "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 walks the DT (from the source ports to the sink ports) looking
> > + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> > + * ports of the sink device(s). If such a bus is found, this function returns
> > + * its configuration (either p1 connected to the even pixels port and p2
> > + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> > + * p2 connected to the even pixels port).
>
> "walking the DT" sounds like the function goes through the whole graph.
> How about the following ?
>
> /**
> * 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 off 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.
> *
> * @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
> */
>
> We could also add -EPIPE as a return code for the case where port1 or
> port2 are not connected.
>
> > + *
> > + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> > + * found, or an error code when no valid dual-LVDS bus is found
> > + *
> > + * Possible codes for the bus configuration are:
> > + *
> > + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> > + * port and p2 is connected to the odd pixels port
> > + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> > + * port and p2 is connected to the even pixels port
> > + *
> > + */
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> > + struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
>
> There's no need to initialize those two variables.
>
> > + struct device_node *ep1 = NULL, *ep2 = NULL;
> > + u32 reg_p1, reg_p2;
> > + int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
>
> Please split this last line, as it otherwise hides the initialization of
> ret in the middle.
>
> > +
> > + if (!p1 || !p2)
> > + return ret;
>
> You can return -EINVAL directly.
>
>
> > + if (of_property_read_u32(p1, "reg", ®_p1) ||
> > + of_property_read_u32(p2, "reg", ®_p2))
> > + return ret;
>
> Same here.
>
> > + parent_p1 = of_get_parent(p1);
> > + parent_p2 = of_get_parent(p2);
> > + if (!parent_p1 || !parent_p2)
> > + goto done;
> > + ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> > + ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> > + if (!ep1 || !ep2)
> > + goto done;
>
> If you only support the first endpoint, this should be mentioned in the
> documentation. Alternatively you could pass the endpoint nodes instead
> of the port nodes, or you could pass the endpoint number.
>
> It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
> you already have the port nodes. How about adding the following helper
> function ?
>
> struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
> {
> struct device_node *endpoint = NULL;
>
> for_each_child_of_node(port, endpoint) {
> u32 id;
>
> if (!of_node_name_eq(endpoint, "endpoint") ||
> continue;
>
> if (reg == -1)
> return endpoint;
>
> if (of_property_read_u32(node, "reg", &id) < 0)
> continue;
>
> if (reg == id)
> return endpoint;
> }
>
> return NULL;
> }
>
> If you're concerned that adding a core helper would delay this patch
> series, you could add it as a local helper, and move it to of_graph.h in
> a second step.
>
> > + remote_p1 = of_graph_get_remote_port(ep1);
> > + remote_p2 = of_graph_get_remote_port(ep2);
> > + if (!remote_p1 || !remote_p2)
> > + goto done;
> > + remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> > + remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> > + /*
> > + * 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 ||
> > + remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > + goto done;
> > + if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> > + /* The sink expects even pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> > + else
> > + /* The sink expects odd pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > +
> > +done:
> > + of_node_put(ep1);
> > + of_node_put(ep2);
> > + of_node_put(parent_p1);
> > + of_node_put(parent_p2);
> > + of_node_put(remote_p1);
> > + of_node_put(remote_p2);
> > + return ret;
>
> This is heavy, I would add blank lines to make the code easier to read.
>
> > +}
> > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> > diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> > new file mode 100644
> > index 0000000..db8a385
> > --- /dev/null
> > +++ b/include/drm/drm_bus_timings.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __DRM_BUS_TIMINGS__
> > +#define __DRM_BUS_TIMINGS__
> > +
> > +struct device_node;
> > +
> > +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
> > +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
>
> These should be documented with kerneldoc. How about also turning them
> into an enum ?
>
> > +
> > +#ifdef CONFIG_OF
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2);
> > +#else
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif
> > +
> > +#endif /* __DRM_BUS_TIMINGS__ */
>
> --
> Regards,
>
> Laurent Pinchart
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:38PM +0100, Fabrizio Castro wrote:
> The driver doesn't support dual-link LVDS displays, and the way
> it identifies bridges won't allow for dual-LVDS displays to be
> connected. Also, it's not possible to swap even and odd pixels
> around in case the wiring isn't taking advantage of the default
> hardware configuration. Further more, the "mode" of the companion
> encoder should be same as the mode of the primary encoder.
>
> Rework the driver to improve all of the above, so that it can
> support dual-LVDS displays.
That's lots of changes in one patch, could it be split to ease review ?
Also, should the commit message be reworded to explain what the patch
does, instead of enumerating issues ? When there's a single issue being
addressed in a patch it's usually fine, but there the change is larger,
without an explanation of how you intend to fix the issues I can't tell
if the code really matches your intent.
> Signed-off-by: Fabrizio Castro <[email protected]>
>
> ---
> v2->v3:
> * reworked to take advantange of the new dt-bindings
> * squashed in the patche for fixing the companion's mode
>
> Laurent,
>
> unfortunately the best way to get the companion encoder to use
> the same mode as the primary encoder is setting the mode directly
> without calling into rcar_lvds_mode_set for the companion encoder,
> as the below test fails for the companion encoder in
> rcar_lvds_get_lvds_mode:
> if (!info->num_bus_formats || !info->bus_formats)
Would "[PATCH] drm: rcar-du: lvds: Get mode from state" help here ?
Maybe you could review that patch, I could then include it in my -next
branch, your work would be simplified, and everybody would be happy ?
:-)
> Anyhow, setting the mode for the companion encoder doesn't seem
> to be mandary according to the experiments I have been running,
> but the HW User's Manual doesn't really say much about this,
> therefore I think the safest option is still to set the mode for
> the companion encoder.
I agree it should be done.
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
> 1 file changed, 65 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3fe0b86..dfec5e7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -20,6 +20,8 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> +#include <drm/drm_bus_timings.h>
> +#include <drm/drm_of.h>
> #include <drm/drm_panel.h>
> #include <drm/drm_probe_helper.h>
>
> @@ -69,6 +71,7 @@ struct rcar_lvds {
>
> struct drm_bridge *companion;
> bool dual_link;
> + bool stripe_swap_data;
> };
>
> #define bridge_to_rcar_lvds(b) \
> @@ -439,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);
> }
>
> /*
> @@ -603,6 +614,11 @@ 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);
> + companion_lvds->mode = lvds->mode;
> + }
> }
>
> static int rcar_lvds_attach(struct drm_bridge *bridge)
> @@ -667,9 +683,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 = NULL, *p1 = NULL;
> struct device *dev = lvds->dev;
> - int ret = 0;
> + struct rcar_lvds *companion_lvds;
> + int ret = 0, dual_link;
>
> /* Locate the companion LVDS encoder for dual-link operation, if any. */
> companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> @@ -687,16 +704,50 @@ 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_configuration(p0, p1);
You can call of_node_put(p0) and of_node_put(p1) here instead of adding
them at the end of the function.
> + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> + dev_dbg(dev, "Dual-link configuration detected\n");
> + lvds->dual_link = true;
> + } else {
> + /* dual-link mode is not required */
> + dev_dbg(dev, "Single-link configuration detected\n");
> + goto done;
> + }
Missing blank line here.
> + /*
> + * We may need to swap even and odd pixels around in case the wiring
> + * doesn't match the default configuration.
> + * By default we generate even pixels from this encoder and odd pixels
> + * from the companion encoder, but if p0 is connected to the port
> + * expecting ood pixels, and p1 is connected to the port expecting even
> + * pixels, then we need to swap even and odd pixels around
> + */
> + if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
> + dev_dbg(dev, "Data swapping required\n");
> + lvds->stripe_swap_data = true;
> + }
> +
> lvds->companion = of_drm_find_bridge(companion);
> if (!lvds->companion) {
> ret = -EPROBE_DEFER;
> goto done;
> }
> + companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> + companion_lvds->dual_link = lvds->dual_link;
I don't like poking directly in the companion like this :-( Can't we let
the companion detect dual link mode itself ?
>
> dev_dbg(dev, "Found companion encoder %pOF\n", companion);
>
> done:
> of_node_put(companion);
> + of_node_put(p0);
> + of_node_put(p1);
>
> return ret;
> }
> @@ -704,10 +755,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);
> @@ -735,45 +783,17 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> goto done;
> }
>
I think you can also drop all the code above.
> - 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;
> - }
> -
> - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> - lvds->dual_link = lvds->next_bridge->timings
> - ? lvds->next_bridge->timings->dual_link
> - : false;
Aren't you breaking backward compatibility with this change ? Unless I'm
mistaken you're now requiring the new DT properties, and the existing DT
that include a thc63lvd1024 won't have them.
> - } else {
> - lvds->panel = of_drm_find_panel(remote);
> - if (IS_ERR(lvds->panel)) {
> - ret = PTR_ERR(lvds->panel);
> - goto done;
> - }
> + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> + &lvds->panel, &lvds->next_bridge);
> + if (ret) {
> + ret = -EPROBE_DEFER;
Shouldn't you return ret instead of overriding it ?
> + goto done;
> }
> -
> - if (lvds->dual_link)
> + if (lvds->info->quirks & RCAR_LVDS_QUIRK_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
Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:39PM +0100, Fabrizio Castro wrote:
> No need to report the input bus mode through bridge timings
> anymore, that's now done through the DT, as specified by the
> dt-bindings.
Doesn't this break backward compatibility with older DT, as mentioned in
the review of 4/8 ?
> Signed-off-by: Fabrizio Castro <[email protected]>
>
> ---
> v2->v3:
> * new patch
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 9 ++++-----
> include/drm/drm_bridge.h | 8 --------
> 2 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index 3d74129b..730f682 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -34,7 +34,7 @@ struct thc63_dev {
> struct drm_bridge bridge;
> struct drm_bridge *next;
>
> - struct drm_bridge_timings timings;
> + bool dual_link;
> };
>
> static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> @@ -62,7 +62,7 @@ static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
> * isn't supported by the driver yet, simply derive the limits from the
> * input mode.
> */
> - if (thc63->timings.dual_link) {
> + if (thc63->dual_link) {
> min_freq = 40000;
> max_freq = 150000;
> } else {
> @@ -157,13 +157,13 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>
> if (remote) {
> if (of_device_is_available(remote))
> - thc63->timings.dual_link = true;
> + thc63->dual_link = true;
> of_node_put(remote);
> }
> }
>
> dev_dbg(thc63->dev, "operating in %s-link mode\n",
> - thc63->timings.dual_link ? "dual" : "single");
> + thc63->dual_link ? "dual" : "single");
>
> return 0;
> }
> @@ -221,7 +221,6 @@ static int thc63_probe(struct platform_device *pdev)
> thc63->bridge.driver_private = thc63;
> thc63->bridge.of_node = pdev->dev.of_node;
> thc63->bridge.funcs = &thc63_bridge_func;
> - thc63->bridge.timings = &thc63->timings;
>
> drm_bridge_add(&thc63->bridge);
>
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 7616f65..3228018 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -362,14 +362,6 @@ struct drm_bridge_timings {
> * input signal after the clock edge.
> */
> u32 hold_time_ps;
> - /**
> - * @dual_link:
> - *
> - * True if the bus operates in dual-link mode. The exact meaning is
> - * dependent on the bus type. For LVDS buses, this indicates that even-
> - * and odd-numbered pixels are received on separate links.
> - */
> - bool dual_link;
> };
>
> /**
--
Regards,
Laurent Pinchart
Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:40PM +0100, Fabrizio Castro wrote:
> 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 <[email protected]>
>
> ---
> v1->v2:
> * Added comment for lvds-connector-en-gpio
> * Renamed &lvds0_panel_in to panel_in0
> * Renamed &lvds1_panel_in to panel_in1
>
> v2->v3:
> * removed renesas,swap-data property
> * added dual-lvds-odd-pixels and dual-lvds-even-pixels properties
>
> Geert,
>
> no need to review this patch unless they like the idea behind this
> series.
I like the idea :-) We still have a few issues to solve, but it should
make it in.
The patch looks sane to me, but I haven't checked if it matches the
hardware, so
Acked-by: Laurent Pinchart <[email protected]>
(even if this really calls for DT overlays)
> ---
> 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 42b74c2..ce48478 100644
> --- a/arch/arm64/boot/dts/renesas/Makefile
> +++ b/arch/arm64/boot/dts/renesas/Makefile
> @@ -1,7 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0
> dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m.dtb
> dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m-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 = <®_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";
> +};
--
Regards,
Laurent Pinchart
Hi Fabrizio,
Thank you for the patch.
The subject is wrong, it should be
[HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
On Wed, Aug 28, 2019 at 07:36:41PM +0100, Fabrizio Castro wrote:
> Enable and connect the second LVDS encoder to the second LVDS input of
> the THC63LVD1024 for dual-link LVDS operation. This requires changing
> the default settings of SW45 and SW47 to OFF and ON respectively.
>
> This patch is based on Laurent's dual-LVDS work:
> https://patchwork.kernel.org/patch/10965045/
>
> Signed-off-by: Fabrizio Castro <[email protected]>
> ---
> v2->v3:
> * new patch
>
> Geert,
>
> no need to review this patch unless they like the idea behind this
> series.
>
> Thanks,
> Fab
>
> ---
> arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> index b38f9d4..38b9c5a 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> @@ -87,11 +87,20 @@
>
> port@0 {
> reg = <0>;
> - thc63lvd1024_in: endpoint {
> + dual-lvds-even-pixels;
> + thc63lvd1024_in0: endpoint {
> remote-endpoint = <&lvds0_out>;
> };
> };
>
> + port@1 {
> + reg = <1>;
> + dual-lvds-odd-pixels;
> + thc63lvd1024_in1: endpoint {
> + remote-endpoint = <&lvds1_out>;
> + };
> + };
> +
> port@2 {
> reg = <2>;
> thc63lvd1024_out: endpoint {
> @@ -489,7 +498,7 @@
> ports {
> port@1 {
> lvds0_out: endpoint {
> - remote-endpoint = <&thc63lvd1024_in>;
> + remote-endpoint = <&thc63lvd1024_in0>;
> };
> };
> };
> @@ -507,6 +516,14 @@
> <&x13_clk>,
> <&extal_clk>;
> clock-names = "fck", "dclkin.0", "extal";
> +
> + ports {
> + port@1 {
> + lvds1_out: endpoint {
> + remote-endpoint = <&thc63lvd1024_in1>;
> + };
> + };
> + };
> };
>
> &ohci0 {
> --
> 2.7.4
>
--
Regards,
Laurent Pinchart
Hi Laurent,
Thank you for your feedback!
> From: Laurent Pinchart <[email protected]>
> Sent: 07 November 2019 18:12
> Subject: Re: [PATCH v3 2/8] dt-bindings: display: Add idk-2121wr binding
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:36PM +0100, 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 <[email protected]>
> >
> > ---
> > v2->v3:
> > * new patch
> > ---
> > .../display/panel/advantech,idk-2121wr.yaml | 90 ++++++++++++++++++++++
> > 1 file changed, 90 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..b2ccdc8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> > @@ -0,0 +1,90 @@
> > +# 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 <[email protected]>
> > + - Thierry Reding <[email protected]>
> > +
> > +description: |
> > + The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
> > +
> > + The panels expects odd pixels from the first port, and even pixels from
>
> s/panels/panel/
> Maybe s/from the/on the/g ?
Will fix
>
> > + the second port, therefore the ports must be marked accordingly.
> > +
> > +allOf:
> > + - $ref: lvds.yaml#
> > + - $ref: ../bus-timings/lvds.yaml#
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: advantech,idk-2121wr
> > + - {} # panel-lvds, but not listed here to avoid false select
> > +
> > + data-mapping:
> > + const: vesa-24
> > +
> > + width-mm:
> > + const: 476
> > +
> > + height-mm:
> > + const: 268
> > +
> > + panel-timing: true
> > + ports: true
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
>
> Shouldn't data-mapping, width-mm, height-mm and ports be required too ?
Those are required by lvds.yaml, and this file has a reference to it.
Anyhow, I think the best course of action is to merge this with the
generic bus-timings/lvds.yaml for the time being, and maybe split
them back later on once we have another use case.
Thanks,
Fab
>
> As you mentioned in the cover letter, validating ports, port and the new
> dual-lvds-*-pixels properties would be nice. I'm not YAML schema
> specialist, so I'm fine with a best effort approach here, but as far as
> I understand Rob proposed a way forward, could you try it ?
>
> Apart from that, the bindings look sne to me, so
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> once the above issues get addressed.
>
> > +
> > +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
Hi Laurent,
Thank you for your feedback!
> From: [email protected] <[email protected]> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:26
> Subject: Re: [PATCH v3 3/8] drm: Add bus timings helper
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> > Helper to provide bus timing information.
>
> You may want to expand this a bit. And actually fix it too, as the
> helper you introduce isn't related to timings (same for the subject
> line).
I'll rework this completely
>
> > Signed-off-by: Fabrizio Castro <[email protected]>
> >
> > ---
> > v2->v3:
> > * new patch
> > ---
> > drivers/gpu/drm/Makefile | 3 +-
> > drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_bus_timings.h | 21 +++++++++
> > 3 files changed, 120 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> > create mode 100644 include/drm/drm_bus_timings.h
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 9f0d2ee..a270063 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > - drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > + drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> > + drm_bus_timings.o
> >
> > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> > new file mode 100644
> > index 0000000..e2ecd22
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_bus_timings.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <drm/drm_bus_timings.h>
> > +#include <linux/errno.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +
> > +#define DRM_OF_LVDS_ODD 1
> > +#define DRM_OF_LVDS_EVEN 2
> > +
> > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > +{
> > + bool even_pixels, odd_pixels;
> > +
> > + even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > + odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > + return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
>
> s/ / /
>
> But I would make these bitflags.
>
> 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)
> {
> bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
> bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
>
> return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> }
Will do
>
> > +}
> > +
> > +/**
> > + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
>
> Should we name this drm_of_lvds_get_dual_link_pixel_order to better
> reflect its purpose ?
>
> > + * @p1: device tree node corresponding to the first port of the source
> > + * @p2: device tree node corresponding to the second port of the source
>
> Maybe port1 and port2 to make this more explicit ?
yeah
>
> > + *
> > + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> > + * connection, and odd pixels transit on the other connection.
>
> To match the DT bindings documentation, I would recommand
>
> "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 walks the DT (from the source ports to the sink ports) looking
> > + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> > + * ports of the sink device(s). If such a bus is found, this function returns
> > + * its configuration (either p1 connected to the even pixels port and p2
> > + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> > + * p2 connected to the even pixels port).
>
> "walking the DT" sounds like the function goes through the whole graph.
> How about the following ?
>
> /**
> * 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 off 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.
> *
> * @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
> */
Can do
>
> We could also add -EPIPE as a return code for the case where port1 or
> port2 are not connected.
Good idea, will add
>
> > + *
> > + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> > + * found, or an error code when no valid dual-LVDS bus is found
> > + *
> > + * Possible codes for the bus configuration are:
> > + *
> > + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> > + * port and p2 is connected to the odd pixels port
> > + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> > + * port and p2 is connected to the even pixels port
> > + *
> > + */
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> > + struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
>
> There's no need to initialize those two variables.
Will change
>
> > + struct device_node *ep1 = NULL, *ep2 = NULL;
> > + u32 reg_p1, reg_p2;
> > + int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
>
> Please split this last line, as it otherwise hides the initialization of
> ret in the middle.
Will change
>
> > +
> > + if (!p1 || !p2)
> > + return ret;
>
> You can return -EINVAL directly.
ok
>
>
> > + if (of_property_read_u32(p1, "reg", ®_p1) ||
> > + of_property_read_u32(p2, "reg", ®_p2))
> > + return ret;
>
> Same here.
ok
>
> > + parent_p1 = of_get_parent(p1);
> > + parent_p2 = of_get_parent(p2);
> > + if (!parent_p1 || !parent_p2)
> > + goto done;
> > + ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> > + ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> > + if (!ep1 || !ep2)
> > + goto done;
>
> If you only support the first endpoint, this should be mentioned in the
> documentation. Alternatively you could pass the endpoint nodes instead
> of the port nodes, or you could pass the endpoint number.
Or alternatively I could inspect all of the remote endpoints
>
> It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
> you already have the port nodes. How about adding the following helper
> function ?
I'll implement something to walk through all of the endpoints we are connected to.
Will send something shortly for you to have a look at.
>
> struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
> {
> struct device_node *endpoint = NULL;
>
> for_each_child_of_node(port, endpoint) {
> u32 id;
>
> if (!of_node_name_eq(endpoint, "endpoint") ||
> continue;
>
> if (reg == -1)
> return endpoint;
>
> if (of_property_read_u32(node, "reg", &id) < 0)
> continue;
>
> if (reg == id)
> return endpoint;
> }
>
> return NULL;
> }
>
> If you're concerned that adding a core helper would delay this patch
> series, you could add it as a local helper, and move it to of_graph.h in
> a second step.
>
> > + remote_p1 = of_graph_get_remote_port(ep1);
> > + remote_p2 = of_graph_get_remote_port(ep2);
> > + if (!remote_p1 || !remote_p2)
> > + goto done;
> > + remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> > + remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> > + /*
> > + * 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 ||
> > + remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > + goto done;
> > + if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> > + /* The sink expects even pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> > + else
> > + /* The sink expects odd pixels through the first port */
> > + ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > +
> > +done:
> > + of_node_put(ep1);
> > + of_node_put(ep2);
> > + of_node_put(parent_p1);
> > + of_node_put(parent_p2);
> > + of_node_put(remote_p1);
> > + of_node_put(remote_p2);
> > + return ret;
>
> This is heavy, I would add blank lines to make the code easier to read.
Most of this will disappear in v4
>
> > +}
> > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> > diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> > new file mode 100644
> > index 0000000..db8a385
> > --- /dev/null
> > +++ b/include/drm/drm_bus_timings.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __DRM_BUS_TIMINGS__
> > +#define __DRM_BUS_TIMINGS__
> > +
> > +struct device_node;
> > +
> > +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
> > +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
>
> These should be documented with kerneldoc. How about also turning them
> into an enum ?
Will change
Thanks,
Fab
>
> > +
> > +#ifdef CONFIG_OF
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2);
> > +#else
> > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > + const struct device_node *p2)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif
> > +
> > +#endif /* __DRM_BUS_TIMINGS__ */
>
> --
> Regards,
>
> Laurent Pinchart
Hi Daniel,
Thank you for your feedback!
> From: Daniel Vetter <[email protected]> On Behalf Of Daniel Vetter
> Sent: 07 November 2019 19:30
> Subject: Re: [PATCH v3 3/8] drm: Add bus timings helper
>
> On Thu, Nov 07, 2019 at 09:26:21PM +0200, Laurent Pinchart wrote:
> > Hi Fabrizio,
> >
> > Thank you for the patch.
> >
> > On Wed, Aug 28, 2019 at 07:36:37PM +0100, Fabrizio Castro wrote:
> > > Helper to provide bus timing information.
> >
> > You may want to expand this a bit. And actually fix it too, as the
> > helper you introduce isn't related to timings (same for the subject
> > line).
>
> Also the kerneldoc needs to be pulled into the templates under
> Documentation/gpu. And since it's just one function, why not put this into
> drm_of.c? Gets rid of a pile of overhead.
Yeah, you are right, will try and pull this into drm_of.c in v4.
Thanks!
Fab
>
> >
> > > Signed-off-by: Fabrizio Castro <[email protected]>
> > >
> > > ---
> > > v2->v3:
> > > * new patch
> > > ---
> > > drivers/gpu/drm/Makefile | 3 +-
> > > drivers/gpu/drm/drm_bus_timings.c | 97 +++++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_bus_timings.h | 21 +++++++++
> > > 3 files changed, 120 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/gpu/drm/drm_bus_timings.c
> > > create mode 100644 include/drm/drm_bus_timings.h
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 9f0d2ee..a270063 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -17,7 +17,8 @@ drm-y := drm_auth.o drm_cache.o \
> > > drm_plane.o drm_color_mgmt.o drm_print.o \
> > > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > - drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > + drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
> > > + drm_bus_timings.o
> > >
> > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > diff --git a/drivers/gpu/drm/drm_bus_timings.c b/drivers/gpu/drm/drm_bus_timings.c
> > > new file mode 100644
> > > index 0000000..e2ecd22
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_bus_timings.c
> > > @@ -0,0 +1,97 @@
> > > +// SPDX-License-Identifier: GPL-2.0
>
> DRM core is supposed to be MIT.
> -Daniel
>
> > > +#include <drm/drm_bus_timings.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/of.h>
> > > +#include <linux/types.h>
> > > +
> > > +#define DRM_OF_LVDS_ODD 1
> > > +#define DRM_OF_LVDS_EVEN 2
> > > +
> > > +static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
> > > +{
> > > + bool even_pixels, odd_pixels;
> > > +
> > > + even_pixels = of_property_read_bool(port_node, "dual-lvds-even-pixels");
> > > + odd_pixels = of_property_read_bool(port_node, "dual-lvds-odd-pixels");
> > > + return even_pixels * DRM_OF_LVDS_EVEN + odd_pixels * DRM_OF_LVDS_ODD;
> >
> > s/ / /
> >
> > But I would make these bitflags.
> >
> > 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)
> > {
> > bool even_pixels = of_property_read_bool(port, "dual-lvds-even-pixels");
> > bool odd_pixels = of_property_read_bool(port, "dual-lvds-odd-pixels");
> >
> > return (even_pixels ? DRM_OF_LVDS_EVEN : 0) |
> > (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> > }
> >
> > > +}
> > > +
> > > +/**
> > > + * drm_of_lvds_get_dual_link_configuration - get the dual-LVDS configuration
> >
> > Should we name this drm_of_lvds_get_dual_link_pixel_order to better
> > reflect its purpose ?
> >
> > > + * @p1: device tree node corresponding to the first port of the source
> > > + * @p2: device tree node corresponding to the second port of the source
> >
> > Maybe port1 and port2 to make this more explicit ?
> >
> > > + *
> > > + * An LVDS dual-link bus is made of two connections, even pixels transit on one
> > > + * connection, and odd pixels transit on the other connection.
> >
> > To match the DT bindings documentation, I would recommand
> >
> > "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 walks the DT (from the source ports to the sink ports) looking
> > > + * for a dual-LVDS bus. A dual-LVDS bus is identfied by markers found on the DT
> > > + * ports of the sink device(s). If such a bus is found, this function returns
> > > + * its configuration (either p1 connected to the even pixels port and p2
> > > + * connected to the odd pixels port, or p1 connected to the odd pixels port and
> > > + * p2 connected to the even pixels port).
> >
> > "walking the DT" sounds like the function goes through the whole graph.
> > How about the following ?
> >
> > /**
> > * 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 off 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.
> > *
> > * @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
> > */
> >
> > We could also add -EPIPE as a return code for the case where port1 or
> > port2 are not connected.
> >
> > > + *
> > > + * Return: A code describing the bus configuration when a valid dual-LVDS bus is
> > > + * found, or an error code when no valid dual-LVDS bus is found
> > > + *
> > > + * Possible codes for the bus configuration are:
> > > + *
> > > + * - DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: when p1 is connected to the even pixels
> > > + * port and p2 is connected to the odd pixels port
> > > + * - DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: when p1 is connected to the odd pixels
> > > + * port and p2 is connected to the even pixels port
> > > + *
> > > + */
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > + const struct device_node *p2)
> > > +{
> > > + struct device_node *remote_p1 = NULL, *remote_p2 = NULL;
> > > + struct device_node *parent_p1 = NULL, *parent_p2 = NULL;
> >
> > There's no need to initialize those two variables.
> >
> > > + struct device_node *ep1 = NULL, *ep2 = NULL;
> > > + u32 reg_p1, reg_p2;
> > > + int ret = -EINVAL, remote_p1_pt, remote_p2_pt;
> >
> > Please split this last line, as it otherwise hides the initialization of
> > ret in the middle.
> >
> > > +
> > > + if (!p1 || !p2)
> > > + return ret;
> >
> > You can return -EINVAL directly.
> >
> >
> > > + if (of_property_read_u32(p1, "reg", ®_p1) ||
> > > + of_property_read_u32(p2, "reg", ®_p2))
> > > + return ret;
> >
> > Same here.
> >
> > > + parent_p1 = of_get_parent(p1);
> > > + parent_p2 = of_get_parent(p2);
> > > + if (!parent_p1 || !parent_p2)
> > > + goto done;
> > > + ep1 = of_graph_get_endpoint_by_regs(parent_p1, reg_p1, 0);
> > > + ep2 = of_graph_get_endpoint_by_regs(parent_p2, reg_p2, 0);
> > > + if (!ep1 || !ep2)
> > > + goto done;
> >
> > If you only support the first endpoint, this should be mentioned in the
> > documentation. Alternatively you could pass the endpoint nodes instead
> > of the port nodes, or you could pass the endpoint number.
> >
> > It's also a bit inefficient to use of_graph_get_endpoint_by_regs() when
> > you already have the port nodes. How about adding the following helper
> > function ?
> >
> > struct device_node *of_graph_get_port_endpoint(struct device_node *port, int reg)
> > {
> > struct device_node *endpoint = NULL;
> >
> > for_each_child_of_node(port, endpoint) {
> > u32 id;
> >
> > if (!of_node_name_eq(endpoint, "endpoint") ||
> > continue;
> >
> > if (reg == -1)
> > return endpoint;
> >
> > if (of_property_read_u32(node, "reg", &id) < 0)
> > continue;
> >
> > if (reg == id)
> > return endpoint;
> > }
> >
> > return NULL;
> > }
> >
> > If you're concerned that adding a core helper would delay this patch
> > series, you could add it as a local helper, and move it to of_graph.h in
> > a second step.
> >
> > > + remote_p1 = of_graph_get_remote_port(ep1);
> > > + remote_p2 = of_graph_get_remote_port(ep2);
> > > + if (!remote_p1 || !remote_p2)
> > > + goto done;
> > > + remote_p1_pt = drm_of_lvds_get_port_pixels_type(remote_p1);
> > > + remote_p2_pt = drm_of_lvds_get_port_pixels_type(remote_p2);
> > > + /*
> > > + * 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 ||
> > > + remote_p1_pt + remote_p2_pt != DRM_OF_LVDS_EVEN + DRM_OF_LVDS_ODD)
> > > + goto done;
> > > + if (remote_p1_pt == DRM_OF_LVDS_EVEN)
> > > + /* The sink expects even pixels through the first port */
> > > + ret = DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> > > + else
> > > + /* The sink expects odd pixels through the first port */
> > > + ret = DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS;
> > > +
> > > +done:
> > > + of_node_put(ep1);
> > > + of_node_put(ep2);
> > > + of_node_put(parent_p1);
> > > + of_node_put(parent_p2);
> > > + of_node_put(remote_p1);
> > > + of_node_put(remote_p2);
> > > + return ret;
> >
> > This is heavy, I would add blank lines to make the code easier to read.
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_of_lvds_get_dual_link_configuration);
> > > diff --git a/include/drm/drm_bus_timings.h b/include/drm/drm_bus_timings.h
> > > new file mode 100644
> > > index 0000000..db8a385
> > > --- /dev/null
> > > +++ b/include/drm/drm_bus_timings.h
> > > @@ -0,0 +1,21 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __DRM_BUS_TIMINGS__
> > > +#define __DRM_BUS_TIMINGS__
> > > +
> > > +struct device_node;
> > > +
> > > +#define DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS 0
> > > +#define DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS 1
> >
> > These should be documented with kerneldoc. How about also turning them
> > into an enum ?
> >
> > > +
> > > +#ifdef CONFIG_OF
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > + const struct device_node *p2);
> > > +#else
> > > +int drm_of_lvds_get_dual_link_configuration(const struct device_node *p1,
> > > + const struct device_node *p2)
> > > +{
> > > + return -EINVAL;
> > > +}
> > > +#endif
> > > +
> > > +#endif /* __DRM_BUS_TIMINGS__ */
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Hi Laurent,
> From: [email protected] <[email protected]> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:51
> Subject: Re: [PATCH v3 4/8] drm: rcar-du: lvds: Add dual-LVDS panels support
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:38PM +0100, Fabrizio Castro wrote:
> > The driver doesn't support dual-link LVDS displays, and the way
> > it identifies bridges won't allow for dual-LVDS displays to be
> > connected. Also, it's not possible to swap even and odd pixels
> > around in case the wiring isn't taking advantage of the default
> > hardware configuration. Further more, the "mode" of the companion
> > encoder should be same as the mode of the primary encoder.
> >
> > Rework the driver to improve all of the above, so that it can
> > support dual-LVDS displays.
>
> That's lots of changes in one patch, could it be split to ease review ?
> Also, should the commit message be reworded to explain what the patch
> does, instead of enumerating issues ? When there's a single issue being
> addressed in a patch it's usually fine, but there the change is larger,
> without an explanation of how you intend to fix the issues I can't tell
> if the code really matches your intent.
Sorry for the pain, I'll split this patch into smaller patches.
>
> > Signed-off-by: Fabrizio Castro <[email protected]>
> >
> > ---
> > v2->v3:
> > * reworked to take advantange of the new dt-bindings
> > * squashed in the patche for fixing the companion's mode
> >
> > Laurent,
> >
> > unfortunately the best way to get the companion encoder to use
> > the same mode as the primary encoder is setting the mode directly
> > without calling into rcar_lvds_mode_set for the companion encoder,
> > as the below test fails for the companion encoder in
> > rcar_lvds_get_lvds_mode:
> > if (!info->num_bus_formats || !info->bus_formats)
>
> Would "[PATCH] drm: rcar-du: lvds: Get mode from state" help here ?
> Maybe you could review that patch, I could then include it in my -next
> branch, your work would be simplified, and everybody would be happy ?
> :-)
I gave that a try, it doesn't work for me, even after fixing the NULL
pointer. Perhaps we could finalize this series first and then we could figure
that patch out next?
>
> > Anyhow, setting the mode for the companion encoder doesn't seem
> > to be mandary according to the experiments I have been running,
> > but the HW User's Manual doesn't really say much about this,
> > therefore I think the safest option is still to set the mode for
> > the companion encoder.
>
> I agree it should be done.
>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
> > 1 file changed, 65 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3fe0b86..dfec5e7 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -20,6 +20,8 @@
> > #include <drm/drm_atomic.h>
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_bridge.h>
> > +#include <drm/drm_bus_timings.h>
> > +#include <drm/drm_of.h>
> > #include <drm/drm_panel.h>
> > #include <drm/drm_probe_helper.h>
> >
> > @@ -69,6 +71,7 @@ struct rcar_lvds {
> >
> > struct drm_bridge *companion;
> > bool dual_link;
> > + bool stripe_swap_data;
> > };
> >
> > #define bridge_to_rcar_lvds(b) \
> > @@ -439,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);
> > }
> >
> > /*
> > @@ -603,6 +614,11 @@ 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);
> > + companion_lvds->mode = lvds->mode;
> > + }
> > }
> >
> > static int rcar_lvds_attach(struct drm_bridge *bridge)
> > @@ -667,9 +683,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 = NULL, *p1 = NULL;
> > struct device *dev = lvds->dev;
> > - int ret = 0;
> > + struct rcar_lvds *companion_lvds;
> > + int ret = 0, dual_link;
> >
> > /* Locate the companion LVDS encoder for dual-link operation, if any. */
> > companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> > @@ -687,16 +704,50 @@ 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_configuration(p0, p1);
>
> You can call of_node_put(p0) and of_node_put(p1) here instead of adding
> them at the end of the function.
I'll be restructuring this code a little, and I'll move the put up here, as you suggested
>
> > + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> > + dev_dbg(dev, "Dual-link configuration detected\n");
> > + lvds->dual_link = true;
> > + } else {
> > + /* dual-link mode is not required */
> > + dev_dbg(dev, "Single-link configuration detected\n");
> > + goto done;
> > + }
>
> Missing blank line here.
Thanks
>
> > + /*
> > + * We may need to swap even and odd pixels around in case the wiring
> > + * doesn't match the default configuration.
> > + * By default we generate even pixels from this encoder and odd pixels
> > + * from the companion encoder, but if p0 is connected to the port
> > + * expecting ood pixels, and p1 is connected to the port expecting even
> > + * pixels, then we need to swap even and odd pixels around
> > + */
> > + if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
> > + dev_dbg(dev, "Data swapping required\n");
> > + lvds->stripe_swap_data = true;
> > + }
> > +
> > lvds->companion = of_drm_find_bridge(companion);
> > if (!lvds->companion) {
> > ret = -EPROBE_DEFER;
> > goto done;
> > }
> > + companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> > + companion_lvds->dual_link = lvds->dual_link;
>
> I don't like poking directly in the companion like this :-( Can't we let
> the companion detect dual link mode itself ?
I don't like it either, but the companion encoder doesn't hold a reference to the
Primary encoder right now, so we would need to change strategy for this.
I think perhaps we could add this solution to the driver, and then we fix it
properly later on?
>
> >
> > dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> >
> > done:
> > of_node_put(companion);
> > + of_node_put(p0);
> > + of_node_put(p1);
> >
> > return ret;
> > }
> > @@ -704,10 +755,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);
> > @@ -735,45 +783,17 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > goto done;
> > }
> >
>
> I think you can also drop all the code above.
>
> > - 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;
> > - }
> > -
> > - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > - lvds->dual_link = lvds->next_bridge->timings
> > - ? lvds->next_bridge->timings->dual_link
> > - : false;
>
> Aren't you breaking backward compatibility with this change ? Unless I'm
> mistaken you're now requiring the new DT properties, and the existing DT
> that include a thc63lvd1024 won't have them.
Unfortunately I am breaking backward compatibility here. Will be more careful
in v4, sorry!
>
> > - } else {
> > - lvds->panel = of_drm_find_panel(remote);
> > - if (IS_ERR(lvds->panel)) {
> > - ret = PTR_ERR(lvds->panel);
> > - goto done;
> > - }
> > + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> > + &lvds->panel, &lvds->next_bridge);
> > + if (ret) {
> > + ret = -EPROBE_DEFER;
>
> Shouldn't you return ret instead of overriding it ?
Can do
Thanks,
Fab
>
> > + goto done;
> > }
> > -
> > - if (lvds->dual_link)
> > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_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
Hi Laurent,
Thank you for your feedback!
> From: [email protected] <[email protected]> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:52
> Subject: Re: [PATCH v3 5/8] drm: bridge: thc63: Do not report input bus mode through bridge timings
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2019 at 07:36:39PM +0100, Fabrizio Castro wrote:
> > No need to report the input bus mode through bridge timings
> > anymore, that's now done through the DT, as specified by the
> > dt-bindings.
>
> Doesn't this break backward compatibility with older DT, as mentioned in
> the review of 4/8 ?
>
I'll drop this patch in v4
Thanks,
Fab
> > Signed-off-by: Fabrizio Castro <[email protected]>
> >
> > ---
> > v2->v3:
> > * new patch
> > ---
> > drivers/gpu/drm/bridge/thc63lvd1024.c | 9 ++++-----
> > include/drm/drm_bridge.h | 8 --------
> > 2 files changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 3d74129b..730f682 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -34,7 +34,7 @@ struct thc63_dev {
> > struct drm_bridge bridge;
> > struct drm_bridge *next;
> >
> > - struct drm_bridge_timings timings;
> > + bool dual_link;
> > };
> >
> > static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> > @@ -62,7 +62,7 @@ static enum drm_mode_status thc63_mode_valid(struct drm_bridge *bridge,
> > * isn't supported by the driver yet, simply derive the limits from the
> > * input mode.
> > */
> > - if (thc63->timings.dual_link) {
> > + if (thc63->dual_link) {
> > min_freq = 40000;
> > max_freq = 150000;
> > } else {
> > @@ -157,13 +157,13 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >
> > if (remote) {
> > if (of_device_is_available(remote))
> > - thc63->timings.dual_link = true;
> > + thc63->dual_link = true;
> > of_node_put(remote);
> > }
> > }
> >
> > dev_dbg(thc63->dev, "operating in %s-link mode\n",
> > - thc63->timings.dual_link ? "dual" : "single");
> > + thc63->dual_link ? "dual" : "single");
> >
> > return 0;
> > }
> > @@ -221,7 +221,6 @@ static int thc63_probe(struct platform_device *pdev)
> > thc63->bridge.driver_private = thc63;
> > thc63->bridge.of_node = pdev->dev.of_node;
> > thc63->bridge.funcs = &thc63_bridge_func;
> > - thc63->bridge.timings = &thc63->timings;
> >
> > drm_bridge_add(&thc63->bridge);
> >
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 7616f65..3228018 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -362,14 +362,6 @@ struct drm_bridge_timings {
> > * input signal after the clock edge.
> > */
> > u32 hold_time_ps;
> > - /**
> > - * @dual_link:
> > - *
> > - * True if the bus operates in dual-link mode. The exact meaning is
> > - * dependent on the bus type. For LVDS buses, this indicates that even-
> > - * and odd-numbered pixels are received on separate links.
> > - */
> > - bool dual_link;
> > };
> >
> > /**
>
> --
> Regards,
>
> Laurent Pinchart
Hello Laurent,
Thank you for the feedback!
> From: [email protected] <[email protected]> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:58
> Subject: Re: [PATCH v3 7/8] [HACK] arm64: dts: renesas: draak: Enable LVDS
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> The subject is wrong, it should be
>
> [HACK] arm64: dts: renesas: ebisu: Enable LVDS dual-link operation
This sounds like a copy and paste gone really wrong!
Sorry about that. I'll be dropping this patch in v4.
Thanks,
Fab
>
> On Wed, Aug 28, 2019 at 07:36:41PM +0100, Fabrizio Castro wrote:
> > Enable and connect the second LVDS encoder to the second LVDS input of
> > the THC63LVD1024 for dual-link LVDS operation. This requires changing
> > the default settings of SW45 and SW47 to OFF and ON respectively.
> >
> > This patch is based on Laurent's dual-LVDS work:
> > https://patchwork.kernel.org/patch/10965045/
> >
> > Signed-off-by: Fabrizio Castro <[email protected]>
> > ---
> > v2->v3:
> > * new patch
> >
> > Geert,
> >
> > no need to review this patch unless they like the idea behind this
> > series.
> >
> > Thanks,
> > Fab
> >
> > ---
> > arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > index b38f9d4..38b9c5a 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> > @@ -87,11 +87,20 @@
> >
> > port@0 {
> > reg = <0>;
> > - thc63lvd1024_in: endpoint {
> > + dual-lvds-even-pixels;
> > + thc63lvd1024_in0: endpoint {
> > remote-endpoint = <&lvds0_out>;
> > };
> > };
> >
> > + port@1 {
> > + reg = <1>;
> > + dual-lvds-odd-pixels;
> > + thc63lvd1024_in1: endpoint {
> > + remote-endpoint = <&lvds1_out>;
> > + };
> > + };
> > +
> > port@2 {
> > reg = <2>;
> > thc63lvd1024_out: endpoint {
> > @@ -489,7 +498,7 @@
> > ports {
> > port@1 {
> > lvds0_out: endpoint {
> > - remote-endpoint = <&thc63lvd1024_in>;
> > + remote-endpoint = <&thc63lvd1024_in0>;
> > };
> > };
> > };
> > @@ -507,6 +516,14 @@
> > <&x13_clk>,
> > <&extal_clk>;
> > clock-names = "fck", "dclkin.0", "extal";
> > +
> > + ports {
> > + port@1 {
> > + lvds1_out: endpoint {
> > + remote-endpoint = <&thc63lvd1024_in1>;
> > + };
> > + };
> > + };
> > };
> >
> > &ohci0 {
> > --
> > 2.7.4
> >
>
> --
> Regards,
>
> Laurent Pinchart