[PATCH v2 0/2] Add generic-display-mux driver and bindings

[PATCH v2 0/2] Add generic-display-mux driver and bindings

[Pin-yen Lin]

This is the v2 the mux driver part of v1. This series is developed for
and tested on MT8173 board, whose layout looks like:

                                  /-- anx7688
-- MT8173 HDMI bridge -- GPIO mux
                                  \-- native HDMI

v1: https://patchwork.kernel.org/project/dri-devel/cover/20191211061911.238393-1-hsinyi@chromium.org/

The other drm bridge callbacks is dropped in this version because:
- The non-atomic callbacks are deprecated
- It would be complicated to pass the atomic state to the downstream
- We actually don't have the hardware to test them

Changes in v2:
- Referenced existing dt-binding schemas from graph.yaml
- Added ddc-i2c-bus into the bindings
- Dropped attach/mode_set/enable/disable callbacks
- Fixed style issues
- Removed the special case for the HDMI connector
- Made the driver only read the GPIO status in IRQ handler
- Rebased to drm-misc-next
- Updated the license: "GPL v2" --> "GPL"

Nicolas Boichat (2):
  dt-bindings: display: bridge: Add GPIO display mux binding
  drm: bridge: Generic GPIO mux driver

 .../bindings/display/bridge/gpio-mux.yaml     |  95 +++++++++
 drivers/gpu/drm/bridge/Kconfig                |  10 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/generic-gpio-mux.c     | 201 ++++++++++++++++++
 4 files changed, 307 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
 create mode 100644 drivers/gpu/drm/bridge/generic-gpio-mux.c

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 0/2] Add generic-display-mux driver and bindings

[Pin-yen Lin]

This is the v2 the mux driver part of v1. This series is developed for
and tested on MT8173 board, whose layout looks like:

                                  /-- anx7688
-- MT8173 HDMI bridge -- GPIO mux
                                  \-- native HDMI

v1: https://patchwork.kernel.org/project/dri-devel/cover/20191211061911.238393-1-hsinyi@chromium.org/

The other drm bridge callbacks is dropped in this version because:
- The non-atomic callbacks are deprecated
- It would be complicated to pass the atomic state to the downstream
- We actually don't have the hardware to test them

Changes in v2:
- Referenced existing dt-binding schemas from graph.yaml
- Added ddc-i2c-bus into the bindings
- Dropped attach/mode_set/enable/disable callbacks
- Fixed style issues
- Removed the special case for the HDMI connector
- Made the driver only read the GPIO status in IRQ handler
- Rebased to drm-misc-next
- Updated the license: "GPL v2" --> "GPL"

Nicolas Boichat (2):
  dt-bindings: display: bridge: Add GPIO display mux binding
  drm: bridge: Generic GPIO mux driver

 .../bindings/display/bridge/gpio-mux.yaml     |  95 +++++++++
 drivers/gpu/drm/bridge/Kconfig                |  10 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/generic-gpio-mux.c     | 201 ++++++++++++++++++
 4 files changed, 307 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
 create mode 100644 drivers/gpu/drm/bridge/generic-gpio-mux.c

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

From: Nicolas Boichat <drinkcat@chromium.org>

Add bindings for Generic GPIO mux driver.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

Changes in v2:
- Referenced existing dt-binding schemas from graph.yaml
- Added ddc-i2c-bus into the bindings

 .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
new file mode 100644
index 000000000000..da29ba078f05
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic display mux (1 input, 2 outputs)
+
+maintainers:
+  - Nicolas Boichat <drinkcat@chromium.org>
+
+description: |
+  This bindings describes a simple display (e.g. HDMI) mux, that has 1
+  input, and 2 outputs. The mux status is controlled by hardware, and
+  its status is read back using a GPIO.
+
+properties:
+  compatible:
+    const: gpio-display-mux
+
+  detect-gpios:
+    maxItems: 1
+    description: GPIO that indicates the active output
+
+  ddc-i2c-bus:
+    description: phandle link to the I2C controller used for DDC EDID probing
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: |
+          Video port for input.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: |
+          2 video ports for output.
+          The reg value in the endpoints matches the GPIO status: when
+          GPIO is asserted, endpoint with reg value <1> is selected.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - detect-gpios
+  - ports
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    hdmi_mux: hdmi_mux {
+      compatible = "gpio-display-mux";
+      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
+      pinctrl-names = "default";
+      pinctrl-0 = <&hdmi_mux_pins>;
+      ddc-i2c-bus = <&hdmiddc0>;
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 { /* input */
+          reg = <0>;
+
+          hdmi_mux_in: endpoint {
+            remote-endpoint = <&hdmi0_out>;
+          };
+        };
+
+        port@1 { /* output */
+          reg = <1>;
+
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          hdmi_mux_out_anx: endpoint@0 {
+            reg = <0>;
+            remote-endpoint = <&dp_bridge_in>;
+          };
+
+          hdmi_mux_out_hdmi: endpoint@1 {
+            reg = <1>;
+            remote-endpoint = <&hdmi_connector_in>;
+          };
+        };
+      };
+    };
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

From: Nicolas Boichat <drinkcat@chromium.org>

Add bindings for Generic GPIO mux driver.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

Changes in v2:
- Referenced existing dt-binding schemas from graph.yaml
- Added ddc-i2c-bus into the bindings

 .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
new file mode 100644
index 000000000000..da29ba078f05
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic display mux (1 input, 2 outputs)
+
+maintainers:
+  - Nicolas Boichat <drinkcat@chromium.org>
+
+description: |
+  This bindings describes a simple display (e.g. HDMI) mux, that has 1
+  input, and 2 outputs. The mux status is controlled by hardware, and
+  its status is read back using a GPIO.
+
+properties:
+  compatible:
+    const: gpio-display-mux
+
+  detect-gpios:
+    maxItems: 1
+    description: GPIO that indicates the active output
+
+  ddc-i2c-bus:
+    description: phandle link to the I2C controller used for DDC EDID probing
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: |
+          Video port for input.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: |
+          2 video ports for output.
+          The reg value in the endpoints matches the GPIO status: when
+          GPIO is asserted, endpoint with reg value <1> is selected.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - detect-gpios
+  - ports
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    hdmi_mux: hdmi_mux {
+      compatible = "gpio-display-mux";
+      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
+      pinctrl-names = "default";
+      pinctrl-0 = <&hdmi_mux_pins>;
+      ddc-i2c-bus = <&hdmiddc0>;
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 { /* input */
+          reg = <0>;
+
+          hdmi_mux_in: endpoint {
+            remote-endpoint = <&hdmi0_out>;
+          };
+        };
+
+        port@1 { /* output */
+          reg = <1>;
+
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          hdmi_mux_out_anx: endpoint@0 {
+            reg = <0>;
+            remote-endpoint = <&dp_bridge_in>;
+          };
+
+          hdmi_mux_out_hdmi: endpoint@1 {
+            reg = <1>;
+            remote-endpoint = <&hdmi_connector_in>;
+          };
+        };
+      };
+    };
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 2/2] drm: bridge: Generic GPIO mux driver

[Pin-yen Lin]

From: Nicolas Boichat <drinkcat@chromium.org>

This driver supports single input, 2 output display mux (e.g.
HDMI mux), that provide its status via a GPIO.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Signed-off-by: Pin-yen Lin <treapking@chromium.org>

---
Laurent in v1 pointed out that the driver doesn't support panels as a
downstream. IIUC this can be done by using drm_of_find_panel_or_bridge
callback, but we don't have the hardware for this use case for testing.

Changes in v2:
- Dropped attach/mode_set/enable/disable callbacks
- Fixed style issues
- Removed the special case for the HDMI connector
- Made the driver only read the GPIO status in IRQ handler
- Rebased to drm-misc-next
- Updated the license: "GPL v2" --> "GPL"

 drivers/gpu/drm/bridge/Kconfig            |  10 ++
 drivers/gpu/drm/bridge/Makefile           |   1 +
 drivers/gpu/drm/bridge/generic-gpio-mux.c | 201 ++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/generic-gpio-mux.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 57946d80b02d..28f2221bc900 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -84,6 +84,16 @@ config DRM_FSL_LDB
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
 
+config DRM_GENERIC_GPIO_MUX
+	tristate "Generic GPIO-controlled mux"
+	depends on OF
+	select DRM_KMS_HELPER
+	help
+	  This bridge driver models a GPIO-controlled display mux with one
+	  input, 2 outputs (e.g. an HDMI mux). The hardware decides which output
+	  is active, reports it as a GPIO, and the driver redirects calls to the
+	  appropriate downstream bridge (if any).
+
 config DRM_ITE_IT6505
         tristate "ITE IT6505 DisplayPort bridge"
         depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 1884803c6860..f5cfab100e8a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
 obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
 obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
 obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
+obj-$(CONFIG_DRM_GENERIC_GPIO_MUX) += generic-gpio-mux.o
 obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
 obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
 obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
diff --git a/drivers/gpu/drm/bridge/generic-gpio-mux.c b/drivers/gpu/drm/bridge/generic-gpio-mux.c
new file mode 100644
index 000000000000..9c26abab7778
--- /dev/null
+++ b/drivers/gpu/drm/bridge/generic-gpio-mux.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Generic gpio mux bridge driver
+ *
+ * Copyright 2016 Google LLC
+ */
+
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_probe_helper.h>
+
+struct gpio_display_mux {
+	struct device *dev;
+
+	struct gpio_desc *gpiod_detect;
+	int detect_irq;
+	int cur_next;
+
+	struct drm_bridge bridge;
+
+	struct drm_bridge *next[2];
+};
+
+static inline struct gpio_display_mux *bridge_to_gpio_display_mux(
+		struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct gpio_display_mux, bridge);
+}
+
+static irqreturn_t gpio_display_mux_det_threaded_handler(int unused, void *data)
+{
+	struct gpio_display_mux *mux = data;
+	int active = gpiod_get_value(mux->gpiod_detect);
+
+	if (active < 0) {
+		dev_err(mux->dev, "Failed to get detect GPIO\n");
+		return IRQ_HANDLED;
+	}
+
+	dev_dbg(mux->dev, "Interrupt %d!\n", active);
+	mux->cur_next = active;
+
+	if (mux->bridge.dev)
+		drm_kms_helper_hotplug_event(mux->bridge.dev);
+
+	return IRQ_HANDLED;
+}
+
+static bool gpio_display_mux_mode_fixup(struct drm_bridge *bridge,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct gpio_display_mux *mux = bridge_to_gpio_display_mux(bridge);
+	struct drm_bridge *next;
+
+	next = mux->next[mux->cur_next];
+
+	/* Assume that we have a most one bridge in both downstreams */
+	if (next && next->funcs->mode_fixup)
+		return next->funcs->mode_fixup(next, mode, adjusted_mode);
+
+	return true;
+}
+
+static const struct drm_bridge_funcs gpio_display_mux_bridge_funcs = {
+	.mode_fixup = gpio_display_mux_mode_fixup,
+};
+
+static int gpio_display_mux_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_display_mux *mux;
+	struct device_node *port, *ep, *remote;
+	int ret;
+	u32 reg;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mux);
+	mux->dev = &pdev->dev;
+
+	mux->bridge.of_node = dev->of_node;
+
+	mux->gpiod_detect = devm_gpiod_get(dev, "detect", GPIOD_IN);
+	if (IS_ERR(mux->gpiod_detect))
+		return PTR_ERR(mux->gpiod_detect);
+
+	mux->detect_irq = gpiod_to_irq(mux->gpiod_detect);
+	if (mux->detect_irq < 0) {
+		dev_err(dev, "Failed to get output irq %d\n",
+			mux->detect_irq);
+		return -ENODEV;
+	}
+
+	port = of_graph_get_port_by_id(dev->of_node, 1);
+	if (!port) {
+		dev_err(dev, "Missing output port node\n");
+		return -EINVAL;
+	}
+
+	for_each_child_of_node(port, ep) {
+		if (!ep->name || (of_node_cmp(ep->name, "endpoint") != 0)) {
+			of_node_put(ep);
+			continue;
+		}
+
+		if (of_property_read_u32(ep, "reg", &reg) < 0 ||
+		    reg >= ARRAY_SIZE(mux->next)) {
+			dev_err(dev,
+				"Missing/invalid reg property for endpoint %s\n",
+				ep->full_name);
+			of_node_put(ep);
+			of_node_put(port);
+			return -EINVAL;
+		}
+
+		remote = of_graph_get_remote_port_parent(ep);
+		if (!remote) {
+			dev_err(dev,
+				"Missing connector/bridge node for endpoint %s\n",
+				ep->full_name);
+			of_node_put(ep);
+			of_node_put(port);
+			return -EINVAL;
+		}
+
+		mux->next[reg] = of_drm_find_bridge(remote);
+		if (!mux->next[reg]) {
+			dev_err(dev, "Waiting for external bridge %s\n",
+				remote->name);
+			of_node_put(ep);
+			of_node_put(remote);
+			of_node_put(port);
+			return -EPROBE_DEFER;
+		}
+
+		of_node_put(remote);
+	}
+	of_node_put(port);
+
+	mux->bridge.funcs = &gpio_display_mux_bridge_funcs;
+	mux->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	drm_bridge_add(&mux->bridge);
+
+	ret = devm_request_threaded_irq(dev, mux->detect_irq, NULL,
+					gpio_display_mux_det_threaded_handler,
+					IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"gpio-display-mux-det", mux);
+	if (ret) {
+		dev_err(dev, "Failed to request MUX_DET threaded irq\n");
+		goto err_bridge_remove;
+	}
+
+	return 0;
+
+err_bridge_remove:
+	drm_bridge_remove(&mux->bridge);
+
+	return ret;
+}
+
+static int gpio_display_mux_remove(struct platform_device *pdev)
+{
+	struct gpio_display_mux *mux = platform_get_drvdata(pdev);
+
+	disable_irq(mux->detect_irq);
+	drm_bridge_remove(&mux->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id gpio_display_mux_match[] = {
+	{ .compatible = "gpio-display-mux", },
+	{},
+};
+
+struct platform_driver gpio_display_mux_driver = {
+	.probe = gpio_display_mux_probe,
+	.remove = gpio_display_mux_remove,
+	.driver = {
+		.name = "gpio-display-mux",
+		.of_match_table = gpio_display_mux_match,
+	},
+};
+
+module_platform_driver(gpio_display_mux_driver);
+
+MODULE_DESCRIPTION("GPIO-controlled display mux");
+MODULE_AUTHOR("Nicolas Boichat <drinkcat@chromium.org>");
+MODULE_LICENSE("GPL");
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v2 2/2] drm: bridge: Generic GPIO mux driver

[Pin-yen Lin]

From: Nicolas Boichat <drinkcat@chromium.org>

This driver supports single input, 2 output display mux (e.g.
HDMI mux), that provide its status via a GPIO.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Signed-off-by: Pin-yen Lin <treapking@chromium.org>

---
Laurent in v1 pointed out that the driver doesn't support panels as a
downstream. IIUC this can be done by using drm_of_find_panel_or_bridge
callback, but we don't have the hardware for this use case for testing.

Changes in v2:
- Dropped attach/mode_set/enable/disable callbacks
- Fixed style issues
- Removed the special case for the HDMI connector
- Made the driver only read the GPIO status in IRQ handler
- Rebased to drm-misc-next
- Updated the license: "GPL v2" --> "GPL"

 drivers/gpu/drm/bridge/Kconfig            |  10 ++
 drivers/gpu/drm/bridge/Makefile           |   1 +
 drivers/gpu/drm/bridge/generic-gpio-mux.c | 201 ++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/generic-gpio-mux.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 57946d80b02d..28f2221bc900 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -84,6 +84,16 @@ config DRM_FSL_LDB
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
 
+config DRM_GENERIC_GPIO_MUX
+	tristate "Generic GPIO-controlled mux"
+	depends on OF
+	select DRM_KMS_HELPER
+	help
+	  This bridge driver models a GPIO-controlled display mux with one
+	  input, 2 outputs (e.g. an HDMI mux). The hardware decides which output
+	  is active, reports it as a GPIO, and the driver redirects calls to the
+	  appropriate downstream bridge (if any).
+
 config DRM_ITE_IT6505
         tristate "ITE IT6505 DisplayPort bridge"
         depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 1884803c6860..f5cfab100e8a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
 obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
 obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
 obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
+obj-$(CONFIG_DRM_GENERIC_GPIO_MUX) += generic-gpio-mux.o
 obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
 obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
 obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
diff --git a/drivers/gpu/drm/bridge/generic-gpio-mux.c b/drivers/gpu/drm/bridge/generic-gpio-mux.c
new file mode 100644
index 000000000000..9c26abab7778
--- /dev/null
+++ b/drivers/gpu/drm/bridge/generic-gpio-mux.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Generic gpio mux bridge driver
+ *
+ * Copyright 2016 Google LLC
+ */
+
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_probe_helper.h>
+
+struct gpio_display_mux {
+	struct device *dev;
+
+	struct gpio_desc *gpiod_detect;
+	int detect_irq;
+	int cur_next;
+
+	struct drm_bridge bridge;
+
+	struct drm_bridge *next[2];
+};
+
+static inline struct gpio_display_mux *bridge_to_gpio_display_mux(
+		struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct gpio_display_mux, bridge);
+}
+
+static irqreturn_t gpio_display_mux_det_threaded_handler(int unused, void *data)
+{
+	struct gpio_display_mux *mux = data;
+	int active = gpiod_get_value(mux->gpiod_detect);
+
+	if (active < 0) {
+		dev_err(mux->dev, "Failed to get detect GPIO\n");
+		return IRQ_HANDLED;
+	}
+
+	dev_dbg(mux->dev, "Interrupt %d!\n", active);
+	mux->cur_next = active;
+
+	if (mux->bridge.dev)
+		drm_kms_helper_hotplug_event(mux->bridge.dev);
+
+	return IRQ_HANDLED;
+}
+
+static bool gpio_display_mux_mode_fixup(struct drm_bridge *bridge,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct gpio_display_mux *mux = bridge_to_gpio_display_mux(bridge);
+	struct drm_bridge *next;
+
+	next = mux->next[mux->cur_next];
+
+	/* Assume that we have a most one bridge in both downstreams */
+	if (next && next->funcs->mode_fixup)
+		return next->funcs->mode_fixup(next, mode, adjusted_mode);
+
+	return true;
+}
+
+static const struct drm_bridge_funcs gpio_display_mux_bridge_funcs = {
+	.mode_fixup = gpio_display_mux_mode_fixup,
+};
+
+static int gpio_display_mux_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_display_mux *mux;
+	struct device_node *port, *ep, *remote;
+	int ret;
+	u32 reg;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mux);
+	mux->dev = &pdev->dev;
+
+	mux->bridge.of_node = dev->of_node;
+
+	mux->gpiod_detect = devm_gpiod_get(dev, "detect", GPIOD_IN);
+	if (IS_ERR(mux->gpiod_detect))
+		return PTR_ERR(mux->gpiod_detect);
+
+	mux->detect_irq = gpiod_to_irq(mux->gpiod_detect);
+	if (mux->detect_irq < 0) {
+		dev_err(dev, "Failed to get output irq %d\n",
+			mux->detect_irq);
+		return -ENODEV;
+	}
+
+	port = of_graph_get_port_by_id(dev->of_node, 1);
+	if (!port) {
+		dev_err(dev, "Missing output port node\n");
+		return -EINVAL;
+	}
+
+	for_each_child_of_node(port, ep) {
+		if (!ep->name || (of_node_cmp(ep->name, "endpoint") != 0)) {
+			of_node_put(ep);
+			continue;
+		}
+
+		if (of_property_read_u32(ep, "reg", &reg) < 0 ||
+		    reg >= ARRAY_SIZE(mux->next)) {
+			dev_err(dev,
+				"Missing/invalid reg property for endpoint %s\n",
+				ep->full_name);
+			of_node_put(ep);
+			of_node_put(port);
+			return -EINVAL;
+		}
+
+		remote = of_graph_get_remote_port_parent(ep);
+		if (!remote) {
+			dev_err(dev,
+				"Missing connector/bridge node for endpoint %s\n",
+				ep->full_name);
+			of_node_put(ep);
+			of_node_put(port);
+			return -EINVAL;
+		}
+
+		mux->next[reg] = of_drm_find_bridge(remote);
+		if (!mux->next[reg]) {
+			dev_err(dev, "Waiting for external bridge %s\n",
+				remote->name);
+			of_node_put(ep);
+			of_node_put(remote);
+			of_node_put(port);
+			return -EPROBE_DEFER;
+		}
+
+		of_node_put(remote);
+	}
+	of_node_put(port);
+
+	mux->bridge.funcs = &gpio_display_mux_bridge_funcs;
+	mux->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	drm_bridge_add(&mux->bridge);
+
+	ret = devm_request_threaded_irq(dev, mux->detect_irq, NULL,
+					gpio_display_mux_det_threaded_handler,
+					IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"gpio-display-mux-det", mux);
+	if (ret) {
+		dev_err(dev, "Failed to request MUX_DET threaded irq\n");
+		goto err_bridge_remove;
+	}
+
+	return 0;
+
+err_bridge_remove:
+	drm_bridge_remove(&mux->bridge);
+
+	return ret;
+}
+
+static int gpio_display_mux_remove(struct platform_device *pdev)
+{
+	struct gpio_display_mux *mux = platform_get_drvdata(pdev);
+
+	disable_irq(mux->detect_irq);
+	drm_bridge_remove(&mux->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id gpio_display_mux_match[] = {
+	{ .compatible = "gpio-display-mux", },
+	{},
+};
+
+struct platform_driver gpio_display_mux_driver = {
+	.probe = gpio_display_mux_probe,
+	.remove = gpio_display_mux_remove,
+	.driver = {
+		.name = "gpio-display-mux",
+		.of_match_table = gpio_display_mux_match,
+	},
+};
+
+module_platform_driver(gpio_display_mux_driver);
+
+MODULE_DESCRIPTION("GPIO-controlled display mux");
+MODULE_AUTHOR("Nicolas Boichat <drinkcat@chromium.org>");
+MODULE_LICENSE("GPL");
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Rob Herring]

On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> From: Nicolas Boichat <drinkcat@chromium.org>
> 
> Add bindings for Generic GPIO mux driver.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
> 
> Changes in v2:
> - Referenced existing dt-binding schemas from graph.yaml
> - Added ddc-i2c-bus into the bindings
> 
>  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> new file mode 100644
> index 000000000000..da29ba078f05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic display mux (1 input, 2 outputs)
> +
> +maintainers:
> +  - Nicolas Boichat <drinkcat@chromium.org>
> +
> +description: |
> +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> +  input, and 2 outputs. The mux status is controlled by hardware, and
> +  its status is read back using a GPIO.
> +
> +properties:
> +  compatible:
> +    const: gpio-display-mux
> +
> +  detect-gpios:
> +    maxItems: 1
> +    description: GPIO that indicates the active output

What are we detecting? That implies an input, but this is selecting the 
output path, right? Or what does 'mux status is controlled by hardware' 
mean exactly? Something else? That does not sound very generic.

In any case, we have a common mux binding so any kind of mux control 
could be used here, not just GPIO. Then you can make this just a generic 
display mux.

> +
> +  ddc-i2c-bus:
> +    description: phandle link to the I2C controller used for DDC EDID probing
> +    $ref: /schemas/types.yaml#/definitions/phandle

This belongs in the connector node(s). 

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: |
> +          Video port for input.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: |
> +          2 video ports for output.
> +          The reg value in the endpoints matches the GPIO status: when
> +          GPIO is asserted, endpoint with reg value <1> is selected.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - detect-gpios
> +  - ports
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    hdmi_mux: hdmi_mux {
> +      compatible = "gpio-display-mux";
> +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&hdmi_mux_pins>;
> +      ddc-i2c-bus = <&hdmiddc0>;
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 { /* input */
> +          reg = <0>;
> +
> +          hdmi_mux_in: endpoint {
> +            remote-endpoint = <&hdmi0_out>;
> +          };
> +        };
> +
> +        port@1 { /* output */
> +          reg = <1>;
> +
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          hdmi_mux_out_anx: endpoint@0 {
> +            reg = <0>;
> +            remote-endpoint = <&dp_bridge_in>;
> +          };
> +
> +          hdmi_mux_out_hdmi: endpoint@1 {
> +            reg = <1>;
> +            remote-endpoint = <&hdmi_connector_in>;
> +          };
> +        };
> +      };
> +    };
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Rob Herring]

On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> From: Nicolas Boichat <drinkcat@chromium.org>
> 
> Add bindings for Generic GPIO mux driver.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
> 
> Changes in v2:
> - Referenced existing dt-binding schemas from graph.yaml
> - Added ddc-i2c-bus into the bindings
> 
>  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> new file mode 100644
> index 000000000000..da29ba078f05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic display mux (1 input, 2 outputs)
> +
> +maintainers:
> +  - Nicolas Boichat <drinkcat@chromium.org>
> +
> +description: |
> +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> +  input, and 2 outputs. The mux status is controlled by hardware, and
> +  its status is read back using a GPIO.
> +
> +properties:
> +  compatible:
> +    const: gpio-display-mux
> +
> +  detect-gpios:
> +    maxItems: 1
> +    description: GPIO that indicates the active output

What are we detecting? That implies an input, but this is selecting the 
output path, right? Or what does 'mux status is controlled by hardware' 
mean exactly? Something else? That does not sound very generic.

In any case, we have a common mux binding so any kind of mux control 
could be used here, not just GPIO. Then you can make this just a generic 
display mux.

> +
> +  ddc-i2c-bus:
> +    description: phandle link to the I2C controller used for DDC EDID probing
> +    $ref: /schemas/types.yaml#/definitions/phandle

This belongs in the connector node(s). 

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: |
> +          Video port for input.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: |
> +          2 video ports for output.
> +          The reg value in the endpoints matches the GPIO status: when
> +          GPIO is asserted, endpoint with reg value <1> is selected.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - detect-gpios
> +  - ports
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    hdmi_mux: hdmi_mux {
> +      compatible = "gpio-display-mux";
> +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&hdmi_mux_pins>;
> +      ddc-i2c-bus = <&hdmiddc0>;
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 { /* input */
> +          reg = <0>;
> +
> +          hdmi_mux_in: endpoint {
> +            remote-endpoint = <&hdmi0_out>;
> +          };
> +        };
> +
> +        port@1 { /* output */
> +          reg = <1>;
> +
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          hdmi_mux_out_anx: endpoint@0 {
> +            reg = <0>;
> +            remote-endpoint = <&dp_bridge_in>;
> +          };
> +
> +          hdmi_mux_out_hdmi: endpoint@1 {
> +            reg = <1>;
> +            remote-endpoint = <&hdmi_connector_in>;
> +          };
> +        };
> +      };
> +    };
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Rob,

Thanks for the review.


On Wed, Jan 18, 2023 at 4:17 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > From: Nicolas Boichat <drinkcat@chromium.org>
> >
> > Add bindings for Generic GPIO mux driver.
> >
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Referenced existing dt-binding schemas from graph.yaml
> > - Added ddc-i2c-bus into the bindings
> >
> >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > new file mode 100644
> > index 000000000000..da29ba078f05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic display mux (1 input, 2 outputs)
> > +
> > +maintainers:
> > +  - Nicolas Boichat <drinkcat@chromium.org>
> > +
> > +description: |
> > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > +  its status is read back using a GPIO.
> > +
> > +properties:
> > +  compatible:
> > +    const: gpio-display-mux
> > +
> > +  detect-gpios:
> > +    maxItems: 1
> > +    description: GPIO that indicates the active output
>
> What are we detecting? That implies an input, but this is selecting the
> output path, right? Or what does 'mux status is controlled by hardware'
> mean exactly? Something else? That does not sound very generic.

The GPIO (or any kind of MUX) is an input that indicates where the
output should go. The actual "output selection" procedure is done in
the driver. That is, the driver monitors this GPIO and selects the
output path accordingly. In our use case, the GPIO is reported by the
embedded controller on the device.

[1] listed other similar bridges that can leverage this driver, so we
called this driver "generic".

[1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/

>
> In any case, we have a common mux binding so any kind of mux control
> could be used here, not just GPIO. Then you can make this just a generic
> display mux.

Thanks for sharing this, I'll update the binding in the next version.
>
> > +
> > +  ddc-i2c-bus:
> > +    description: phandle link to the I2C controller used for DDC EDID probing
> > +    $ref: /schemas/types.yaml#/definitions/phandle
>
> This belongs in the connector node(s).

The HDMI bridge before the MUX doesn't (and doesn't have to) know that
its next bridge is a MUX. We put it here so that the HDMI bridge can
parse the phandle and get the bus node.

Regards,
Pin-yen

>
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          Video port for input.
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          2 video ports for output.
> > +          The reg value in the endpoints matches the GPIO status: when
> > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - detect-gpios
> > +  - ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    hdmi_mux: hdmi_mux {
> > +      compatible = "gpio-display-mux";
> > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&hdmi_mux_pins>;
> > +      ddc-i2c-bus = <&hdmiddc0>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 { /* input */
> > +          reg = <0>;
> > +
> > +          hdmi_mux_in: endpoint {
> > +            remote-endpoint = <&hdmi0_out>;
> > +          };
> > +        };
> > +
> > +        port@1 { /* output */
> > +          reg = <1>;
> > +
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          hdmi_mux_out_anx: endpoint@0 {
> > +            reg = <0>;
> > +            remote-endpoint = <&dp_bridge_in>;
> > +          };
> > +
> > +          hdmi_mux_out_hdmi: endpoint@1 {
> > +            reg = <1>;
> > +            remote-endpoint = <&hdmi_connector_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Rob,

Thanks for the review.


On Wed, Jan 18, 2023 at 4:17 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > From: Nicolas Boichat <drinkcat@chromium.org>
> >
> > Add bindings for Generic GPIO mux driver.
> >
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Referenced existing dt-binding schemas from graph.yaml
> > - Added ddc-i2c-bus into the bindings
> >
> >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > new file mode 100644
> > index 000000000000..da29ba078f05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic display mux (1 input, 2 outputs)
> > +
> > +maintainers:
> > +  - Nicolas Boichat <drinkcat@chromium.org>
> > +
> > +description: |
> > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > +  its status is read back using a GPIO.
> > +
> > +properties:
> > +  compatible:
> > +    const: gpio-display-mux
> > +
> > +  detect-gpios:
> > +    maxItems: 1
> > +    description: GPIO that indicates the active output
>
> What are we detecting? That implies an input, but this is selecting the
> output path, right? Or what does 'mux status is controlled by hardware'
> mean exactly? Something else? That does not sound very generic.

The GPIO (or any kind of MUX) is an input that indicates where the
output should go. The actual "output selection" procedure is done in
the driver. That is, the driver monitors this GPIO and selects the
output path accordingly. In our use case, the GPIO is reported by the
embedded controller on the device.

[1] listed other similar bridges that can leverage this driver, so we
called this driver "generic".

[1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/

>
> In any case, we have a common mux binding so any kind of mux control
> could be used here, not just GPIO. Then you can make this just a generic
> display mux.

Thanks for sharing this, I'll update the binding in the next version.
>
> > +
> > +  ddc-i2c-bus:
> > +    description: phandle link to the I2C controller used for DDC EDID probing
> > +    $ref: /schemas/types.yaml#/definitions/phandle
>
> This belongs in the connector node(s).

The HDMI bridge before the MUX doesn't (and doesn't have to) know that
its next bridge is a MUX. We put it here so that the HDMI bridge can
parse the phandle and get the bus node.

Regards,
Pin-yen

>
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          Video port for input.
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          2 video ports for output.
> > +          The reg value in the endpoints matches the GPIO status: when
> > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - detect-gpios
> > +  - ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    hdmi_mux: hdmi_mux {
> > +      compatible = "gpio-display-mux";
> > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&hdmi_mux_pins>;
> > +      ddc-i2c-bus = <&hdmiddc0>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 { /* input */
> > +          reg = <0>;
> > +
> > +          hdmi_mux_in: endpoint {
> > +            remote-endpoint = <&hdmi0_out>;
> > +          };
> > +        };
> > +
> > +        port@1 { /* output */
> > +          reg = <1>;
> > +
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          hdmi_mux_out_anx: endpoint@0 {
> > +            reg = <0>;
> > +            remote-endpoint = <&dp_bridge_in>;
> > +          };
> > +
> > +          hdmi_mux_out_hdmi: endpoint@1 {
> > +            reg = <1>;
> > +            remote-endpoint = <&hdmi_connector_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > From: Nicolas Boichat <drinkcat@chromium.org>
> > >
> > > Add bindings for Generic GPIO mux driver.
> > >
> > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Referenced existing dt-binding schemas from graph.yaml
> > > - Added ddc-i2c-bus into the bindings
> > >
> > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > new file mode 100644
> > > index 000000000000..da29ba078f05
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Generic display mux (1 input, 2 outputs)
> > > +
> > > +maintainers:
> > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > +
> > > +description: |
> > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > +  its status is read back using a GPIO.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: gpio-display-mux
> > > +
> > > +  detect-gpios:
> > > +    maxItems: 1
> > > +    description: GPIO that indicates the active output
> >
> > What are we detecting? That implies an input, but this is selecting the
> > output path, right? Or what does 'mux status is controlled by hardware'
> > mean exactly? Something else? That does not sound very generic.
> 
> The GPIO (or any kind of MUX) is an input that indicates where the
> output should go. The actual "output selection" procedure is done in
> the driver. That is, the driver monitors this GPIO and selects the
> output path accordingly. In our use case, the GPIO is reported by the
> embedded controller on the device.
> 
> [1] listed other similar bridges that can leverage this driver, so we
> called this driver "generic".
> 
> [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> 
> > In any case, we have a common mux binding so any kind of mux control
> > could be used here, not just GPIO. Then you can make this just a generic
> > display mux.
> 
> Thanks for sharing this, I'll update the binding in the next version.
>
> > > +
> > > +  ddc-i2c-bus:
> > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> >
> > This belongs in the connector node(s).
> 
> The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> its next bridge is a MUX. We put it here so that the HDMI bridge can
> parse the phandle and get the bus node.

How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
property of the next DT node in the OF graph ?

> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: |
> > > +          Video port for input.
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: |
> > > +          2 video ports for output.
> > > +          The reg value in the endpoints matches the GPIO status: when
> > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - detect-gpios
> > > +  - ports
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    hdmi_mux: hdmi_mux {
> > > +      compatible = "gpio-display-mux";
> > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > +      pinctrl-names = "default";
> > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > +
> > > +      ports {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        port@0 { /* input */
> > > +          reg = <0>;
> > > +
> > > +          hdmi_mux_in: endpoint {
> > > +            remote-endpoint = <&hdmi0_out>;
> > > +          };
> > > +        };
> > > +
> > > +        port@1 { /* output */
> > > +          reg = <1>;
> > > +
> > > +          #address-cells = <1>;
> > > +          #size-cells = <0>;
> > > +
> > > +          hdmi_mux_out_anx: endpoint@0 {
> > > +            reg = <0>;
> > > +            remote-endpoint = <&dp_bridge_in>;
> > > +          };
> > > +
> > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > +            reg = <1>;
> > > +            remote-endpoint = <&hdmi_connector_in>;
> > > +          };
> > > +        };
> > > +      };
> > > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > From: Nicolas Boichat <drinkcat@chromium.org>
> > >
> > > Add bindings for Generic GPIO mux driver.
> > >
> > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Referenced existing dt-binding schemas from graph.yaml
> > > - Added ddc-i2c-bus into the bindings
> > >
> > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > new file mode 100644
> > > index 000000000000..da29ba078f05
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Generic display mux (1 input, 2 outputs)
> > > +
> > > +maintainers:
> > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > +
> > > +description: |
> > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > +  its status is read back using a GPIO.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: gpio-display-mux
> > > +
> > > +  detect-gpios:
> > > +    maxItems: 1
> > > +    description: GPIO that indicates the active output
> >
> > What are we detecting? That implies an input, but this is selecting the
> > output path, right? Or what does 'mux status is controlled by hardware'
> > mean exactly? Something else? That does not sound very generic.
> 
> The GPIO (or any kind of MUX) is an input that indicates where the
> output should go. The actual "output selection" procedure is done in
> the driver. That is, the driver monitors this GPIO and selects the
> output path accordingly. In our use case, the GPIO is reported by the
> embedded controller on the device.
> 
> [1] listed other similar bridges that can leverage this driver, so we
> called this driver "generic".
> 
> [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> 
> > In any case, we have a common mux binding so any kind of mux control
> > could be used here, not just GPIO. Then you can make this just a generic
> > display mux.
> 
> Thanks for sharing this, I'll update the binding in the next version.
>
> > > +
> > > +  ddc-i2c-bus:
> > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> >
> > This belongs in the connector node(s).
> 
> The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> its next bridge is a MUX. We put it here so that the HDMI bridge can
> parse the phandle and get the bus node.

How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
property of the next DT node in the OF graph ?

> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: |
> > > +          Video port for input.
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: |
> > > +          2 video ports for output.
> > > +          The reg value in the endpoints matches the GPIO status: when
> > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - detect-gpios
> > > +  - ports
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    hdmi_mux: hdmi_mux {
> > > +      compatible = "gpio-display-mux";
> > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > +      pinctrl-names = "default";
> > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > +
> > > +      ports {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        port@0 { /* input */
> > > +          reg = <0>;
> > > +
> > > +          hdmi_mux_in: endpoint {
> > > +            remote-endpoint = <&hdmi0_out>;
> > > +          };
> > > +        };
> > > +
> > > +        port@1 { /* output */
> > > +          reg = <1>;
> > > +
> > > +          #address-cells = <1>;
> > > +          #size-cells = <0>;
> > > +
> > > +          hdmi_mux_out_anx: endpoint@0 {
> > > +            reg = <0>;
> > > +            remote-endpoint = <&dp_bridge_in>;
> > > +          };
> > > +
> > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > +            reg = <1>;
> > > +            remote-endpoint = <&hdmi_connector_in>;
> > > +          };
> > > +        };
> > > +      };
> > > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Laurent,

On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > >
> > > > Add bindings for Generic GPIO mux driver.
> > > >
> > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > - Added ddc-i2c-bus into the bindings
> > > >
> > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > >  1 file changed, 95 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > new file mode 100644
> > > > index 000000000000..da29ba078f05
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > @@ -0,0 +1,95 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Generic display mux (1 input, 2 outputs)
> > > > +
> > > > +maintainers:
> > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > +
> > > > +description: |
> > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > +  its status is read back using a GPIO.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: gpio-display-mux
> > > > +
> > > > +  detect-gpios:
> > > > +    maxItems: 1
> > > > +    description: GPIO that indicates the active output
> > >
> > > What are we detecting? That implies an input, but this is selecting the
> > > output path, right? Or what does 'mux status is controlled by hardware'
> > > mean exactly? Something else? That does not sound very generic.
> >
> > The GPIO (or any kind of MUX) is an input that indicates where the
> > output should go. The actual "output selection" procedure is done in
> > the driver. That is, the driver monitors this GPIO and selects the
> > output path accordingly. In our use case, the GPIO is reported by the
> > embedded controller on the device.
> >
> > [1] listed other similar bridges that can leverage this driver, so we
> > called this driver "generic".
> >
> > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> >
> > > In any case, we have a common mux binding so any kind of mux control
> > > could be used here, not just GPIO. Then you can make this just a generic
> > > display mux.
> >
> > Thanks for sharing this, I'll update the binding in the next version.
> >
> > > > +
> > > > +  ddc-i2c-bus:
> > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > >
> > > This belongs in the connector node(s).
> >
> > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > parse the phandle and get the bus node.
>
> How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> property of the next DT node in the OF graph ?

Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
similar approach as well.

[2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
[3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240

Regards,
Pin-yen
>
> > > > +
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: |
> > > > +          Video port for input.
> > > > +
> > > > +      port@1:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: |
> > > > +          2 video ports for output.
> > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > +
> > > > +    required:
> > > > +      - port@0
> > > > +      - port@1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - detect-gpios
> > > > +  - ports
> > > > +
> > > > +unevaluatedProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > +    hdmi_mux: hdmi_mux {
> > > > +      compatible = "gpio-display-mux";
> > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > +      pinctrl-names = "default";
> > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > +
> > > > +      ports {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        port@0 { /* input */
> > > > +          reg = <0>;
> > > > +
> > > > +          hdmi_mux_in: endpoint {
> > > > +            remote-endpoint = <&hdmi0_out>;
> > > > +          };
> > > > +        };
> > > > +
> > > > +        port@1 { /* output */
> > > > +          reg = <1>;
> > > > +
> > > > +          #address-cells = <1>;
> > > > +          #size-cells = <0>;
> > > > +
> > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > +            reg = <0>;
> > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > +          };
> > > > +
> > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > +            reg = <1>;
> > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > +          };
> > > > +        };
> > > > +      };
> > > > +    };
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Laurent,

On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > >
> > > > Add bindings for Generic GPIO mux driver.
> > > >
> > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > - Added ddc-i2c-bus into the bindings
> > > >
> > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > >  1 file changed, 95 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > new file mode 100644
> > > > index 000000000000..da29ba078f05
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > @@ -0,0 +1,95 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Generic display mux (1 input, 2 outputs)
> > > > +
> > > > +maintainers:
> > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > +
> > > > +description: |
> > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > +  its status is read back using a GPIO.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: gpio-display-mux
> > > > +
> > > > +  detect-gpios:
> > > > +    maxItems: 1
> > > > +    description: GPIO that indicates the active output
> > >
> > > What are we detecting? That implies an input, but this is selecting the
> > > output path, right? Or what does 'mux status is controlled by hardware'
> > > mean exactly? Something else? That does not sound very generic.
> >
> > The GPIO (or any kind of MUX) is an input that indicates where the
> > output should go. The actual "output selection" procedure is done in
> > the driver. That is, the driver monitors this GPIO and selects the
> > output path accordingly. In our use case, the GPIO is reported by the
> > embedded controller on the device.
> >
> > [1] listed other similar bridges that can leverage this driver, so we
> > called this driver "generic".
> >
> > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> >
> > > In any case, we have a common mux binding so any kind of mux control
> > > could be used here, not just GPIO. Then you can make this just a generic
> > > display mux.
> >
> > Thanks for sharing this, I'll update the binding in the next version.
> >
> > > > +
> > > > +  ddc-i2c-bus:
> > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > >
> > > This belongs in the connector node(s).
> >
> > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > parse the phandle and get the bus node.
>
> How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> property of the next DT node in the OF graph ?

Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
similar approach as well.

[2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
[3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240

Regards,
Pin-yen
>
> > > > +
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: |
> > > > +          Video port for input.
> > > > +
> > > > +      port@1:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: |
> > > > +          2 video ports for output.
> > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > +
> > > > +    required:
> > > > +      - port@0
> > > > +      - port@1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - detect-gpios
> > > > +  - ports
> > > > +
> > > > +unevaluatedProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > +    hdmi_mux: hdmi_mux {
> > > > +      compatible = "gpio-display-mux";
> > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > +      pinctrl-names = "default";
> > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > +
> > > > +      ports {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        port@0 { /* input */
> > > > +          reg = <0>;
> > > > +
> > > > +          hdmi_mux_in: endpoint {
> > > > +            remote-endpoint = <&hdmi0_out>;
> > > > +          };
> > > > +        };
> > > > +
> > > > +        port@1 { /* output */
> > > > +          reg = <1>;
> > > > +
> > > > +          #address-cells = <1>;
> > > > +          #size-cells = <0>;
> > > > +
> > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > +            reg = <0>;
> > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > +          };
> > > > +
> > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > +            reg = <1>;
> > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > +          };
> > > > +        };
> > > > +      };
> > > > +    };
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

Hello Pin-yen,

On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > >
> > > > > Add bindings for Generic GPIO mux driver.
> > > > >
> > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > - Added ddc-i2c-bus into the bindings
> > > > >
> > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > >  1 file changed, 95 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..da29ba078f05
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > @@ -0,0 +1,95 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > +
> > > > > +maintainers:
> > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > +
> > > > > +description: |
> > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > +  its status is read back using a GPIO.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: gpio-display-mux
> > > > > +
> > > > > +  detect-gpios:
> > > > > +    maxItems: 1
> > > > > +    description: GPIO that indicates the active output
> > > >
> > > > What are we detecting? That implies an input, but this is selecting the
> > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > mean exactly? Something else? That does not sound very generic.
> > >
> > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > output should go. The actual "output selection" procedure is done in
> > > the driver. That is, the driver monitors this GPIO and selects the
> > > output path accordingly. In our use case, the GPIO is reported by the
> > > embedded controller on the device.
> > >
> > > [1] listed other similar bridges that can leverage this driver, so we
> > > called this driver "generic".
> > >
> > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > >
> > > > In any case, we have a common mux binding so any kind of mux control
> > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > display mux.
> > >
> > > Thanks for sharing this, I'll update the binding in the next version.
> > >
> > > > > +
> > > > > +  ddc-i2c-bus:
> > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > >
> > > > This belongs in the connector node(s).
> > >
> > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > parse the phandle and get the bus node.
> >
> > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > property of the next DT node in the OF graph ?
> 
> Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> similar approach as well.

Peeking into nodes of other devices is a bad practice. I don't know how
the code you mention below got merged, but I'm pretty sure I would have
flagged it if I had reviewed the patches :-)

The ddc-i2c-bus property should instead be specified in the node where
it logically belongs (in this case, the connector node), and handled by
the connector driver. You can then use drm_bridge operations to tie
things together, like done in the drm_bridge_connector helper. I'd
recommend using the drm_bridge_connector helper if you can, either
as-is, or by extending it.

> [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> 
> > > > > +
> > > > > +  ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +
> > > > > +    properties:
> > > > > +      port@0:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: |
> > > > > +          Video port for input.
> > > > > +
> > > > > +      port@1:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: |
> > > > > +          2 video ports for output.
> > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > +
> > > > > +    required:
> > > > > +      - port@0
> > > > > +      - port@1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - detect-gpios
> > > > > +  - ports
> > > > > +
> > > > > +unevaluatedProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > +    hdmi_mux: hdmi_mux {
> > > > > +      compatible = "gpio-display-mux";
> > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > +      pinctrl-names = "default";
> > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > +
> > > > > +      ports {
> > > > > +        #address-cells = <1>;
> > > > > +        #size-cells = <0>;
> > > > > +
> > > > > +        port@0 { /* input */
> > > > > +          reg = <0>;
> > > > > +
> > > > > +          hdmi_mux_in: endpoint {
> > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > +          };
> > > > > +        };
> > > > > +
> > > > > +        port@1 { /* output */
> > > > > +          reg = <1>;
> > > > > +
> > > > > +          #address-cells = <1>;
> > > > > +          #size-cells = <0>;
> > > > > +
> > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > +            reg = <0>;
> > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > +          };
> > > > > +
> > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > +            reg = <1>;
> > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > +          };
> > > > > +        };
> > > > > +      };
> > > > > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

Hello Pin-yen,

On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > >
> > > > > Add bindings for Generic GPIO mux driver.
> > > > >
> > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > - Added ddc-i2c-bus into the bindings
> > > > >
> > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > >  1 file changed, 95 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..da29ba078f05
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > @@ -0,0 +1,95 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > +
> > > > > +maintainers:
> > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > +
> > > > > +description: |
> > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > +  its status is read back using a GPIO.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: gpio-display-mux
> > > > > +
> > > > > +  detect-gpios:
> > > > > +    maxItems: 1
> > > > > +    description: GPIO that indicates the active output
> > > >
> > > > What are we detecting? That implies an input, but this is selecting the
> > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > mean exactly? Something else? That does not sound very generic.
> > >
> > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > output should go. The actual "output selection" procedure is done in
> > > the driver. That is, the driver monitors this GPIO and selects the
> > > output path accordingly. In our use case, the GPIO is reported by the
> > > embedded controller on the device.
> > >
> > > [1] listed other similar bridges that can leverage this driver, so we
> > > called this driver "generic".
> > >
> > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > >
> > > > In any case, we have a common mux binding so any kind of mux control
> > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > display mux.
> > >
> > > Thanks for sharing this, I'll update the binding in the next version.
> > >
> > > > > +
> > > > > +  ddc-i2c-bus:
> > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > >
> > > > This belongs in the connector node(s).
> > >
> > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > parse the phandle and get the bus node.
> >
> > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > property of the next DT node in the OF graph ?
> 
> Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> similar approach as well.

Peeking into nodes of other devices is a bad practice. I don't know how
the code you mention below got merged, but I'm pretty sure I would have
flagged it if I had reviewed the patches :-)

The ddc-i2c-bus property should instead be specified in the node where
it logically belongs (in this case, the connector node), and handled by
the connector driver. You can then use drm_bridge operations to tie
things together, like done in the drm_bridge_connector helper. I'd
recommend using the drm_bridge_connector helper if you can, either
as-is, or by extending it.

> [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> 
> > > > > +
> > > > > +  ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +
> > > > > +    properties:
> > > > > +      port@0:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: |
> > > > > +          Video port for input.
> > > > > +
> > > > > +      port@1:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: |
> > > > > +          2 video ports for output.
> > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > +
> > > > > +    required:
> > > > > +      - port@0
> > > > > +      - port@1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - detect-gpios
> > > > > +  - ports
> > > > > +
> > > > > +unevaluatedProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > +    hdmi_mux: hdmi_mux {
> > > > > +      compatible = "gpio-display-mux";
> > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > +      pinctrl-names = "default";
> > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > +
> > > > > +      ports {
> > > > > +        #address-cells = <1>;
> > > > > +        #size-cells = <0>;
> > > > > +
> > > > > +        port@0 { /* input */
> > > > > +          reg = <0>;
> > > > > +
> > > > > +          hdmi_mux_in: endpoint {
> > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > +          };
> > > > > +        };
> > > > > +
> > > > > +        port@1 { /* output */
> > > > > +          reg = <1>;
> > > > > +
> > > > > +          #address-cells = <1>;
> > > > > +          #size-cells = <0>;
> > > > > +
> > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > +            reg = <0>;
> > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > +          };
> > > > > +
> > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > +            reg = <1>;
> > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > +          };
> > > > > +        };
> > > > > +      };
> > > > > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Laurent,

On Tue, Feb 7, 2023 at 11:21 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello Pin-yen,
>
> On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> > On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > > >
> > > > > > Add bindings for Generic GPIO mux driver.
> > > > > >
> > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > > - Added ddc-i2c-bus into the bindings
> > > > > >
> > > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > > >  1 file changed, 95 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..da29ba078f05
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > @@ -0,0 +1,95 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > > +
> > > > > > +description: |
> > > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > > +  its status is read back using a GPIO.
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: gpio-display-mux
> > > > > > +
> > > > > > +  detect-gpios:
> > > > > > +    maxItems: 1
> > > > > > +    description: GPIO that indicates the active output
> > > > >
> > > > > What are we detecting? That implies an input, but this is selecting the
> > > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > > mean exactly? Something else? That does not sound very generic.
> > > >
> > > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > > output should go. The actual "output selection" procedure is done in
> > > > the driver. That is, the driver monitors this GPIO and selects the
> > > > output path accordingly. In our use case, the GPIO is reported by the
> > > > embedded controller on the device.
> > > >
> > > > [1] listed other similar bridges that can leverage this driver, so we
> > > > called this driver "generic".
> > > >
> > > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > > >
> > > > > In any case, we have a common mux binding so any kind of mux control
> > > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > > display mux.
> > > >
> > > > Thanks for sharing this, I'll update the binding in the next version.
> > > >
> > > > > > +
> > > > > > +  ddc-i2c-bus:
> > > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > >
> > > > > This belongs in the connector node(s).
> > > >
> > > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > > parse the phandle and get the bus node.
> > >
> > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > > property of the next DT node in the OF graph ?
> >
> > Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> > output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> > similar approach as well.
>
> Peeking into nodes of other devices is a bad practice. I don't know how
> the code you mention below got merged, but I'm pretty sure I would have
> flagged it if I had reviewed the patches :-)
>
> The ddc-i2c-bus property should instead be specified in the node where
> it logically belongs (in this case, the connector node), and handled by
> the connector driver. You can then use drm_bridge operations to tie
> things together, like done in the drm_bridge_connector helper. I'd
> recommend using the drm_bridge_connector helper if you can, either
> as-is, or by extending it.

So, even if the connector does not have its own i2c controller, we
should put ddc-i2c-bus property in the connector DT node and tell the
DRM core that this bridge (driver) has the ability to read EDID?

If so, I'll fix up the mtk_hdmi.c driver and update the anx7688 driver
in the next version.

Regards,
Pin-yen
>
> > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> > [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> >
> > > > > > +
> > > > > > +  ports:
> > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > +
> > > > > > +    properties:
> > > > > > +      port@0:
> > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > +        description: |
> > > > > > +          Video port for input.
> > > > > > +
> > > > > > +      port@1:
> > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > +        description: |
> > > > > > +          2 video ports for output.
> > > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > > +
> > > > > > +    required:
> > > > > > +      - port@0
> > > > > > +      - port@1
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - detect-gpios
> > > > > > +  - ports
> > > > > > +
> > > > > > +unevaluatedProperties: false
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > > +    hdmi_mux: hdmi_mux {
> > > > > > +      compatible = "gpio-display-mux";
> > > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > > +      pinctrl-names = "default";
> > > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > > +
> > > > > > +      ports {
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <0>;
> > > > > > +
> > > > > > +        port@0 { /* input */
> > > > > > +          reg = <0>;
> > > > > > +
> > > > > > +          hdmi_mux_in: endpoint {
> > > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > > +          };
> > > > > > +        };
> > > > > > +
> > > > > > +        port@1 { /* output */
> > > > > > +          reg = <1>;
> > > > > > +
> > > > > > +          #address-cells = <1>;
> > > > > > +          #size-cells = <0>;
> > > > > > +
> > > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > > +            reg = <0>;
> > > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > > +          };
> > > > > > +
> > > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > > +            reg = <1>;
> > > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > > +          };
> > > > > > +        };
> > > > > > +      };
> > > > > > +    };
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Laurent,

On Tue, Feb 7, 2023 at 11:21 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello Pin-yen,
>
> On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> > On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > > >
> > > > > > Add bindings for Generic GPIO mux driver.
> > > > > >
> > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > > - Added ddc-i2c-bus into the bindings
> > > > > >
> > > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > > >  1 file changed, 95 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..da29ba078f05
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > @@ -0,0 +1,95 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > > +
> > > > > > +description: |
> > > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > > +  its status is read back using a GPIO.
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: gpio-display-mux
> > > > > > +
> > > > > > +  detect-gpios:
> > > > > > +    maxItems: 1
> > > > > > +    description: GPIO that indicates the active output
> > > > >
> > > > > What are we detecting? That implies an input, but this is selecting the
> > > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > > mean exactly? Something else? That does not sound very generic.
> > > >
> > > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > > output should go. The actual "output selection" procedure is done in
> > > > the driver. That is, the driver monitors this GPIO and selects the
> > > > output path accordingly. In our use case, the GPIO is reported by the
> > > > embedded controller on the device.
> > > >
> > > > [1] listed other similar bridges that can leverage this driver, so we
> > > > called this driver "generic".
> > > >
> > > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > > >
> > > > > In any case, we have a common mux binding so any kind of mux control
> > > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > > display mux.
> > > >
> > > > Thanks for sharing this, I'll update the binding in the next version.
> > > >
> > > > > > +
> > > > > > +  ddc-i2c-bus:
> > > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > >
> > > > > This belongs in the connector node(s).
> > > >
> > > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > > parse the phandle and get the bus node.
> > >
> > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > > property of the next DT node in the OF graph ?
> >
> > Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> > output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> > similar approach as well.
>
> Peeking into nodes of other devices is a bad practice. I don't know how
> the code you mention below got merged, but I'm pretty sure I would have
> flagged it if I had reviewed the patches :-)
>
> The ddc-i2c-bus property should instead be specified in the node where
> it logically belongs (in this case, the connector node), and handled by
> the connector driver. You can then use drm_bridge operations to tie
> things together, like done in the drm_bridge_connector helper. I'd
> recommend using the drm_bridge_connector helper if you can, either
> as-is, or by extending it.

So, even if the connector does not have its own i2c controller, we
should put ddc-i2c-bus property in the connector DT node and tell the
DRM core that this bridge (driver) has the ability to read EDID?

If so, I'll fix up the mtk_hdmi.c driver and update the anx7688 driver
in the next version.

Regards,
Pin-yen
>
> > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> > [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> >
> > > > > > +
> > > > > > +  ports:
> > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > +
> > > > > > +    properties:
> > > > > > +      port@0:
> > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > +        description: |
> > > > > > +          Video port for input.
> > > > > > +
> > > > > > +      port@1:
> > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > +        description: |
> > > > > > +          2 video ports for output.
> > > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > > +
> > > > > > +    required:
> > > > > > +      - port@0
> > > > > > +      - port@1
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - detect-gpios
> > > > > > +  - ports
> > > > > > +
> > > > > > +unevaluatedProperties: false
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > > +    hdmi_mux: hdmi_mux {
> > > > > > +      compatible = "gpio-display-mux";
> > > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > > +      pinctrl-names = "default";
> > > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > > +
> > > > > > +      ports {
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <0>;
> > > > > > +
> > > > > > +        port@0 { /* input */
> > > > > > +          reg = <0>;
> > > > > > +
> > > > > > +          hdmi_mux_in: endpoint {
> > > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > > +          };
> > > > > > +        };
> > > > > > +
> > > > > > +        port@1 { /* output */
> > > > > > +          reg = <1>;
> > > > > > +
> > > > > > +          #address-cells = <1>;
> > > > > > +          #size-cells = <0>;
> > > > > > +
> > > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > > +            reg = <0>;
> > > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > > +          };
> > > > > > +
> > > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > > +            reg = <1>;
> > > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > > +          };
> > > > > > +        };
> > > > > > +      };
> > > > > > +    };
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

Hello Pin-yen,

On Fri, Feb 10, 2023 at 03:38:00PM +0800, Pin-yen Lin wrote:
> On Tue, Feb 7, 2023 at 11:21 PM Laurent Pinchart wrote:
> > On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> > > On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > >
> > > > > > > Add bindings for Generic GPIO mux driver.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > > > - Added ddc-i2c-bus into the bindings
> > > > > > >
> > > > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > > > >  1 file changed, 95 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..da29ba078f05
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > > @@ -0,0 +1,95 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > +
> > > > > > > +description: |
> > > > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > > > +  its status is read back using a GPIO.
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: gpio-display-mux
> > > > > > > +
> > > > > > > +  detect-gpios:
> > > > > > > +    maxItems: 1
> > > > > > > +    description: GPIO that indicates the active output
> > > > > >
> > > > > > What are we detecting? That implies an input, but this is selecting the
> > > > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > > > mean exactly? Something else? That does not sound very generic.
> > > > >
> > > > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > > > output should go. The actual "output selection" procedure is done in
> > > > > the driver. That is, the driver monitors this GPIO and selects the
> > > > > output path accordingly. In our use case, the GPIO is reported by the
> > > > > embedded controller on the device.
> > > > >
> > > > > [1] listed other similar bridges that can leverage this driver, so we
> > > > > called this driver "generic".
> > > > >
> > > > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > > > >
> > > > > > In any case, we have a common mux binding so any kind of mux control
> > > > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > > > display mux.
> > > > >
> > > > > Thanks for sharing this, I'll update the binding in the next version.
> > > > >
> > > > > > > +
> > > > > > > +  ddc-i2c-bus:
> > > > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > >
> > > > > > This belongs in the connector node(s).
> > > > >
> > > > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > > > parse the phandle and get the bus node.
> > > >
> > > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > > > property of the next DT node in the OF graph ?
> > >
> > > Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> > > output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> > > similar approach as well.
> >
> > Peeking into nodes of other devices is a bad practice. I don't know how
> > the code you mention below got merged, but I'm pretty sure I would have
> > flagged it if I had reviewed the patches :-)
> >
> > The ddc-i2c-bus property should instead be specified in the node where
> > it logically belongs (in this case, the connector node), and handled by
> > the connector driver. You can then use drm_bridge operations to tie
> > things together, like done in the drm_bridge_connector helper. I'd
> > recommend using the drm_bridge_connector helper if you can, either
> > as-is, or by extending it.
> 
> So, even if the connector does not have its own i2c controller, we
> should put ddc-i2c-bus property in the connector DT node and tell the
> DRM core that this bridge (driver) has the ability to read EDID?

That's the idea, yes, if the DDC lines are connected to an I2C
controller on the SoC. Of course, if the DDC lines are connected
directly to an HDMI encoder that has an internal I2C controller, then
the ddc-i2c-bus property shouldn't be added, and the DRM bridge driver
for the HDMI encoder should report the EDID read ability.

> If so, I'll fix up the mtk_hdmi.c driver and update the anx7688 driver
> in the next version.

Thank you. I had a quick look at the mtk_hdmi driver, ideally support
for reading EDID should be dropped from there. As far as I understand,
the mtk_hdmi bridge is connected to the mtk_dpi bridge, whose driver
uses the drm_bridge_connector helper, so it shouldn't be too difficult.

> > > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> > > [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> > >
> > > > > > > +
> > > > > > > +  ports:
> > > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > +
> > > > > > > +    properties:
> > > > > > > +      port@0:
> > > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > > +        description: |
> > > > > > > +          Video port for input.
> > > > > > > +
> > > > > > > +      port@1:
> > > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > > +        description: |
> > > > > > > +          2 video ports for output.
> > > > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > > > +
> > > > > > > +    required:
> > > > > > > +      - port@0
> > > > > > > +      - port@1
> > > > > > > +
> > > > > > > +required:
> > > > > > > +  - compatible
> > > > > > > +  - detect-gpios
> > > > > > > +  - ports
> > > > > > > +
> > > > > > > +unevaluatedProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > +  - |
> > > > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > > > +    hdmi_mux: hdmi_mux {
> > > > > > > +      compatible = "gpio-display-mux";
> > > > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > > > +      pinctrl-names = "default";
> > > > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > > > +
> > > > > > > +      ports {
> > > > > > > +        #address-cells = <1>;
> > > > > > > +        #size-cells = <0>;
> > > > > > > +
> > > > > > > +        port@0 { /* input */
> > > > > > > +          reg = <0>;
> > > > > > > +
> > > > > > > +          hdmi_mux_in: endpoint {
> > > > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > > > +          };
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        port@1 { /* output */
> > > > > > > +          reg = <1>;
> > > > > > > +
> > > > > > > +          #address-cells = <1>;
> > > > > > > +          #size-cells = <0>;
> > > > > > > +
> > > > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > > > +            reg = <0>;
> > > > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > > > +          };
> > > > > > > +
> > > > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > > > +            reg = <1>;
> > > > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > > > +          };
> > > > > > > +        };
> > > > > > > +      };
> > > > > > > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Laurent Pinchart]

Hello Pin-yen,

On Fri, Feb 10, 2023 at 03:38:00PM +0800, Pin-yen Lin wrote:
> On Tue, Feb 7, 2023 at 11:21 PM Laurent Pinchart wrote:
> > On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote:
> > > On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote:
> > > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote:
> > > > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote:
> > > > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > > > > > > From: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > >
> > > > > > > Add bindings for Generic GPIO mux driver.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > - Referenced existing dt-binding schemas from graph.yaml
> > > > > > > - Added ddc-i2c-bus into the bindings
> > > > > > >
> > > > > > >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> > > > > > >  1 file changed, 95 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..da29ba078f05
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > > > > > > @@ -0,0 +1,95 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Generic display mux (1 input, 2 outputs)
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > +
> > > > > > > +description: |
> > > > > > > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > > > > > > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > > > > > > +  its status is read back using a GPIO.
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: gpio-display-mux
> > > > > > > +
> > > > > > > +  detect-gpios:
> > > > > > > +    maxItems: 1
> > > > > > > +    description: GPIO that indicates the active output
> > > > > >
> > > > > > What are we detecting? That implies an input, but this is selecting the
> > > > > > output path, right? Or what does 'mux status is controlled by hardware'
> > > > > > mean exactly? Something else? That does not sound very generic.
> > > > >
> > > > > The GPIO (or any kind of MUX) is an input that indicates where the
> > > > > output should go. The actual "output selection" procedure is done in
> > > > > the driver. That is, the driver monitors this GPIO and selects the
> > > > > output path accordingly. In our use case, the GPIO is reported by the
> > > > > embedded controller on the device.
> > > > >
> > > > > [1] listed other similar bridges that can leverage this driver, so we
> > > > > called this driver "generic".
> > > > >
> > > > > [1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@mail.gmail.com/
> > > > >
> > > > > > In any case, we have a common mux binding so any kind of mux control
> > > > > > could be used here, not just GPIO. Then you can make this just a generic
> > > > > > display mux.
> > > > >
> > > > > Thanks for sharing this, I'll update the binding in the next version.
> > > > >
> > > > > > > +
> > > > > > > +  ddc-i2c-bus:
> > > > > > > +    description: phandle link to the I2C controller used for DDC EDID probing
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > >
> > > > > > This belongs in the connector node(s).
> > > > >
> > > > > The HDMI bridge before the MUX doesn't (and doesn't have to) know that
> > > > > its next bridge is a MUX. We put it here so that the HDMI bridge can
> > > > > parse the phandle and get the bus node.
> > > >
> > > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus
> > > > property of the next DT node in the OF graph ?
> > >
> > > Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its
> > > output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a
> > > similar approach as well.
> >
> > Peeking into nodes of other devices is a bad practice. I don't know how
> > the code you mention below got merged, but I'm pretty sure I would have
> > flagged it if I had reviewed the patches :-)
> >
> > The ddc-i2c-bus property should instead be specified in the node where
> > it logically belongs (in this case, the connector node), and handled by
> > the connector driver. You can then use drm_bridge operations to tie
> > things together, like done in the drm_bridge_connector helper. I'd
> > recommend using the drm_bridge_connector helper if you can, either
> > as-is, or by extending it.
> 
> So, even if the connector does not have its own i2c controller, we
> should put ddc-i2c-bus property in the connector DT node and tell the
> DRM core that this bridge (driver) has the ability to read EDID?

That's the idea, yes, if the DDC lines are connected to an I2C
controller on the SoC. Of course, if the DDC lines are connected
directly to an HDMI encoder that has an internal I2C controller, then
the ddc-i2c-bus property shouldn't be added, and the DRM bridge driver
for the HDMI encoder should report the EDID read ability.

> If so, I'll fix up the mtk_hdmi.c driver and update the anx7688 driver
> in the next version.

Thank you. I had a quick look at the mtk_hdmi driver, ideally support
for reading EDID should be dropped from there. As far as I understand,
the mtk_hdmi bridge is connected to the mtk_dpi bridge, whose driver
uses the drm_bridge_connector helper, so it shouldn't be too difficult.

> > > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500
> > > [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240
> > >
> > > > > > > +
> > > > > > > +  ports:
> > > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > > +
> > > > > > > +    properties:
> > > > > > > +      port@0:
> > > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > > +        description: |
> > > > > > > +          Video port for input.
> > > > > > > +
> > > > > > > +      port@1:
> > > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > > +        description: |
> > > > > > > +          2 video ports for output.
> > > > > > > +          The reg value in the endpoints matches the GPIO status: when
> > > > > > > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > > > > > > +
> > > > > > > +    required:
> > > > > > > +      - port@0
> > > > > > > +      - port@1
> > > > > > > +
> > > > > > > +required:
> > > > > > > +  - compatible
> > > > > > > +  - detect-gpios
> > > > > > > +  - ports
> > > > > > > +
> > > > > > > +unevaluatedProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > +  - |
> > > > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > > > +    hdmi_mux: hdmi_mux {
> > > > > > > +      compatible = "gpio-display-mux";
> > > > > > > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > > > > > > +      pinctrl-names = "default";
> > > > > > > +      pinctrl-0 = <&hdmi_mux_pins>;
> > > > > > > +      ddc-i2c-bus = <&hdmiddc0>;
> > > > > > > +
> > > > > > > +      ports {
> > > > > > > +        #address-cells = <1>;
> > > > > > > +        #size-cells = <0>;
> > > > > > > +
> > > > > > > +        port@0 { /* input */
> > > > > > > +          reg = <0>;
> > > > > > > +
> > > > > > > +          hdmi_mux_in: endpoint {
> > > > > > > +            remote-endpoint = <&hdmi0_out>;
> > > > > > > +          };
> > > > > > > +        };
> > > > > > > +
> > > > > > > +        port@1 { /* output */
> > > > > > > +          reg = <1>;
> > > > > > > +
> > > > > > > +          #address-cells = <1>;
> > > > > > > +          #size-cells = <0>;
> > > > > > > +
> > > > > > > +          hdmi_mux_out_anx: endpoint@0 {
> > > > > > > +            reg = <0>;
> > > > > > > +            remote-endpoint = <&dp_bridge_in>;
> > > > > > > +          };
> > > > > > > +
> > > > > > > +          hdmi_mux_out_hdmi: endpoint@1 {
> > > > > > > +            reg = <1>;
> > > > > > > +            remote-endpoint = <&hdmi_connector_in>;
> > > > > > > +          };
> > > > > > > +        };
> > > > > > > +      };
> > > > > > > +    };

-- 
Regards,

Laurent Pinchart