[PATCH v2 0/4] Add LCD panel support to iwg20d

[PATCH v2 0/4] Add LCD panel support to iwg20d

[Fabrizio Castro]

The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform
comes with a 7" capacitive display kit from Emerging Display
Technologies Corporation (EDT). This series adds all that's
necessary for supporting it.

Thanks,
Fab

v1->v2:
* Convert dt-bindings to dt-schema

Fabrizio Castro (4):
  drm/bridge: Repurpose lvds-encoder.c
  dt-bindings: display: bridge: Repurpose lvds-encoder
  ARM: dts: iwg20d-q7-common: Add LCD support
  ARM: shmobile_defconfig: Enable support for panels from EDT

 .../bindings/display/bridge/lvds-codec.yaml        | 117 ++++++++++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  66 --------
 arch/arm/boot/dts/iwg20d-q7-common.dtsi            |  85 +++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi           |   1 -
 arch/arm/configs/shmobile_defconfig                |   3 +
 drivers/gpu/drm/bridge/Kconfig                     |   8 +-
 drivers/gpu/drm/bridge/Makefile                    |   2 +-
 drivers/gpu/drm/bridge/lvds-codec.c                | 169 +++++++++++++++++++++
 drivers/gpu/drm/bridge/lvds-encoder.c              | 155 -------------------
 9 files changed, 379 insertions(+), 227 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
 create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
 delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c

-- 
2.7.4

[PATCH v2 0/4] Add LCD panel support to iwg20d

[Fabrizio Castro]

The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform
comes with a 7" capacitive display kit from Emerging Display
Technologies Corporation (EDT). This series adds all that's
necessary for supporting it.

Thanks,
Fab

v1->v2:
* Convert dt-bindings to dt-schema

Fabrizio Castro (4):
  drm/bridge: Repurpose lvds-encoder.c
  dt-bindings: display: bridge: Repurpose lvds-encoder
  ARM: dts: iwg20d-q7-common: Add LCD support
  ARM: shmobile_defconfig: Enable support for panels from EDT

 .../bindings/display/bridge/lvds-codec.yaml        | 117 ++++++++++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  66 --------
 arch/arm/boot/dts/iwg20d-q7-common.dtsi            |  85 +++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi           |   1 -
 arch/arm/configs/shmobile_defconfig                |   3 +
 drivers/gpu/drm/bridge/Kconfig                     |   8 +-
 drivers/gpu/drm/bridge/Makefile                    |   2 +-
 drivers/gpu/drm/bridge/lvds-codec.c                | 169 +++++++++++++++++++++
 drivers/gpu/drm/bridge/lvds-encoder.c              | 155 -------------------
 9 files changed, 379 insertions(+), 227 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
 create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
 delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c

-- 
2.7.4

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

[PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Fabrizio Castro]

lvds-encoder.c implementation is also suitable for LVDS decoders,
not just LVDS encoders.
Instead of creating a new driver for addressing support for
transparent LVDS decoders, repurpose lvds-encoder.c for the greater
good.

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

---
v1->v2:
* No change
---
 drivers/gpu/drm/bridge/Kconfig        |   8 +-
 drivers/gpu/drm/bridge/Makefile       |   2 +-
 drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
 4 files changed, 174 insertions(+), 160 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
 delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3436297..9e75ca4e 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
 	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
 	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
 
-config DRM_LVDS_ENCODER
-	tristate "Transparent parallel to LVDS encoder support"
+config DRM_LVDS_CODEC
+	tristate "Transparent LVDS encoders and decoders support"
 	depends on OF
 	select DRM_KMS_HELPER
 	select DRM_PANEL_BRIDGE
 	help
-	  Support for transparent parallel to LVDS encoders that don't require
-	  any configuration.
+	  Support for transparent LVDS encoders and LVDS decoders that don't
+	  require any configuration.
 
 config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
 	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 4934fcf..8a9178a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
-obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
+obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
new file mode 100644
index 0000000..8a1979c
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+struct lvds_codec {
+	struct drm_bridge bridge;
+	struct drm_bridge *panel_bridge;
+	struct gpio_desc *powerdown_gpio;
+	u32 connector_type;
+};
+
+static int lvds_codec_attach(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
+				 bridge);
+}
+
+static void lvds_codec_enable(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
+}
+
+static void lvds_codec_disable(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
+}
+
+static struct drm_bridge_funcs funcs = {
+	.attach = lvds_codec_attach,
+	.enable = lvds_codec_enable,
+	.disable = lvds_codec_disable,
+};
+
+static int lvds_codec_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *port;
+	struct device_node *endpoint;
+	struct device_node *panel_node;
+	struct drm_panel *panel;
+	struct lvds_codec *lvds_codec;
+
+	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
+	if (!lvds_codec)
+		return -ENOMEM;
+
+	lvds_codec->connector_type = (u32)
+		of_device_get_match_data(&pdev->dev);
+	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+							       GPIOD_OUT_HIGH);
+	if (IS_ERR(lvds_codec->powerdown_gpio)) {
+		int err = PTR_ERR(lvds_codec->powerdown_gpio);
+
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "powerdown GPIO failure: %d\n", err);
+		return err;
+	}
+
+	/* Locate the panel DT node. */
+	port = of_graph_get_port_by_id(dev->of_node, 1);
+	if (!port) {
+		dev_dbg(dev, "port 1 not found\n");
+		return -ENXIO;
+	}
+
+	endpoint = of_get_child_by_name(port, "endpoint");
+	of_node_put(port);
+	if (!endpoint) {
+		dev_dbg(dev, "no endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	panel_node = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (!panel_node) {
+		dev_dbg(dev, "no remote endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	panel = of_drm_find_panel(panel_node);
+	of_node_put(panel_node);
+	if (IS_ERR(panel)) {
+		dev_dbg(dev, "panel not found, deferring probe\n");
+		return PTR_ERR(panel);
+	}
+
+	lvds_codec->panel_bridge =
+		devm_drm_panel_bridge_add_typed(dev, panel,
+						lvds_codec->connector_type);
+	if (IS_ERR(lvds_codec->panel_bridge))
+		return PTR_ERR(lvds_codec->panel_bridge);
+
+	/* The panel_bridge bridge is attached to the panel's of_node,
+	 * but we need a bridge attached to our of_node for our user
+	 * to look up.
+	 */
+	lvds_codec->bridge.of_node = dev->of_node;
+	lvds_codec->bridge.funcs = &funcs;
+	drm_bridge_add(&lvds_codec->bridge);
+
+	platform_set_drvdata(pdev, lvds_codec);
+
+	return 0;
+}
+
+static int lvds_codec_remove(struct platform_device *pdev)
+{
+	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&lvds_codec->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id lvds_codec_match[] = {
+	{
+		.compatible = "lvds-encoder",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS
+	},
+	{
+		.compatible = "thine,thc63lvdm83d",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+	},
+	{
+		.compatible = "lvds-decoder",
+		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, lvds_codec_match);
+
+static struct platform_driver lvds_codec_driver = {
+	.probe	= lvds_codec_probe,
+	.remove	= lvds_codec_remove,
+	.driver		= {
+		.name		= "lvds-codec",
+		.of_match_table	= lvds_codec_match,
+	},
+};
+module_platform_driver(lvds_codec_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
deleted file mode 100644
index e2132a8..0000000
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ /dev/null
@@ -1,155 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
- */
-
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_graph.h>
-#include <linux/platform_device.h>
-
-#include <drm/drm_bridge.h>
-#include <drm/drm_panel.h>
-
-struct lvds_encoder {
-	struct drm_bridge bridge;
-	struct drm_bridge *panel_bridge;
-	struct gpio_desc *powerdown_gpio;
-};
-
-static int lvds_encoder_attach(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
-				 bridge);
-}
-
-static void lvds_encoder_enable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
-}
-
-static void lvds_encoder_disable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
-}
-
-static struct drm_bridge_funcs funcs = {
-	.attach = lvds_encoder_attach,
-	.enable = lvds_encoder_enable,
-	.disable = lvds_encoder_disable,
-};
-
-static int lvds_encoder_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct device_node *port;
-	struct device_node *endpoint;
-	struct device_node *panel_node;
-	struct drm_panel *panel;
-	struct lvds_encoder *lvds_encoder;
-
-	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
-	if (!lvds_encoder)
-		return -ENOMEM;
-
-	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
-							       GPIOD_OUT_HIGH);
-	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
-		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
-
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "powerdown GPIO failure: %d\n", err);
-		return err;
-	}
-
-	/* Locate the panel DT node. */
-	port = of_graph_get_port_by_id(dev->of_node, 1);
-	if (!port) {
-		dev_dbg(dev, "port 1 not found\n");
-		return -ENXIO;
-	}
-
-	endpoint = of_get_child_by_name(port, "endpoint");
-	of_node_put(port);
-	if (!endpoint) {
-		dev_dbg(dev, "no endpoint for port 1\n");
-		return -ENXIO;
-	}
-
-	panel_node = of_graph_get_remote_port_parent(endpoint);
-	of_node_put(endpoint);
-	if (!panel_node) {
-		dev_dbg(dev, "no remote endpoint for port 1\n");
-		return -ENXIO;
-	}
-
-	panel = of_drm_find_panel(panel_node);
-	of_node_put(panel_node);
-	if (IS_ERR(panel)) {
-		dev_dbg(dev, "panel not found, deferring probe\n");
-		return PTR_ERR(panel);
-	}
-
-	lvds_encoder->panel_bridge =
-		devm_drm_panel_bridge_add_typed(dev, panel,
-						DRM_MODE_CONNECTOR_LVDS);
-	if (IS_ERR(lvds_encoder->panel_bridge))
-		return PTR_ERR(lvds_encoder->panel_bridge);
-
-	/* The panel_bridge bridge is attached to the panel's of_node,
-	 * but we need a bridge attached to our of_node for our user
-	 * to look up.
-	 */
-	lvds_encoder->bridge.of_node = dev->of_node;
-	lvds_encoder->bridge.funcs = &funcs;
-	drm_bridge_add(&lvds_encoder->bridge);
-
-	platform_set_drvdata(pdev, lvds_encoder);
-
-	return 0;
-}
-
-static int lvds_encoder_remove(struct platform_device *pdev)
-{
-	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
-
-	drm_bridge_remove(&lvds_encoder->bridge);
-
-	return 0;
-}
-
-static const struct of_device_id lvds_encoder_match[] = {
-	{ .compatible = "lvds-encoder" },
-	{ .compatible = "thine,thc63lvdm83d" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, lvds_encoder_match);
-
-static struct platform_driver lvds_encoder_driver = {
-	.probe	= lvds_encoder_probe,
-	.remove	= lvds_encoder_remove,
-	.driver		= {
-		.name		= "lvds-encoder",
-		.of_match_table	= lvds_encoder_match,
-	},
-};
-module_platform_driver(lvds_encoder_driver);
-
-MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
-MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
-MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Fabrizio Castro]

lvds-encoder.c implementation is also suitable for LVDS decoders,
not just LVDS encoders.
Instead of creating a new driver for addressing support for
transparent LVDS decoders, repurpose lvds-encoder.c for the greater
good.

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

---
v1->v2:
* No change
---
 drivers/gpu/drm/bridge/Kconfig        |   8 +-
 drivers/gpu/drm/bridge/Makefile       |   2 +-
 drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
 4 files changed, 174 insertions(+), 160 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
 delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3436297..9e75ca4e 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
 	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
 	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
 
-config DRM_LVDS_ENCODER
-	tristate "Transparent parallel to LVDS encoder support"
+config DRM_LVDS_CODEC
+	tristate "Transparent LVDS encoders and decoders support"
 	depends on OF
 	select DRM_KMS_HELPER
 	select DRM_PANEL_BRIDGE
 	help
-	  Support for transparent parallel to LVDS encoders that don't require
-	  any configuration.
+	  Support for transparent LVDS encoders and LVDS decoders that don't
+	  require any configuration.
 
 config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
 	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 4934fcf..8a9178a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
-obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
+obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
new file mode 100644
index 0000000..8a1979c
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+struct lvds_codec {
+	struct drm_bridge bridge;
+	struct drm_bridge *panel_bridge;
+	struct gpio_desc *powerdown_gpio;
+	u32 connector_type;
+};
+
+static int lvds_codec_attach(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
+				 bridge);
+}
+
+static void lvds_codec_enable(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
+}
+
+static void lvds_codec_disable(struct drm_bridge *bridge)
+{
+	struct lvds_codec *lvds_codec = container_of(bridge,
+							 struct lvds_codec,
+							 bridge);
+
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
+}
+
+static struct drm_bridge_funcs funcs = {
+	.attach = lvds_codec_attach,
+	.enable = lvds_codec_enable,
+	.disable = lvds_codec_disable,
+};
+
+static int lvds_codec_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *port;
+	struct device_node *endpoint;
+	struct device_node *panel_node;
+	struct drm_panel *panel;
+	struct lvds_codec *lvds_codec;
+
+	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
+	if (!lvds_codec)
+		return -ENOMEM;
+
+	lvds_codec->connector_type = (u32)
+		of_device_get_match_data(&pdev->dev);
+	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+							       GPIOD_OUT_HIGH);
+	if (IS_ERR(lvds_codec->powerdown_gpio)) {
+		int err = PTR_ERR(lvds_codec->powerdown_gpio);
+
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "powerdown GPIO failure: %d\n", err);
+		return err;
+	}
+
+	/* Locate the panel DT node. */
+	port = of_graph_get_port_by_id(dev->of_node, 1);
+	if (!port) {
+		dev_dbg(dev, "port 1 not found\n");
+		return -ENXIO;
+	}
+
+	endpoint = of_get_child_by_name(port, "endpoint");
+	of_node_put(port);
+	if (!endpoint) {
+		dev_dbg(dev, "no endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	panel_node = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (!panel_node) {
+		dev_dbg(dev, "no remote endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	panel = of_drm_find_panel(panel_node);
+	of_node_put(panel_node);
+	if (IS_ERR(panel)) {
+		dev_dbg(dev, "panel not found, deferring probe\n");
+		return PTR_ERR(panel);
+	}
+
+	lvds_codec->panel_bridge =
+		devm_drm_panel_bridge_add_typed(dev, panel,
+						lvds_codec->connector_type);
+	if (IS_ERR(lvds_codec->panel_bridge))
+		return PTR_ERR(lvds_codec->panel_bridge);
+
+	/* The panel_bridge bridge is attached to the panel's of_node,
+	 * but we need a bridge attached to our of_node for our user
+	 * to look up.
+	 */
+	lvds_codec->bridge.of_node = dev->of_node;
+	lvds_codec->bridge.funcs = &funcs;
+	drm_bridge_add(&lvds_codec->bridge);
+
+	platform_set_drvdata(pdev, lvds_codec);
+
+	return 0;
+}
+
+static int lvds_codec_remove(struct platform_device *pdev)
+{
+	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&lvds_codec->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id lvds_codec_match[] = {
+	{
+		.compatible = "lvds-encoder",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS
+	},
+	{
+		.compatible = "thine,thc63lvdm83d",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+	},
+	{
+		.compatible = "lvds-decoder",
+		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, lvds_codec_match);
+
+static struct platform_driver lvds_codec_driver = {
+	.probe	= lvds_codec_probe,
+	.remove	= lvds_codec_remove,
+	.driver		= {
+		.name		= "lvds-codec",
+		.of_match_table	= lvds_codec_match,
+	},
+};
+module_platform_driver(lvds_codec_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
deleted file mode 100644
index e2132a8..0000000
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ /dev/null
@@ -1,155 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
- */
-
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_graph.h>
-#include <linux/platform_device.h>
-
-#include <drm/drm_bridge.h>
-#include <drm/drm_panel.h>
-
-struct lvds_encoder {
-	struct drm_bridge bridge;
-	struct drm_bridge *panel_bridge;
-	struct gpio_desc *powerdown_gpio;
-};
-
-static int lvds_encoder_attach(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
-				 bridge);
-}
-
-static void lvds_encoder_enable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
-}
-
-static void lvds_encoder_disable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
-
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
-}
-
-static struct drm_bridge_funcs funcs = {
-	.attach = lvds_encoder_attach,
-	.enable = lvds_encoder_enable,
-	.disable = lvds_encoder_disable,
-};
-
-static int lvds_encoder_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct device_node *port;
-	struct device_node *endpoint;
-	struct device_node *panel_node;
-	struct drm_panel *panel;
-	struct lvds_encoder *lvds_encoder;
-
-	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
-	if (!lvds_encoder)
-		return -ENOMEM;
-
-	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
-							       GPIOD_OUT_HIGH);
-	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
-		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
-
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "powerdown GPIO failure: %d\n", err);
-		return err;
-	}
-
-	/* Locate the panel DT node. */
-	port = of_graph_get_port_by_id(dev->of_node, 1);
-	if (!port) {
-		dev_dbg(dev, "port 1 not found\n");
-		return -ENXIO;
-	}
-
-	endpoint = of_get_child_by_name(port, "endpoint");
-	of_node_put(port);
-	if (!endpoint) {
-		dev_dbg(dev, "no endpoint for port 1\n");
-		return -ENXIO;
-	}
-
-	panel_node = of_graph_get_remote_port_parent(endpoint);
-	of_node_put(endpoint);
-	if (!panel_node) {
-		dev_dbg(dev, "no remote endpoint for port 1\n");
-		return -ENXIO;
-	}
-
-	panel = of_drm_find_panel(panel_node);
-	of_node_put(panel_node);
-	if (IS_ERR(panel)) {
-		dev_dbg(dev, "panel not found, deferring probe\n");
-		return PTR_ERR(panel);
-	}
-
-	lvds_encoder->panel_bridge =
-		devm_drm_panel_bridge_add_typed(dev, panel,
-						DRM_MODE_CONNECTOR_LVDS);
-	if (IS_ERR(lvds_encoder->panel_bridge))
-		return PTR_ERR(lvds_encoder->panel_bridge);
-
-	/* The panel_bridge bridge is attached to the panel's of_node,
-	 * but we need a bridge attached to our of_node for our user
-	 * to look up.
-	 */
-	lvds_encoder->bridge.of_node = dev->of_node;
-	lvds_encoder->bridge.funcs = &funcs;
-	drm_bridge_add(&lvds_encoder->bridge);
-
-	platform_set_drvdata(pdev, lvds_encoder);
-
-	return 0;
-}
-
-static int lvds_encoder_remove(struct platform_device *pdev)
-{
-	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
-
-	drm_bridge_remove(&lvds_encoder->bridge);
-
-	return 0;
-}
-
-static const struct of_device_id lvds_encoder_match[] = {
-	{ .compatible = "lvds-encoder" },
-	{ .compatible = "thine,thc63lvdm83d" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, lvds_encoder_match);
-
-static struct platform_driver lvds_encoder_driver = {
-	.probe	= lvds_encoder_probe,
-	.remove	= lvds_encoder_remove,
-	.driver		= {
-		.name		= "lvds-encoder",
-		.of_match_table	= lvds_encoder_match,
-	},
-};
-module_platform_driver(lvds_encoder_driver);
-
-MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
-MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
-MODULE_LICENSE("GPL");
-- 
2.7.4

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

Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Jacopo Mondi]

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

Hi Fabrizio,
  thanks for the patch.

On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> lvds-encoder.c implementation is also suitable for LVDS decoders,
> not just LVDS encoders.
> Instead of creating a new driver for addressing support for
> transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> good.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * No change
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +-
>  drivers/gpu/drm/bridge/Makefile       |   2 +-
>  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
>  4 files changed, 174 insertions(+), 160 deletions(-)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
>  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3436297..9e75ca4e 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
>  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
>  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
>
> -config DRM_LVDS_ENCODER
> -	tristate "Transparent parallel to LVDS encoder support"
> +config DRM_LVDS_CODEC
> +	tristate "Transparent LVDS encoders and decoders support"
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	select DRM_PANEL_BRIDGE
>  	help
> -	  Support for transparent parallel to LVDS encoders that don't require
> -	  any configuration.
> +	  Support for transparent LVDS encoders and LVDS decoders that don't
> +	  require any configuration.
>
>  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf..8a9178a 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -2,7 +2,7 @@
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> new file mode 100644
> index 0000000..8a1979c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +struct lvds_codec {
> +	struct drm_bridge bridge;
> +	struct drm_bridge *panel_bridge;
> +	struct gpio_desc *powerdown_gpio;
> +	u32 connector_type;
> +};
> +
> +static int lvds_codec_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +

Weird indentation. Align to open ( to match the rest of the driver's
style.

> +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> +				 bridge);
> +}
> +
> +static void lvds_codec_enable(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +

Here too

> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> +}
> +
> +static void lvds_codec_disable(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +
> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> +}
> +
> +static struct drm_bridge_funcs funcs = {
> +	.attach = lvds_codec_attach,
> +	.enable = lvds_codec_enable,
> +	.disable = lvds_codec_disable,
> +};
> +
> +static int lvds_codec_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *port;
> +	struct device_node *endpoint;
> +	struct device_node *panel_node;
> +	struct drm_panel *panel;
> +	struct lvds_codec *lvds_codec;
> +
> +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> +	if (!lvds_codec)
> +		return -ENOMEM;
> +
> +	lvds_codec->connector_type = (u32)
> +		of_device_get_match_data(&pdev->dev);

Fits in 1 line

> +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +							       GPIOD_OUT_HIGH);
> +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> +
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> +		return err;

I know it was there already, but this seems a bit unusual for the
minimal gain of having a printout in the very unlikely case the
gpiod_get() operations fails. I would just return PTR_ERR().

> +	}
> +
> +	/* Locate the panel DT node. */
> +	port = of_graph_get_port_by_id(dev->of_node, 1);
> +	if (!port) {
> +		dev_dbg(dev, "port 1 not found\n");
> +		return -ENXIO;
> +	}
> +
> +	endpoint = of_get_child_by_name(port, "endpoint");
> +	of_node_put(port);
> +	if (!endpoint) {
> +		dev_dbg(dev, "no endpoint for port 1\n");
> +		return -ENXIO;
> +	}

I know it was there already, but this could be simplified with
of_graph_get_endpoint_by_regs()
> +
> +	panel_node = of_graph_get_remote_port_parent(endpoint);
> +	of_node_put(endpoint);
> +	if (!panel_node) {
> +		dev_dbg(dev, "no remote endpoint for port 1\n");
> +		return -ENXIO;
> +	}

Or even better, simplify these three with of_graph_get_remote_node()

> +
> +	panel = of_drm_find_panel(panel_node);
> +	of_node_put(panel_node);
> +	if (IS_ERR(panel)) {
> +		dev_dbg(dev, "panel not found, deferring probe\n");
> +		return PTR_ERR(panel);
> +	}
> +
> +	lvds_codec->panel_bridge =
> +		devm_drm_panel_bridge_add_typed(dev, panel,
> +						lvds_codec->connector_type);

The function documentation reports this as deprecated and suggested to
use the non-typed version. Do you think it could work for this new
codec driver ?

> +	if (IS_ERR(lvds_codec->panel_bridge))
> +		return PTR_ERR(lvds_codec->panel_bridge);
> +
> +	/* The panel_bridge bridge is attached to the panel's of_node,
> +	 * but we need a bridge attached to our of_node for our user
> +	 * to look up.
> +	 */
> +	lvds_codec->bridge.of_node = dev->of_node;
> +	lvds_codec->bridge.funcs = &funcs;
> +	drm_bridge_add(&lvds_codec->bridge);
> +
> +	platform_set_drvdata(pdev, lvds_codec);
> +
> +	return 0;
> +}
> +
> +static int lvds_codec_remove(struct platform_device *pdev)
> +{
> +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds_codec->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lvds_codec_match[] = {
> +	{
> +		.compatible = "lvds-encoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS

In case you for for drm_panel_bridge_add() you could drop the type

> +	},
> +	{
> +		.compatible = "thine,thc63lvdm83d",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
> +	{
> +		.compatible = "lvds-decoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> +	},

Which decoder are you using? This is a generic fallback, but I would
expect compatible for a real device to appear in DTS.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> +
> +static struct platform_driver lvds_codec_driver = {
> +	.probe	= lvds_codec_probe,
> +	.remove	= lvds_codec_remove,
> +	.driver		= {
> +		.name		= "lvds-codec",
> +		.of_match_table	= lvds_codec_match,
> +	},
> +};
> +module_platform_driver(lvds_codec_driver);
> +
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> deleted file mode 100644
> index e2132a8..0000000
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ /dev/null
> @@ -1,155 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -/*
> - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> - */
> -
> -#include <linux/gpio/consumer.h>
> -#include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_graph.h>
> -#include <linux/platform_device.h>
> -
> -#include <drm/drm_bridge.h>
> -#include <drm/drm_panel.h>
> -
> -struct lvds_encoder {
> -	struct drm_bridge bridge;
> -	struct drm_bridge *panel_bridge;
> -	struct gpio_desc *powerdown_gpio;
> -};
> -
> -static int lvds_encoder_attach(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> -				 bridge);
> -}
> -
> -static void lvds_encoder_enable(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> -}
> -
> -static void lvds_encoder_disable(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> -}
> -
> -static struct drm_bridge_funcs funcs = {
> -	.attach = lvds_encoder_attach,
> -	.enable = lvds_encoder_enable,
> -	.disable = lvds_encoder_disable,
> -};
> -
> -static int lvds_encoder_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct device_node *port;
> -	struct device_node *endpoint;
> -	struct device_node *panel_node;
> -	struct drm_panel *panel;
> -	struct lvds_encoder *lvds_encoder;
> -
> -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> -	if (!lvds_encoder)
> -		return -ENOMEM;
> -
> -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> -							       GPIOD_OUT_HIGH);
> -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> -
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> -		return err;
> -	}
> -
> -	/* Locate the panel DT node. */
> -	port = of_graph_get_port_by_id(dev->of_node, 1);
> -	if (!port) {
> -		dev_dbg(dev, "port 1 not found\n");
> -		return -ENXIO;
> -	}
> -
> -	endpoint = of_get_child_by_name(port, "endpoint");
> -	of_node_put(port);
> -	if (!endpoint) {
> -		dev_dbg(dev, "no endpoint for port 1\n");
> -		return -ENXIO;
> -	}
> -
> -	panel_node = of_graph_get_remote_port_parent(endpoint);
> -	of_node_put(endpoint);
> -	if (!panel_node) {
> -		dev_dbg(dev, "no remote endpoint for port 1\n");
> -		return -ENXIO;
> -	}
> -
> -	panel = of_drm_find_panel(panel_node);
> -	of_node_put(panel_node);
> -	if (IS_ERR(panel)) {
> -		dev_dbg(dev, "panel not found, deferring probe\n");
> -		return PTR_ERR(panel);
> -	}
> -
> -	lvds_encoder->panel_bridge =
> -		devm_drm_panel_bridge_add_typed(dev, panel,
> -						DRM_MODE_CONNECTOR_LVDS);
> -	if (IS_ERR(lvds_encoder->panel_bridge))
> -		return PTR_ERR(lvds_encoder->panel_bridge);
> -
> -	/* The panel_bridge bridge is attached to the panel's of_node,
> -	 * but we need a bridge attached to our of_node for our user
> -	 * to look up.
> -	 */
> -	lvds_encoder->bridge.of_node = dev->of_node;
> -	lvds_encoder->bridge.funcs = &funcs;
> -	drm_bridge_add(&lvds_encoder->bridge);
> -
> -	platform_set_drvdata(pdev, lvds_encoder);
> -
> -	return 0;
> -}
> -
> -static int lvds_encoder_remove(struct platform_device *pdev)
> -{
> -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> -
> -	drm_bridge_remove(&lvds_encoder->bridge);
> -
> -	return 0;
> -}
> -
> -static const struct of_device_id lvds_encoder_match[] = {
> -	{ .compatible = "lvds-encoder" },
> -	{ .compatible = "thine,thc63lvdm83d" },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> -
> -static struct platform_driver lvds_encoder_driver = {
> -	.probe	= lvds_encoder_probe,
> -	.remove	= lvds_encoder_remove,
> -	.driver		= {
> -		.name		= "lvds-encoder",
> -		.of_match_table	= lvds_encoder_match,
> -	},
> -};
> -module_platform_driver(lvds_encoder_driver);
> -
> -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> -MODULE_LICENSE("GPL");
> --
> 2.7.4
>

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

Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Jacopo Mondi]

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

Hi Fabrizio,
  thanks for the patch.

On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> lvds-encoder.c implementation is also suitable for LVDS decoders,
> not just LVDS encoders.
> Instead of creating a new driver for addressing support for
> transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> good.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * No change
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   8 +-
>  drivers/gpu/drm/bridge/Makefile       |   2 +-
>  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
>  4 files changed, 174 insertions(+), 160 deletions(-)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
>  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3436297..9e75ca4e 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
>  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
>  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
>
> -config DRM_LVDS_ENCODER
> -	tristate "Transparent parallel to LVDS encoder support"
> +config DRM_LVDS_CODEC
> +	tristate "Transparent LVDS encoders and decoders support"
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	select DRM_PANEL_BRIDGE
>  	help
> -	  Support for transparent parallel to LVDS encoders that don't require
> -	  any configuration.
> +	  Support for transparent LVDS encoders and LVDS decoders that don't
> +	  require any configuration.
>
>  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf..8a9178a 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -2,7 +2,7 @@
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> new file mode 100644
> index 0000000..8a1979c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +struct lvds_codec {
> +	struct drm_bridge bridge;
> +	struct drm_bridge *panel_bridge;
> +	struct gpio_desc *powerdown_gpio;
> +	u32 connector_type;
> +};
> +
> +static int lvds_codec_attach(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +

Weird indentation. Align to open ( to match the rest of the driver's
style.

> +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> +				 bridge);
> +}
> +
> +static void lvds_codec_enable(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +

Here too

> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> +}
> +
> +static void lvds_codec_disable(struct drm_bridge *bridge)
> +{
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +							 struct lvds_codec,
> +							 bridge);
> +
> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> +}
> +
> +static struct drm_bridge_funcs funcs = {
> +	.attach = lvds_codec_attach,
> +	.enable = lvds_codec_enable,
> +	.disable = lvds_codec_disable,
> +};
> +
> +static int lvds_codec_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *port;
> +	struct device_node *endpoint;
> +	struct device_node *panel_node;
> +	struct drm_panel *panel;
> +	struct lvds_codec *lvds_codec;
> +
> +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> +	if (!lvds_codec)
> +		return -ENOMEM;
> +
> +	lvds_codec->connector_type = (u32)
> +		of_device_get_match_data(&pdev->dev);

Fits in 1 line

> +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +							       GPIOD_OUT_HIGH);
> +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> +
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> +		return err;

I know it was there already, but this seems a bit unusual for the
minimal gain of having a printout in the very unlikely case the
gpiod_get() operations fails. I would just return PTR_ERR().

> +	}
> +
> +	/* Locate the panel DT node. */
> +	port = of_graph_get_port_by_id(dev->of_node, 1);
> +	if (!port) {
> +		dev_dbg(dev, "port 1 not found\n");
> +		return -ENXIO;
> +	}
> +
> +	endpoint = of_get_child_by_name(port, "endpoint");
> +	of_node_put(port);
> +	if (!endpoint) {
> +		dev_dbg(dev, "no endpoint for port 1\n");
> +		return -ENXIO;
> +	}

I know it was there already, but this could be simplified with
of_graph_get_endpoint_by_regs()
> +
> +	panel_node = of_graph_get_remote_port_parent(endpoint);
> +	of_node_put(endpoint);
> +	if (!panel_node) {
> +		dev_dbg(dev, "no remote endpoint for port 1\n");
> +		return -ENXIO;
> +	}

Or even better, simplify these three with of_graph_get_remote_node()

> +
> +	panel = of_drm_find_panel(panel_node);
> +	of_node_put(panel_node);
> +	if (IS_ERR(panel)) {
> +		dev_dbg(dev, "panel not found, deferring probe\n");
> +		return PTR_ERR(panel);
> +	}
> +
> +	lvds_codec->panel_bridge =
> +		devm_drm_panel_bridge_add_typed(dev, panel,
> +						lvds_codec->connector_type);

The function documentation reports this as deprecated and suggested to
use the non-typed version. Do you think it could work for this new
codec driver ?

> +	if (IS_ERR(lvds_codec->panel_bridge))
> +		return PTR_ERR(lvds_codec->panel_bridge);
> +
> +	/* The panel_bridge bridge is attached to the panel's of_node,
> +	 * but we need a bridge attached to our of_node for our user
> +	 * to look up.
> +	 */
> +	lvds_codec->bridge.of_node = dev->of_node;
> +	lvds_codec->bridge.funcs = &funcs;
> +	drm_bridge_add(&lvds_codec->bridge);
> +
> +	platform_set_drvdata(pdev, lvds_codec);
> +
> +	return 0;
> +}
> +
> +static int lvds_codec_remove(struct platform_device *pdev)
> +{
> +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&lvds_codec->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lvds_codec_match[] = {
> +	{
> +		.compatible = "lvds-encoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS

In case you for for drm_panel_bridge_add() you could drop the type

> +	},
> +	{
> +		.compatible = "thine,thc63lvdm83d",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
> +	{
> +		.compatible = "lvds-decoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> +	},

Which decoder are you using? This is a generic fallback, but I would
expect compatible for a real device to appear in DTS.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> +
> +static struct platform_driver lvds_codec_driver = {
> +	.probe	= lvds_codec_probe,
> +	.remove	= lvds_codec_remove,
> +	.driver		= {
> +		.name		= "lvds-codec",
> +		.of_match_table	= lvds_codec_match,
> +	},
> +};
> +module_platform_driver(lvds_codec_driver);
> +
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> deleted file mode 100644
> index e2132a8..0000000
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ /dev/null
> @@ -1,155 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -/*
> - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> - */
> -
> -#include <linux/gpio/consumer.h>
> -#include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_graph.h>
> -#include <linux/platform_device.h>
> -
> -#include <drm/drm_bridge.h>
> -#include <drm/drm_panel.h>
> -
> -struct lvds_encoder {
> -	struct drm_bridge bridge;
> -	struct drm_bridge *panel_bridge;
> -	struct gpio_desc *powerdown_gpio;
> -};
> -
> -static int lvds_encoder_attach(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> -				 bridge);
> -}
> -
> -static void lvds_encoder_enable(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> -}
> -
> -static void lvds_encoder_disable(struct drm_bridge *bridge)
> -{
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> -
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> -}
> -
> -static struct drm_bridge_funcs funcs = {
> -	.attach = lvds_encoder_attach,
> -	.enable = lvds_encoder_enable,
> -	.disable = lvds_encoder_disable,
> -};
> -
> -static int lvds_encoder_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct device_node *port;
> -	struct device_node *endpoint;
> -	struct device_node *panel_node;
> -	struct drm_panel *panel;
> -	struct lvds_encoder *lvds_encoder;
> -
> -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> -	if (!lvds_encoder)
> -		return -ENOMEM;
> -
> -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> -							       GPIOD_OUT_HIGH);
> -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> -
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> -		return err;
> -	}
> -
> -	/* Locate the panel DT node. */
> -	port = of_graph_get_port_by_id(dev->of_node, 1);
> -	if (!port) {
> -		dev_dbg(dev, "port 1 not found\n");
> -		return -ENXIO;
> -	}
> -
> -	endpoint = of_get_child_by_name(port, "endpoint");
> -	of_node_put(port);
> -	if (!endpoint) {
> -		dev_dbg(dev, "no endpoint for port 1\n");
> -		return -ENXIO;
> -	}
> -
> -	panel_node = of_graph_get_remote_port_parent(endpoint);
> -	of_node_put(endpoint);
> -	if (!panel_node) {
> -		dev_dbg(dev, "no remote endpoint for port 1\n");
> -		return -ENXIO;
> -	}
> -
> -	panel = of_drm_find_panel(panel_node);
> -	of_node_put(panel_node);
> -	if (IS_ERR(panel)) {
> -		dev_dbg(dev, "panel not found, deferring probe\n");
> -		return PTR_ERR(panel);
> -	}
> -
> -	lvds_encoder->panel_bridge =
> -		devm_drm_panel_bridge_add_typed(dev, panel,
> -						DRM_MODE_CONNECTOR_LVDS);
> -	if (IS_ERR(lvds_encoder->panel_bridge))
> -		return PTR_ERR(lvds_encoder->panel_bridge);
> -
> -	/* The panel_bridge bridge is attached to the panel's of_node,
> -	 * but we need a bridge attached to our of_node for our user
> -	 * to look up.
> -	 */
> -	lvds_encoder->bridge.of_node = dev->of_node;
> -	lvds_encoder->bridge.funcs = &funcs;
> -	drm_bridge_add(&lvds_encoder->bridge);
> -
> -	platform_set_drvdata(pdev, lvds_encoder);
> -
> -	return 0;
> -}
> -
> -static int lvds_encoder_remove(struct platform_device *pdev)
> -{
> -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> -
> -	drm_bridge_remove(&lvds_encoder->bridge);
> -
> -	return 0;
> -}
> -
> -static const struct of_device_id lvds_encoder_match[] = {
> -	{ .compatible = "lvds-encoder" },
> -	{ .compatible = "thine,thc63lvdm83d" },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> -
> -static struct platform_driver lvds_encoder_driver = {
> -	.probe	= lvds_encoder_probe,
> -	.remove	= lvds_encoder_remove,
> -	.driver		= {
> -		.name		= "lvds-encoder",
> -		.of_match_table	= lvds_encoder_match,
> -	},
> -};
> -module_platform_driver(lvds_encoder_driver);
> -
> -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> -MODULE_LICENSE("GPL");
> --
> 2.7.4
>

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

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

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

RE: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Fabrizio Castro]

Hi Jacopo,

Thank you for your feedback!

> From: Jacopo Mondi <jacopo@jmondi.org>
> Sent: 07 November 2019 18:19
> Subject: Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
> 
> Hi Fabrizio,
>   thanks for the patch.
> 
> On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> > lvds-encoder.c implementation is also suitable for LVDS decoders,
> > not just LVDS encoders.
> > Instead of creating a new driver for addressing support for
> > transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> > good.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * No change
> > ---
> >  drivers/gpu/drm/bridge/Kconfig        |   8 +-
> >  drivers/gpu/drm/bridge/Makefile       |   2 +-
> >  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
> >  4 files changed, 174 insertions(+), 160 deletions(-)
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> >  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 3436297..9e75ca4e 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
> >  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
> >  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
> >
> > -config DRM_LVDS_ENCODER
> > -	tristate "Transparent parallel to LVDS encoder support"
> > +config DRM_LVDS_CODEC
> > +	tristate "Transparent LVDS encoders and decoders support"
> >  	depends on OF
> >  	select DRM_KMS_HELPER
> >  	select DRM_PANEL_BRIDGE
> >  	help
> > -	  Support for transparent parallel to LVDS encoders that don't require
> > -	  any configuration.
> > +	  Support for transparent LVDS encoders and LVDS decoders that don't
> > +	  require any configuration.
> >
> >  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
> >  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf..8a9178a 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -2,7 +2,7 @@
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> >  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > new file mode 100644
> > index 0000000..8a1979c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_panel.h>
> > +
> > +struct lvds_codec {
> > +	struct drm_bridge bridge;
> > +	struct drm_bridge *panel_bridge;
> > +	struct gpio_desc *powerdown_gpio;
> > +	u32 connector_type;
> > +};
> > +
> > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> 
> Weird indentation. Align to open ( to match the rest of the driver's
> style.

Will do, throughout the file.

> 
> > +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > +				 bridge);
> > +}
> > +
> > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> 
> Here too
> 
> > +	if (lvds_codec->powerdown_gpio)
> > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > +}
> > +
> > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> > +	if (lvds_codec->powerdown_gpio)
> > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > +}
> > +
> > +static struct drm_bridge_funcs funcs = {
> > +	.attach = lvds_codec_attach,
> > +	.enable = lvds_codec_enable,
> > +	.disable = lvds_codec_disable,
> > +};
> > +
> > +static int lvds_codec_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *port;
> > +	struct device_node *endpoint;
> > +	struct device_node *panel_node;
> > +	struct drm_panel *panel;
> > +	struct lvds_codec *lvds_codec;
> > +
> > +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > +	if (!lvds_codec)
> > +		return -ENOMEM;
> > +
> > +	lvds_codec->connector_type = (u32)
> > +		of_device_get_match_data(&pdev->dev);
> 
> Fits in 1 line

Will take it out

> 
> > +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > +							       GPIOD_OUT_HIGH);
> > +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> > +
> > +		if (err != -EPROBE_DEFER)
> > +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > +		return err;
> 
> I know it was there already, but this seems a bit unusual for the
> minimal gain of having a printout in the very unlikely case the
> gpiod_get() operations fails. I would just return PTR_ERR().

Will change

> 
> > +	}
> > +
> > +	/* Locate the panel DT node. */
> > +	port = of_graph_get_port_by_id(dev->of_node, 1);
> > +	if (!port) {
> > +		dev_dbg(dev, "port 1 not found\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	endpoint = of_get_child_by_name(port, "endpoint");
> > +	of_node_put(port);
> > +	if (!endpoint) {
> > +		dev_dbg(dev, "no endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> 
> I know it was there already, but this could be simplified with
> of_graph_get_endpoint_by_regs()
> > +
> > +	panel_node = of_graph_get_remote_port_parent(endpoint);
> > +	of_node_put(endpoint);
> > +	if (!panel_node) {
> > +		dev_dbg(dev, "no remote endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> 
> Or even better, simplify these three with of_graph_get_remote_node()

Good catch,  of_graph_get_remote_node seems the best approach,
I will go with that

> 
> > +
> > +	panel = of_drm_find_panel(panel_node);
> > +	of_node_put(panel_node);
> > +	if (IS_ERR(panel)) {
> > +		dev_dbg(dev, "panel not found, deferring probe\n");
> > +		return PTR_ERR(panel);
> > +	}
> > +
> > +	lvds_codec->panel_bridge =
> > +		devm_drm_panel_bridge_add_typed(dev, panel,
> > +						lvds_codec->connector_type);
> 
> The function documentation reports this as deprecated and suggested to
> use the non-typed version. Do you think it could work for this new
> codec driver ?

I think so. However, the panel I am using doesn't define the connector type,
probably because none of the DRM_MODE_CONNECTOR_* seem to fit
perfectly, therefore I am probably going to need to define
DRM_MODE_CONNECTOR_PARALLEL.
I'll send v3 shortly so that you can have a look at what I mean, please advise
on the best course of action.

> 
> > +	if (IS_ERR(lvds_codec->panel_bridge))
> > +		return PTR_ERR(lvds_codec->panel_bridge);
> > +
> > +	/* The panel_bridge bridge is attached to the panel's of_node,
> > +	 * but we need a bridge attached to our of_node for our user
> > +	 * to look up.
> > +	 */
> > +	lvds_codec->bridge.of_node = dev->of_node;
> > +	lvds_codec->bridge.funcs = &funcs;
> > +	drm_bridge_add(&lvds_codec->bridge);
> > +
> > +	platform_set_drvdata(pdev, lvds_codec);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lvds_codec_remove(struct platform_device *pdev)
> > +{
> > +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > +
> > +	drm_bridge_remove(&lvds_codec->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id lvds_codec_match[] = {
> > +	{
> > +		.compatible = "lvds-encoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS
> 
> In case you for for drm_panel_bridge_add() you could drop the type

Will do

> 
> > +	},
> > +	{
> > +		.compatible = "thine,thc63lvdm83d",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> > +	{
> > +		.compatible = "lvds-decoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> > +	},
> 
> Which decoder are you using? This is a generic fallback, but I would
> expect compatible for a real device to appear in DTS.

I am using the DS90CF384AMTDX/NOPB.

This driver is for transparent encoders and decoders, which means
there is no way you can control them. I don't think we should add
any device specific compatible string here, as it would be of no use.
The original dt-bindings for the encoders requires a device specific
compatible string for encoders with additional properties, I think
we should keep it that way for decoders too. I am not going to
use a device specific compatible string in the DT either for the
time being, unless really necessary.

I will send a v3 shortly. Thank you for your help so far!

Fab

> 
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > +
> > +static struct platform_driver lvds_codec_driver = {
> > +	.probe	= lvds_codec_probe,
> > +	.remove	= lvds_codec_remove,
> > +	.driver		= {
> > +		.name		= "lvds-codec",
> > +		.of_match_table	= lvds_codec_match,
> > +	},
> > +};
> > +module_platform_driver(lvds_codec_driver);
> > +
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > deleted file mode 100644
> > index e2132a8..0000000
> > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > +++ /dev/null
> > @@ -1,155 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0-or-later
> > -/*
> > - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > - */
> > -
> > -#include <linux/gpio/consumer.h>
> > -#include <linux/module.h>
> > -#include <linux/of.h>
> > -#include <linux/of_graph.h>
> > -#include <linux/platform_device.h>
> > -
> > -#include <drm/drm_bridge.h>
> > -#include <drm/drm_panel.h>
> > -
> > -struct lvds_encoder {
> > -	struct drm_bridge bridge;
> > -	struct drm_bridge *panel_bridge;
> > -	struct gpio_desc *powerdown_gpio;
> > -};
> > -
> > -static int lvds_encoder_attach(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> > -				 bridge);
> > -}
> > -
> > -static void lvds_encoder_enable(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	if (lvds_encoder->powerdown_gpio)
> > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> > -}
> > -
> > -static void lvds_encoder_disable(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	if (lvds_encoder->powerdown_gpio)
> > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> > -}
> > -
> > -static struct drm_bridge_funcs funcs = {
> > -	.attach = lvds_encoder_attach,
> > -	.enable = lvds_encoder_enable,
> > -	.disable = lvds_encoder_disable,
> > -};
> > -
> > -static int lvds_encoder_probe(struct platform_device *pdev)
> > -{
> > -	struct device *dev = &pdev->dev;
> > -	struct device_node *port;
> > -	struct device_node *endpoint;
> > -	struct device_node *panel_node;
> > -	struct drm_panel *panel;
> > -	struct lvds_encoder *lvds_encoder;
> > -
> > -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> > -	if (!lvds_encoder)
> > -		return -ENOMEM;
> > -
> > -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > -							       GPIOD_OUT_HIGH);
> > -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> > -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> > -
> > -		if (err != -EPROBE_DEFER)
> > -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > -		return err;
> > -	}
> > -
> > -	/* Locate the panel DT node. */
> > -	port = of_graph_get_port_by_id(dev->of_node, 1);
> > -	if (!port) {
> > -		dev_dbg(dev, "port 1 not found\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	endpoint = of_get_child_by_name(port, "endpoint");
> > -	of_node_put(port);
> > -	if (!endpoint) {
> > -		dev_dbg(dev, "no endpoint for port 1\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	panel_node = of_graph_get_remote_port_parent(endpoint);
> > -	of_node_put(endpoint);
> > -	if (!panel_node) {
> > -		dev_dbg(dev, "no remote endpoint for port 1\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	panel = of_drm_find_panel(panel_node);
> > -	of_node_put(panel_node);
> > -	if (IS_ERR(panel)) {
> > -		dev_dbg(dev, "panel not found, deferring probe\n");
> > -		return PTR_ERR(panel);
> > -	}
> > -
> > -	lvds_encoder->panel_bridge =
> > -		devm_drm_panel_bridge_add_typed(dev, panel,
> > -						DRM_MODE_CONNECTOR_LVDS);
> > -	if (IS_ERR(lvds_encoder->panel_bridge))
> > -		return PTR_ERR(lvds_encoder->panel_bridge);
> > -
> > -	/* The panel_bridge bridge is attached to the panel's of_node,
> > -	 * but we need a bridge attached to our of_node for our user
> > -	 * to look up.
> > -	 */
> > -	lvds_encoder->bridge.of_node = dev->of_node;
> > -	lvds_encoder->bridge.funcs = &funcs;
> > -	drm_bridge_add(&lvds_encoder->bridge);
> > -
> > -	platform_set_drvdata(pdev, lvds_encoder);
> > -
> > -	return 0;
> > -}
> > -
> > -static int lvds_encoder_remove(struct platform_device *pdev)
> > -{
> > -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> > -
> > -	drm_bridge_remove(&lvds_encoder->bridge);
> > -
> > -	return 0;
> > -}
> > -
> > -static const struct of_device_id lvds_encoder_match[] = {
> > -	{ .compatible = "lvds-encoder" },
> > -	{ .compatible = "thine,thc63lvdm83d" },
> > -	{},
> > -};
> > -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> > -
> > -static struct platform_driver lvds_encoder_driver = {
> > -	.probe	= lvds_encoder_probe,
> > -	.remove	= lvds_encoder_remove,
> > -	.driver		= {
> > -		.name		= "lvds-encoder",
> > -		.of_match_table	= lvds_encoder_match,
> > -	},
> > -};
> > -module_platform_driver(lvds_encoder_driver);
> > -
> > -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> > -MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> >

RE: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Fabrizio Castro]

Hi Jacopo,

Thank you for your feedback!

> From: Jacopo Mondi <jacopo@jmondi.org>
> Sent: 07 November 2019 18:19
> Subject: Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
> 
> Hi Fabrizio,
>   thanks for the patch.
> 
> On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> > lvds-encoder.c implementation is also suitable for LVDS decoders,
> > not just LVDS encoders.
> > Instead of creating a new driver for addressing support for
> > transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> > good.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * No change
> > ---
> >  drivers/gpu/drm/bridge/Kconfig        |   8 +-
> >  drivers/gpu/drm/bridge/Makefile       |   2 +-
> >  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
> >  4 files changed, 174 insertions(+), 160 deletions(-)
> >  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> >  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 3436297..9e75ca4e 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
> >  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
> >  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
> >
> > -config DRM_LVDS_ENCODER
> > -	tristate "Transparent parallel to LVDS encoder support"
> > +config DRM_LVDS_CODEC
> > +	tristate "Transparent LVDS encoders and decoders support"
> >  	depends on OF
> >  	select DRM_KMS_HELPER
> >  	select DRM_PANEL_BRIDGE
> >  	help
> > -	  Support for transparent parallel to LVDS encoders that don't require
> > -	  any configuration.
> > +	  Support for transparent LVDS encoders and LVDS decoders that don't
> > +	  require any configuration.
> >
> >  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
> >  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf..8a9178a 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -2,7 +2,7 @@
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> >  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > new file mode 100644
> > index 0000000..8a1979c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_panel.h>
> > +
> > +struct lvds_codec {
> > +	struct drm_bridge bridge;
> > +	struct drm_bridge *panel_bridge;
> > +	struct gpio_desc *powerdown_gpio;
> > +	u32 connector_type;
> > +};
> > +
> > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> 
> Weird indentation. Align to open ( to match the rest of the driver's
> style.

Will do, throughout the file.

> 
> > +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > +				 bridge);
> > +}
> > +
> > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> 
> Here too
> 
> > +	if (lvds_codec->powerdown_gpio)
> > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > +}
> > +
> > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > +{
> > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > +							 struct lvds_codec,
> > +							 bridge);
> > +
> > +	if (lvds_codec->powerdown_gpio)
> > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > +}
> > +
> > +static struct drm_bridge_funcs funcs = {
> > +	.attach = lvds_codec_attach,
> > +	.enable = lvds_codec_enable,
> > +	.disable = lvds_codec_disable,
> > +};
> > +
> > +static int lvds_codec_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *port;
> > +	struct device_node *endpoint;
> > +	struct device_node *panel_node;
> > +	struct drm_panel *panel;
> > +	struct lvds_codec *lvds_codec;
> > +
> > +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > +	if (!lvds_codec)
> > +		return -ENOMEM;
> > +
> > +	lvds_codec->connector_type = (u32)
> > +		of_device_get_match_data(&pdev->dev);
> 
> Fits in 1 line

Will take it out

> 
> > +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > +							       GPIOD_OUT_HIGH);
> > +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> > +
> > +		if (err != -EPROBE_DEFER)
> > +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > +		return err;
> 
> I know it was there already, but this seems a bit unusual for the
> minimal gain of having a printout in the very unlikely case the
> gpiod_get() operations fails. I would just return PTR_ERR().

Will change

> 
> > +	}
> > +
> > +	/* Locate the panel DT node. */
> > +	port = of_graph_get_port_by_id(dev->of_node, 1);
> > +	if (!port) {
> > +		dev_dbg(dev, "port 1 not found\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	endpoint = of_get_child_by_name(port, "endpoint");
> > +	of_node_put(port);
> > +	if (!endpoint) {
> > +		dev_dbg(dev, "no endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> 
> I know it was there already, but this could be simplified with
> of_graph_get_endpoint_by_regs()
> > +
> > +	panel_node = of_graph_get_remote_port_parent(endpoint);
> > +	of_node_put(endpoint);
> > +	if (!panel_node) {
> > +		dev_dbg(dev, "no remote endpoint for port 1\n");
> > +		return -ENXIO;
> > +	}
> 
> Or even better, simplify these three with of_graph_get_remote_node()

Good catch,  of_graph_get_remote_node seems the best approach,
I will go with that

> 
> > +
> > +	panel = of_drm_find_panel(panel_node);
> > +	of_node_put(panel_node);
> > +	if (IS_ERR(panel)) {
> > +		dev_dbg(dev, "panel not found, deferring probe\n");
> > +		return PTR_ERR(panel);
> > +	}
> > +
> > +	lvds_codec->panel_bridge =
> > +		devm_drm_panel_bridge_add_typed(dev, panel,
> > +						lvds_codec->connector_type);
> 
> The function documentation reports this as deprecated and suggested to
> use the non-typed version. Do you think it could work for this new
> codec driver ?

I think so. However, the panel I am using doesn't define the connector type,
probably because none of the DRM_MODE_CONNECTOR_* seem to fit
perfectly, therefore I am probably going to need to define
DRM_MODE_CONNECTOR_PARALLEL.
I'll send v3 shortly so that you can have a look at what I mean, please advise
on the best course of action.

> 
> > +	if (IS_ERR(lvds_codec->panel_bridge))
> > +		return PTR_ERR(lvds_codec->panel_bridge);
> > +
> > +	/* The panel_bridge bridge is attached to the panel's of_node,
> > +	 * but we need a bridge attached to our of_node for our user
> > +	 * to look up.
> > +	 */
> > +	lvds_codec->bridge.of_node = dev->of_node;
> > +	lvds_codec->bridge.funcs = &funcs;
> > +	drm_bridge_add(&lvds_codec->bridge);
> > +
> > +	platform_set_drvdata(pdev, lvds_codec);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lvds_codec_remove(struct platform_device *pdev)
> > +{
> > +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > +
> > +	drm_bridge_remove(&lvds_codec->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id lvds_codec_match[] = {
> > +	{
> > +		.compatible = "lvds-encoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS
> 
> In case you for for drm_panel_bridge_add() you could drop the type

Will do

> 
> > +	},
> > +	{
> > +		.compatible = "thine,thc63lvdm83d",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> > +	{
> > +		.compatible = "lvds-decoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> > +	},
> 
> Which decoder are you using? This is a generic fallback, but I would
> expect compatible for a real device to appear in DTS.

I am using the DS90CF384AMTDX/NOPB.

This driver is for transparent encoders and decoders, which means
there is no way you can control them. I don't think we should add
any device specific compatible string here, as it would be of no use.
The original dt-bindings for the encoders requires a device specific
compatible string for encoders with additional properties, I think
we should keep it that way for decoders too. I am not going to
use a device specific compatible string in the DT either for the
time being, unless really necessary.

I will send a v3 shortly. Thank you for your help so far!

Fab

> 
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > +
> > +static struct platform_driver lvds_codec_driver = {
> > +	.probe	= lvds_codec_probe,
> > +	.remove	= lvds_codec_remove,
> > +	.driver		= {
> > +		.name		= "lvds-codec",
> > +		.of_match_table	= lvds_codec_match,
> > +	},
> > +};
> > +module_platform_driver(lvds_codec_driver);
> > +
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > deleted file mode 100644
> > index e2132a8..0000000
> > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > +++ /dev/null
> > @@ -1,155 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0-or-later
> > -/*
> > - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > - */
> > -
> > -#include <linux/gpio/consumer.h>
> > -#include <linux/module.h>
> > -#include <linux/of.h>
> > -#include <linux/of_graph.h>
> > -#include <linux/platform_device.h>
> > -
> > -#include <drm/drm_bridge.h>
> > -#include <drm/drm_panel.h>
> > -
> > -struct lvds_encoder {
> > -	struct drm_bridge bridge;
> > -	struct drm_bridge *panel_bridge;
> > -	struct gpio_desc *powerdown_gpio;
> > -};
> > -
> > -static int lvds_encoder_attach(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> > -				 bridge);
> > -}
> > -
> > -static void lvds_encoder_enable(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	if (lvds_encoder->powerdown_gpio)
> > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> > -}
> > -
> > -static void lvds_encoder_disable(struct drm_bridge *bridge)
> > -{
> > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > -							 struct lvds_encoder,
> > -							 bridge);
> > -
> > -	if (lvds_encoder->powerdown_gpio)
> > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> > -}
> > -
> > -static struct drm_bridge_funcs funcs = {
> > -	.attach = lvds_encoder_attach,
> > -	.enable = lvds_encoder_enable,
> > -	.disable = lvds_encoder_disable,
> > -};
> > -
> > -static int lvds_encoder_probe(struct platform_device *pdev)
> > -{
> > -	struct device *dev = &pdev->dev;
> > -	struct device_node *port;
> > -	struct device_node *endpoint;
> > -	struct device_node *panel_node;
> > -	struct drm_panel *panel;
> > -	struct lvds_encoder *lvds_encoder;
> > -
> > -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> > -	if (!lvds_encoder)
> > -		return -ENOMEM;
> > -
> > -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > -							       GPIOD_OUT_HIGH);
> > -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> > -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> > -
> > -		if (err != -EPROBE_DEFER)
> > -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > -		return err;
> > -	}
> > -
> > -	/* Locate the panel DT node. */
> > -	port = of_graph_get_port_by_id(dev->of_node, 1);
> > -	if (!port) {
> > -		dev_dbg(dev, "port 1 not found\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	endpoint = of_get_child_by_name(port, "endpoint");
> > -	of_node_put(port);
> > -	if (!endpoint) {
> > -		dev_dbg(dev, "no endpoint for port 1\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	panel_node = of_graph_get_remote_port_parent(endpoint);
> > -	of_node_put(endpoint);
> > -	if (!panel_node) {
> > -		dev_dbg(dev, "no remote endpoint for port 1\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	panel = of_drm_find_panel(panel_node);
> > -	of_node_put(panel_node);
> > -	if (IS_ERR(panel)) {
> > -		dev_dbg(dev, "panel not found, deferring probe\n");
> > -		return PTR_ERR(panel);
> > -	}
> > -
> > -	lvds_encoder->panel_bridge =
> > -		devm_drm_panel_bridge_add_typed(dev, panel,
> > -						DRM_MODE_CONNECTOR_LVDS);
> > -	if (IS_ERR(lvds_encoder->panel_bridge))
> > -		return PTR_ERR(lvds_encoder->panel_bridge);
> > -
> > -	/* The panel_bridge bridge is attached to the panel's of_node,
> > -	 * but we need a bridge attached to our of_node for our user
> > -	 * to look up.
> > -	 */
> > -	lvds_encoder->bridge.of_node = dev->of_node;
> > -	lvds_encoder->bridge.funcs = &funcs;
> > -	drm_bridge_add(&lvds_encoder->bridge);
> > -
> > -	platform_set_drvdata(pdev, lvds_encoder);
> > -
> > -	return 0;
> > -}
> > -
> > -static int lvds_encoder_remove(struct platform_device *pdev)
> > -{
> > -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> > -
> > -	drm_bridge_remove(&lvds_encoder->bridge);
> > -
> > -	return 0;
> > -}
> > -
> > -static const struct of_device_id lvds_encoder_match[] = {
> > -	{ .compatible = "lvds-encoder" },
> > -	{ .compatible = "thine,thc63lvdm83d" },
> > -	{},
> > -};
> > -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> > -
> > -static struct platform_driver lvds_encoder_driver = {
> > -	.probe	= lvds_encoder_probe,
> > -	.remove	= lvds_encoder_remove,
> > -	.driver		= {
> > -		.name		= "lvds-encoder",
> > -		.of_match_table	= lvds_encoder_match,
> > -	},
> > -};
> > -module_platform_driver(lvds_encoder_driver);
> > -
> > -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> > -MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Jacopo Mondi]

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

Hi Fabrizio,

On Thu, Nov 07, 2019 at 08:02:25PM +0000, Fabrizio Castro wrote:
> Hi Jacopo,
>
> Thank you for your feedback!
>
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > Sent: 07 November 2019 18:19
> > Subject: Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
> >
> > Hi Fabrizio,
> >   thanks for the patch.
> >
> > On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> > > lvds-encoder.c implementation is also suitable for LVDS decoders,
> > > not just LVDS encoders.
> > > Instead of creating a new driver for addressing support for
> > > transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> > > good.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * No change
> > > ---
> > >  drivers/gpu/drm/bridge/Kconfig        |   8 +-
> > >  drivers/gpu/drm/bridge/Makefile       |   2 +-
> > >  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
> > >  4 files changed, 174 insertions(+), 160 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > >  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > >
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index 3436297..9e75ca4e 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
> > >  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
> > >  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
> > >
> > > -config DRM_LVDS_ENCODER
> > > -	tristate "Transparent parallel to LVDS encoder support"
> > > +config DRM_LVDS_CODEC
> > > +	tristate "Transparent LVDS encoders and decoders support"
> > >  	depends on OF
> > >  	select DRM_KMS_HELPER
> > >  	select DRM_PANEL_BRIDGE
> > >  	help
> > > -	  Support for transparent parallel to LVDS encoders that don't require
> > > -	  any configuration.
> > > +	  Support for transparent LVDS encoders and LVDS decoders that don't
> > > +	  require any configuration.
> > >
> > >  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
> > >  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > index 4934fcf..8a9178a 100644
> > > --- a/drivers/gpu/drm/bridge/Makefile
> > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > @@ -2,7 +2,7 @@
> > >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > > -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > > +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> > >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> > >  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > new file mode 100644
> > > index 0000000..8a1979c
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -0,0 +1,169 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > + */
> > > +
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +struct lvds_codec {
> > > +	struct drm_bridge bridge;
> > > +	struct drm_bridge *panel_bridge;
> > > +	struct gpio_desc *powerdown_gpio;
> > > +	u32 connector_type;
> > > +};
> > > +
> > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> >
> > Weird indentation. Align to open ( to match the rest of the driver's
> > style.
>
> Will do, throughout the file.
>
> >
> > > +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > +				 bridge);
> > > +}
> > > +
> > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> >
> > Here too
> >
> > > +	if (lvds_codec->powerdown_gpio)
> > > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > +}
> > > +
> > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> > > +	if (lvds_codec->powerdown_gpio)
> > > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > +}
> > > +
> > > +static struct drm_bridge_funcs funcs = {
> > > +	.attach = lvds_codec_attach,
> > > +	.enable = lvds_codec_enable,
> > > +	.disable = lvds_codec_disable,
> > > +};
> > > +
> > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *port;
> > > +	struct device_node *endpoint;
> > > +	struct device_node *panel_node;
> > > +	struct drm_panel *panel;
> > > +	struct lvds_codec *lvds_codec;
> > > +
> > > +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > +	if (!lvds_codec)
> > > +		return -ENOMEM;
> > > +
> > > +	lvds_codec->connector_type = (u32)
> > > +		of_device_get_match_data(&pdev->dev);
> >
> > Fits in 1 line
>
> Will take it out
>
> >
> > > +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > +							       GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> > > +
> > > +		if (err != -EPROBE_DEFER)
> > > +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > > +		return err;
> >
> > I know it was there already, but this seems a bit unusual for the
> > minimal gain of having a printout in the very unlikely case the
> > gpiod_get() operations fails. I would just return PTR_ERR().
>
> Will change
>
> >
> > > +	}
> > > +
> > > +	/* Locate the panel DT node. */
> > > +	port = of_graph_get_port_by_id(dev->of_node, 1);
> > > +	if (!port) {
> > > +		dev_dbg(dev, "port 1 not found\n");
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	endpoint = of_get_child_by_name(port, "endpoint");
> > > +	of_node_put(port);
> > > +	if (!endpoint) {
> > > +		dev_dbg(dev, "no endpoint for port 1\n");
> > > +		return -ENXIO;
> > > +	}
> >
> > I know it was there already, but this could be simplified with
> > of_graph_get_endpoint_by_regs()
> > > +
> > > +	panel_node = of_graph_get_remote_port_parent(endpoint);
> > > +	of_node_put(endpoint);
> > > +	if (!panel_node) {
> > > +		dev_dbg(dev, "no remote endpoint for port 1\n");
> > > +		return -ENXIO;
> > > +	}
> >
> > Or even better, simplify these three with of_graph_get_remote_node()
>
> Good catch,  of_graph_get_remote_node seems the best approach,
> I will go with that
>
> >
> > > +
> > > +	panel = of_drm_find_panel(panel_node);
> > > +	of_node_put(panel_node);
> > > +	if (IS_ERR(panel)) {
> > > +		dev_dbg(dev, "panel not found, deferring probe\n");
> > > +		return PTR_ERR(panel);
> > > +	}
> > > +
> > > +	lvds_codec->panel_bridge =
> > > +		devm_drm_panel_bridge_add_typed(dev, panel,
> > > +						lvds_codec->connector_type);
> >
> > The function documentation reports this as deprecated and suggested to
> > use the non-typed version. Do you think it could work for this new
> > codec driver ?
>
> I think so. However, the panel I am using doesn't define the connector type,
> probably because none of the DRM_MODE_CONNECTOR_* seem to fit
> perfectly, therefore I am probably going to need to define
> DRM_MODE_CONNECTOR_PARALLEL.

I see.. I'm no expert of DRM at all, but it seems to me too that none
of the existing DRM_MODE_CONNECTOR_* fits. I found no documentation
for them and that doesn't help... Although it seems weird there are no
similar use cases in mainline..

> I'll send v3 shortly so that you can have a look at what I mean, please advise
> on the best course of action.
>
> >
> > > +	if (IS_ERR(lvds_codec->panel_bridge))
> > > +		return PTR_ERR(lvds_codec->panel_bridge);
> > > +
> > > +	/* The panel_bridge bridge is attached to the panel's of_node,
> > > +	 * but we need a bridge attached to our of_node for our user
> > > +	 * to look up.
> > > +	 */
> > > +	lvds_codec->bridge.of_node = dev->of_node;
> > > +	lvds_codec->bridge.funcs = &funcs;
> > > +	drm_bridge_add(&lvds_codec->bridge);
> > > +
> > > +	platform_set_drvdata(pdev, lvds_codec);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > +{
> > > +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > +
> > > +	drm_bridge_remove(&lvds_codec->bridge);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id lvds_codec_match[] = {
> > > +	{
> > > +		.compatible = "lvds-encoder",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS
> >
> > In case you for for drm_panel_bridge_add() you could drop the type
>
> Will do
>
> >
> > > +	},
> > > +	{
> > > +		.compatible = "thine,thc63lvdm83d",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > +	},
> > > +	{
> > > +		.compatible = "lvds-decoder",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> > > +	},
> >
> > Which decoder are you using? This is a generic fallback, but I would
> > expect compatible for a real device to appear in DTS.
>
> I am using the DS90CF384AMTDX/NOPB.
>
> This driver is for transparent encoders and decoders, which means
> there is no way you can control them. I don't think we should add
> any device specific compatible string here, as it would be of no use.
> The original dt-bindings for the encoders requires a device specific
> compatible string for encoders with additional properties, I think
> we should keep it that way for decoders too. I am not going to
> use a device specific compatible string in the DT either for the
> time being, unless really necessary.
>

I see, but in example your chip has a powerdown gpio pin, which is not
defined in the lvds-codec bindings. A compatible string for that device
should be added, with the associated DT binding file, much like it's
done for the "thine,thc63lvdm83d" one. Nothing that could not be
done later on top of what you have here though

Thanks
  j

> I will send a v3 shortly. Thank you for your help so far!
>
> Fab
>
> >
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > +
> > > +static struct platform_driver lvds_codec_driver = {
> > > +	.probe	= lvds_codec_probe,
> > > +	.remove	= lvds_codec_remove,
> > > +	.driver		= {
> > > +		.name		= "lvds-codec",
> > > +		.of_match_table	= lvds_codec_match,
> > > +	},
> > > +};
> > > +module_platform_driver(lvds_codec_driver);
> > > +
> > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > > deleted file mode 100644
> > > index e2132a8..0000000
> > > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > > +++ /dev/null
> > > @@ -1,155 +0,0 @@
> > > -// SPDX-License-Identifier: GPL-2.0-or-later
> > > -/*
> > > - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > - */
> > > -
> > > -#include <linux/gpio/consumer.h>
> > > -#include <linux/module.h>
> > > -#include <linux/of.h>
> > > -#include <linux/of_graph.h>
> > > -#include <linux/platform_device.h>
> > > -
> > > -#include <drm/drm_bridge.h>
> > > -#include <drm/drm_panel.h>
> > > -
> > > -struct lvds_encoder {
> > > -	struct drm_bridge bridge;
> > > -	struct drm_bridge *panel_bridge;
> > > -	struct gpio_desc *powerdown_gpio;
> > > -};
> > > -
> > > -static int lvds_encoder_attach(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> > > -				 bridge);
> > > -}
> > > -
> > > -static void lvds_encoder_enable(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	if (lvds_encoder->powerdown_gpio)
> > > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> > > -}
> > > -
> > > -static void lvds_encoder_disable(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	if (lvds_encoder->powerdown_gpio)
> > > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> > > -}
> > > -
> > > -static struct drm_bridge_funcs funcs = {
> > > -	.attach = lvds_encoder_attach,
> > > -	.enable = lvds_encoder_enable,
> > > -	.disable = lvds_encoder_disable,
> > > -};
> > > -
> > > -static int lvds_encoder_probe(struct platform_device *pdev)
> > > -{
> > > -	struct device *dev = &pdev->dev;
> > > -	struct device_node *port;
> > > -	struct device_node *endpoint;
> > > -	struct device_node *panel_node;
> > > -	struct drm_panel *panel;
> > > -	struct lvds_encoder *lvds_encoder;
> > > -
> > > -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> > > -	if (!lvds_encoder)
> > > -		return -ENOMEM;
> > > -
> > > -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > -							       GPIOD_OUT_HIGH);
> > > -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> > > -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> > > -
> > > -		if (err != -EPROBE_DEFER)
> > > -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > > -		return err;
> > > -	}
> > > -
> > > -	/* Locate the panel DT node. */
> > > -	port = of_graph_get_port_by_id(dev->of_node, 1);
> > > -	if (!port) {
> > > -		dev_dbg(dev, "port 1 not found\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	endpoint = of_get_child_by_name(port, "endpoint");
> > > -	of_node_put(port);
> > > -	if (!endpoint) {
> > > -		dev_dbg(dev, "no endpoint for port 1\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	panel_node = of_graph_get_remote_port_parent(endpoint);
> > > -	of_node_put(endpoint);
> > > -	if (!panel_node) {
> > > -		dev_dbg(dev, "no remote endpoint for port 1\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	panel = of_drm_find_panel(panel_node);
> > > -	of_node_put(panel_node);
> > > -	if (IS_ERR(panel)) {
> > > -		dev_dbg(dev, "panel not found, deferring probe\n");
> > > -		return PTR_ERR(panel);
> > > -	}
> > > -
> > > -	lvds_encoder->panel_bridge =
> > > -		devm_drm_panel_bridge_add_typed(dev, panel,
> > > -						DRM_MODE_CONNECTOR_LVDS);
> > > -	if (IS_ERR(lvds_encoder->panel_bridge))
> > > -		return PTR_ERR(lvds_encoder->panel_bridge);
> > > -
> > > -	/* The panel_bridge bridge is attached to the panel's of_node,
> > > -	 * but we need a bridge attached to our of_node for our user
> > > -	 * to look up.
> > > -	 */
> > > -	lvds_encoder->bridge.of_node = dev->of_node;
> > > -	lvds_encoder->bridge.funcs = &funcs;
> > > -	drm_bridge_add(&lvds_encoder->bridge);
> > > -
> > > -	platform_set_drvdata(pdev, lvds_encoder);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static int lvds_encoder_remove(struct platform_device *pdev)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> > > -
> > > -	drm_bridge_remove(&lvds_encoder->bridge);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static const struct of_device_id lvds_encoder_match[] = {
> > > -	{ .compatible = "lvds-encoder" },
> > > -	{ .compatible = "thine,thc63lvdm83d" },
> > > -	{},
> > > -};
> > > -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> > > -
> > > -static struct platform_driver lvds_encoder_driver = {
> > > -	.probe	= lvds_encoder_probe,
> > > -	.remove	= lvds_encoder_remove,
> > > -	.driver		= {
> > > -		.name		= "lvds-encoder",
> > > -		.of_match_table	= lvds_encoder_match,
> > > -	},
> > > -};
> > > -module_platform_driver(lvds_encoder_driver);
> > > -
> > > -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> > > -MODULE_LICENSE("GPL");
> > > --
> > > 2.7.4
> > >

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

Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c

[Jacopo Mondi]

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

Hi Fabrizio,

On Thu, Nov 07, 2019 at 08:02:25PM +0000, Fabrizio Castro wrote:
> Hi Jacopo,
>
> Thank you for your feedback!
>
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > Sent: 07 November 2019 18:19
> > Subject: Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
> >
> > Hi Fabrizio,
> >   thanks for the patch.
> >
> > On Mon, Nov 04, 2019 at 04:58:00PM +0000, Fabrizio Castro wrote:
> > > lvds-encoder.c implementation is also suitable for LVDS decoders,
> > > not just LVDS encoders.
> > > Instead of creating a new driver for addressing support for
> > > transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> > > good.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * No change
> > > ---
> > >  drivers/gpu/drm/bridge/Kconfig        |   8 +-
> > >  drivers/gpu/drm/bridge/Makefile       |   2 +-
> > >  drivers/gpu/drm/bridge/lvds-codec.c   | 169 ++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/bridge/lvds-encoder.c | 155 -------------------------------
> > >  4 files changed, 174 insertions(+), 160 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > >  delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > >
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index 3436297..9e75ca4e 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
> > >  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
> > >  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
> > >
> > > -config DRM_LVDS_ENCODER
> > > -	tristate "Transparent parallel to LVDS encoder support"
> > > +config DRM_LVDS_CODEC
> > > +	tristate "Transparent LVDS encoders and decoders support"
> > >  	depends on OF
> > >  	select DRM_KMS_HELPER
> > >  	select DRM_PANEL_BRIDGE
> > >  	help
> > > -	  Support for transparent parallel to LVDS encoders that don't require
> > > -	  any configuration.
> > > +	  Support for transparent LVDS encoders and LVDS decoders that don't
> > > +	  require any configuration.
> > >
> > >  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
> > >  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > index 4934fcf..8a9178a 100644
> > > --- a/drivers/gpu/drm/bridge/Makefile
> > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > @@ -2,7 +2,7 @@
> > >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > > -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > > +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> > >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> > >  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > new file mode 100644
> > > index 0000000..8a1979c
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -0,0 +1,169 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > + */
> > > +
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +struct lvds_codec {
> > > +	struct drm_bridge bridge;
> > > +	struct drm_bridge *panel_bridge;
> > > +	struct gpio_desc *powerdown_gpio;
> > > +	u32 connector_type;
> > > +};
> > > +
> > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> >
> > Weird indentation. Align to open ( to match the rest of the driver's
> > style.
>
> Will do, throughout the file.
>
> >
> > > +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > +				 bridge);
> > > +}
> > > +
> > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> >
> > Here too
> >
> > > +	if (lvds_codec->powerdown_gpio)
> > > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > +}
> > > +
> > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > +{
> > > +	struct lvds_codec *lvds_codec = container_of(bridge,
> > > +							 struct lvds_codec,
> > > +							 bridge);
> > > +
> > > +	if (lvds_codec->powerdown_gpio)
> > > +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > +}
> > > +
> > > +static struct drm_bridge_funcs funcs = {
> > > +	.attach = lvds_codec_attach,
> > > +	.enable = lvds_codec_enable,
> > > +	.disable = lvds_codec_disable,
> > > +};
> > > +
> > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *port;
> > > +	struct device_node *endpoint;
> > > +	struct device_node *panel_node;
> > > +	struct drm_panel *panel;
> > > +	struct lvds_codec *lvds_codec;
> > > +
> > > +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > +	if (!lvds_codec)
> > > +		return -ENOMEM;
> > > +
> > > +	lvds_codec->connector_type = (u32)
> > > +		of_device_get_match_data(&pdev->dev);
> >
> > Fits in 1 line
>
> Will take it out
>
> >
> > > +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > +							       GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> > > +
> > > +		if (err != -EPROBE_DEFER)
> > > +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > > +		return err;
> >
> > I know it was there already, but this seems a bit unusual for the
> > minimal gain of having a printout in the very unlikely case the
> > gpiod_get() operations fails. I would just return PTR_ERR().
>
> Will change
>
> >
> > > +	}
> > > +
> > > +	/* Locate the panel DT node. */
> > > +	port = of_graph_get_port_by_id(dev->of_node, 1);
> > > +	if (!port) {
> > > +		dev_dbg(dev, "port 1 not found\n");
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	endpoint = of_get_child_by_name(port, "endpoint");
> > > +	of_node_put(port);
> > > +	if (!endpoint) {
> > > +		dev_dbg(dev, "no endpoint for port 1\n");
> > > +		return -ENXIO;
> > > +	}
> >
> > I know it was there already, but this could be simplified with
> > of_graph_get_endpoint_by_regs()
> > > +
> > > +	panel_node = of_graph_get_remote_port_parent(endpoint);
> > > +	of_node_put(endpoint);
> > > +	if (!panel_node) {
> > > +		dev_dbg(dev, "no remote endpoint for port 1\n");
> > > +		return -ENXIO;
> > > +	}
> >
> > Or even better, simplify these three with of_graph_get_remote_node()
>
> Good catch,  of_graph_get_remote_node seems the best approach,
> I will go with that
>
> >
> > > +
> > > +	panel = of_drm_find_panel(panel_node);
> > > +	of_node_put(panel_node);
> > > +	if (IS_ERR(panel)) {
> > > +		dev_dbg(dev, "panel not found, deferring probe\n");
> > > +		return PTR_ERR(panel);
> > > +	}
> > > +
> > > +	lvds_codec->panel_bridge =
> > > +		devm_drm_panel_bridge_add_typed(dev, panel,
> > > +						lvds_codec->connector_type);
> >
> > The function documentation reports this as deprecated and suggested to
> > use the non-typed version. Do you think it could work for this new
> > codec driver ?
>
> I think so. However, the panel I am using doesn't define the connector type,
> probably because none of the DRM_MODE_CONNECTOR_* seem to fit
> perfectly, therefore I am probably going to need to define
> DRM_MODE_CONNECTOR_PARALLEL.

I see.. I'm no expert of DRM at all, but it seems to me too that none
of the existing DRM_MODE_CONNECTOR_* fits. I found no documentation
for them and that doesn't help... Although it seems weird there are no
similar use cases in mainline..

> I'll send v3 shortly so that you can have a look at what I mean, please advise
> on the best course of action.
>
> >
> > > +	if (IS_ERR(lvds_codec->panel_bridge))
> > > +		return PTR_ERR(lvds_codec->panel_bridge);
> > > +
> > > +	/* The panel_bridge bridge is attached to the panel's of_node,
> > > +	 * but we need a bridge attached to our of_node for our user
> > > +	 * to look up.
> > > +	 */
> > > +	lvds_codec->bridge.of_node = dev->of_node;
> > > +	lvds_codec->bridge.funcs = &funcs;
> > > +	drm_bridge_add(&lvds_codec->bridge);
> > > +
> > > +	platform_set_drvdata(pdev, lvds_codec);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > +{
> > > +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > +
> > > +	drm_bridge_remove(&lvds_codec->bridge);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id lvds_codec_match[] = {
> > > +	{
> > > +		.compatible = "lvds-encoder",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS
> >
> > In case you for for drm_panel_bridge_add() you could drop the type
>
> Will do
>
> >
> > > +	},
> > > +	{
> > > +		.compatible = "thine,thc63lvdm83d",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > +	},
> > > +	{
> > > +		.compatible = "lvds-decoder",
> > > +		.data = (void *)DRM_MODE_CONNECTOR_Unknown,
> > > +	},
> >
> > Which decoder are you using? This is a generic fallback, but I would
> > expect compatible for a real device to appear in DTS.
>
> I am using the DS90CF384AMTDX/NOPB.
>
> This driver is for transparent encoders and decoders, which means
> there is no way you can control them. I don't think we should add
> any device specific compatible string here, as it would be of no use.
> The original dt-bindings for the encoders requires a device specific
> compatible string for encoders with additional properties, I think
> we should keep it that way for decoders too. I am not going to
> use a device specific compatible string in the DT either for the
> time being, unless really necessary.
>

I see, but in example your chip has a powerdown gpio pin, which is not
defined in the lvds-codec bindings. A compatible string for that device
should be added, with the associated DT binding file, much like it's
done for the "thine,thc63lvdm83d" one. Nothing that could not be
done later on top of what you have here though

Thanks
  j

> I will send a v3 shortly. Thank you for your help so far!
>
> Fab
>
> >
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > +
> > > +static struct platform_driver lvds_codec_driver = {
> > > +	.probe	= lvds_codec_probe,
> > > +	.remove	= lvds_codec_remove,
> > > +	.driver		= {
> > > +		.name		= "lvds-codec",
> > > +		.of_match_table	= lvds_codec_match,
> > > +	},
> > > +};
> > > +module_platform_driver(lvds_codec_driver);
> > > +
> > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > > deleted file mode 100644
> > > index e2132a8..0000000
> > > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > > +++ /dev/null
> > > @@ -1,155 +0,0 @@
> > > -// SPDX-License-Identifier: GPL-2.0-or-later
> > > -/*
> > > - * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > - */
> > > -
> > > -#include <linux/gpio/consumer.h>
> > > -#include <linux/module.h>
> > > -#include <linux/of.h>
> > > -#include <linux/of_graph.h>
> > > -#include <linux/platform_device.h>
> > > -
> > > -#include <drm/drm_bridge.h>
> > > -#include <drm/drm_panel.h>
> > > -
> > > -struct lvds_encoder {
> > > -	struct drm_bridge bridge;
> > > -	struct drm_bridge *panel_bridge;
> > > -	struct gpio_desc *powerdown_gpio;
> > > -};
> > > -
> > > -static int lvds_encoder_attach(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> > > -				 bridge);
> > > -}
> > > -
> > > -static void lvds_encoder_enable(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	if (lvds_encoder->powerdown_gpio)
> > > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> > > -}
> > > -
> > > -static void lvds_encoder_disable(struct drm_bridge *bridge)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> > > -							 struct lvds_encoder,
> > > -							 bridge);
> > > -
> > > -	if (lvds_encoder->powerdown_gpio)
> > > -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> > > -}
> > > -
> > > -static struct drm_bridge_funcs funcs = {
> > > -	.attach = lvds_encoder_attach,
> > > -	.enable = lvds_encoder_enable,
> > > -	.disable = lvds_encoder_disable,
> > > -};
> > > -
> > > -static int lvds_encoder_probe(struct platform_device *pdev)
> > > -{
> > > -	struct device *dev = &pdev->dev;
> > > -	struct device_node *port;
> > > -	struct device_node *endpoint;
> > > -	struct device_node *panel_node;
> > > -	struct drm_panel *panel;
> > > -	struct lvds_encoder *lvds_encoder;
> > > -
> > > -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> > > -	if (!lvds_encoder)
> > > -		return -ENOMEM;
> > > -
> > > -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > -							       GPIOD_OUT_HIGH);
> > > -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> > > -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> > > -
> > > -		if (err != -EPROBE_DEFER)
> > > -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> > > -		return err;
> > > -	}
> > > -
> > > -	/* Locate the panel DT node. */
> > > -	port = of_graph_get_port_by_id(dev->of_node, 1);
> > > -	if (!port) {
> > > -		dev_dbg(dev, "port 1 not found\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	endpoint = of_get_child_by_name(port, "endpoint");
> > > -	of_node_put(port);
> > > -	if (!endpoint) {
> > > -		dev_dbg(dev, "no endpoint for port 1\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	panel_node = of_graph_get_remote_port_parent(endpoint);
> > > -	of_node_put(endpoint);
> > > -	if (!panel_node) {
> > > -		dev_dbg(dev, "no remote endpoint for port 1\n");
> > > -		return -ENXIO;
> > > -	}
> > > -
> > > -	panel = of_drm_find_panel(panel_node);
> > > -	of_node_put(panel_node);
> > > -	if (IS_ERR(panel)) {
> > > -		dev_dbg(dev, "panel not found, deferring probe\n");
> > > -		return PTR_ERR(panel);
> > > -	}
> > > -
> > > -	lvds_encoder->panel_bridge =
> > > -		devm_drm_panel_bridge_add_typed(dev, panel,
> > > -						DRM_MODE_CONNECTOR_LVDS);
> > > -	if (IS_ERR(lvds_encoder->panel_bridge))
> > > -		return PTR_ERR(lvds_encoder->panel_bridge);
> > > -
> > > -	/* The panel_bridge bridge is attached to the panel's of_node,
> > > -	 * but we need a bridge attached to our of_node for our user
> > > -	 * to look up.
> > > -	 */
> > > -	lvds_encoder->bridge.of_node = dev->of_node;
> > > -	lvds_encoder->bridge.funcs = &funcs;
> > > -	drm_bridge_add(&lvds_encoder->bridge);
> > > -
> > > -	platform_set_drvdata(pdev, lvds_encoder);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static int lvds_encoder_remove(struct platform_device *pdev)
> > > -{
> > > -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> > > -
> > > -	drm_bridge_remove(&lvds_encoder->bridge);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static const struct of_device_id lvds_encoder_match[] = {
> > > -	{ .compatible = "lvds-encoder" },
> > > -	{ .compatible = "thine,thc63lvdm83d" },
> > > -	{},
> > > -};
> > > -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> > > -
> > > -static struct platform_driver lvds_encoder_driver = {
> > > -	.probe	= lvds_encoder_probe,
> > > -	.remove	= lvds_encoder_remove,
> > > -	.driver		= {
> > > -		.name		= "lvds-encoder",
> > > -		.of_match_table	= lvds_encoder_match,
> > > -	},
> > > -};
> > > -module_platform_driver(lvds_encoder_driver);
> > > -
> > > -MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> > > -MODULE_LICENSE("GPL");
> > > --
> > > 2.7.4
> > >

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

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

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

[PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

In an effort to repurpose lvds-encoder.c to also serve the
function of LVDS decoders, we ended up defining a new "generic"
compatible string, therefore adapt the dt-bindings to fit the
new purpose. Also, convert the dt-bindings from .txt to .yaml
while at it.

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

---
v1->v2:
* Converted to dt-schema as per Neil's comment
---
 .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
 2 files changed, 117 insertions(+), 66 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
new file mode 100644
index 0000000..ff79bc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Trasnparent LVDS encoders and LVDS decoders
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+
+description: |
+  This binding supports transparent LVDS encoders and LVDS decoders that don't
+  require any configuration.
+
+  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
+  incompatible data link layers have been used over time to transmit image data
+  to LVDS panels. This binding targets devices compatible with the following
+  specifications only.
+
+  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
+  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
+  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
+  Semiconductor
+  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
+  Electronics Standards Association (VESA)
+
+  Those devices have been marketed under the FPD-Link and FlatLink brand names
+  among others.
+
+properties:
+  compatible:
+    description: |
+      Any encoder or decoder compatible with this generic binding, but with
+      additional properties not listed here, must define its own binding and
+      list a device specific compatible first followed by the generic compatible
+    items:
+      - enum:
+        - lvds-encoder # for LVDS encoders
+        - lvds-decoder # for LVDS decoders
+
+  ports:
+    type: object
+    description: |
+      This device has two video ports. Their connections are modeled using the
+      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
+    properties:
+      port@0:
+        type: object
+        description: |
+          With LVDS encoders port 0 is for parallel input
+          With LVDS decoders port 0 is for LVDS input
+
+      port@1:
+        type: object
+        description: |
+          With LVDS encoders port 1 is for LVDS output
+          With LVDS decoders port 1 is for parallel output
+
+required:
+  - compatible
+  - ports
+
+examples:
+  - |
+    lvds-encoder {
+      compatible = "lvds-encoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_enc_in: endpoint {
+            remote-endpoint = <&display_out_rgb>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_enc_out: endpoint {
+            remote-endpoint = <&lvds_panel_in>;
+          };
+        };
+      };
+    };
+
+  - |
+    lvds-decoder {
+      compatible = "lvds-decoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_dec_in: endpoint {
+            remote-endpoint = <&display_out_lvds>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_dec_out: endpoint {
+            remote-endpoint = <&rgb_panel_in>;
+          };
+        };
+      };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
deleted file mode 100644
index 60091db..0000000
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-Parallel to LVDS Encoder
-------------------------
-
-This binding supports the parallel to LVDS encoders that don't require any
-configuration.
-
-LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
-incompatible data link layers have been used over time to transmit image data
-to LVDS panels. This binding targets devices compatible with the following
-specifications only.
-
-[JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
-1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
-[LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
-Semiconductor
-[VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
-Electronics Standards Association (VESA)
-
-Those devices have been marketed under the FPD-Link and FlatLink brand names
-among others.
-
-
-Required properties:
-
-- compatible: Must be "lvds-encoder"
-
-  Any encoder compatible with this generic binding, but with additional
-  properties not listed here, must list a device specific compatible first
-  followed by this generic compatible.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for parallel input
-- Video port 1 for LVDS output
-
-
-Example
--------
-
-lvds-encoder {
-	compatible = "lvds-encoder";
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			lvds_enc_in: endpoint {
-				remote-endpoint = <&display_out_rgb>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			lvds_enc_out: endpoint {
-				remote-endpoint = <&lvds_panel_in>;
-			};
-		};
-	};
-};
-- 
2.7.4

[PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

In an effort to repurpose lvds-encoder.c to also serve the
function of LVDS decoders, we ended up defining a new "generic"
compatible string, therefore adapt the dt-bindings to fit the
new purpose. Also, convert the dt-bindings from .txt to .yaml
while at it.

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

---
v1->v2:
* Converted to dt-schema as per Neil's comment
---
 .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
 2 files changed, 117 insertions(+), 66 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
new file mode 100644
index 0000000..ff79bc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Trasnparent LVDS encoders and LVDS decoders
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+
+description: |
+  This binding supports transparent LVDS encoders and LVDS decoders that don't
+  require any configuration.
+
+  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
+  incompatible data link layers have been used over time to transmit image data
+  to LVDS panels. This binding targets devices compatible with the following
+  specifications only.
+
+  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
+  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
+  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
+  Semiconductor
+  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
+  Electronics Standards Association (VESA)
+
+  Those devices have been marketed under the FPD-Link and FlatLink brand names
+  among others.
+
+properties:
+  compatible:
+    description: |
+      Any encoder or decoder compatible with this generic binding, but with
+      additional properties not listed here, must define its own binding and
+      list a device specific compatible first followed by the generic compatible
+    items:
+      - enum:
+        - lvds-encoder # for LVDS encoders
+        - lvds-decoder # for LVDS decoders
+
+  ports:
+    type: object
+    description: |
+      This device has two video ports. Their connections are modeled using the
+      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
+    properties:
+      port@0:
+        type: object
+        description: |
+          With LVDS encoders port 0 is for parallel input
+          With LVDS decoders port 0 is for LVDS input
+
+      port@1:
+        type: object
+        description: |
+          With LVDS encoders port 1 is for LVDS output
+          With LVDS decoders port 1 is for parallel output
+
+required:
+  - compatible
+  - ports
+
+examples:
+  - |
+    lvds-encoder {
+      compatible = "lvds-encoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_enc_in: endpoint {
+            remote-endpoint = <&display_out_rgb>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_enc_out: endpoint {
+            remote-endpoint = <&lvds_panel_in>;
+          };
+        };
+      };
+    };
+
+  - |
+    lvds-decoder {
+      compatible = "lvds-decoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_dec_in: endpoint {
+            remote-endpoint = <&display_out_lvds>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_dec_out: endpoint {
+            remote-endpoint = <&rgb_panel_in>;
+          };
+        };
+      };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
deleted file mode 100644
index 60091db..0000000
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-Parallel to LVDS Encoder
-------------------------
-
-This binding supports the parallel to LVDS encoders that don't require any
-configuration.
-
-LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
-incompatible data link layers have been used over time to transmit image data
-to LVDS panels. This binding targets devices compatible with the following
-specifications only.
-
-[JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
-1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
-[LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
-Semiconductor
-[VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
-Electronics Standards Association (VESA)
-
-Those devices have been marketed under the FPD-Link and FlatLink brand names
-among others.
-
-
-Required properties:
-
-- compatible: Must be "lvds-encoder"
-
-  Any encoder compatible with this generic binding, but with additional
-  properties not listed here, must list a device specific compatible first
-  followed by this generic compatible.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for parallel input
-- Video port 1 for LVDS output
-
-
-Example
--------
-
-lvds-encoder {
-	compatible = "lvds-encoder";
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			lvds_enc_in: endpoint {
-				remote-endpoint = <&display_out_rgb>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			lvds_enc_out: endpoint {
-				remote-endpoint = <&lvds_panel_in>;
-			};
-		};
-	};
-};
-- 
2.7.4

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

Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Rob Herring]

On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>
> In an effort to repurpose lvds-encoder.c to also serve the
> function of LVDS decoders, we ended up defining a new "generic"
> compatible string, therefore adapt the dt-bindings to fit the
> new purpose. Also, convert the dt-bindings from .txt to .yaml
> while at it.

"Also, ... while at it." is a sign for split into 2 patches.

> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
>  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
>  2 files changed, 117 insertions(+), 66 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> new file mode 100644
> index 0000000..ff79bc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Trasnparent LVDS encoders and LVDS decoders

Typo

> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +
> +description: |
> +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> +  require any configuration.
> +
> +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> +  incompatible data link layers have been used over time to transmit image data
> +  to LVDS panels. This binding targets devices compatible with the following
> +  specifications only.
> +
> +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> +  Semiconductor
> +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> +  Electronics Standards Association (VESA)
> +
> +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> +  among others.
> +
> +properties:
> +  compatible:
> +    description: |
> +      Any encoder or decoder compatible with this generic binding, but with
> +      additional properties not listed here, must define its own binding and
> +      list a device specific compatible first followed by the generic compatible
> +    items:
> +      - enum:

You can drop 'items' when there's only 1.

> +        - lvds-encoder # for LVDS encoders
> +        - lvds-decoder # for LVDS decoders
> +
> +  ports:
> +    type: object
> +    description: |
> +      This device has two video ports. Their connections are modeled using the
> +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> +    properties:
> +      port@0:
> +        type: object
> +        description: |
> +          With LVDS encoders port 0 is for parallel input
> +          With LVDS decoders port 0 is for LVDS input
> +
> +      port@1:
> +        type: object
> +        description: |
> +          With LVDS encoders port 1 is for LVDS output
> +          With LVDS decoders port 1 is for parallel output

port@* are required, right?

> +
> +required:
> +  - compatible
> +  - ports
> +
> +examples:
> +  - |
> +    lvds-encoder {
> +      compatible = "lvds-encoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_enc_in: endpoint {
> +            remote-endpoint = <&display_out_rgb>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_enc_out: endpoint {
> +            remote-endpoint = <&lvds_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
> +  - |
> +    lvds-decoder {
> +      compatible = "lvds-decoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_dec_in: endpoint {
> +            remote-endpoint = <&display_out_lvds>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_dec_out: endpoint {
> +            remote-endpoint = <&rgb_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
> +...

Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Rob Herring]

On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>
> In an effort to repurpose lvds-encoder.c to also serve the
> function of LVDS decoders, we ended up defining a new "generic"
> compatible string, therefore adapt the dt-bindings to fit the
> new purpose. Also, convert the dt-bindings from .txt to .yaml
> while at it.

"Also, ... while at it." is a sign for split into 2 patches.

> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
>  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
>  2 files changed, 117 insertions(+), 66 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> new file mode 100644
> index 0000000..ff79bc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Trasnparent LVDS encoders and LVDS decoders

Typo

> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +
> +description: |
> +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> +  require any configuration.
> +
> +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> +  incompatible data link layers have been used over time to transmit image data
> +  to LVDS panels. This binding targets devices compatible with the following
> +  specifications only.
> +
> +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> +  Semiconductor
> +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> +  Electronics Standards Association (VESA)
> +
> +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> +  among others.
> +
> +properties:
> +  compatible:
> +    description: |
> +      Any encoder or decoder compatible with this generic binding, but with
> +      additional properties not listed here, must define its own binding and
> +      list a device specific compatible first followed by the generic compatible
> +    items:
> +      - enum:

You can drop 'items' when there's only 1.

> +        - lvds-encoder # for LVDS encoders
> +        - lvds-decoder # for LVDS decoders
> +
> +  ports:
> +    type: object
> +    description: |
> +      This device has two video ports. Their connections are modeled using the
> +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> +    properties:
> +      port@0:
> +        type: object
> +        description: |
> +          With LVDS encoders port 0 is for parallel input
> +          With LVDS decoders port 0 is for LVDS input
> +
> +      port@1:
> +        type: object
> +        description: |
> +          With LVDS encoders port 1 is for LVDS output
> +          With LVDS decoders port 1 is for parallel output

port@* are required, right?

> +
> +required:
> +  - compatible
> +  - ports
> +
> +examples:
> +  - |
> +    lvds-encoder {
> +      compatible = "lvds-encoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_enc_in: endpoint {
> +            remote-endpoint = <&display_out_rgb>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_enc_out: endpoint {
> +            remote-endpoint = <&lvds_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
> +  - |
> +    lvds-decoder {
> +      compatible = "lvds-decoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_dec_in: endpoint {
> +            remote-endpoint = <&display_out_lvds>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_dec_out: endpoint {
> +            remote-endpoint = <&rgb_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

Hello Rob,

Thank you for your feedback!

> From: Rob Herring <robh+dt@kernel.org>
> Sent: 04 November 2019 21:23
> Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> 
> On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> >
> > In an effort to repurpose lvds-encoder.c to also serve the
> > function of LVDS decoders, we ended up defining a new "generic"
> > compatible string, therefore adapt the dt-bindings to fit the
> > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > while at it.
> 
> "Also, ... while at it." is a sign for split into 2 patches.

Will split into 2 patches

> 
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * Converted to dt-schema as per Neil's comment
> > ---
> >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> >  2 files changed, 117 insertions(+), 66 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > new file mode 100644
> > index 0000000..ff79bc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Trasnparent LVDS encoders and LVDS decoders
> 
> Typo

Will fix

> 
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > +
> > +description: |
> > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > +  require any configuration.
> > +
> > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > +  incompatible data link layers have been used over time to transmit image data
> > +  to LVDS panels. This binding targets devices compatible with the following
> > +  specifications only.
> > +
> > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > +  Semiconductor
> > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > +  Electronics Standards Association (VESA)
> > +
> > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > +  among others.
> > +
> > +properties:
> > +  compatible:
> > +    description: |
> > +      Any encoder or decoder compatible with this generic binding, but with
> > +      additional properties not listed here, must define its own binding and
> > +      list a device specific compatible first followed by the generic compatible
> > +    items:
> > +      - enum:
> 
> You can drop 'items' when there's only 1.

Will drop

> 
> > +        - lvds-encoder # for LVDS encoders
> > +        - lvds-decoder # for LVDS decoders
> > +
> > +  ports:
> > +    type: object
> > +    description: |
> > +      This device has two video ports. Their connections are modeled using the
> > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > +    properties:
> > +      port@0:
> > +        type: object
> > +        description: |
> > +          With LVDS encoders port 0 is for parallel input
> > +          With LVDS decoders port 0 is for LVDS input
> > +
> > +      port@1:
> > +        type: object
> > +        description: |
> > +          With LVDS encoders port 1 is for LVDS output
> > +          With LVDS decoders port 1 is for parallel output
> 
> port@* are required, right?

Yes, port@0 and port@1 are both required, similarly to:
Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
therefore I have put "ports" under "required", in a similar fashion.
What's the right way of specifying this?

Thanks,
Fab

> 
> > +
> > +required:
> > +  - compatible
> > +  - ports
> > +
> > +examples:
> > +  - |
> > +    lvds-encoder {
> > +      compatible = "lvds-encoder";
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +
> > +          lvds_enc_in: endpoint {
> > +            remote-endpoint = <&display_out_rgb>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +
> > +          lvds_enc_out: endpoint {
> > +            remote-endpoint = <&lvds_panel_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +  - |
> > +    lvds-decoder {
> > +      compatible = "lvds-decoder";
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +
> > +          lvds_dec_in: endpoint {
> > +            remote-endpoint = <&display_out_lvds>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +
> > +          lvds_dec_out: endpoint {
> > +            remote-endpoint = <&rgb_panel_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +...

RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

Hello Rob,

Thank you for your feedback!

> From: Rob Herring <robh+dt@kernel.org>
> Sent: 04 November 2019 21:23
> Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> 
> On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> >
> > In an effort to repurpose lvds-encoder.c to also serve the
> > function of LVDS decoders, we ended up defining a new "generic"
> > compatible string, therefore adapt the dt-bindings to fit the
> > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > while at it.
> 
> "Also, ... while at it." is a sign for split into 2 patches.

Will split into 2 patches

> 
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * Converted to dt-schema as per Neil's comment
> > ---
> >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> >  2 files changed, 117 insertions(+), 66 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > new file mode 100644
> > index 0000000..ff79bc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Trasnparent LVDS encoders and LVDS decoders
> 
> Typo

Will fix

> 
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > +
> > +description: |
> > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > +  require any configuration.
> > +
> > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > +  incompatible data link layers have been used over time to transmit image data
> > +  to LVDS panels. This binding targets devices compatible with the following
> > +  specifications only.
> > +
> > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > +  Semiconductor
> > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > +  Electronics Standards Association (VESA)
> > +
> > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > +  among others.
> > +
> > +properties:
> > +  compatible:
> > +    description: |
> > +      Any encoder or decoder compatible with this generic binding, but with
> > +      additional properties not listed here, must define its own binding and
> > +      list a device specific compatible first followed by the generic compatible
> > +    items:
> > +      - enum:
> 
> You can drop 'items' when there's only 1.

Will drop

> 
> > +        - lvds-encoder # for LVDS encoders
> > +        - lvds-decoder # for LVDS decoders
> > +
> > +  ports:
> > +    type: object
> > +    description: |
> > +      This device has two video ports. Their connections are modeled using the
> > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > +    properties:
> > +      port@0:
> > +        type: object
> > +        description: |
> > +          With LVDS encoders port 0 is for parallel input
> > +          With LVDS decoders port 0 is for LVDS input
> > +
> > +      port@1:
> > +        type: object
> > +        description: |
> > +          With LVDS encoders port 1 is for LVDS output
> > +          With LVDS decoders port 1 is for parallel output
> 
> port@* are required, right?

Yes, port@0 and port@1 are both required, similarly to:
Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
therefore I have put "ports" under "required", in a similar fashion.
What's the right way of specifying this?

Thanks,
Fab

> 
> > +
> > +required:
> > +  - compatible
> > +  - ports
> > +
> > +examples:
> > +  - |
> > +    lvds-encoder {
> > +      compatible = "lvds-encoder";
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +
> > +          lvds_enc_in: endpoint {
> > +            remote-endpoint = <&display_out_rgb>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +
> > +          lvds_enc_out: endpoint {
> > +            remote-endpoint = <&lvds_panel_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +  - |
> > +    lvds-decoder {
> > +      compatible = "lvds-decoder";
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +
> > +          lvds_dec_in: endpoint {
> > +            remote-endpoint = <&display_out_lvds>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +
> > +          lvds_dec_out: endpoint {
> > +            remote-endpoint = <&rgb_panel_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +...
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Rob Herring]

On Tue, Nov 5, 2019 at 3:33 AM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>
> Hello Rob,
>
> Thank you for your feedback!
>
> > From: Rob Herring <robh+dt@kernel.org>
> > Sent: 04 November 2019 21:23
> > Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> >
> > On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> > <fabrizio.castro@bp.renesas.com> wrote:
> > >
> > > In an effort to repurpose lvds-encoder.c to also serve the
> > > function of LVDS decoders, we ended up defining a new "generic"
> > > compatible string, therefore adapt the dt-bindings to fit the
> > > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > > while at it.
> >
> > "Also, ... while at it." is a sign for split into 2 patches.
>
> Will split into 2 patches
>
> >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * Converted to dt-schema as per Neil's comment
> > > ---
> > >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> > >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> > >  2 files changed, 117 insertions(+), 66 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > new file mode 100644
> > > index 0000000..ff79bc2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > @@ -0,0 +1,117 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Trasnparent LVDS encoders and LVDS decoders
> >
> > Typo
>
> Will fix
>
> >
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > +
> > > +description: |
> > > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > > +  require any configuration.
> > > +
> > > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > > +  incompatible data link layers have been used over time to transmit image data
> > > +  to LVDS panels. This binding targets devices compatible with the following
> > > +  specifications only.
> > > +
> > > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > > +  Semiconductor
> > > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > > +  Electronics Standards Association (VESA)
> > > +
> > > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > > +  among others.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    description: |
> > > +      Any encoder or decoder compatible with this generic binding, but with
> > > +      additional properties not listed here, must define its own binding and
> > > +      list a device specific compatible first followed by the generic compatible
> > > +    items:
> > > +      - enum:
> >
> > You can drop 'items' when there's only 1.
>
> Will drop
>
> >
> > > +        - lvds-encoder # for LVDS encoders
> > > +        - lvds-decoder # for LVDS decoders
> > > +
> > > +  ports:
> > > +    type: object
> > > +    description: |
> > > +      This device has two video ports. Their connections are modeled using the
> > > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > > +    properties:
> > > +      port@0:
> > > +        type: object
> > > +        description: |
> > > +          With LVDS encoders port 0 is for parallel input
> > > +          With LVDS decoders port 0 is for LVDS input
> > > +
> > > +      port@1:
> > > +        type: object
> > > +        description: |
> > > +          With LVDS encoders port 1 is for LVDS output
> > > +          With LVDS decoders port 1 is for parallel output
> >
> > port@* are required, right?
>
> Yes, port@0 and port@1 are both required, similarly to:
> Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> therefore I have put "ports" under "required", in a similar fashion.
> What's the right way of specifying this?

Same as any other property:

required:
  - port@0
  - port@1

At the correct level of course.

Rob

Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Rob Herring]

On Tue, Nov 5, 2019 at 3:33 AM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>
> Hello Rob,
>
> Thank you for your feedback!
>
> > From: Rob Herring <robh+dt@kernel.org>
> > Sent: 04 November 2019 21:23
> > Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> >
> > On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> > <fabrizio.castro@bp.renesas.com> wrote:
> > >
> > > In an effort to repurpose lvds-encoder.c to also serve the
> > > function of LVDS decoders, we ended up defining a new "generic"
> > > compatible string, therefore adapt the dt-bindings to fit the
> > > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > > while at it.
> >
> > "Also, ... while at it." is a sign for split into 2 patches.
>
> Will split into 2 patches
>
> >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * Converted to dt-schema as per Neil's comment
> > > ---
> > >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> > >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> > >  2 files changed, 117 insertions(+), 66 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > new file mode 100644
> > > index 0000000..ff79bc2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > @@ -0,0 +1,117 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Trasnparent LVDS encoders and LVDS decoders
> >
> > Typo
>
> Will fix
>
> >
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > +
> > > +description: |
> > > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > > +  require any configuration.
> > > +
> > > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > > +  incompatible data link layers have been used over time to transmit image data
> > > +  to LVDS panels. This binding targets devices compatible with the following
> > > +  specifications only.
> > > +
> > > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > > +  Semiconductor
> > > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > > +  Electronics Standards Association (VESA)
> > > +
> > > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > > +  among others.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    description: |
> > > +      Any encoder or decoder compatible with this generic binding, but with
> > > +      additional properties not listed here, must define its own binding and
> > > +      list a device specific compatible first followed by the generic compatible
> > > +    items:
> > > +      - enum:
> >
> > You can drop 'items' when there's only 1.
>
> Will drop
>
> >
> > > +        - lvds-encoder # for LVDS encoders
> > > +        - lvds-decoder # for LVDS decoders
> > > +
> > > +  ports:
> > > +    type: object
> > > +    description: |
> > > +      This device has two video ports. Their connections are modeled using the
> > > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > > +    properties:
> > > +      port@0:
> > > +        type: object
> > > +        description: |
> > > +          With LVDS encoders port 0 is for parallel input
> > > +          With LVDS decoders port 0 is for LVDS input
> > > +
> > > +      port@1:
> > > +        type: object
> > > +        description: |
> > > +          With LVDS encoders port 1 is for LVDS output
> > > +          With LVDS decoders port 1 is for parallel output
> >
> > port@* are required, right?
>
> Yes, port@0 and port@1 are both required, similarly to:
> Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> therefore I have put "ports" under "required", in a similar fashion.
> What's the right way of specifying this?

Same as any other property:

required:
  - port@0
  - port@1

At the correct level of course.

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

RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

Hello Rob,

Thank you for your feedback!

> From: Rob Herring <robh+dt@kernel.org>
> Sent: 05 November 2019 17:08
> Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> 
> On Tue, Nov 5, 2019 at 3:33 AM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> >
> > Hello Rob,
> >
> > Thank you for your feedback!
> >
> > > From: Rob Herring <robh+dt@kernel.org>
> > > Sent: 04 November 2019 21:23
> > > Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> > >
> > > On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> > > <fabrizio.castro@bp.renesas.com> wrote:
> > > >
> > > > In an effort to repurpose lvds-encoder.c to also serve the
> > > > function of LVDS decoders, we ended up defining a new "generic"
> > > > compatible string, therefore adapt the dt-bindings to fit the
> > > > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > > > while at it.
> > >
> > > "Also, ... while at it." is a sign for split into 2 patches.
> >
> > Will split into 2 patches
> >
> > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v1->v2:
> > > > * Converted to dt-schema as per Neil's comment
> > > > ---
> > > >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> > > >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> > > >  2 files changed, 117 insertions(+), 66 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > > new file mode 100644
> > > > index 0000000..ff79bc2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > > @@ -0,0 +1,117 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Trasnparent LVDS encoders and LVDS decoders
> > >
> > > Typo
> >
> > Will fix
> >
> > >
> > > > +
> > > > +maintainers:
> > > > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > +
> > > > +description: |
> > > > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > > > +  require any configuration.
> > > > +
> > > > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > > > +  incompatible data link layers have been used over time to transmit image data
> > > > +  to LVDS panels. This binding targets devices compatible with the following
> > > > +  specifications only.
> > > > +
> > > > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > > > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > > > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > > > +  Semiconductor
> > > > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > > > +  Electronics Standards Association (VESA)
> > > > +
> > > > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > > > +  among others.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    description: |
> > > > +      Any encoder or decoder compatible with this generic binding, but with
> > > > +      additional properties not listed here, must define its own binding and
> > > > +      list a device specific compatible first followed by the generic compatible
> > > > +    items:
> > > > +      - enum:
> > >
> > > You can drop 'items' when there's only 1.
> >
> > Will drop
> >
> > >
> > > > +        - lvds-encoder # for LVDS encoders
> > > > +        - lvds-decoder # for LVDS decoders
> > > > +
> > > > +  ports:
> > > > +    type: object
> > > > +    description: |
> > > > +      This device has two video ports. Their connections are modeled using the
> > > > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > > > +    properties:
> > > > +      port@0:
> > > > +        type: object
> > > > +        description: |
> > > > +          With LVDS encoders port 0 is for parallel input
> > > > +          With LVDS decoders port 0 is for LVDS input
> > > > +
> > > > +      port@1:
> > > > +        type: object
> > > > +        description: |
> > > > +          With LVDS encoders port 1 is for LVDS output
> > > > +          With LVDS decoders port 1 is for parallel output
> > >
> > > port@* are required, right?
> >
> > Yes, port@0 and port@1 are both required, similarly to:
> > Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> > therefore I have put "ports" under "required", in a similar fashion.
> > What's the right way of specifying this?
> 
> Same as any other property:
> 
> required:
>   - port@0
>   - port@1
> 
> At the correct level of course.

Gotcha

Will send v3 shortly.

Thanks,
Fab

> 
> Rob

RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

Hello Rob,

Thank you for your feedback!

> From: Rob Herring <robh+dt@kernel.org>
> Sent: 05 November 2019 17:08
> Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> 
> On Tue, Nov 5, 2019 at 3:33 AM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> >
> > Hello Rob,
> >
> > Thank you for your feedback!
> >
> > > From: Rob Herring <robh+dt@kernel.org>
> > > Sent: 04 November 2019 21:23
> > > Subject: Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
> > >
> > > On Mon, Nov 4, 2019 at 10:58 AM Fabrizio Castro
> > > <fabrizio.castro@bp.renesas.com> wrote:
> > > >
> > > > In an effort to repurpose lvds-encoder.c to also serve the
> > > > function of LVDS decoders, we ended up defining a new "generic"
> > > > compatible string, therefore adapt the dt-bindings to fit the
> > > > new purpose. Also, convert the dt-bindings from .txt to .yaml
> > > > while at it.
> > >
> > > "Also, ... while at it." is a sign for split into 2 patches.
> >
> > Will split into 2 patches
> >
> > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v1->v2:
> > > > * Converted to dt-schema as per Neil's comment
> > > > ---
> > > >  .../bindings/display/bridge/lvds-codec.yaml        | 117 +++++++++++++++++++++
> > > >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 ------------
> > > >  2 files changed, 117 insertions(+), 66 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > > new file mode 100644
> > > > index 0000000..ff79bc2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > > @@ -0,0 +1,117 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Trasnparent LVDS encoders and LVDS decoders
> > >
> > > Typo
> >
> > Will fix
> >
> > >
> > > > +
> > > > +maintainers:
> > > > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > +
> > > > +description: |
> > > > +  This binding supports transparent LVDS encoders and LVDS decoders that don't
> > > > +  require any configuration.
> > > > +
> > > > +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> > > > +  incompatible data link layers have been used over time to transmit image data
> > > > +  to LVDS panels. This binding targets devices compatible with the following
> > > > +  specifications only.
> > > > +
> > > > +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> > > > +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> > > > +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> > > > +  Semiconductor
> > > > +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> > > > +  Electronics Standards Association (VESA)
> > > > +
> > > > +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> > > > +  among others.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    description: |
> > > > +      Any encoder or decoder compatible with this generic binding, but with
> > > > +      additional properties not listed here, must define its own binding and
> > > > +      list a device specific compatible first followed by the generic compatible
> > > > +    items:
> > > > +      - enum:
> > >
> > > You can drop 'items' when there's only 1.
> >
> > Will drop
> >
> > >
> > > > +        - lvds-encoder # for LVDS encoders
> > > > +        - lvds-decoder # for LVDS decoders
> > > > +
> > > > +  ports:
> > > > +    type: object
> > > > +    description: |
> > > > +      This device has two video ports. Their connections are modeled using the
> > > > +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> > > > +    properties:
> > > > +      port@0:
> > > > +        type: object
> > > > +        description: |
> > > > +          With LVDS encoders port 0 is for parallel input
> > > > +          With LVDS decoders port 0 is for LVDS input
> > > > +
> > > > +      port@1:
> > > > +        type: object
> > > > +        description: |
> > > > +          With LVDS encoders port 1 is for LVDS output
> > > > +          With LVDS decoders port 1 is for parallel output
> > >
> > > port@* are required, right?
> >
> > Yes, port@0 and port@1 are both required, similarly to:
> > Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> > therefore I have put "ports" under "required", in a similar fashion.
> > What's the right way of specifying this?
> 
> Same as any other property:
> 
> required:
>   - port@0
>   - port@1
> 
> At the correct level of course.

Gotcha

Will send v3 shortly.

Thanks,
Fab

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

[PATCH v2 3/4] ARM: dts: iwg20d-q7-common: Add LCD support

[Fabrizio Castro]

The iwg20d comes with a 7" capacitive touch screen, therefore
add support for it.

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

---
v1->v2:
* No change
---
 arch/arm/boot/dts/iwg20d-q7-common.dtsi  | 85 ++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi |  1 -
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/iwg20d-q7-common.dtsi b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
index ae75a1db..3428b8d 100644
--- a/arch/arm/boot/dts/iwg20d-q7-common.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
@@ -46,6 +46,49 @@
 		clock-frequency = <26000000>;
 	};
 
+	lcd_backlight: backlight {
+		compatible = "pwm-backlight";
+
+		pwms = <&pwm3 0 5000000 0>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <7>;
+		enable-gpios = <&gpio5 14 GPIO_ACTIVE_HIGH>;
+	};
+
+	lvds-receiver {
+		compatible = "lvds-decoder";
+		powerdown = <&gpio7 25 GPIO_ACTIVE_LOW>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				lvds_receiver_in: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				lvds_receiver_out: endpoint {
+					remote-endpoint = <&panel_in>;
+				};
+			};
+		};
+	};
+
+	panel {
+		compatible = "edt,etm0700g0dh6", "simple-panel";
+		backlight = <&lcd_backlight>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&lvds_receiver_out>;
+			};
+		};
+	};
+
 	reg_1p5v: 1p5v {
 		compatible = "regulator-fixed";
 		regulator-name = "1P5V";
@@ -120,6 +163,18 @@
 	status = "okay";
 };
 
+&du {
+	status = "okay";
+};
+
+&gpio2 {
+	touch-interrupt {
+		gpio-hog;
+		gpios = <12 GPIO_ACTIVE_LOW>;
+		input;
+	};
+};
+
 &hsusb {
 	status = "okay";
 	pinctrl-0 = <&usb0_pins>;
@@ -147,6 +202,25 @@
 		VDDIO-supply = <&reg_3p3v>;
 		VDDD-supply = <&reg_1p5v>;
 	};
+
+	touch: touchpanel@38 {
+		compatible = "edt,edt-ft5406";
+		reg = <0x38>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+	};
+};
+
+&lvds0 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			lvds0_out: endpoint {
+				remote-endpoint = <&lvds_receiver_in>;
+			};
+		};
+	};
 };
 
 &pci0 {
@@ -180,6 +254,11 @@
 		function = "i2c2";
 	};
 
+	pwm3_pins: pwm3 {
+		groups = "pwm3";
+		function = "pwm3";
+	};
+
 	scif0_pins: scif0 {
 		groups = "scif0_data_d";
 		function = "scif0";
@@ -218,6 +297,12 @@
 	};
 };
 
+&pwm3 {
+	pinctrl-0 = <&pwm3_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &rcar_sound {
 	pinctrl-0 = <&sound_pins>;
 	pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
index 0e99df2..ede2e0c 100644
--- a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
@@ -39,7 +39,6 @@
 &du {
 	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
-	status = "okay";
 
 	ports {
 		port@0 {
-- 
2.7.4

[PATCH v2 3/4] ARM: dts: iwg20d-q7-common: Add LCD support

[Fabrizio Castro]

The iwg20d comes with a 7" capacitive touch screen, therefore
add support for it.

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

---
v1->v2:
* No change
---
 arch/arm/boot/dts/iwg20d-q7-common.dtsi  | 85 ++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi |  1 -
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/iwg20d-q7-common.dtsi b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
index ae75a1db..3428b8d 100644
--- a/arch/arm/boot/dts/iwg20d-q7-common.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
@@ -46,6 +46,49 @@
 		clock-frequency = <26000000>;
 	};
 
+	lcd_backlight: backlight {
+		compatible = "pwm-backlight";
+
+		pwms = <&pwm3 0 5000000 0>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <7>;
+		enable-gpios = <&gpio5 14 GPIO_ACTIVE_HIGH>;
+	};
+
+	lvds-receiver {
+		compatible = "lvds-decoder";
+		powerdown = <&gpio7 25 GPIO_ACTIVE_LOW>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				lvds_receiver_in: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				lvds_receiver_out: endpoint {
+					remote-endpoint = <&panel_in>;
+				};
+			};
+		};
+	};
+
+	panel {
+		compatible = "edt,etm0700g0dh6", "simple-panel";
+		backlight = <&lcd_backlight>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&lvds_receiver_out>;
+			};
+		};
+	};
+
 	reg_1p5v: 1p5v {
 		compatible = "regulator-fixed";
 		regulator-name = "1P5V";
@@ -120,6 +163,18 @@
 	status = "okay";
 };
 
+&du {
+	status = "okay";
+};
+
+&gpio2 {
+	touch-interrupt {
+		gpio-hog;
+		gpios = <12 GPIO_ACTIVE_LOW>;
+		input;
+	};
+};
+
 &hsusb {
 	status = "okay";
 	pinctrl-0 = <&usb0_pins>;
@@ -147,6 +202,25 @@
 		VDDIO-supply = <&reg_3p3v>;
 		VDDD-supply = <&reg_1p5v>;
 	};
+
+	touch: touchpanel@38 {
+		compatible = "edt,edt-ft5406";
+		reg = <0x38>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+	};
+};
+
+&lvds0 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			lvds0_out: endpoint {
+				remote-endpoint = <&lvds_receiver_in>;
+			};
+		};
+	};
 };
 
 &pci0 {
@@ -180,6 +254,11 @@
 		function = "i2c2";
 	};
 
+	pwm3_pins: pwm3 {
+		groups = "pwm3";
+		function = "pwm3";
+	};
+
 	scif0_pins: scif0 {
 		groups = "scif0_data_d";
 		function = "scif0";
@@ -218,6 +297,12 @@
 	};
 };
 
+&pwm3 {
+	pinctrl-0 = <&pwm3_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &rcar_sound {
 	pinctrl-0 = <&sound_pins>;
 	pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
index 0e99df2..ede2e0c 100644
--- a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
@@ -39,7 +39,6 @@
 &du {
 	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
-	status = "okay";
 
 	ports {
 		port@0 {
-- 
2.7.4

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

[PATCH v2 4/4] ARM: shmobile_defconfig: Enable support for panels from EDT

[Fabrizio Castro]

The iwg20d comes with an LCD panel from Emerging Display
Technologies Corporation (EDT), therefore enable what's
required to support it.

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

---
v1->v2:
* No change
---
 arch/arm/configs/shmobile_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index c6c7035..ab416a5 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -66,6 +66,7 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_KEYBOARD_GPIO=y
 # CONFIG_INPUT_MOUSE is not set
 CONFIG_INPUT_TOUCHSCREEN=y
+CONFIG_TOUCHSCREEN_EDT_FT5X06=y
 CONFIG_TOUCHSCREEN_ST1232=y
 CONFIG_INPUT_MISC=y
 CONFIG_INPUT_ADXL34X=y
@@ -125,7 +126,9 @@ CONFIG_VIDEO_ADV7604=y
 CONFIG_VIDEO_ML86V7667=y
 CONFIG_DRM=y
 CONFIG_DRM_RCAR_DU=y
+CONFIG_DRM_PANEL_SIMPLE=y
 CONFIG_DRM_DUMB_VGA_DAC=y
+CONFIG_DRM_LVDS_CODEC=y
 CONFIG_DRM_SII902X=y
 CONFIG_DRM_I2C_ADV7511=y
 CONFIG_DRM_I2C_ADV7511_AUDIO=y
-- 
2.7.4

[PATCH v2 4/4] ARM: shmobile_defconfig: Enable support for panels from EDT

[Fabrizio Castro]

The iwg20d comes with an LCD panel from Emerging Display
Technologies Corporation (EDT), therefore enable what's
required to support it.

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

---
v1->v2:
* No change
---
 arch/arm/configs/shmobile_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index c6c7035..ab416a5 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -66,6 +66,7 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_KEYBOARD_GPIO=y
 # CONFIG_INPUT_MOUSE is not set
 CONFIG_INPUT_TOUCHSCREEN=y
+CONFIG_TOUCHSCREEN_EDT_FT5X06=y
 CONFIG_TOUCHSCREEN_ST1232=y
 CONFIG_INPUT_MISC=y
 CONFIG_INPUT_ADXL34X=y
@@ -125,7 +126,9 @@ CONFIG_VIDEO_ADV7604=y
 CONFIG_VIDEO_ML86V7667=y
 CONFIG_DRM=y
 CONFIG_DRM_RCAR_DU=y
+CONFIG_DRM_PANEL_SIMPLE=y
 CONFIG_DRM_DUMB_VGA_DAC=y
+CONFIG_DRM_LVDS_CODEC=y
 CONFIG_DRM_SII902X=y
 CONFIG_DRM_I2C_ADV7511=y
 CONFIG_DRM_I2C_ADV7511_AUDIO=y
-- 
2.7.4

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