[PATCH v2 0/3] drm: Add LVDS decoder bridge

[PATCH v2 0/3] drm: Add LVDS decoder bridge

[Jacopo Mondi]

Hello,
   after some discussion on the proposed bindings for generic lvds decoder and
Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
a transparent decoder that does not support any configuration from DT.

Dropping THC63 support to avoid discussion on how to better implement support
for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
bridges as explained in v1 cover letter (for DRM people: please see [1] as why
I find difficult to implement support for bridges with multiple input endpoints)

Same base branch as v1, with same patches for V3M Eagle applied on top.
git://jmondi.org/linux v3m/v4.16-rc3/base

Thanks
   j

v1 -> v2:
- Drop support for THC63LVD1024

[1] I had a quick at how to model a DRM bridge with multiple input
ports, and I see a blocker in how DRM identifies and matches bridges using
the devices node in place of the endpoint nodes.

As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
a few ways to support that:
 1) register 2 drm bridges from the same driver (one for each input/output pair)
    but they would both be matches on the same device node when the preceding
    bridge calls "of_drm_find_bridge()".
 2) register a single bridge with multiple "next bridges", but when the bridge
    gets attached I don't see a way on how to identify on which next bridge
    "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
    has been attached on first, and we don't have that information.
 3) Register more instances of the same chip in DTS, one for each input/output
    pair. They gonna share supplies and gpios, and I don't like that.

I had a quick look at the currently in mainline bridges and none of them has
multiple input endpoints, except for HDMI audio endpoint, which I haven't found
in use in any DTS. I guess the problem has been already debated and maybe solved
in the past, so feel free to point me to other sources.

Jacopo Mondi (3):
  dt-bindings: display: bridge: Document LVDS to parallel decoder
  drm: bridge: Add LVDS decoder driver
  arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

 .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
 drivers/gpu/drm/bridge/Kconfig                     |   8 ++
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
 5 files changed, 237 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
 create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c

--
2.7.4

[PATCH v2 1/3] dt-bindings: display: bridge: Document LVDS to parallel decoder

[Jacopo Mondi]

Document transparent LVDS to CMOS/TTL decoder that do not require any
configuration.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/display/bridge/lvds-decoder.txt       | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt b/Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
new file mode 100644
index 0000000..a9b1d9e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
@@ -0,0 +1,42 @@
+LVDS to Parallel CMOS/TTL decoder
+---------------------------------
+
+Bindings for transparent LVDS to CMOS/TTL parallel data decoder that don't
+require any configuration.
+
+Required properties:
+- compatible: shall be "lvds-decoder"
+
+The LVDS decoder has two video ports, whose connections are modeled according
+to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
+
+- Port@0: LVDS input port
+- Port@1: Digital CMOS/TTL parallel output
+
+Example:
+-------
+
+	lvds,decoder: decoder-0 {
+		compatible = "lvds-decoder";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				lvds_dec_in: endpoint {
+					remote-endpoint = <&lvds_out>;
+				};
+			};
+
+			port@1{
+				reg = <1>;
+
+				lvds_dec_out: endpoint {
+					remote-endpoint = <&adv7511_in>;
+				};
+			};
+		};
+	};
-- 
2.7.4

[PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Jacopo Mondi]

Add transparent LVDS decoder driver.

A transparent LVDS decoder is a DRM bridge device that does not require
any configuration and converts LVDS input to digital CMOS/TTL parallel
data output.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/bridge/Kconfig        |   8 +++
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/lvds-decoder.c | 129 ++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3b99d5a..e52a5af 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
 	help
 	  Support for RGB to VGA DAC based bridges

+config DRM_LVDS_DECODER
+	tristate "Transparent LVDS to parallel decoder support"
+	depends on OF
+	select DRM_PANEL_BRIDGE
+	help
+	  Support for transparent LVDS to parallel decoders that don't require
+	  any configuration.
+
 config DRM_LVDS_ENCODER
 	tristate "Transparent parallel to LVDS encoder support"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..edc2332 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
+obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
 obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c b/drivers/gpu/drm/bridge/lvds-decoder.c
new file mode 100644
index 0000000..319f4d5
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lvds-decoder.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+#include <linux/of_graph.h>
+
+struct lvds_decoder {
+	struct device *dev;
+
+	struct drm_bridge bridge;
+	struct drm_bridge *next;
+	struct drm_encoder *bridge_encoder;
+};
+
+static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct lvds_decoder, bridge);
+}
+
+static int lvds_decoder_attach(struct drm_bridge *bridge)
+{
+	struct lvds_decoder *lvds = to_lvds_decoder(bridge);
+
+	return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
+}
+
+struct drm_bridge_funcs lvds_dec_bridge_func = {
+	.attach	= lvds_decoder_attach,
+};
+
+static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
+{
+	struct device_node *lvds_output;
+	struct device_node *remote;
+	int ret = 0;
+
+	lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
+	if (!lvds_output) {
+		dev_err(lvds->dev, "Missing endpoint in Port@1\n");
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(lvds_output);
+	if (!remote) {
+		dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
+		ret = -ENODEV;
+		goto error_put_lvds_node;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
+		ret = -ENODEV;
+		goto error_put_remote_node;
+	}
+
+	lvds->next = of_drm_find_bridge(remote);
+	if (!lvds->next)
+		ret = -EPROBE_DEFER;
+
+error_put_remote_node:
+	of_node_put(remote);
+error_put_lvds_node:
+	of_node_put(lvds_output);
+
+	return ret;
+}
+
+static int lvds_decoder_probe(struct platform_device *pdev)
+{
+	struct lvds_decoder *lvds;
+	int ret;
+
+	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
+	if (!lvds)
+		return -ENOMEM;
+
+	lvds->dev = &pdev->dev;
+	platform_set_drvdata(pdev, lvds);
+
+	ret = lvds_decoder_parse_dt(lvds);
+	if (ret)
+		return ret;
+
+	lvds->bridge.driver_private = lvds;
+	lvds->bridge.of_node = pdev->dev.of_node;
+	lvds->bridge.funcs = &lvds_dec_bridge_func;
+
+	drm_bridge_add(&lvds->bridge);
+
+	return 0;
+}
+
+static int lvds_decoder_remove(struct platform_device *pdev)
+{
+	struct lvds_decoder *lvds = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&lvds->bridge);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id lvds_decoder_match[] = {
+	{ .compatible = "lvds-decoder", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lvds_decoder_match);
+#endif
+
+static struct platform_driver lvds_decoder_driver = {
+	.probe	= lvds_decoder_probe,
+	.remove	= lvds_decoder_remove,
+	.driver	= {
+		.name		= "lvds_decoder",
+		.of_match_table	= lvds_decoder_match,
+	},
+};
+module_platform_driver(lvds_decoder_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Transparent LVDS to parallel data decoder");
+MODULE_LICENSE("GPL v2");
--
2.7.4

[PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Jacopo Mondi]

The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
to the on-chip LVDS encoder output on one side and to HDMI encoder
ADV7511w on the other one.

As the decoder does not need any configuration it has been so-far
omitted from DTS. Now that a driver for transparent LVDS decoder is
available, describe it in DT as well.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 31 ++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index c0fd144..0a95c20 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -42,6 +42,33 @@
 			};
 		};
 	};
+
+	lvds,decoder {
+		compatible = "lvds-decoder";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				lvds_dec_in: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+
+			port@1{
+				reg = <1>;
+
+				lvds_dec_out: endpoint {
+					remote-endpoint = <&adv7511_in>;
+				};
+
+			};
+
+		};
+	};
 };
 
 &avb {
@@ -98,7 +125,7 @@
 			port@0 {
 				reg = <0>;
 				adv7511_in: endpoint {
-					remote-endpoint = <&lvds0_out>;
+					remote-endpoint = <&lvds_dec_out>;
 				};
 			};
 
@@ -153,7 +180,7 @@
 	ports {
 		port@1 {
 			endpoint {
-				remote-endpoint = <&adv7511_in>;
+				remote-endpoint = <&lvds_dec_in>;
 			};
 		};
 	};
-- 
2.7.4

Re: [PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Simon Horman]

On Fri, Mar 09, 2018 at 02:51:40PM +0100, Jacopo Mondi wrote:
> The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
> to the on-chip LVDS encoder output on one side and to HDMI encoder
> ADV7511w on the other one.
> 
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver for transparent LVDS decoder is
> available, describe it in DT as well.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks, I have marked this as deferred pending acceptance of the bindings.
Please repost or otherwise ping me once they have been accepted.

Re: [PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Sergei Shtylyov]

On 03/09/2018 04:51 PM, Jacopo Mondi wrote:

> The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
> to the on-chip LVDS encoder output on one side and to HDMI encoder
> ADV7511w on the other one.
> 
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver for transparent LVDS decoder is
> available, describe it in DT as well.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 31 ++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index c0fd144..0a95c20 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -42,6 +42,33 @@
>  			};
>  		};
>  	};
> +
> +	lvds,decoder {

   I said hyphen (-), not comma (,)! :-)

[...]

MBR, Sergei

Re: [PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Sergei Shtylyov]

On 03/09/2018 04:51 PM, Jacopo Mondi wrote:

> The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
> to the on-chip LVDS encoder output on one side and to HDMI encoder
> ADV7511w on the other one.
> 
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver for transparent LVDS decoder is
> available, describe it in DT as well.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 31 ++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index c0fd144..0a95c20 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -42,6 +42,33 @@
>  			};
>  		};
>  	};
> +
> +	lvds,decoder {

   I said hyphen (-), not comma (,)! :-)

[...]

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Archit Taneja]

Hi,

On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> Hello,
>     after some discussion on the proposed bindings for generic lvds decoder and
> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> a transparent decoder that does not support any configuration from DT.
> 
> Dropping THC63 support to avoid discussion on how to better implement support
> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> I find difficult to implement support for bridges with multiple input endpoints)
> 
> Same base branch as v1, with same patches for V3M Eagle applied on top.
> git://jmondi.org/linux v3m/v4.16-rc3/base
> 
> Thanks
>     j
> 
> v1 -> v2:
> - Drop support for THC63LVD1024
> 
> [1] I had a quick at how to model a DRM bridge with multiple input
> ports, and I see a blocker in how DRM identifies and matches bridges using
> the devices node in place of the endpoint nodes.
> 
> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> a few ways to support that:
>   1) register 2 drm bridges from the same driver (one for each input/output pair)
>      but they would both be matches on the same device node when the preceding
>      bridge calls "of_drm_find_bridge()".

I think this is the way to go. DRM doesn't say anywhere that we can't 
have 2 drm_bridge-s contained in a single device. About the issue with
of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. 
 From what I know, we don't necessarily need to set the bridge's of_node
to the device (i.e, thschip) itself.

thschip {
	...
	ports {
		bridge1: port@0 {
			...
		};

		bridge2: port@1 {
			...
		};
	};
};


Thanks,
Archit

>   2) register a single bridge with multiple "next bridges", but when the bridge
>      gets attached I don't see a way on how to identify on which next bridge
>      "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>      has been attached on first, and we don't have that information.
>   3) Register more instances of the same chip in DTS, one for each input/output
>      pair. They gonna share supplies and gpios, and I don't like that.
> 
> I had a quick look at the currently in mainline bridges and none of them has
> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> in use in any DTS. I guess the problem has been already debated and maybe solved
> in the past, so feel free to point me to other sources.
> 
> Jacopo Mondi (3):
>    dt-bindings: display: bridge: Document LVDS to parallel decoder
>    drm: bridge: Add LVDS decoder driver
>    arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> 
>   .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>   arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>   drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>   drivers/gpu/drm/bridge/Makefile                    |   1 +
>   drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>   5 files changed, 237 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>   create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> --
> 2.7.4
> 

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Archit Taneja]

Hi,

On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> Hello,
>     after some discussion on the proposed bindings for generic lvds decoder and
> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> a transparent decoder that does not support any configuration from DT.
> 
> Dropping THC63 support to avoid discussion on how to better implement support
> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> I find difficult to implement support for bridges with multiple input endpoints)
> 
> Same base branch as v1, with same patches for V3M Eagle applied on top.
> git://jmondi.org/linux v3m/v4.16-rc3/base
> 
> Thanks
>     j
> 
> v1 -> v2:
> - Drop support for THC63LVD1024
> 
> [1] I had a quick at how to model a DRM bridge with multiple input
> ports, and I see a blocker in how DRM identifies and matches bridges using
> the devices node in place of the endpoint nodes.
> 
> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> a few ways to support that:
>   1) register 2 drm bridges from the same driver (one for each input/output pair)
>      but they would both be matches on the same device node when the preceding
>      bridge calls "of_drm_find_bridge()".

I think this is the way to go. DRM doesn't say anywhere that we can't 
have 2 drm_bridge-s contained in a single device. About the issue with
of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. 
 From what I know, we don't necessarily need to set the bridge's of_node
to the device (i.e, thschip) itself.

thschip {
	...
	ports {
		bridge1: port@0 {
			...
		};

		bridge2: port@1 {
			...
		};
	};
};


Thanks,
Archit

>   2) register a single bridge with multiple "next bridges", but when the bridge
>      gets attached I don't see a way on how to identify on which next bridge
>      "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>      has been attached on first, and we don't have that information.
>   3) Register more instances of the same chip in DTS, one for each input/output
>      pair. They gonna share supplies and gpios, and I don't like that.
> 
> I had a quick look at the currently in mainline bridges and none of them has
> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> in use in any DTS. I guess the problem has been already debated and maybe solved
> in the past, so feel free to point me to other sources.
> 
> Jacopo Mondi (3):
>    dt-bindings: display: bridge: Document LVDS to parallel decoder
>    drm: bridge: Add LVDS decoder driver
>    arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> 
>   .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>   arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>   drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>   drivers/gpu/drm/bridge/Makefile                    |   1 +
>   drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>   5 files changed, 237 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>   create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> --
> 2.7.4
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[jacopo mondi]

Hi Sergei,

On Fri, Mar 09, 2018 at 08:30:36PM +0300, Sergei Shtylyov wrote:
> On 03/09/2018 04:51 PM, Jacopo Mondi wrote:
>
> > The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
> > to the on-chip LVDS encoder output on one side and to HDMI encoder
> > ADV7511w on the other one.
> >
> > As the decoder does not need any configuration it has been so-far
> > omitted from DTS. Now that a driver for transparent LVDS decoder is
> > available, describe it in DT as well.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 31 ++++++++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > index c0fd144..0a95c20 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > @@ -42,6 +42,33 @@
> >  			};
> >  		};
> >  	};
> > +
> > +	lvds,decoder {
>
>    I said hyphen (-), not comma (,)! :-)

Ahah ok, my bad, I somehow confused the two :)
It looked weird to me, but I thought you were pointing me to some
convention I didn't know about!

I'll change this in v3

Thanks
  j
>
> [...]
>
> MBR, Sergei

Re: [PATCH v2 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[jacopo mondi]

Hi Sergei,

On Fri, Mar 09, 2018 at 08:30:36PM +0300, Sergei Shtylyov wrote:
> On 03/09/2018 04:51 PM, Jacopo Mondi wrote:
>
> > The R-Car V3M Eagle board includes a transparent LVDS decoder, connected
> > to the on-chip LVDS encoder output on one side and to HDMI encoder
> > ADV7511w on the other one.
> >
> > As the decoder does not need any configuration it has been so-far
> > omitted from DTS. Now that a driver for transparent LVDS decoder is
> > available, describe it in DT as well.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 31 ++++++++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > index c0fd144..0a95c20 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > @@ -42,6 +42,33 @@
> >  			};
> >  		};
> >  	};
> > +
> > +	lvds,decoder {
>
>    I said hyphen (-), not comma (,)! :-)

Ahah ok, my bad, I somehow confused the two :)
It looked weird to me, but I thought you were pointing me to some
convention I didn't know about!

I'll change this in v3

Thanks
  j
>
> [...]
>
> MBR, Sergei
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[jacopo mondi]

Hello Archit,

On Sat, Mar 10, 2018 at 11:23:19AM +0530, Archit Taneja wrote:
> Hi,
>
> On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> >Hello,
> >    after some discussion on the proposed bindings for generic lvds decoder and
> >Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> >a transparent decoder that does not support any configuration from DT.
> >
> >Dropping THC63 support to avoid discussion on how to better implement support
> >for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> >bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> >I find difficult to implement support for bridges with multiple input endpoints)
> >
> >Same base branch as v1, with same patches for V3M Eagle applied on top.
> >git://jmondi.org/linux v3m/v4.16-rc3/base
> >
> >Thanks
> >    j
> >
> >v1 -> v2:
> >- Drop support for THC63LVD1024
> >
> >[1] I had a quick at how to model a DRM bridge with multiple input
> >ports, and I see a blocker in how DRM identifies and matches bridges using
> >the devices node in place of the endpoint nodes.
> >
> >As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> >a few ways to support that:
> >  1) register 2 drm bridges from the same driver (one for each input/output pair)
> >     but they would both be matches on the same device node when the preceding
> >     bridge calls "of_drm_find_bridge()".
>
> I think this is the way to go. DRM doesn't say anywhere that we can't have 2
> drm_bridge-s contained in a single device. About the issue with
> of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
> the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. From
> what I know, we don't necessarily need to set the bridge's of_node
> to the device (i.e, thschip) itself.

That's fine, but this implies that the preceding bridge (or encoder) in the
DRM pipeline has then to collect the "port" node and match on it
specifically when it attaches this driver. This introduces an ad-hoc
policy that prevents this driver from being easily integrated in
existing pipelines (where bridges are matched on the device node).

Also, I don't know much about the DRM framework, but it seems to me
that the helper function designed to find the next component to attach
to in the DRM pipeline is:

drm_of_find_panel_or_bridge():
        remote = of_graph_get_remote_node();
                ep = of_graph_get_endpoint_by_regs();
                return of_graph_get_remote_port_parent();  <-- This returns the device node
        of_drm_find_panel(remote);
        of_drm_find_bridge(remote);

I so dare to say matching on device node is kind of the intended way
to do things in DRM, and this driver with two endpoints that wants to
be matched on port nodes would be kind of special one that does not
work as other driver expects to.

Is my understanding correct, or have I misinterpreted something?

Thanks
   j

>
> thschip {
> 	...
> 	ports {
> 		bridge1: port@0 {
> 			...
> 		};
>
> 		bridge2: port@1 {
> 			...
> 		};
> 	};
> };
>
>
> Thanks,
> Archit
>
> >  2) register a single bridge with multiple "next bridges", but when the bridge
> >     gets attached I don't see a way on how to identify on which next bridge
> >     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
> >     has been attached on first, and we don't have that information.
> >  3) Register more instances of the same chip in DTS, one for each input/output
> >     pair. They gonna share supplies and gpios, and I don't like that.
> >
> >I had a quick look at the currently in mainline bridges and none of them has
> >multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> >in use in any DTS. I guess the problem has been already debated and maybe solved
> >in the past, so feel free to point me to other sources.
> >
> >Jacopo Mondi (3):
> >   dt-bindings: display: bridge: Document LVDS to parallel decoder
> >   drm: bridge: Add LVDS decoder driver
> >   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> >
> >  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> >  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
> >  5 files changed, 237 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> >
> >--
> >2.7.4
> >

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[jacopo mondi]

Hello Archit,

On Sat, Mar 10, 2018 at 11:23:19AM +0530, Archit Taneja wrote:
> Hi,
>
> On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> >Hello,
> >    after some discussion on the proposed bindings for generic lvds decoder and
> >Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> >a transparent decoder that does not support any configuration from DT.
> >
> >Dropping THC63 support to avoid discussion on how to better implement support
> >for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> >bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> >I find difficult to implement support for bridges with multiple input endpoints)
> >
> >Same base branch as v1, with same patches for V3M Eagle applied on top.
> >git://jmondi.org/linux v3m/v4.16-rc3/base
> >
> >Thanks
> >    j
> >
> >v1 -> v2:
> >- Drop support for THC63LVD1024
> >
> >[1] I had a quick at how to model a DRM bridge with multiple input
> >ports, and I see a blocker in how DRM identifies and matches bridges using
> >the devices node in place of the endpoint nodes.
> >
> >As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> >a few ways to support that:
> >  1) register 2 drm bridges from the same driver (one for each input/output pair)
> >     but they would both be matches on the same device node when the preceding
> >     bridge calls "of_drm_find_bridge()".
>
> I think this is the way to go. DRM doesn't say anywhere that we can't have 2
> drm_bridge-s contained in a single device. About the issue with
> of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
> the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. From
> what I know, we don't necessarily need to set the bridge's of_node
> to the device (i.e, thschip) itself.

That's fine, but this implies that the preceding bridge (or encoder) in the
DRM pipeline has then to collect the "port" node and match on it
specifically when it attaches this driver. This introduces an ad-hoc
policy that prevents this driver from being easily integrated in
existing pipelines (where bridges are matched on the device node).

Also, I don't know much about the DRM framework, but it seems to me
that the helper function designed to find the next component to attach
to in the DRM pipeline is:

drm_of_find_panel_or_bridge():
        remote = of_graph_get_remote_node();
                ep = of_graph_get_endpoint_by_regs();
                return of_graph_get_remote_port_parent();  <-- This returns the device node
        of_drm_find_panel(remote);
        of_drm_find_bridge(remote);

I so dare to say matching on device node is kind of the intended way
to do things in DRM, and this driver with two endpoints that wants to
be matched on port nodes would be kind of special one that does not
work as other driver expects to.

Is my understanding correct, or have I misinterpreted something?

Thanks
   j

>
> thschip {
> 	...
> 	ports {
> 		bridge1: port@0 {
> 			...
> 		};
>
> 		bridge2: port@1 {
> 			...
> 		};
> 	};
> };
>
>
> Thanks,
> Archit
>
> >  2) register a single bridge with multiple "next bridges", but when the bridge
> >     gets attached I don't see a way on how to identify on which next bridge
> >     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
> >     has been attached on first, and we don't have that information.
> >  3) Register more instances of the same chip in DTS, one for each input/output
> >     pair. They gonna share supplies and gpios, and I don't like that.
> >
> >I had a quick look at the currently in mainline bridges and none of them has
> >multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> >in use in any DTS. I guess the problem has been already debated and maybe solved
> >in the past, so feel free to point me to other sources.
> >
> >Jacopo Mondi (3):
> >   dt-bindings: display: bridge: Document LVDS to parallel decoder
> >   drm: bridge: Add LVDS decoder driver
> >   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> >
> >  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> >  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
> >  5 files changed, 237 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> >
> >--
> >2.7.4
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Andrzej Hajda]

On 09.03.2018 14:51, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
>
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.

Neither code, neither bindings are LVDS specific, this is driver for any
bridge w/o configuration.
I  am not sure how many bridges does not need configuration, usually
they need to be powered at least.
I guess in many cases power line is always on, but this is board
specific not device specific.

Looking at previous version of the patchset it looks little bit as a
hack, and certainly it will be a hack in case of R-Car Gen3 V3M Eagle,
which I guess you want to use it.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +++
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 ++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>  	help
>  	  Support for RGB to VGA DAC based bridges
>
> +config DRM_LVDS_DECODER
> +	tristate "Transparent LVDS to parallel decoder support"
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	help
> +	  Support for transparent LVDS to parallel decoders that don't require
> +	  any configuration.
> +
>  config DRM_LVDS_ENCODER
>  	tristate "Transparent parallel to LVDS encoder support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 0000000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/of_graph.h>
> +
> +struct lvds_decoder {
> +	struct device *dev;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +	struct drm_encoder *bridge_encoder;

Unused field.


Regards
Andrzej

> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {
> +	.attach	= lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> +	struct device_node *lvds_output;
> +	struct device_node *remote;
> +	int ret = 0;
> +
> +	lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> +	if (!lvds_output) {
> +		dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(lvds_output);
> +	if (!remote) {
> +		dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_lvds_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	lvds->next = of_drm_find_bridge(remote);
> +	if (!lvds->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_lvds_node:
> +	of_node_put(lvds_output);
> +
> +	return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds;
> +	int ret;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, lvds);
> +
> +	ret = lvds_decoder_parse_dt(lvds);
> +	if (ret)
> +		return ret;
> +
> +	lvds->bridge.driver_private = lvds;
> +	lvds->bridge.of_node = pdev->dev.of_node;
> +	lvds->bridge.funcs = &lvds_dec_bridge_func;
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lvds_decoder_match[] = {
> +	{ .compatible = "lvds-decoder", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lvds_decoder_match);
> +#endif
> +
> +static struct platform_driver lvds_decoder_driver = {
> +	.probe	= lvds_decoder_probe,
> +	.remove	= lvds_decoder_remove,
> +	.driver	= {
> +		.name		= "lvds_decoder",
> +		.of_match_table	= lvds_decoder_match,
> +	},
> +};
> +module_platform_driver(lvds_decoder_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Transparent LVDS to parallel data decoder");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
>
>
>

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Andrzej Hajda]

On 09.03.2018 14:51, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
>
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.

Neither code, neither bindings are LVDS specific, this is driver for any
bridge w/o configuration.
I  am not sure how many bridges does not need configuration, usually
they need to be powered at least.
I guess in many cases power line is always on, but this is board
specific not device specific.

Looking at previous version of the patchset it looks little bit as a
hack, and certainly it will be a hack in case of R-Car Gen3 V3M Eagle,
which I guess you want to use it.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +++
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 ++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>  	help
>  	  Support for RGB to VGA DAC based bridges
>
> +config DRM_LVDS_DECODER
> +	tristate "Transparent LVDS to parallel decoder support"
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	help
> +	  Support for transparent LVDS to parallel decoders that don't require
> +	  any configuration.
> +
>  config DRM_LVDS_ENCODER
>  	tristate "Transparent parallel to LVDS encoder support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 0000000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/of_graph.h>
> +
> +struct lvds_decoder {
> +	struct device *dev;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +	struct drm_encoder *bridge_encoder;

Unused field.


Regards
Andrzej

> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {
> +	.attach	= lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> +	struct device_node *lvds_output;
> +	struct device_node *remote;
> +	int ret = 0;
> +
> +	lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> +	if (!lvds_output) {
> +		dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(lvds_output);
> +	if (!remote) {
> +		dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_lvds_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	lvds->next = of_drm_find_bridge(remote);
> +	if (!lvds->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_lvds_node:
> +	of_node_put(lvds_output);
> +
> +	return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds;
> +	int ret;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, lvds);
> +
> +	ret = lvds_decoder_parse_dt(lvds);
> +	if (ret)
> +		return ret;
> +
> +	lvds->bridge.driver_private = lvds;
> +	lvds->bridge.of_node = pdev->dev.of_node;
> +	lvds->bridge.funcs = &lvds_dec_bridge_func;
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lvds_decoder_match[] = {
> +	{ .compatible = "lvds-decoder", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lvds_decoder_match);
> +#endif
> +
> +static struct platform_driver lvds_decoder_driver = {
> +	.probe	= lvds_decoder_probe,
> +	.remove	= lvds_decoder_remove,
> +	.driver	= {
> +		.name		= "lvds_decoder",
> +		.of_match_table	= lvds_decoder_match,
> +	},
> +};
> +module_platform_driver(lvds_decoder_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Transparent LVDS to parallel data decoder");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
>
>
>

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 09.03.2018 14:51, Jacopo Mondi wrote:
> Hello,
>    after some discussion on the proposed bindings for generic lvds decoder and
> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> a transparent decoder that does not support any configuration from DT.
>
> Dropping THC63 support to avoid discussion on how to better implement support
> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> I find difficult to implement support for bridges with multiple input endpoints)
>
> Same base branch as v1, with same patches for V3M Eagle applied on top.
> git://jmondi.org/linux v3m/v4.16-rc3/base
>
> Thanks
>    j
>
> v1 -> v2:
> - Drop support for THC63LVD1024
>
> [1] I had a quick at how to model a DRM bridge with multiple input
> ports, and I see a blocker in how DRM identifies and matches bridges using
> the devices node in place of the endpoint nodes.
>
> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> a few ways to support that:
>  1) register 2 drm bridges from the same driver (one for each input/output pair)
>     but they would both be matches on the same device node when the preceding
>     bridge calls "of_drm_find_bridge()".
>  2) register a single bridge with multiple "next bridges", but when the bridge
>     gets attached I don't see a way on how to identify on which next bridge
>     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>     has been attached on first, and we don't have that information.
>  3) Register more instances of the same chip in DTS, one for each input/output
>     pair. They gonna share supplies and gpios, and I don't like that.
>
> I had a quick look at the currently in mainline bridges and none of them has
> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> in use in any DTS. I guess the problem has been already debated and maybe solved
> in the past, so feel free to point me to other sources.

I think this is is a step in wrong direction, IMHO. Your previous
patchset was quite OK, at least bindings, IMHO. Few things needed only
polishing.
Here we have unmanaged/transparent bridge, which is totally different,
what happened to regulators and gpios from previous iteration.
I do not have schematics of the board, but I guess respective pins of
the bridge must be connected somehow.
I think the problem you want to avoid (double bridge) should not be a
problem at all.
I suppose the most important is to have correct bindings - as they need
to be stable.
If you really must to do hacks better is to put them into driver.

Regards
Andrzej

>
> Jacopo Mondi (3):
>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>   drm: bridge: Add LVDS decoder driver
>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>
>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>  5 files changed, 237 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>
> --
> 2.7.4
>
>
>
>

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 09.03.2018 14:51, Jacopo Mondi wrote:
> Hello,
>    after some discussion on the proposed bindings for generic lvds decoder and
> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> a transparent decoder that does not support any configuration from DT.
>
> Dropping THC63 support to avoid discussion on how to better implement support
> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> I find difficult to implement support for bridges with multiple input endpoints)
>
> Same base branch as v1, with same patches for V3M Eagle applied on top.
> git://jmondi.org/linux v3m/v4.16-rc3/base
>
> Thanks
>    j
>
> v1 -> v2:
> - Drop support for THC63LVD1024
>
> [1] I had a quick at how to model a DRM bridge with multiple input
> ports, and I see a blocker in how DRM identifies and matches bridges using
> the devices node in place of the endpoint nodes.
>
> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> a few ways to support that:
>  1) register 2 drm bridges from the same driver (one for each input/output pair)
>     but they would both be matches on the same device node when the preceding
>     bridge calls "of_drm_find_bridge()".
>  2) register a single bridge with multiple "next bridges", but when the bridge
>     gets attached I don't see a way on how to identify on which next bridge
>     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>     has been attached on first, and we don't have that information.
>  3) Register more instances of the same chip in DTS, one for each input/output
>     pair. They gonna share supplies and gpios, and I don't like that.
>
> I had a quick look at the currently in mainline bridges and none of them has
> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> in use in any DTS. I guess the problem has been already debated and maybe solved
> in the past, so feel free to point me to other sources.

I think this is is a step in wrong direction, IMHO. Your previous
patchset was quite OK, at least bindings, IMHO. Few things needed only
polishing.
Here we have unmanaged/transparent bridge, which is totally different,
what happened to regulators and gpios from previous iteration.
I do not have schematics of the board, but I guess respective pins of
the bridge must be connected somehow.
I think the problem you want to avoid (double bridge) should not be a
problem at all.
I suppose the most important is to have correct bindings - as they need
to be stable.
If you really must to do hacks better is to put them into driver.

Regards
Andrzej

>
> Jacopo Mondi (3):
>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>   drm: bridge: Add LVDS decoder driver
>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>
>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>  5 files changed, 237 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>
> --
> 2.7.4
>
>
>
>

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]

Hi Andrzej,

On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
> On 09.03.2018 14:51, Jacopo Mondi wrote:
> > Hello,
> >    after some discussion on the proposed bindings for generic lvds decoder and
> > Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
> > a transparent decoder that does not support any configuration from DT.
> >
> > Dropping THC63 support to avoid discussion on how to better implement support
> > for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
> > bridges as explained in v1 cover letter (for DRM people: please see [1] as why
> > I find difficult to implement support for bridges with multiple input endpoints)
> >
> > Same base branch as v1, with same patches for V3M Eagle applied on top.
> > git://jmondi.org/linux v3m/v4.16-rc3/base
> >
> > Thanks
> >    j
> >
> > v1 -> v2:
> > - Drop support for THC63LVD1024
> >
> > [1] I had a quick at how to model a DRM bridge with multiple input
> > ports, and I see a blocker in how DRM identifies and matches bridges using
> > the devices node in place of the endpoint nodes.
> >
> > As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
> > a few ways to support that:
> >  1) register 2 drm bridges from the same driver (one for each input/output pair)
> >     but they would both be matches on the same device node when the preceding
> >     bridge calls "of_drm_find_bridge()".
> >  2) register a single bridge with multiple "next bridges", but when the bridge
> >     gets attached I don't see a way on how to identify on which next bridge
> >     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
> >     has been attached on first, and we don't have that information.
> >  3) Register more instances of the same chip in DTS, one for each input/output
> >     pair. They gonna share supplies and gpios, and I don't like that.
> >
> > I had a quick look at the currently in mainline bridges and none of them has
> > multiple input endpoints, except for HDMI audio endpoint, which I haven't found
> > in use in any DTS. I guess the problem has been already debated and maybe solved
> > in the past, so feel free to point me to other sources.
>
> I think this is is a step in wrong direction, IMHO. Your previous
> patchset was quite OK, at least bindings, IMHO. Few things needed only
> polishing.
> Here we have unmanaged/transparent bridge, which is totally different,
> what happened to regulators and gpios from previous iteration.
> I do not have schematics of the board, but I guess respective pins of
> the bridge must be connected somehow.
> I think the problem you want to avoid (double bridge) should not be a
> problem at all.
> I suppose the most important is to have correct bindings - as they need
> to be stable.
> If you really must to do hacks better is to put them into driver.
>

I understand. The "transparent bridge" is of no actual use, but I don't see
how the "double bridge" thing is not an issue if I were to develop
support for the actual Thine chip.

Please see my reply from yesterday to Archit. I still think having two
bridges is somehow an issue...

While we clarify that, would it be fine an initial driver version for
the actualt Thine chip with a single input support[1]? I would ditch this
transparent bridge and resume working on a THC63LVD1024 driver from
comments received on v1.

Thanks
  j

[1] Single input support implies a single input port in DT bindings
even if the chip supports two, and my understanding was that you
didn't like this.


> Regards
> Andrzej
>
> >
> > Jacopo Mondi (3):
> >   dt-bindings: display: bridge: Document LVDS to parallel decoder
> >   drm: bridge: Add LVDS decoder driver
> >   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> >
> >  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> >  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
> >  5 files changed, 237 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> >
> > --
> > 2.7.4
> >
> >
> >
> >
>

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 12.03.2018 13:30, jacopo mondi wrote:
> Hi Andrzej,
>
> On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
>> On 09.03.2018 14:51, Jacopo Mondi wrote:
>>> Hello,
>>>    after some discussion on the proposed bindings for generic lvds decoder and
>>> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
>>> a transparent decoder that does not support any configuration from DT.
>>>
>>> Dropping THC63 support to avoid discussion on how to better implement support
>>> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
>>> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
>>> I find difficult to implement support for bridges with multiple input endpoints)
>>>
>>> Same base branch as v1, with same patches for V3M Eagle applied on top.
>>> git://jmondi.org/linux v3m/v4.16-rc3/base
>>>
>>> Thanks
>>>    j
>>>
>>> v1 -> v2:
>>> - Drop support for THC63LVD1024
>>>
>>> [1] I had a quick at how to model a DRM bridge with multiple input
>>> ports, and I see a blocker in how DRM identifies and matches bridges using
>>> the devices node in place of the endpoint nodes.
>>>
>>> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
>>> a few ways to support that:
>>>  1) register 2 drm bridges from the same driver (one for each input/output pair)
>>>     but they would both be matches on the same device node when the preceding
>>>     bridge calls "of_drm_find_bridge()".
>>>  2) register a single bridge with multiple "next bridges", but when the bridge
>>>     gets attached I don't see a way on how to identify on which next bridge
>>>     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>>>     has been attached on first, and we don't have that information.
>>>  3) Register more instances of the same chip in DTS, one for each input/output
>>>     pair. They gonna share supplies and gpios, and I don't like that.
>>>
>>> I had a quick look at the currently in mainline bridges and none of them has
>>> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
>>> in use in any DTS. I guess the problem has been already debated and maybe solved
>>> in the past, so feel free to point me to other sources.
>> I think this is is a step in wrong direction, IMHO. Your previous
>> patchset was quite OK, at least bindings, IMHO. Few things needed only
>> polishing.
>> Here we have unmanaged/transparent bridge, which is totally different,
>> what happened to regulators and gpios from previous iteration.
>> I do not have schematics of the board, but I guess respective pins of
>> the bridge must be connected somehow.
>> I think the problem you want to avoid (double bridge) should not be a
>> problem at all.
>> I suppose the most important is to have correct bindings - as they need
>> to be stable.
>> If you really must to do hacks better is to put them into driver.
>>
> I understand. The "transparent bridge" is of no actual use, but I don't see
> how the "double bridge" thing is not an issue if I were to develop
> support for the actual Thine chip.

What is exactly your configuration: single/dual input, single/dual output?
Current bindings suggests single/single, am I right? In such case you do
not need to implement dual link functionality in the driver - since you
even do not have possibility to test it.
But the bindings should support all modes of operation, or at least
should be easy to extend them in the future with backward compatibility.

>
> Please see my reply from yesterday to Archit. I still think having two
> bridges is somehow an issue...

Yes, I agree. But do we have such case? If no - no problem ATM :), if
yes - lets try to implement it and see where is the problem, as Archit
already suggested it would be just a matter of assigning bridge to port
node, instead of device node.

>
> While we clarify that, would it be fine an initial driver version for
> the actualt Thine chip with a single input support[1]? I would ditch this
> transparent bridge and resume working on a THC63LVD1024 driver from
> comments received on v1.

I think, yes: driver with only single input, and full or extend-able
bindings.

>
> Thanks
>   j
>
> [1] Single input support implies a single input port in DT bindings
> even if the chip supports two, and my understanding was that you
> didn't like this.

I am sorry if I was not clear enough, only thing I wanted was complete
or at least consistent/extend-able binding.
So when I saw word "multiple LVDS streams" in bindings I was looking
further to see how these multiple LVDS bindings are defined, and found
nothing :)
Btw I think it may be better to use "Dual Link" instead of "Multiple
streams", it is more precise and quite well established in docs.

Regards
Andrzej

>
>
>> Regards
>> Andrzej
>>
>>> Jacopo Mondi (3):
>>>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>>>   drm: bridge: Add LVDS decoder driver
>>>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>>>
>>>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>>>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>>>  5 files changed, 237 insertions(+), 2 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>>>
>>> --
>>> 2.7.4
>>>
>>>
>>>
>>>

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Andrzej Hajda]

On 12.03.2018 13:30, jacopo mondi wrote:
> Hi Andrzej,
>
> On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
>> On 09.03.2018 14:51, Jacopo Mondi wrote:
>>> Hello,
>>>    after some discussion on the proposed bindings for generic lvds decoder and
>>> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
>>> a transparent decoder that does not support any configuration from DT.
>>>
>>> Dropping THC63 support to avoid discussion on how to better implement support
>>> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
>>> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
>>> I find difficult to implement support for bridges with multiple input endpoints)
>>>
>>> Same base branch as v1, with same patches for V3M Eagle applied on top.
>>> git://jmondi.org/linux v3m/v4.16-rc3/base
>>>
>>> Thanks
>>>    j
>>>
>>> v1 -> v2:
>>> - Drop support for THC63LVD1024
>>>
>>> [1] I had a quick at how to model a DRM bridge with multiple input
>>> ports, and I see a blocker in how DRM identifies and matches bridges using
>>> the devices node in place of the endpoint nodes.
>>>
>>> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
>>> a few ways to support that:
>>>  1) register 2 drm bridges from the same driver (one for each input/output pair)
>>>     but they would both be matches on the same device node when the preceding
>>>     bridge calls "of_drm_find_bridge()".
>>>  2) register a single bridge with multiple "next bridges", but when the bridge
>>>     gets attached I don't see a way on how to identify on which next bridge
>>>     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>>>     has been attached on first, and we don't have that information.
>>>  3) Register more instances of the same chip in DTS, one for each input/output
>>>     pair. They gonna share supplies and gpios, and I don't like that.
>>>
>>> I had a quick look at the currently in mainline bridges and none of them has
>>> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
>>> in use in any DTS. I guess the problem has been already debated and maybe solved
>>> in the past, so feel free to point me to other sources.
>> I think this is is a step in wrong direction, IMHO. Your previous
>> patchset was quite OK, at least bindings, IMHO. Few things needed only
>> polishing.
>> Here we have unmanaged/transparent bridge, which is totally different,
>> what happened to regulators and gpios from previous iteration.
>> I do not have schematics of the board, but I guess respective pins of
>> the bridge must be connected somehow.
>> I think the problem you want to avoid (double bridge) should not be a
>> problem at all.
>> I suppose the most important is to have correct bindings - as they need
>> to be stable.
>> If you really must to do hacks better is to put them into driver.
>>
> I understand. The "transparent bridge" is of no actual use, but I don't see
> how the "double bridge" thing is not an issue if I were to develop
> support for the actual Thine chip.

What is exactly your configuration: single/dual input, single/dual output?
Current bindings suggests single/single, am I right? In such case you do
not need to implement dual link functionality in the driver - since you
even do not have possibility to test it.
But the bindings should support all modes of operation, or at least
should be easy to extend them in the future with backward compatibility.

>
> Please see my reply from yesterday to Archit. I still think having two
> bridges is somehow an issue...

Yes, I agree. But do we have such case? If no - no problem ATM :), if
yes - lets try to implement it and see where is the problem, as Archit
already suggested it would be just a matter of assigning bridge to port
node, instead of device node.

>
> While we clarify that, would it be fine an initial driver version for
> the actualt Thine chip with a single input support[1]? I would ditch this
> transparent bridge and resume working on a THC63LVD1024 driver from
> comments received on v1.

I think, yes: driver with only single input, and full or extend-able
bindings.

>
> Thanks
>   j
>
> [1] Single input support implies a single input port in DT bindings
> even if the chip supports two, and my understanding was that you
> didn't like this.

I am sorry if I was not clear enough, only thing I wanted was complete
or at least consistent/extend-able binding.
So when I saw word "multiple LVDS streams" in bindings I was looking
further to see how these multiple LVDS bindings are defined, and found
nothing :)
Btw I think it may be better to use "Dual Link" instead of "Multiple
streams", it is more precise and quite well established in docs.

Regards
Andrzej

>
>
>> Regards
>> Andrzej
>>
>>> Jacopo Mondi (3):
>>>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>>>   drm: bridge: Add LVDS decoder driver
>>>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>>>
>>>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>>>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>>>  5 files changed, 237 insertions(+), 2 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>>>
>>> --
>>> 2.7.4
>>>
>>>
>>>
>>>

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

Hi Andrzej,
    thanks for the detailed reply, much appreciated :)

On Mon, Mar 12, 2018 at 02:47:20PM +0100, Andrzej Hajda wrote:
> On 12.03.2018 13:30, jacopo mondi wrote:
> > Hi Andrzej,
> >
> > On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
> >>

[snip]

> > I understand. The "transparent bridge" is of no actual use, but I don't see
> > how the "double bridge" thing is not an issue if I were to develop
> > support for the actual Thine chip.
>
> What is exactly your configuration: single/dual input, single/dual output?
> Current bindings suggests single/single, am I right? In such case you do
> not need to implement dual link functionality in the driver - since you
> even do not have possibility to test it.

Correct, I'm running in single/single mode.

> But the bindings should support all modes of operation, or at least
> should be easy to extend them in the future with backward compatibility.
>

And here is where I always get lost. For sake of being compatible with
future extensions, bindings shall describe hardware, not what is currently
supported in driver. One day I'll get this right!

> >
> > Please see my reply from yesterday to Archit. I still think having two
> > bridges is somehow an issue...
>
> Yes, I agree. But do we have such case? If no - no problem ATM :), if
> yes - lets try to implement it and see where is the problem, as Archit
> already suggested it would be just a matter of assigning bridge to port
> node, instead of device node.
>

Again, I've been fooled by the idea that if bindings describe
something, the driver should implement it (but I'm still not sure that
"just assign the port node to the bridge" is the right thing to do
here, but let's leave this out for now :)

> >
> > While we clarify that, would it be fine an initial driver version for
> > the actualt Thine chip with a single input support[1]? I would ditch this
> > transparent bridge and resume working on a THC63LVD1024 driver from
> > comments received on v1.
>
> I think, yes: driver with only single input, and full or extend-able
> bindings.
>
> >
> > Thanks
> >   j
> >
> > [1] Single input support implies a single input port in DT bindings
> > even if the chip supports two, and my understanding was that you
> > didn't like this.
>
> I am sorry if I was not clear enough, only thing I wanted was complete
> or at least consistent/extend-able binding.
> So when I saw word "multiple LVDS streams" in bindings I was looking
> further to see how these multiple LVDS bindings are defined, and found
> nothing :)

No worries, you're right here, again it's me confused by bindings
description vs driver features...

> Btw I think it may be better to use "Dual Link" instead of "Multiple
> streams", it is more precise and quite well established in docs.

Ack, I will follow up with a v3 where I'll get rid of generic LVDS
decoder and target the actual chip.

Thanks
   j
>
> Regards
> Andrzej
>
> >
> >
> >> Regards
> >> Andrzej
> >>
> >>> Jacopo Mondi (3):
> >>>   dt-bindings: display: bridge: Document LVDS to parallel decoder
> >>>   drm: bridge: Add LVDS decoder driver
> >>>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> >>>
> >>>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> >>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> >>>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> >>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >>>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
> >>>  5 files changed, 237 insertions(+), 2 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> >>>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> >>>
> >>> --
> >>> 2.7.4
> >>>
> >>>
> >>>
> >>>
>

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[jacopo mondi]

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

Hi Andrzej,
    thanks for the detailed reply, much appreciated :)

On Mon, Mar 12, 2018 at 02:47:20PM +0100, Andrzej Hajda wrote:
> On 12.03.2018 13:30, jacopo mondi wrote:
> > Hi Andrzej,
> >
> > On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
> >>

[snip]

> > I understand. The "transparent bridge" is of no actual use, but I don't see
> > how the "double bridge" thing is not an issue if I were to develop
> > support for the actual Thine chip.
>
> What is exactly your configuration: single/dual input, single/dual output?
> Current bindings suggests single/single, am I right? In such case you do
> not need to implement dual link functionality in the driver - since you
> even do not have possibility to test it.

Correct, I'm running in single/single mode.

> But the bindings should support all modes of operation, or at least
> should be easy to extend them in the future with backward compatibility.
>

And here is where I always get lost. For sake of being compatible with
future extensions, bindings shall describe hardware, not what is currently
supported in driver. One day I'll get this right!

> >
> > Please see my reply from yesterday to Archit. I still think having two
> > bridges is somehow an issue...
>
> Yes, I agree. But do we have such case? If no - no problem ATM :), if
> yes - lets try to implement it and see where is the problem, as Archit
> already suggested it would be just a matter of assigning bridge to port
> node, instead of device node.
>

Again, I've been fooled by the idea that if bindings describe
something, the driver should implement it (but I'm still not sure that
"just assign the port node to the bridge" is the right thing to do
here, but let's leave this out for now :)

> >
> > While we clarify that, would it be fine an initial driver version for
> > the actualt Thine chip with a single input support[1]? I would ditch this
> > transparent bridge and resume working on a THC63LVD1024 driver from
> > comments received on v1.
>
> I think, yes: driver with only single input, and full or extend-able
> bindings.
>
> >
> > Thanks
> >   j
> >
> > [1] Single input support implies a single input port in DT bindings
> > even if the chip supports two, and my understanding was that you
> > didn't like this.
>
> I am sorry if I was not clear enough, only thing I wanted was complete
> or at least consistent/extend-able binding.
> So when I saw word "multiple LVDS streams" in bindings I was looking
> further to see how these multiple LVDS bindings are defined, and found
> nothing :)

No worries, you're right here, again it's me confused by bindings
description vs driver features...

> Btw I think it may be better to use "Dual Link" instead of "Multiple
> streams", it is more precise and quite well established in docs.

Ack, I will follow up with a v3 where I'll get rid of generic LVDS
decoder and target the actual chip.

Thanks
   j
>
> Regards
> Andrzej
>
> >
> >
> >> Regards
> >> Andrzej
> >>
> >>> Jacopo Mondi (3):
> >>>   dt-bindings: display: bridge: Document LVDS to parallel decoder
> >>>   drm: bridge: Add LVDS decoder driver
> >>>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> >>>
> >>>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> >>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> >>>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> >>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
> >>>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
> >>>  5 files changed, 237 insertions(+), 2 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> >>>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> >>>
> >>> --
> >>> 2.7.4
> >>>
> >>>
> >>>
> >>>
>

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

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

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

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Laurent Pinchart]

Hi Jacopo,

On Saturday, 10 March 2018 20:00:31 EET jacopo mondi wrote:
> On Sat, Mar 10, 2018 at 11:23:19AM +0530, Archit Taneja wrote:
> > On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> > > Hello,
> > >
> > > after some discussion on the proposed bindings for generic lvds
> > > decoder and Thine THC63LVD1024, I decided to drop the THC63 specific
> > > part and just live with a transparent decoder that does not support any
> > > configuration from DT.
> > >
> > > Dropping THC63 support to avoid discussion on how to better implement
> > > support for a DRM bridge with 2 input ports and focus on LVDS mode
> > > propagation through bridges as explained in v1 cover letter (for DRM
> > > people: please see [1] as why I find difficult to implement support for
> > > bridges with multiple input endpoints)
> > >
> > > Same base branch as v1, with same patches for V3M Eagle applied on top.
> > > git://jmondi.org/linux v3m/v4.16-rc3/base
> > >
> > > Thanks
> > >
> > >    j
> > >
> > > v1 -> v2:
> > > - Drop support for THC63LVD1024
> > >
> > > [1] I had a quick at how to model a DRM bridge with multiple input
> > > ports, and I see a blocker in how DRM identifies and matches bridges
> > > using the devices node in place of the endpoint nodes.
> > >
> > > As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see
> > > only a few ways to support that:
> > > 
> > >  1) register 2 drm bridges from the same driver (one for each
> > >     input/output pair) but they would both be matches on the same device
> > >     node when the preceding bridge calls "of_drm_find_bridge()".
> > 
> > I think this is the way to go. DRM doesn't say anywhere that we can't have
> > 2 drm_bridge-s contained in a single device.

I agree, but it only works as long as the two bridges are independent. The 
THC63LVD1024 can also work in single-in, dual-out and dual-in, single-out 
modes, in which case we will need a more generic solution. I'm fine ignoring 
the problem on the driver side for now until someone needs to support such a 
setup, but the DT bindings need to be right.

> > About the issue with
> > of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
> > the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. From
> > what I know, we don't necessarily need to set the bridge's of_node
> > to the device (i.e, thschip) itself.
> 
> That's fine, but this implies that the preceding bridge (or encoder) in the
> DRM pipeline has then to collect the "port" node and match on it
> specifically when it attaches this driver. This introduces an ad-hoc
> policy that prevents this driver from being easily integrated in
> existing pipelines (where bridges are matched on the device node).
> 
> Also, I don't know much about the DRM framework, but it seems to me
> that the helper function designed to find the next component to attach
> to in the DRM pipeline is:
> 
> drm_of_find_panel_or_bridge():
>         remote = of_graph_get_remote_node();
>                 ep = of_graph_get_endpoint_by_regs();
>                 return of_graph_get_remote_port_parent();  <-- This returns
> the device node of_drm_find_panel(remote);
>         of_drm_find_bridge(remote);
> 
> I so dare to say matching on device node is kind of the intended way
> to do things in DRM, and this driver with two endpoints that wants to
> be matched on port nodes would be kind of special one that does not
> work as other driver expects to.
> 
> Is my understanding correct, or have I misinterpreted something?

I agree with you, so I hope your understanding is correct :-) We have the 
exact same issue in V4L2 where we used to always match on the device node, but 
recently the ADV7482 required matching on port nodes. The solution that was 
proposed was to support matching on both the device node and the port nodes, 
with automatic lookup of the device node from the port node if no direct match 
is found. I think this would work well in DRM too. Regardless of what we 
decide to do, we need to ensure that we don't create two separate categories 
of bridges with different matching rules that won't be compatible.

> > thschip {
> > 	...
> > 	ports {
> > 		bridge1: port@0 {
> > 			...
> > 		};
> > 		bridge2: port@1 {
> > 			...
> > 		};
> > 	};
> > };
> > 
> > >  2) register a single bridge with multiple "next bridges", but when the
> > >     bridge gets attached I don't see a way on how to identify on which
> > >     next bridge "drm_bridge_attach()" on, as it depends on the endpoint
> > >     the current bridge has been attached on first, and we don't have
> > >     that information.

For the general case I think we'll need to start adding port index arguments 
to the bridge API, but as stated above I'm fine ignoring this for now and 
creating two bridges.

> > >  3) Register more instances of the same chip in DTS, one for each
> > >     input/output pair. They gonna share supplies and gpios, and I don't
> > >     like that.

I don't think this is a good idea.

> > > I had a quick look at the currently in mainline bridges and none of them
> > > has multiple input endpoints, except for HDMI audio endpoint, which I
> > > haven't found in use in any DTS. I guess the problem has been already
> > > debated and maybe solved in the past, so feel free to point me to other
> > > sources.
> > >
> > > Jacopo Mondi (3):
> > >   dt-bindings: display: bridge: Document LVDS to parallel decoder
> > >   drm: bridge: Add LVDS decoder driver
> > >   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> > >  
> > >  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> > >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> > >  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> > >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> > >  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 ++++++++++++++
> > >  5 files changed, 237 insertions(+), 2 deletions(-)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> > >  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

[Laurent Pinchart]

Hi Jacopo,

On Saturday, 10 March 2018 20:00:31 EET jacopo mondi wrote:
> On Sat, Mar 10, 2018 at 11:23:19AM +0530, Archit Taneja wrote:
> > On Friday 09 March 2018 07:21 PM, Jacopo Mondi wrote:
> > > Hello,
> > >
> > > after some discussion on the proposed bindings for generic lvds
> > > decoder and Thine THC63LVD1024, I decided to drop the THC63 specific
> > > part and just live with a transparent decoder that does not support any
> > > configuration from DT.
> > >
> > > Dropping THC63 support to avoid discussion on how to better implement
> > > support for a DRM bridge with 2 input ports and focus on LVDS mode
> > > propagation through bridges as explained in v1 cover letter (for DRM
> > > people: please see [1] as why I find difficult to implement support for
> > > bridges with multiple input endpoints)
> > >
> > > Same base branch as v1, with same patches for V3M Eagle applied on top.
> > > git://jmondi.org/linux v3m/v4.16-rc3/base
> > >
> > > Thanks
> > >
> > >    j
> > >
> > > v1 -> v2:
> > > - Drop support for THC63LVD1024
> > >
> > > [1] I had a quick at how to model a DRM bridge with multiple input
> > > ports, and I see a blocker in how DRM identifies and matches bridges
> > > using the devices node in place of the endpoint nodes.
> > >
> > > As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see
> > > only a few ways to support that:
> > > 
> > >  1) register 2 drm bridges from the same driver (one for each
> > >     input/output pair) but they would both be matches on the same device
> > >     node when the preceding bridge calls "of_drm_find_bridge()".
> > 
> > I think this is the way to go. DRM doesn't say anywhere that we can't have
> > 2 drm_bridge-s contained in a single device.

I agree, but it only works as long as the two bridges are independent. The 
THC63LVD1024 can also work in single-in, dual-out and dual-in, single-out 
modes, in which case we will need a more generic solution. I'm fine ignoring 
the problem on the driver side for now until someone needs to support such a 
setup, but the DT bindings need to be right.

> > About the issue with
> > of_drm_find_bridge(), if you set the 2 bridge's 'of_node' field to
> > the bridge1 and bridge2 nodes as shown below, wouldn't that suffice. From
> > what I know, we don't necessarily need to set the bridge's of_node
> > to the device (i.e, thschip) itself.
> 
> That's fine, but this implies that the preceding bridge (or encoder) in the
> DRM pipeline has then to collect the "port" node and match on it
> specifically when it attaches this driver. This introduces an ad-hoc
> policy that prevents this driver from being easily integrated in
> existing pipelines (where bridges are matched on the device node).
> 
> Also, I don't know much about the DRM framework, but it seems to me
> that the helper function designed to find the next component to attach
> to in the DRM pipeline is:
> 
> drm_of_find_panel_or_bridge():
>         remote = of_graph_get_remote_node();
>                 ep = of_graph_get_endpoint_by_regs();
>                 return of_graph_get_remote_port_parent();  <-- This returns
> the device node of_drm_find_panel(remote);
>         of_drm_find_bridge(remote);
> 
> I so dare to say matching on device node is kind of the intended way
> to do things in DRM, and this driver with two endpoints that wants to
> be matched on port nodes would be kind of special one that does not
> work as other driver expects to.
> 
> Is my understanding correct, or have I misinterpreted something?

I agree with you, so I hope your understanding is correct :-) We have the 
exact same issue in V4L2 where we used to always match on the device node, but 
recently the ADV7482 required matching on port nodes. The solution that was 
proposed was to support matching on both the device node and the port nodes, 
with automatic lookup of the device node from the port node if no direct match 
is found. I think this would work well in DRM too. Regardless of what we 
decide to do, we need to ensure that we don't create two separate categories 
of bridges with different matching rules that won't be compatible.

> > thschip {
> > 	...
> > 	ports {
> > 		bridge1: port@0 {
> > 			...
> > 		};
> > 		bridge2: port@1 {
> > 			...
> > 		};
> > 	};
> > };
> > 
> > >  2) register a single bridge with multiple "next bridges", but when the
> > >     bridge gets attached I don't see a way on how to identify on which
> > >     next bridge "drm_bridge_attach()" on, as it depends on the endpoint
> > >     the current bridge has been attached on first, and we don't have
> > >     that information.

For the general case I think we'll need to start adding port index arguments 
to the bridge API, but as stated above I'm fine ignoring this for now and 
creating two bridges.

> > >  3) Register more instances of the same chip in DTS, one for each
> > >     input/output pair. They gonna share supplies and gpios, and I don't
> > >     like that.

I don't think this is a good idea.

> > > I had a quick look at the currently in mainline bridges and none of them
> > > has multiple input endpoints, except for HDMI audio endpoint, which I
> > > haven't found in use in any DTS. I guess the problem has been already
> > > debated and maybe solved in the past, so feel free to point me to other
> > > sources.
> > >
> > > Jacopo Mondi (3):
> > >   dt-bindings: display: bridge: Document LVDS to parallel decoder
> > >   drm: bridge: Add LVDS decoder driver
> > >   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
> > >  
> > >  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
> > >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
> > >  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
> > >  drivers/gpu/drm/bridge/Makefile                    |   1 +
> > >  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 ++++++++++++++
> > >  5 files changed, 237 insertions(+), 2 deletions(-)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
> > >  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/09/2018 03:51 PM, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
> 
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +++
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 ++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>  	help
>  	  Support for RGB to VGA DAC based bridges
> 
> +config DRM_LVDS_DECODER
> +	tristate "Transparent LVDS to parallel decoder support"
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	help
> +	  Support for transparent LVDS to parallel decoders that don't require
> +	  any configuration.
> +
>  config DRM_LVDS_ENCODER
>  	tristate "Transparent parallel to LVDS encoder support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 0000000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/of_graph.h>
> +
> +struct lvds_decoder {
> +	struct device *dev;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +	struct drm_encoder *bridge_encoder;
> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {

static const struct drm_bridge_funcs lvds_dec_bridge_func

> +	.attach	= lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> +	struct device_node *lvds_output;
> +	struct device_node *remote;
> +	int ret = 0;
> +
> +	lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> +	if (!lvds_output) {
> +		dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(lvds_output);

Add of_node_put(lvds_output) right here to get rid of error_put_lvds_node
goto label.

> +	if (!remote) {
> +		dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_lvds_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	lvds->next = of_drm_find_bridge(remote);
> +	if (!lvds->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_lvds_node:
> +	of_node_put(lvds_output);
> +
> +	return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds;
> +	int ret;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, lvds);
> +
> +	ret = lvds_decoder_parse_dt(lvds);
> +	if (ret)
> +		return ret;
> +
> +	lvds->bridge.driver_private = lvds;
> +	lvds->bridge.of_node = pdev->dev.of_node;
> +	lvds->bridge.funcs = &lvds_dec_bridge_func;
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lvds_decoder_match[] = {
> +	{ .compatible = "lvds-decoder", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lvds_decoder_match);
> +#endif
> +

I would say "#ifdef CONFIG_OF" is redundant, because the driver "depends on OF".

> +static struct platform_driver lvds_decoder_driver = {
> +	.probe	= lvds_decoder_probe,
> +	.remove	= lvds_decoder_remove,
> +	.driver	= {
> +		.name		= "lvds_decoder",
> +		.of_match_table	= lvds_decoder_match,
> +	},
> +};
> +module_platform_driver(lvds_decoder_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Transparent LVDS to parallel data decoder");
> +MODULE_LICENSE("GPL v2");

--
With best wishes,
Vladimir

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/09/2018 03:51 PM, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
> 
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +++
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 ++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>  	help
>  	  Support for RGB to VGA DAC based bridges
> 
> +config DRM_LVDS_DECODER
> +	tristate "Transparent LVDS to parallel decoder support"
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	help
> +	  Support for transparent LVDS to parallel decoders that don't require
> +	  any configuration.
> +
>  config DRM_LVDS_ENCODER
>  	tristate "Transparent parallel to LVDS encoder support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 0000000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/of_graph.h>
> +
> +struct lvds_decoder {
> +	struct device *dev;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +	struct drm_encoder *bridge_encoder;
> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {

static const struct drm_bridge_funcs lvds_dec_bridge_func

> +	.attach	= lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> +	struct device_node *lvds_output;
> +	struct device_node *remote;
> +	int ret = 0;
> +
> +	lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> +	if (!lvds_output) {
> +		dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(lvds_output);

Add of_node_put(lvds_output) right here to get rid of error_put_lvds_node
goto label.

> +	if (!remote) {
> +		dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_lvds_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	lvds->next = of_drm_find_bridge(remote);
> +	if (!lvds->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_lvds_node:
> +	of_node_put(lvds_output);
> +
> +	return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds;
> +	int ret;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, lvds);
> +
> +	ret = lvds_decoder_parse_dt(lvds);
> +	if (ret)
> +		return ret;
> +
> +	lvds->bridge.driver_private = lvds;
> +	lvds->bridge.of_node = pdev->dev.of_node;
> +	lvds->bridge.funcs = &lvds_dec_bridge_func;
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> +	struct lvds_decoder *lvds = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lvds_decoder_match[] = {
> +	{ .compatible = "lvds-decoder", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lvds_decoder_match);
> +#endif
> +

I would say "#ifdef CONFIG_OF" is redundant, because the driver "depends on OF".

> +static struct platform_driver lvds_decoder_driver = {
> +	.probe	= lvds_decoder_probe,
> +	.remove	= lvds_decoder_remove,
> +	.driver	= {
> +		.name		= "lvds_decoder",
> +		.of_match_table	= lvds_decoder_match,
> +	},
> +};
> +module_platform_driver(lvds_decoder_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Transparent LVDS to parallel data decoder");
> +MODULE_LICENSE("GPL v2");

--
With best wishes,
Vladimir