* [PATCH v2 0/4] Add LCD panel support to iwg20d
@ 2019-11-04 16:57 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:57 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/4] Add LCD panel support to iwg20d
@ 2019-11-04 16:57 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:57 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, devicetree, Chris Paterson, Geert Uytterhoeven,
Kieran Bingham, Magnus Damm, dri-devel, Biju Das,
linux-renesas-soc, Simon Horman, Jacopo Mondi, Laurent Pinchart,
Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
2019-11-04 16:57 ` Fabrizio Castro
@ 2019-11-04 16:58 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
@ 2019-11-04 16:58 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, devicetree, Chris Paterson, Geert Uytterhoeven,
Kieran Bingham, Magnus Damm, dri-devel, Biju Das,
linux-renesas-soc, Simon Horman, Jacopo Mondi, Laurent Pinchart,
Peter Rosin
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-04 16:57 ` Fabrizio Castro
@ 2019-11-04 16:58 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
@ 2019-11-04 16:58 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, devicetree, Chris Paterson, Geert Uytterhoeven,
Kieran Bingham, Magnus Damm, dri-devel, Biju Das,
linux-renesas-soc, Simon Horman, Jacopo Mondi, Laurent Pinchart,
Peter Rosin
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 3/4] ARM: dts: iwg20d-q7-common: Add LCD support
2019-11-04 16:57 ` Fabrizio Castro
@ 2019-11-04 16:58 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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 = <®_3p3v>;
VDDD-supply = <®_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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 3/4] ARM: dts: iwg20d-q7-common: Add LCD support
@ 2019-11-04 16:58 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, devicetree, Chris Paterson, Geert Uytterhoeven,
Kieran Bingham, Magnus Damm, dri-devel, Biju Das,
linux-renesas-soc, Simon Horman, Jacopo Mondi, Laurent Pinchart,
Peter Rosin
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 = <®_3p3v>;
VDDD-supply = <®_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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 4/4] ARM: shmobile_defconfig: Enable support for panels from EDT
2019-11-04 16:57 ` Fabrizio Castro
@ 2019-11-04 16:58 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 4/4] ARM: shmobile_defconfig: Enable support for panels from EDT
@ 2019-11-04 16:58 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-04 16:58 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, devicetree, Chris Paterson, Geert Uytterhoeven,
Kieran Bingham, Magnus Damm, dri-devel, Biju Das,
linux-renesas-soc, Simon Horman, Jacopo Mondi, Laurent Pinchart,
Peter Rosin
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-04 16:58 ` Fabrizio Castro
@ 2019-11-04 21:22 ` Rob Herring
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-11-04 21:22 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Mark Rutland,
Andrzej Hajda, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Chris Paterson,
Biju Das, Kieran Bingham, Jacopo Mondi
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>;
> + };
> + };
> + };
> + };
> +
> +...
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
@ 2019-11-04 21:22 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-11-04 21:22 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Neil Armstrong, David Airlie, Kieran Bingham, Magnus Damm,
dri-devel, Biju Das, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Simon Horman, Jacopo Mondi, Laurent Pinchart, Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-04 21:22 ` Rob Herring
@ 2019-11-05 9:33 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-05 9:33 UTC (permalink / raw)
To: Rob Herring
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Mark Rutland,
Andrzej Hajda, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Chris Paterson,
Biju Das, Kieran Bingham, Jacopo Mondi
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>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
@ 2019-11-05 9:33 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-05 9:33 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Neil Armstrong, David Airlie, Kieran Bingham, Magnus Damm,
dri-devel, Biju Das, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Simon Horman, Jacopo Mondi, Laurent Pinchart, Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-05 9:33 ` Fabrizio Castro
@ 2019-11-05 17:07 ` Rob Herring
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-11-05 17:07 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Mark Rutland,
Andrzej Hajda, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Chris Paterson,
Biju Das, Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
@ 2019-11-05 17:07 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-11-05 17:07 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Neil Armstrong, David Airlie, Kieran Bingham, Magnus Damm,
dri-devel, Biju Das, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Simon Horman, Jacopo Mondi, Laurent Pinchart, Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
2019-11-04 16:58 ` Fabrizio Castro
@ 2019-11-07 18:19 ` Jacopo Mondi
-1 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2019-11-07 18:19 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Laurent Pinchart, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
@ 2019-11-07 18:19 ` Jacopo Mondi
0 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2019-11-07 18:19 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Simon Horman, Neil Armstrong, David Airlie, Kieran Bingham,
Magnus Damm, dri-devel, Biju Das, linux-renesas-soc, Rob Herring,
Jacopo Mondi, Laurent Pinchart, Peter Rosin
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-05 17:07 ` Rob Herring
@ 2019-11-07 19:42 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-07 19:42 UTC (permalink / raw)
To: Rob Herring
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Mark Rutland,
Andrzej Hajda, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Chris Paterson,
Biju Das, Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder
@ 2019-11-07 19:42 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-07 19:42 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Neil Armstrong, David Airlie, Kieran Bingham, Magnus Damm,
dri-devel, Biju Das, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Simon Horman, Jacopo Mondi, Laurent Pinchart, Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
2019-11-07 18:19 ` Jacopo Mondi
@ 2019-11-07 20:02 ` Fabrizio Castro
-1 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:02 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Laurent Pinchart, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, Jacopo Mondi
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
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
@ 2019-11-07 20:02 ` Fabrizio Castro
0 siblings, 0 replies; 24+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:02 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Simon Horman, Neil Armstrong, David Airlie, Kieran Bingham,
Magnus Damm, dri-devel, Biju Das, linux-renesas-soc, Rob Herring,
Jacopo Mondi, Laurent Pinchart, Peter Rosin
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
2019-11-07 20:02 ` Fabrizio Castro
@ 2019-11-07 22:21 ` Jacopo Mondi
-1 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2019-11-07 22:21 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Laurent Pinchart, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c
@ 2019-11-07 22:21 ` Jacopo Mondi
0 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2019-11-07 22:21 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Mark Rutland, devicetree, Chris Paterson, Geert Uytterhoeven,
Simon Horman, Neil Armstrong, David Airlie, Kieran Bingham,
Magnus Damm, dri-devel, Biju Das, linux-renesas-soc, Rob Herring,
Jacopo Mondi, Laurent Pinchart, Peter Rosin
[-- 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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-11-07 22:19 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 16:57 [PATCH v2 0/4] Add LCD panel support to iwg20d Fabrizio Castro
2019-11-04 16:57 ` Fabrizio Castro
2019-11-04 16:58 ` [PATCH v2 1/4] drm/bridge: Repurpose lvds-encoder.c Fabrizio Castro
2019-11-04 16:58 ` Fabrizio Castro
2019-11-07 18:19 ` Jacopo Mondi
2019-11-07 18:19 ` Jacopo Mondi
2019-11-07 20:02 ` Fabrizio Castro
2019-11-07 20:02 ` Fabrizio Castro
2019-11-07 22:21 ` Jacopo Mondi
2019-11-07 22:21 ` Jacopo Mondi
2019-11-04 16:58 ` [PATCH v2 2/4] dt-bindings: display: bridge: Repurpose lvds-encoder Fabrizio Castro
2019-11-04 16:58 ` Fabrizio Castro
2019-11-04 21:22 ` Rob Herring
2019-11-04 21:22 ` Rob Herring
2019-11-05 9:33 ` Fabrizio Castro
2019-11-05 9:33 ` Fabrizio Castro
2019-11-05 17:07 ` Rob Herring
2019-11-05 17:07 ` Rob Herring
2019-11-07 19:42 ` Fabrizio Castro
2019-11-07 19:42 ` Fabrizio Castro
2019-11-04 16:58 ` [PATCH v2 3/4] ARM: dts: iwg20d-q7-common: Add LCD support Fabrizio Castro
2019-11-04 16:58 ` Fabrizio Castro
2019-11-04 16:58 ` [PATCH v2 4/4] ARM: shmobile_defconfig: Enable support for panels from EDT Fabrizio Castro
2019-11-04 16:58 ` Fabrizio Castro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.