[PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml

[PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml

[Douglas Anderson]

This moves the bindings over, based a lot on toshiba,tc358768.yaml.
Unless there's someone known to be better, I've set the maintainer in
the yaml as the first person to submit bindings.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../bindings/display/bridge/ti,sn65dsi86.txt  |  87 --------
 .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++
 2 files changed, 188 insertions(+), 87 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
deleted file mode 100644
index 8ec4a7f2623a..000000000000
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
+++ /dev/null
@@ -1,87 +0,0 @@
-SN65DSI86 DSI to eDP bridge chip
---------------------------------
-
-This is the binding for Texas Instruments SN65DSI86 bridge.
-http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
-
-Required properties:
-- compatible: Must be "ti,sn65dsi86"
-- reg: i2c address of the chip, 0x2d as per datasheet
-- enable-gpios: gpio specification for bridge_en pin (active high)
-
-- vccio-supply: A 1.8V supply that powers up the digital IOs.
-- vpll-supply: A 1.8V supply that powers up the displayport PLL.
-- vcca-supply: A 1.2V supply that powers up the analog circuits.
-- vcc-supply: A 1.2V supply that powers up the digital core.
-
-Optional properties:
-- interrupts-extended: Specifier for the SN65DSI86 interrupt line.
-
-- gpio-controller: Marks the device has a GPIO controller.
-- #gpio-cells    : Should be two. The first cell is the pin number and
-                   the second cell is used to specify flags.
-                   See ../../gpio/gpio.txt for more information.
-- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of
-               the cell formats.
-
-- clock-names: should be "refclk"
-- clocks: Specification for input reference clock. The reference
-	  clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
-
-- data-lanes: See ../../media/video-interface.txt
-- lane-polarities: See ../../media/video-interface.txt
-
-- suspend-gpios: specification for GPIO1 pin on bridge (active low)
-
-Required nodes:
-This device has two video ports. Their connections are modelled using the
-OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for DSI input
-- Video port 1 for eDP output
-
-Example
--------
-
-edp-bridge@2d {
-	compatible = "ti,sn65dsi86";
-	#address-cells = <1>;
-	#size-cells = <0>;
-	reg = <0x2d>;
-
-	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
-	suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>;
-
-	interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>;
-
-	vccio-supply = <&pm8916_l17>;
-	vcca-supply = <&pm8916_l6>;
-	vpll-supply = <&pm8916_l17>;
-	vcc-supply = <&pm8916_l6>;
-
-	clock-names = "refclk";
-	clocks = <&input_refclk>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			edp_bridge_in: endpoint {
-				remote-endpoint = <&dsi_out>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			edp_bridge_out: endpoint {
-				data-lanes = <2 1 3 0>;
-				lane-polarities = <0 1 0 1>;
-				remote-endpoint = <&edp_panel_in>;
-			};
-		};
-	};
-}
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
new file mode 100644
index 000000000000..8cacc6db33a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
@@ -0,0 +1,188 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SN65DSI86 DSI to eDP bridge chip
+
+maintainers:
+  - Sandeep Panda <spanda@codeaurora.org>
+
+description: |
+  The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP.
+  http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
+
+properties:
+  compatible:
+    const: ti,sn65dsi86
+
+  reg:
+    const: 0x2d
+
+  enable-gpios:
+    maxItems: 1
+    description: GPIO specification for bridge_en pin (active high).
+
+  vccio-supply:
+    description: A 1.8V supply that powers up the digital IOs.
+
+  vpll-supply:
+    description: A 1.8V supply that powers up the DisplayPort PLL.
+
+  vcca-supply:
+    description: A 1.2V supply that powers up the analog circuits.
+
+  vcc-supply:
+    description: A 1.2V supply that powers up the digital core.
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description:
+      Specification for input reference clock. The reference clock rate must
+      be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
+
+  clock-names:
+    const: refclk
+
+  gpio-controller: true
+  '#gpio-cells':
+    const: 2
+    description:
+      First cell is pin number, second cell is flags.  GPIO pin numbers are
+      1-based to match the datasheet.  See ../../gpio/gpio.txt for more
+      information.
+
+  '#pwm-cells':
+    const: 1
+    description: See ../../pwm/pwm.yaml for description of the cell formats.
+
+  data-lanes:
+    description: See ../../media/video-interface.txt
+
+  lane-polarities:
+    description: See ../../media/video-interface.txt
+
+  ports:
+    type: object
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+      port@0:
+        type: object
+        additionalProperties: false
+
+        description:
+          Video port for MIPI DSI input
+
+        properties:
+          reg:
+            const: 0
+
+        patternProperties:
+          endpoint:
+            type: object
+            additionalProperties: false
+
+            properties:
+              remote-endpoint: true
+
+        required:
+          - reg
+
+      port@1:
+        type: object
+        additionalProperties: false
+
+        description:
+          Video port for eDP output (panel or connector).
+
+        properties:
+          reg:
+            const: 1
+
+        patternProperties:
+          endpoint:
+            type: object
+            additionalProperties: false
+
+            properties:
+              remote-endpoint: true
+
+        required:
+          - reg
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - enable-gpios
+  - vccio-supply
+  - vpll-supply
+  - vcca-supply
+  - vcc-supply
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c1 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      bridge@2d {
+        compatible = "ti,sn65dsi86";
+        reg = <0x2d>;
+
+        interrupt-parent = <&tlmm>;
+        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
+
+        enable-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>;
+
+        vpll-supply = <&src_pp1800_s4a>;
+        vccio-supply = <&src_pp1800_s4a>;
+        vcca-supply = <&src_pp1200_l2a>;
+        vcc-supply = <&src_pp1200_l2a>;
+
+        clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
+        clock-names = "refclk";
+
+        ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          port@0 {
+            reg = <0>;
+            endpoint {
+              remote-endpoint = <&dsi0_out>;
+            };
+          };
+
+          port@1 {
+            reg = <1>;
+            endpoint {
+              remote-endpoint = <&panel_in_edp>;
+            };
+          };
+        };
+      };
+    };
+
-- 
2.26.0.110.g2183baf09c-goog

[PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Douglas Anderson]

Allow people to specify to use a GPIO for hot-plug-detect.  Add an
example.

NOTE: The current patch adding support for hpd-gpios to the Linux
driver for hpd-gpios only adds enough support to the driver so that
the bridge can use one of its own GPIOs.  The bindings, however, are
written generically.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
index 8cacc6db33a9..554bfd003000 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
@@ -60,6 +60,10 @@ properties:
     const: 1
     description: See ../../pwm/pwm.yaml for description of the cell formats.
 
+  hpd-gpios:
+    maxItems: 1
+    description: If present use the given GPIO for hot-plug-detect.
+
   data-lanes:
     description: See ../../media/video-interface.txt
 
@@ -148,7 +152,7 @@ examples:
       #address-cells = <1>;
       #size-cells = <0>;
 
-      bridge@2d {
+      sn65dsi86_bridge: bridge@2d {
         compatible = "ti,sn65dsi86";
         reg = <0x2d>;
 
@@ -165,6 +169,10 @@ examples:
         clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
         clock-names = "refclk";
 
+        gpio-controller;
+        #gpio-cells = <2>;
+        hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+
         ports {
           #address-cells = <1>;
           #size-cells = <0>;
-- 
2.26.0.110.g2183baf09c-goog

[PATCH 3/3] drm/bridge: ti-sn65dsi86: Allow one of the bridge GPIOs to be HPD

[Douglas Anderson]

As talked about in commit c2bfc223882d ("drm/bridge: ti-sn65dsi86:
Remove the mystery delay"), the normal HPD pin on ti-sn65dsi86 is
kinda useless, at least for embedded DisplayPort (eDP).  However,
despite the fact that the actual HPD pin on the bridge is mostly
useless for eDP, the concept of HPD for eDP still makes sense.  It
allows us to optimize out a hardcoded delay that many panels need if
HPD isn't hooked up.  Panel timing diagrams show HPD as one of the
events to measure timing from and we have to assume the worst case if
we can't actually read HPD.

One way to use HPD for eDP without using the mostly useless HPD pin on
ti-sn65dsi86 is to route the panel's HPD somewhere else in the system,
like to a GPIO.  This works great because eDP panels aren't physically
hotplugged.  That means the debouncing logic that caused us problems
wasn't really needed and a raw GPIO works great.

As per the above, a smart board designer would realize the value of
HPD and choose to route it to a GPIO somewhere on the board to avoid
the silly sn65dsi86 debouncing.  While said "smart designer" could
theoretically route HPD anywhere on the board, a really smart designer
would realize that there are several GPIOs on the bridge itself that
are nearly useless for anything but this purpose and route HPD to one
of those.

Specifically, to argue that the GPIOs on the bridge are mostly useless
for non-bridge-related things:
- You need to power on the bridge to use them.  Ideally a system only
  powers on the bridge when the screen is on which means you could
  only use the GPIOs on the bridge when the screen is on (or you're
  willing to burn a bunch of extra power).
- You need to control the GPIOs via i2c commands, which means that you
  can only use these GPIOs for drivers that can call
  gpio_set_value_cansleep().

With the above, let's assume that:
- If someone was going to route the HPD to a GPIO they'd use one of
  the GPIOs on the bridge.
- There's not lots of value exposing the GPIOs on the bridge through
  the normal Linux GPIO framework because nobody will likely use them
  for something non-bridge-related.

As you can probably guess from the above arguments, this patch adds
the ability to use one of the bridge's GPIOs as the HPD pin but it
doesn't add it in the completely generic (and a bit heavier) way of
going through the GPIO subsystem.  This means:
- With this patch you can't use bridge GPIOs for non-bridge purposes.
- With this patch you can't use a different GPIO in the SoC for HPD.

Despite the above limitations, which keep the code simpler, we still
use the generic GPIO device tree bindings.  If someone later has a
need to relax some of the restrictions here, it would just need a code
patch.  We wouldn't need to change the device tree bindings.

This patch has been tested on a board that is not yet mainline.  On
the hardware I have:
- Panel spec says HPD could take up to 200 ms to come up, so without
  HPD hooked up we need to delay 200 ms.
- On my board the panel is powered by the same rail as the
  touchscreen.  By chance of probe order the touchscreen comes up
  first.  This means by the time we check HPD in ti_sn_bridge_enable()
  it's already up.  Thus we can use the panel on 200 ms earlier.
- If I hack out the touscreen, I see that I wait ~31 ms for HPD to go
  high.  This means I save ~169 ms of delay.
- If I measure HPD on this pane it comes up ~56 ms after the panel is
  powered.  The delta between 31 and 56 ms is because the panel
  regulator is turned on at the end of ti_sn_bridge_pre_enable() in
  drm_panel_prepare().  There is apparently some time between that and
  ti_sn_bridge_enable().

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 107 ++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 6ad688b320ae..187b2bdd0cb4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -54,6 +54,13 @@
 #define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
+#define SN_GPIO_IO_REG				0x5E
+#define  SN_GPIO_INPUT_SHIFT			4
+#define  SN_GPIO_OUTPUT_SHIFT			0
+#define SN_GPIO_CTRL_REG			0x5F
+#define  SN_GPIO_MUX_INPUT			0
+#define  SN_GPIO_MUX_OUTPUT			1
+#define  SN_GPIO_MUX_SPECIAL			2
 #define SN_AUX_WDATA_REG(x)			(0x64 + (x))
 #define SN_AUX_ADDR_19_16_REG			0x74
 #define SN_AUX_ADDR_15_8_REG			0x75
@@ -102,6 +109,7 @@ struct ti_sn_bridge {
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 	int				dp_lanes;
+	int				hpd_gpio_pin;
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -120,6 +128,45 @@ static const struct regmap_config ti_sn_bridge_regmap_config = {
 	.cache_type = REGCACHE_NONE,
 };
 
+/**
+ * ti_sn_read_gpio() - Read a GPIO pin.
+ * @pdata: Platform data.
+ * @gpio_num: The GPIO to read.  This is 1-based to match the pin names so
+ *            valid values are 1-4.
+ *
+ * This function assumes the pin direction / muxing is already configured.
+ *
+ * Return: 0 if the pin is low; 1 if the pin is high; -error upon failure
+ */
+static int ti_sn_read_gpio(struct ti_sn_bridge *pdata, int gpio_num)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val);
+	if (ret)
+		return ret;
+
+	return (val >> (SN_GPIO_INPUT_SHIFT + gpio_num - 1)) & 1;
+}
+
+/**
+ * ti_sn_setup_mux() - Setup a GPIO pinmux.
+ * @pdata: Platform data.
+ * @gpio_num: The GPIO to setup.  This is 1-based to match the pin names so
+ *            valid values are 1-4.
+ * @val: The mux value; One of the SN_GPIO_MUX_... constants.
+ *
+ * Return: 0 if success; either error.
+ */
+static int ti_sn_setup_mux(struct ti_sn_bridge *pdata, int gpio_num, int val)
+{
+	int shift = (gpio_num - 1) * 2;
+
+	return regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
+				  0x3 << shift, val << shift);
+}
+
 static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
 				   unsigned int reg, u16 val)
 {
@@ -658,6 +705,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
 	return ret;
 }
 
+static bool ti_sn_read_hpd_gpio(struct ti_sn_bridge *pdata)
+{
+	return ti_sn_read_gpio(pdata, pdata->hpd_gpio_pin) == 1;
+}
+
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
@@ -666,6 +718,25 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	int dp_rate_idx;
 	unsigned int val;
 	int ret = -EINVAL;
+	bool hpd_asserted;
+
+	/*
+	 * On some designs HPD could be routed to one of the GPIOs on the
+	 * bridge.  In that case, delay up to 2 seconds waiting for HPD to be
+	 * asserted.
+	 */
+	if (pdata->hpd_gpio_pin) {
+		ret = ti_sn_setup_mux(pdata, pdata->hpd_gpio_pin,
+				      SN_GPIO_MUX_INPUT);
+		if (ret) {
+			DRM_ERROR("failed to setup HPD mux\n");
+			return;
+		}
+
+		ret = readx_poll_timeout(ti_sn_read_hpd_gpio, pdata,
+					 hpd_asserted, hpd_asserted,
+					 1000, 2000000);
+	}
 
 	/*
 	 * Run with the maximum number of lanes that the DP sink supports.
@@ -874,6 +945,40 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
 	return 0;
 }
 
+static void ti_sn_probe_hpd_gpio(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	int num_elems;
+	u32 hpd_gpio_pin;
+
+	num_elems = of_property_count_u32_elems(np, "hpd-gpios");
+
+	/* It's optional, so no worries if it's not there */
+	if (num_elems == -EINVAL)
+		return;
+
+	if (num_elems != 3) {
+		dev_warn(pdata->dev,
+			 "Unexpected hpd-gpios count (%d); ignoring\n",
+			 num_elems);
+		return;
+	}
+
+	/*
+	 * Right now, we only support using one of our own GPIOs for
+	 * this, but our device tree bindings are more generic.  Confirm
+	 * we're actually a client of ourselves.
+	 */
+	if (of_parse_phandle(np, "hpd-gpios", 0) != np) {
+		dev_warn(pdata->dev,
+			 "Only bridge-local hpd-gpios supported; ignoring\n");
+		return;
+	}
+
+	if (!of_property_read_u32_index(np, "hpd-gpios", 1, &hpd_gpio_pin))
+		pdata->hpd_gpio_pin = hpd_gpio_pin;
+}
+
 static int ti_sn_bridge_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -916,6 +1021,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ti_sn_probe_hpd_gpio(pdata);
+
 	ret = ti_sn_bridge_parse_regulators(pdata);
 	if (ret) {
 		DRM_ERROR("failed to parse regulators\n");
-- 
2.26.0.110.g2183baf09c-goog

Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml

[Stephen Boyd]

Quoting Douglas Anderson (2020-04-15 08:48:39)
> This moves the bindings over, based a lot on toshiba,tc358768.yaml.
> Unless there's someone known to be better, I've set the maintainer in
> the yaml as the first person to submit bindings.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Awesome!

> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> new file mode 100644
> index 000000000000..8cacc6db33a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> @@ -0,0 +1,188 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SN65DSI86 DSI to eDP bridge chip
> +
> +maintainers:
> +  - Sandeep Panda <spanda@codeaurora.org>
> +
> +description: |
> +  The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP.
> +  http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> +
> +properties:
> +  compatible:
> +    const: ti,sn65dsi86
> +
> +  reg:
> +    const: 0x2d
> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO specification for bridge_en pin (active high).

s/specification/specifier/ ? I know the previous binding said
specification but I don't know what that is. It's a specifier.

> +
> +  vccio-supply:
> +    description: A 1.8V supply that powers up the digital IOs.
> +
> +  vpll-supply:
> +    description: A 1.8V supply that powers up the DisplayPort PLL.
> +
> +  vcca-supply:
> +    description: A 1.2V supply that powers up the analog circuits.
> +
> +  vcc-supply:
> +    description: A 1.2V supply that powers up the digital core.

Nitpick: Can we remove 'up' from these descriptions?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description:
> +      Specification for input reference clock. The reference clock rate must

Clock specifier for input reference clock?

> +      be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
> +
> +  clock-names:
> +    const: refclk
> +

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Stephen Boyd]

Quoting Douglas Anderson (2020-04-15 08:48:40)
> Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> example.
> 
> NOTE: The current patch adding support for hpd-gpios to the Linux
> driver for hpd-gpios only adds enough support to the driver so that
> the bridge can use one of its own GPIOs.  The bindings, however, are
> written generically.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> index 8cacc6db33a9..554bfd003000 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> @@ -60,6 +60,10 @@ properties:
>      const: 1
>      description: See ../../pwm/pwm.yaml for description of the cell formats.
>  
> +  hpd-gpios:
> +    maxItems: 1
> +    description: If present use the given GPIO for hot-plug-detect.

Shouldn't this go in the panel node? And the panel driver should get the
gpio and poll it after powering up the panel? Presumably that's why we
have the no-hpd property in the simple panel binding vs. putting it here
in the bridge.

Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml

[Laurent Pinchart]

Hi Douglas,

Thank you for the patch.

On Wed, Apr 15, 2020 at 08:48:39AM -0700, Douglas Anderson wrote:
> This moves the bindings over, based a lot on toshiba,tc358768.yaml.
> Unless there's someone known to be better, I've set the maintainer in
> the yaml as the first person to submit bindings.

You can also add your name ;-)

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../bindings/display/bridge/ti,sn65dsi86.txt  |  87 --------
>  .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++
>  2 files changed, 188 insertions(+), 87 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> deleted file mode 100644
> index 8ec4a7f2623a..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> +++ /dev/null
> @@ -1,87 +0,0 @@
> -SN65DSI86 DSI to eDP bridge chip
> ---------------------------------
> -
> -This is the binding for Texas Instruments SN65DSI86 bridge.
> -http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> -
> -Required properties:
> -- compatible: Must be "ti,sn65dsi86"
> -- reg: i2c address of the chip, 0x2d as per datasheet
> -- enable-gpios: gpio specification for bridge_en pin (active high)
> -
> -- vccio-supply: A 1.8V supply that powers up the digital IOs.
> -- vpll-supply: A 1.8V supply that powers up the displayport PLL.
> -- vcca-supply: A 1.2V supply that powers up the analog circuits.
> -- vcc-supply: A 1.2V supply that powers up the digital core.
> -
> -Optional properties:
> -- interrupts-extended: Specifier for the SN65DSI86 interrupt line.
> -
> -- gpio-controller: Marks the device has a GPIO controller.
> -- #gpio-cells    : Should be two. The first cell is the pin number and
> -                   the second cell is used to specify flags.
> -                   See ../../gpio/gpio.txt for more information.
> -- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of
> -               the cell formats.
> -
> -- clock-names: should be "refclk"
> -- clocks: Specification for input reference clock. The reference
> -	  clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
> -
> -- data-lanes: See ../../media/video-interface.txt
> -- lane-polarities: See ../../media/video-interface.txt
> -
> -- suspend-gpios: specification for GPIO1 pin on bridge (active low)

Where has suspend-gpios gone ? :-)

> -
> -Required nodes:
> -This device has two video ports. Their connections are modelled using the
> -OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for DSI input
> -- Video port 1 for eDP output
> -
> -Example
> --------
> -
> -edp-bridge@2d {
> -	compatible = "ti,sn65dsi86";
> -	#address-cells = <1>;
> -	#size-cells = <0>;
> -	reg = <0x2d>;
> -
> -	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
> -	suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>;
> -
> -	interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>;
> -
> -	vccio-supply = <&pm8916_l17>;
> -	vcca-supply = <&pm8916_l6>;
> -	vpll-supply = <&pm8916_l17>;
> -	vcc-supply = <&pm8916_l6>;
> -
> -	clock-names = "refclk";
> -	clocks = <&input_refclk>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			edp_bridge_in: endpoint {
> -				remote-endpoint = <&dsi_out>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			edp_bridge_out: endpoint {
> -				data-lanes = <2 1 3 0>;
> -				lane-polarities = <0 1 0 1>;
> -				remote-endpoint = <&edp_panel_in>;
> -			};
> -		};
> -	};
> -}
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> new file mode 100644
> index 000000000000..8cacc6db33a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> @@ -0,0 +1,188 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SN65DSI86 DSI to eDP bridge chip
> +
> +maintainers:
> +  - Sandeep Panda <spanda@codeaurora.org>
> +
> +description: |
> +  The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP.
> +  http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> +
> +properties:
> +  compatible:
> +    const: ti,sn65dsi86
> +
> +  reg:
> +    const: 0x2d
> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO specification for bridge_en pin (active high).
> +
> +  vccio-supply:
> +    description: A 1.8V supply that powers up the digital IOs.
> +
> +  vpll-supply:
> +    description: A 1.8V supply that powers up the DisplayPort PLL.
> +
> +  vcca-supply:
> +    description: A 1.2V supply that powers up the analog circuits.
> +
> +  vcc-supply:
> +    description: A 1.2V supply that powers up the digital core.
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description:
> +      Specification for input reference clock. The reference clock rate must
> +      be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
> +
> +  clock-names:
> +    const: refclk
> +
> +  gpio-controller: true
> +  '#gpio-cells':
> +    const: 2
> +    description:
> +      First cell is pin number, second cell is flags.  GPIO pin numbers are
> +      1-based to match the datasheet.  See ../../gpio/gpio.txt for more
> +      information.
> +
> +  '#pwm-cells':
> +    const: 1
> +    description: See ../../pwm/pwm.yaml for description of the cell formats.
> +
> +  data-lanes:

Should this have

	minItems: 1
	maxItems: 4
	items:
	  enum:
	    - 0
	    - 1
	    - 2
	    - 3

> +    description: See ../../media/video-interface.txt
> +
> +  lane-polarities:
> +    description: See ../../media/video-interface.txt

And something similar here,

	minItems: 1
	maxItems: 4
	items:
	  enum:
	    - 0
	    - 1
	uniqueItems: false

I'm not entirely sure where uniqueItems should be placed. I'm also not
sure how to specify that both data-lanes and lane-polarities need to
have the same number of items, maybe

dependencies:
  data-lanes: [lane-polarities]

> +
> +  ports:
> +    type: object
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      port@0:
> +        type: object
> +        additionalProperties: false
> +
> +        description:
> +          Video port for MIPI DSI input
> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +        patternProperties:
> +          endpoint:

If there's a single endpoint, you don't need patternProperties, it can
be specified in properties.

> +            type: object
> +            additionalProperties: false
> +
> +            properties:
> +              remote-endpoint: true
> +
> +        required:
> +          - reg
> +
> +      port@1:
> +        type: object
> +        additionalProperties: false
> +
> +        description:
> +          Video port for eDP output (panel or connector).
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +        patternProperties:
> +          endpoint:

Same here.

> +            type: object
> +            additionalProperties: false
> +
> +            properties:
> +              remote-endpoint: true
> +
> +        required:
> +          - reg
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - enable-gpios
> +  - vccio-supply
> +  - vpll-supply
> +  - vcca-supply
> +  - vcc-supply
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c1 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      bridge@2d {
> +        compatible = "ti,sn65dsi86";
> +        reg = <0x2d>;
> +
> +        interrupt-parent = <&tlmm>;
> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        enable-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>;
> +
> +        vpll-supply = <&src_pp1800_s4a>;
> +        vccio-supply = <&src_pp1800_s4a>;
> +        vcca-supply = <&src_pp1200_l2a>;
> +        vcc-supply = <&src_pp1200_l2a>;
> +
> +        clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
> +        clock-names = "refclk";
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          port@0 {
> +            reg = <0>;
> +            endpoint {
> +              remote-endpoint = <&dsi0_out>;
> +            };
> +          };
> +
> +          port@1 {
> +            reg = <1>;
> +            endpoint {
> +              remote-endpoint = <&panel_in_edp>;
> +            };
> +          };
> +        };
> +      };
> +    };
> +

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Laurent Pinchart]

On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote:
> Quoting Douglas Anderson (2020-04-15 08:48:40)
> > Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> > example.
> > 
> > NOTE: The current patch adding support for hpd-gpios to the Linux
> > driver for hpd-gpios only adds enough support to the driver so that
> > the bridge can use one of its own GPIOs.  The bindings, however, are
> > written generically.
> > 
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > 
> >  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > index 8cacc6db33a9..554bfd003000 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > @@ -60,6 +60,10 @@ properties:
> >      const: 1
> >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> >  
> > +  hpd-gpios:
> > +    maxItems: 1
> > +    description: If present use the given GPIO for hot-plug-detect.
> 
> Shouldn't this go in the panel node? And the panel driver should get the
> gpio and poll it after powering up the panel? Presumably that's why we
> have the no-hpd property in the simple panel binding vs. putting it here
> in the bridge.

Same question really, I think this belongs to the panel (or connector)
node indeed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Doug Anderson]

Hi,

On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote:
> > Quoting Douglas Anderson (2020-04-15 08:48:40)
> > > Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> > > example.
> > >
> > > NOTE: The current patch adding support for hpd-gpios to the Linux
> > > driver for hpd-gpios only adds enough support to the driver so that
> > > the bridge can use one of its own GPIOs.  The bindings, however, are
> > > written generically.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > >  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > index 8cacc6db33a9..554bfd003000 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > @@ -60,6 +60,10 @@ properties:
> > >      const: 1
> > >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> > >
> > > +  hpd-gpios:
> > > +    maxItems: 1
> > > +    description: If present use the given GPIO for hot-plug-detect.
> >
> > Shouldn't this go in the panel node? And the panel driver should get the
> > gpio and poll it after powering up the panel? Presumably that's why we
> > have the no-hpd property in the simple panel binding vs. putting it here
> > in the bridge.
>
> Same question really, I think this belongs to the panel (or connector)
> node indeed.

Hrm.

To me "no-hpd" feels OK in the panel because the lack of a connection
is somewhat symmetric.  Thus it's OK to say either "HPD isn't hooked
up to the panel in this system" or "HPD isn't hooked up to the bridge
in this system" and both express the same thing (AKA that there is no
HPD connection between the bridge and the panel).  In the case of
"no-hpd" it's more convenient to express it on the panel side because
the panel driver is the one whose behavior has to change if HPD isn't
hooked up.  The panel datasheet is the one that says how long of a
delay we need if HPD isn't hooked up.

...but when you're talking about where the bridge driver should look
to find the HPD signal that it needs, that really feels like it should
be described as part of the bridge.  Specifically imagine we were
using our bridge for DP, not for eDP.  In that case simple-panel
wouldn't be involved because we could get any type of display plugged
in.  Thus it couldn't go in the panel node.  Here it feels clearer
that hpd-gpio needs to be a property of the bridge driver.

Looking at other usages of "hpd-gpio" in the kernel, it seems like the
usage I'm proposing is also common.  Grepping for "hpd-gpios" shows
numerous examples of "hpd-gpios" being defined at the display
controller level and (effectively) I believe the bridge is at the
equivalent level.


-Doug

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Laurent Pinchart]

Hi Doug,

On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote:
> On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote:
> > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote:
> > > Quoting Douglas Anderson (2020-04-15 08:48:40)
> > > > Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> > > > example.
> > > >
> > > > NOTE: The current patch adding support for hpd-gpios to the Linux
> > > > driver for hpd-gpios only adds enough support to the driver so that
> > > > the bridge can use one of its own GPIOs.  The bindings, however, are
> > > > written generically.
> > > >
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > >
> > > >  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > index 8cacc6db33a9..554bfd003000 100644
> > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > @@ -60,6 +60,10 @@ properties:
> > > >      const: 1
> > > >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> > > >
> > > > +  hpd-gpios:
> > > > +    maxItems: 1
> > > > +    description: If present use the given GPIO for hot-plug-detect.
> > >
> > > Shouldn't this go in the panel node? And the panel driver should get the
> > > gpio and poll it after powering up the panel? Presumably that's why we
> > > have the no-hpd property in the simple panel binding vs. putting it here
> > > in the bridge.
> >
> > Same question really, I think this belongs to the panel (or connector)
> > node indeed.
> 
> Hrm.
> 
> To me "no-hpd" feels OK in the panel because the lack of a connection
> is somewhat symmetric.  Thus it's OK to say either "HPD isn't hooked
> up to the panel in this system" or "HPD isn't hooked up to the bridge
> in this system" and both express the same thing (AKA that there is no
> HPD connection between the bridge and the panel).  In the case of
> "no-hpd" it's more convenient to express it on the panel side because
> the panel driver is the one whose behavior has to change if HPD isn't
> hooked up.  The panel datasheet is the one that says how long of a
> delay we need if HPD isn't hooked up.
> 
> ...but when you're talking about where the bridge driver should look
> to find the HPD signal that it needs, that really feels like it should
> be described as part of the bridge.  Specifically imagine we were
> using our bridge for DP, not for eDP.  In that case simple-panel
> wouldn't be involved because we could get any type of display plugged
> in.  Thus it couldn't go in the panel node.  Here it feels clearer
> that hpd-gpio needs to be a property of the bridge driver.

If you were using it for DP, you would need a DT node for the DP
connector (with bindings to be added to
Documentation/devicetree/bindings/display/connector/, similar to the
ones we already have for other connectors). That DT node should
reference the HPD pin GPIO. The bridge driver for the connector
(drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The
good news is that it already does :-)

> Looking at other usages of "hpd-gpio" in the kernel, it seems like the
> usage I'm proposing is also common.  Grepping for "hpd-gpios" shows
> numerous examples of "hpd-gpios" being defined at the display
> controller level and (effectively) I believe the bridge is at the
> equivalent level.

Bridge drivers should only implement support for features available from
the corresponding hardware. If an HPD signal is connected to a dedicated
pin of the bridge, and the bridge can generate an interrupt and expose
the HPD status through I2C, then it should implement HPD-related
operations. If the HPD pin from the connector is hooked up to a GPIO of
the SoC, it should be handled by the connector bridge driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Doug Anderson]

Hi,

On Wed, Apr 15, 2020 at 5:54 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Doug,
>
> On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote:
> > On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote:
> > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote:
> > > > Quoting Douglas Anderson (2020-04-15 08:48:40)
> > > > > Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> > > > > example.
> > > > >
> > > > > NOTE: The current patch adding support for hpd-gpios to the Linux
> > > > > driver for hpd-gpios only adds enough support to the driver so that
> > > > > the bridge can use one of its own GPIOs.  The bindings, however, are
> > > > > written generically.
> > > > >
> > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > ---
> > > > >
> > > > >  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > index 8cacc6db33a9..554bfd003000 100644
> > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > @@ -60,6 +60,10 @@ properties:
> > > > >      const: 1
> > > > >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> > > > >
> > > > > +  hpd-gpios:
> > > > > +    maxItems: 1
> > > > > +    description: If present use the given GPIO for hot-plug-detect.
> > > >
> > > > Shouldn't this go in the panel node? And the panel driver should get the
> > > > gpio and poll it after powering up the panel? Presumably that's why we
> > > > have the no-hpd property in the simple panel binding vs. putting it here
> > > > in the bridge.
> > >
> > > Same question really, I think this belongs to the panel (or connector)
> > > node indeed.
> >
> > Hrm.
> >
> > To me "no-hpd" feels OK in the panel because the lack of a connection
> > is somewhat symmetric.  Thus it's OK to say either "HPD isn't hooked
> > up to the panel in this system" or "HPD isn't hooked up to the bridge
> > in this system" and both express the same thing (AKA that there is no
> > HPD connection between the bridge and the panel).  In the case of
> > "no-hpd" it's more convenient to express it on the panel side because
> > the panel driver is the one whose behavior has to change if HPD isn't
> > hooked up.  The panel datasheet is the one that says how long of a
> > delay we need if HPD isn't hooked up.
> >
> > ...but when you're talking about where the bridge driver should look
> > to find the HPD signal that it needs, that really feels like it should
> > be described as part of the bridge.  Specifically imagine we were
> > using our bridge for DP, not for eDP.  In that case simple-panel
> > wouldn't be involved because we could get any type of display plugged
> > in.  Thus it couldn't go in the panel node.  Here it feels clearer
> > that hpd-gpio needs to be a property of the bridge driver.
>
> If you were using it for DP, you would need a DT node for the DP
> connector (with bindings to be added to
> Documentation/devicetree/bindings/display/connector/, similar to the
> ones we already have for other connectors). That DT node should
> reference the HPD pin GPIO. The bridge driver for the connector
> (drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The
> good news is that it already does :-)

I'm having a really hard time following, but maybe it's because my
knowledge of the DRM terminology is feeble at best?

Looking at it from a DRM driver perspective and thus looking in
'drm/bridge/ti-sn65dsi86.c' I see that the driver for this bridge chip
effectively is both the bridge and the connector.  The struct
encapsulating the driver data has both:

  struct drm_bridge bridge;
  struct drm_connector connector;

...in ti_sn_bridge_attach() the code calls drm_connector_init() for
the connector.

Looking at it from a device tree point of view, there is no separate
node representing an eDP connector for one mainline user of
'ti,sn65dsi86' (sdm845-cheza).  The device tree node has one input
port (from "dsi0_out") and one output port (to "panel_in_edp").  There
is no separate connector node as I can see with "hdmi-connector".
...and, as far as I can tell, sdm845-cheza is using the bindings as
documented.  The bindings say that the 'ti,sn65dsi86' node needs two
ports:
- Video port 0 for DSI input
- Video port 1 for eDP output

So, though I'm probably terribly confused, I would tentatively say that:

- I'd guess that the 'ti,sn65dsi86' bindings were written / approved
back before people were encouraged to model the connector as a
separate node.

- In the case of 'ti,sn65dsi86' the current dts node is both the node
for the bridge and the connector.

- If we want to try to deprecate the way that 'ti,sn65dsi86' works it
feels like a big-ish effort.  This would include adding a new "eDP"
connector class and trying to figure out how to deal with backward
compatibility for old dts files (assuming folks care).

Did I get that right?  If so, maybe my "hpd-gpios" is already part of
the "connector" node?


> > Looking at other usages of "hpd-gpio" in the kernel, it seems like the
> > usage I'm proposing is also common.  Grepping for "hpd-gpios" shows
> > numerous examples of "hpd-gpios" being defined at the display
> > controller level and (effectively) I believe the bridge is at the
> > equivalent level.
>
> Bridge drivers should only implement support for features available from
> the corresponding hardware. If an HPD signal is connected to a dedicated
> pin of the bridge, and the bridge can generate an interrupt and expose
> the HPD status through I2C, then it should implement HPD-related
> operations. If the HPD pin from the connector is hooked up to a GPIO of
> the SoC, it should be handled by the connector bridge driver.

So the case I'm trying to deal with is a little odd.  I tried to spell
it all out in patch #3 [1] but to talk about it here too:

1. The 'ti,sn65dsi86' does have a hardware HPD pin.  That pin can
generate an interrupt.

2. For reasons described in patch #3 (and the other commit it
references, c2bfc223882d), the hardware HPD pin on 'ti,sn65dsi86' is
nearly useless for eDP.  Specifically, eDP panels are usually
(always?) not removable and thus HPD isn't a signal that needs
debouncing.  ...yet the signal is debounced in hardware on
'ti,sn65dsi86' and that means a delay of 100 - 200ms before you can
see the true value of HPD.  That's an extra 100 - 200ms before the
panel can turn on.

3. Even if eDP panels aren't actually hot plugged, HPD is still a
useful concept for eDP.  It can be used to avoid hardcoded delays
since panels use it to signal when they're ready.  ...but if HPD is
debounced that doesn't work so well.

4. 'ti,sn65dsi86' has some pins that can be used as GPIOs.  These are
ideal places to route HPD since they are not debounced and pretty much
a perfect fit for this signal (don't waste SoC GPIOs, routing signals
on your board is easier, pins are powered exactly when you need them).

5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be
polled.  ...but this is OK.  I'm specifically trying to support the
case of a panel that is always connected and I just want HPD to be the
signal that the panel is ready for me to talk to it.  Polling is fine.
Specifically the bridge driver doesn't try to poll HPD to decide if we
have something connected--it always returns
'connector_status_connected'.  ...and this is the correct behavior for
eDP because you know the hardware is always there and HPD won't even
be asserted until you start to power up the panel.

6. My current implementation in patch #3 actually doesn't fully
implement a Linux GPIO provider in the bridge driver.  See that patch
for justification.  While I could do the work to do this and I'll do
it if folks insist, I think the current simpler code is nice.  If
there was a separate "edp-connector" driver then presumably I'd have
to add the complexity of implementing the GPIO provider API.


I guess to summarize my understanding of all the above:

- I think designing and adding a separate 'edp-connector' driver and
device tree node and migrating existing users over would be a big
chunk of work and is out of scope for me.

- I'm hoping that the current approach is still OK.

- If people really like the edp-connector concept and want to try to
adapt old code later there's nothing here that prevents it--it's just
a bunch of extra code.


[1] https://lore.kernel.org/r/20200415084758.3.Ia50267a5549392af8b37e67092ca653a59c95886@changeid

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Laurent Pinchart]

Hi Doug,

On Thu, Apr 16, 2020 at 02:53:18PM -0700, Doug Anderson wrote:
> On Wed, Apr 15, 2020 at 5:54 PM Laurent Pinchart wrote:
> > On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote:
> > > On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote:
> > > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote:
> > > > > Quoting Douglas Anderson (2020-04-15 08:48:40)
> > > > > > Allow people to specify to use a GPIO for hot-plug-detect.  Add an
> > > > > > example.
> > > > > >
> > > > > > NOTE: The current patch adding support for hpd-gpios to the Linux
> > > > > > driver for hpd-gpios only adds enough support to the driver so that
> > > > > > the bridge can use one of its own GPIOs.  The bindings, however, are
> > > > > > written generically.
> > > > > >
> > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  .../bindings/display/bridge/ti,sn65dsi86.yaml          | 10 +++++++++-
> > > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > > index 8cacc6db33a9..554bfd003000 100644
> > > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > > > @@ -60,6 +60,10 @@ properties:
> > > > > >      const: 1
> > > > > >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> > > > > >
> > > > > > +  hpd-gpios:
> > > > > > +    maxItems: 1
> > > > > > +    description: If present use the given GPIO for hot-plug-detect.
> > > > >
> > > > > Shouldn't this go in the panel node? And the panel driver should get the
> > > > > gpio and poll it after powering up the panel? Presumably that's why we
> > > > > have the no-hpd property in the simple panel binding vs. putting it here
> > > > > in the bridge.
> > > >
> > > > Same question really, I think this belongs to the panel (or connector)
> > > > node indeed.
> > >
> > > Hrm.
> > >
> > > To me "no-hpd" feels OK in the panel because the lack of a connection
> > > is somewhat symmetric.  Thus it's OK to say either "HPD isn't hooked
> > > up to the panel in this system" or "HPD isn't hooked up to the bridge
> > > in this system" and both express the same thing (AKA that there is no
> > > HPD connection between the bridge and the panel).  In the case of
> > > "no-hpd" it's more convenient to express it on the panel side because
> > > the panel driver is the one whose behavior has to change if HPD isn't
> > > hooked up.  The panel datasheet is the one that says how long of a
> > > delay we need if HPD isn't hooked up.
> > >
> > > ...but when you're talking about where the bridge driver should look
> > > to find the HPD signal that it needs, that really feels like it should
> > > be described as part of the bridge.  Specifically imagine we were
> > > using our bridge for DP, not for eDP.  In that case simple-panel
> > > wouldn't be involved because we could get any type of display plugged
> > > in.  Thus it couldn't go in the panel node.  Here it feels clearer
> > > that hpd-gpio needs to be a property of the bridge driver.
> >
> > If you were using it for DP, you would need a DT node for the DP
> > connector (with bindings to be added to
> > Documentation/devicetree/bindings/display/connector/, similar to the
> > ones we already have for other connectors). That DT node should
> > reference the HPD pin GPIO. The bridge driver for the connector
> > (drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The
> > good news is that it already does :-)

That's a long e-mail. Taking a deep breath :-)

> I'm having a really hard time following, but maybe it's because my
> knowledge of the DRM terminology is feeble at best?
> 
> Looking at it from a DRM driver perspective and thus looking in
> 'drm/bridge/ti-sn65dsi86.c' I see that the driver for this bridge chip
> effectively is both the bridge and the connector.  The struct
> encapsulating the driver data has both:
> 
>   struct drm_bridge bridge;
>   struct drm_connector connector;
> 
> ...in ti_sn_bridge_attach() the code calls drm_connector_init() for
> the connector.

That is correct, and that's the old mode. With the new model (based on
the DRM_BRIDGE_ATTACH_NO_CONNECTOR flags and the drm_bridge_funcs
.get_modes(), .get_edid(), .detect(), .hpd_enable() and .hpd_disabled()
operations), the ti-sn65dsi86 will not create a drm_connector anymore.
The two modes of operation will be temporarily supported to all display
controller drivers to be migrated one by one to the new model. Once the
conversion of all display controller drivers that use the ti-sn65dsi86
will be complete, creation of a drm_connector will be removed completely
from the ti-sn65dsi86 driver.

> Looking at it from a device tree point of view, there is no separate
> node representing an eDP connector for one mainline user of
> 'ti,sn65dsi86' (sdm845-cheza).  The device tree node has one input
> port (from "dsi0_out") and one output port (to "panel_in_edp").  There
> is no separate connector node as I can see with "hdmi-connector".
> ...and, as far as I can tell, sdm845-cheza is using the bindings as
> documented.  The bindings say that the 'ti,sn65dsi86' node needs two
> ports:
> - Video port 0 for DSI input
> - Video port 1 for eDP output

You are right here. The confusing part is the word "connector".
Historically, a DRM connector models a physical connector (VGA, HDMI,
...). The concept was extended over time with more connector types,
including connectors for display panels (LVDS, DSI, eDP, ...) that are
integrated in the system and can't be hotplugged, unlike the user-facing
VGA or HDMI connectors. A DRM connector thus represents the sink at the
output of a display pipeline.

When dealing with external monitors, they can't be represented in the
device tree (or any other firmware) as they're hot-pluggable. Panels,
however, are different, as they're part of the system, and are thus
described in the firmware (DT in this case). Technically speaking,
there's a physical connector for the panel, but it's irrelevant for the
user, and the end of the display pipeline in the system isn't the
connector, but the panel. For this reason, when dealing with panel,
there's no DT node to represent the connector.

The drm_connector object abstracts either a physical user-facing
connector or a panel, and contains properties that are applicable to
either the panel or the external monitor (such as the format of the
data, the connection type, the physical size of the display, ...). It is
part of the API exposed to userspace.

Inside the kernel, when the need for panel-specific code arose, the
drm_panel object was created. It represents a panel, has properties and
methods specific to panels, and is not exposed to userspace.

> So, though I'm probably terribly confused, I would tentatively say that:
> 
> - I'd guess that the 'ti,sn65dsi86' bindings were written / approved
> back before people were encouraged to model the connector as a
> separate node.

Not exactly. The physical connector should only be modelled as a
separate node when the connector is the output of the display pipeline
for the system. As explained above, when the output of the display
pipeline is a panel, there's no need for a connector DT node.

> - In the case of 'ti,sn65dsi86' the current dts node is both the node
> for the bridge and the connector.

This one is a bit tricky :-) The DT node for the ti,sn65dsi86 represents
the bridge only (it is linked in DT to a separate node for the panel).
The corresponding driver instantiates a drm_bridge and a drm_connector,
and delegates the drm_connector operations to the drm_panel object that
handles the connected panel. This is the model that we're replacing, as
the ti-sn65dsi86 driver should focus on the SN65DSI86 chip only, and
shouldn't handle the connected panel. For all we know, it could be
connected to an eDP to HDMI bridge, itself connected to an HDMI
connector. We now want bridge drivers to focus only on the device they
correspond to.

> - If we want to try to deprecate the way that 'ti,sn65dsi86' works it
> feels like a big-ish effort.  This would include adding a new "eDP"
> connector class and trying to figure out how to deal with backward
> compatibility for old dts files (assuming folks care).

As I explained above (hopefully in a not too confusing way), no change
is required in DT. On the kernel side, the ti-sn65dsi86 should stop
creating a connector. The next component in the pipeline, in your case a
panel, should be handled by a driver of its own, and in this case this
is already possible with the help of drm_panel_bridge_add() (and related
functions) that will wrap the drm_panel in a drm_bridge. This currently
needs to be done by the ti-sn65dsi86 driver, but once all users of
panels will use drm_panel_bridge_add(), I plan to make this handled by
the DRM panel core, transparently for bridge drivers.

For the case where a physical DP connector is present at the end of the
pipeline, connected to an external DP monitor, there shall be a DT node
for the DP connector (we have DT bindings for several connector types,
creating bindings for DP connectors should be straightforward). The
bridge/display-connector.c driver shall be extended to support the new
compatible string, and I expect it won't take much more than

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 4d278573cdb9..27185f324b2d 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -63,10 +63,11 @@ display_connector_detect(struct drm_bridge *bridge)

 	case DRM_MODE_CONNECTOR_Composite:
 	case DRM_MODE_CONNECTOR_SVIDEO:
+	case DRM_MODE_CONNECTOR_DisplayPort:
 	case DRM_MODE_CONNECTOR_VGA:
 	default:
 		/*
-		 * Composite and S-Video connectors have no other detection
+		 * Composite, S-Video and DP connectors have no other detection
 		 * mean than the HPD GPIO. For VGA connectors, even if we have
 		 * an I2C bus, we can't assume that the cable is disconnected
 		 * if drm_probe_ddc fails, as some cables don't wire the DDC
@@ -172,11 +173,12 @@ static int display_connector_probe(struct platform_device *pdev)
 	of_property_read_string(pdev->dev.of_node, "label", &label);

 	/*
-	 * Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide
-	 * edge interrupts, register an interrupt handler.
+	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can
+	 * provide edge interrupts, register an interrupt handler.
 	 */
 	if (type == DRM_MODE_CONNECTOR_DVII ||
-	    type == DRM_MODE_CONNECTOR_HDMIA) {
+	    type == DRM_MODE_CONNECTOR_HDMIA ||
+	    type == DRM_MODE_CONNECTOR_DisplayPort) {
 		conn->hpd_gpio = devm_gpiod_get_optional(&pdev->dev, "hpd",
 							 GPIOD_IN);
 		if (IS_ERR(conn->hpd_gpio)) {
@@ -263,6 +265,9 @@ static const struct of_device_id display_connector_match[] = {
 	{
 		.compatible = "composite-video-connector",
 		.data = (void *)DRM_MODE_CONNECTOR_Composite,
+	}, {
+		.compatible = "dp-connector",
+		.data = (void *)DRM_MODE_CONNECTOR_DisplayPort,
 	}, {
 		.compatible = "dvi-connector",
 		.data = (void *)DRM_MODE_CONNECTOR_DVII,

With this, the display-connector driver will create a drm_bridge
instance for the DP connector. The ti-sn65dsi86 will thus always be
followed by a bridge, either a bridge wrapping a drm_panel, or a bridge
for the connector.

Once all pipelines are fully made of bridges until the last component,
the drm_connector shall be created by the display controller driver,
which shall pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to the bridges
to prevent them from creating a drm_connector themselves. The
drm_connector created by the display controller driver will need to
implement operations such as .get_modes(), which will be delegated to
the appropriate bridge in the chain. The drm_bridge_connector_init()
helper automates this, making it straightforward for the display
controller driver to create such a drm_connector. As explained in the
helper's documentation,

 * To use the helper, display controller drivers create a bridge connector with
 * a call to drm_bridge_connector_init(). This associates the newly created
 * connector with the chain of bridges passed to the function and registers it
 * with the DRM device. At that point the connector becomes fully usable, no
 * further operation is needed.

> Did I get that right?  If so, maybe my "hpd-gpios" is already part of
> the "connector" node?

You got it mostly right :-)

As for the hpd-gpios, it should be specified in the DT node of the
component that provides the HPD signal, and contain a GPIO specifier
describing what the signal is connected to. When dealing with a physical
DP connector and external monitor, the HPD signal is provided by the DP
connector, the hpd-gpios property shall then be specified in the DP
connector DT node. The display-connector driver already handles that
property. When dealing with an eDP panel, the HPD signal is provided by
the panel, the hpd-gpios property shall be specified in the panel DT
node.

> > > Looking at other usages of "hpd-gpio" in the kernel, it seems like the
> > > usage I'm proposing is also common.  Grepping for "hpd-gpios" shows
> > > numerous examples of "hpd-gpios" being defined at the display
> > > controller level and (effectively) I believe the bridge is at the
> > > equivalent level.
> >
> > Bridge drivers should only implement support for features available from
> > the corresponding hardware. If an HPD signal is connected to a dedicated
> > pin of the bridge, and the bridge can generate an interrupt and expose
> > the HPD status through I2C, then it should implement HPD-related
> > operations. If the HPD pin from the connector is hooked up to a GPIO of
> > the SoC, it should be handled by the connector bridge driver.
> 
> So the case I'm trying to deal with is a little odd.  I tried to spell
> it all out in patch #3 [1] but to talk about it here too:
> 
> 1. The 'ti,sn65dsi86' does have a hardware HPD pin.  That pin can
> generate an interrupt.

As the SN65DSI86 has native HPD detect capability with a dedicated HPD
input (note that this doesn't make the SN65DSI86 a providder of the HPD
signal in the sense described above), the bridge driver, in the new
model, shall implement the HPD-related operations and the .detect()
operation. The drm_bridge_connector_init() helper will then delegate HPD
and detection to the ti-sn65dsi86 driver.

This assumes that the HPD signal is routed to the SN65DSI86. That's not
always the case, see below for an explanation of how to handle that.

> 2. For reasons described in patch #3 (and the other commit it
> references, c2bfc223882d), the hardware HPD pin on 'ti,sn65dsi86' is
> nearly useless for eDP.  Specifically, eDP panels are usually
> (always?) not removable and thus HPD isn't a signal that needs
> debouncing.  ...yet the signal is debounced in hardware on
> 'ti,sn65dsi86' and that means a delay of 100 - 200ms before you can
> see the true value of HPD.  That's an extra 100 - 200ms before the
> panel can turn on.
> 
> 3. Even if eDP panels aren't actually hot plugged, HPD is still a
> useful concept for eDP.  It can be used to avoid hardcoded delays
> since panels use it to signal when they're ready.  ...but if HPD is
> debounced that doesn't work so well.
> 
> 4. 'ti,sn65dsi86' has some pins that can be used as GPIOs.  These are
> ideal places to route HPD since they are not debounced and pretty much
> a perfect fit for this signal (don't waste SoC GPIOs, routing signals
> on your board is easier, pins are powered exactly when you need them).

The new drm_bridge model has support for this use case. It makes a
difference between the intrinsic capability of a device to provide a
feature (for instance the SN65DSI86 has the intrinsic capability to
provide the HPD feature), and the fact that the feature is actually
provided by that device on a particular system (in the case you describe
here, the SN65DSI86 intrinsic HPD capability isn't used, as the HPD
signal isn't connect to the SN65DSI86 HPD input). The former is reported
by implementing the corresponding drm_bridge_funcs operations, the
latter is reported by setting DRM_BRIGE_OP_* flags in drm_bridge.ops.
This mechanism allows bridge drivers to unconditionally set function
pointers in their drm_bridge_funcs structure (allowing the structure to
make stored in read-only memory), while exposing, for each device
instance, whether the feature is actually provided or not.

The drm_bridge_connector_init() helper, to delegate drm_connector
operations to bridges, will look for the first bridge in the chain,
starting at the output of the pipeline (connector or panel), that
supports the corresponding feature. If your DP connector DT node, or
your eDP connector DT node, specifies that the HPD signal is routed to a
GPIO (through the hpd-gpios property), then the corresponding bridge
driver shall reprot the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD
capabilities. The display-connector driver already supports this, the
panel bridge driver doesn't and needs to be extended. The
drm_bridge_connector_init() helper will then detect that the drm_bridge
for the DP connector or eDP panel supports HPD, and will delegate the
related drm_connector operations to that bridge. If the HPD signal is
routed to the HPD pin of the SN65DSI86, the DP connector or eDP panel DT
node should not contain an hpd-gpios property, the corresponding
drm_bridge will not set DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD, and
the drm_bridge_connector_init() will look at the next component in the
next bridge in the chain, which will be the ti-sn65dsi86. That bridge
will report support for the HPD-related operations, and will be used.

To be fully correct the ti-sn65dsi86 shouldn't set the
DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD flags when the HPD signal
isn't routed to its HPD input pin. As it should not peek into the DT
node of the DP connector or eDP panel for its output, it should have an
additional no-hpd DT property in this case. In practice that's may not
always be required, as if an hpd-gpios property is specified in the DP
connector or eDP panel DT node, the drm_bridge_connector_init() will not
look further, but for the case where the HPD signal isn't routed
anywhere, we need to make sure that the ti-sn65dsi86 driver will not
incorrectly advertise HPD support.

> 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be
> polled.  ...but this is OK.  I'm specifically trying to support the
> case of a panel that is always connected and I just want HPD to be the
> signal that the panel is ready for me to talk to it.  Polling is fine.
> Specifically the bridge driver doesn't try to poll HPD to decide if we
> have something connected--it always returns
> 'connector_status_connected'.  ...and this is the correct behavior for
> eDP because you know the hardware is always there and HPD won't even
> be asserted until you start to power up the panel.

If you look at bridge/display-connector.c, you will see that it reports
DRM_BRIDGE_OP_DETECT if there's an hpd-gpios property, and additionally
reports DRM_BRIDGE_OP_HPD if that GPIO has interrupt capability. If a
bridge in the pipeline reports DRM_BRIDGE_OP_DETECT but no bridge
reports DRM_BRIDGE_OP_HPD, drm_bridge_connector_init() creates a
connector that uses polling. This is another reason why a no-hpd
property is needed for the ti,sn65dsi86, as otherwise the helper would
incorrectly consider that the SN65DSI86 will report HPD through an
interrupt.

> 6. My current implementation in patch #3 actually doesn't fully
> implement a Linux GPIO provider in the bridge driver.  See that patch
> for justification.  While I could do the work to do this and I'll do
> it if folks insist, I think the current simpler code is nice.  If
> there was a separate "edp-connector" driver then presumably I'd have
> to add the complexity of implementing the GPIO provider API.

This is the only reason why I don't like asking you to change your
implementation, due to the additional complexity required to expose a
GPIO provider. However, I think that the new bridge usage model is much
cleaner than the current one, and this justifies in my opinion
additional complexity in a small number of places, even if it's
unfortunate. That being said, if we can put the DT properties where they
belong for the new model with isolated bridge drivers to only handle the
features of the hardware they correspond to, I wouldn't be opposed to a
localized hack (without any derogatory meaning implied) on the driver
side to ease the implementation. I'm willing to look at you at how this
could be done, once we complete this discussion about the new model,
with the hard rule that DT bindings should be designed based on the new
model.

> I guess to summarize my understanding of all the above:
> 
> - I think designing and adding a separate 'edp-connector' driver and
> device tree node and migrating existing users over would be a big
> chunk of work and is out of scope for me.
> 
> - I'm hoping that the current approach is still OK.
> 
> - If people really like the edp-connector concept and want to try to
> adapt old code later there's nothing here that prevents it--it's just
> a bunch of extra code.
> 
> 
> [1] https://lore.kernel.org/r/20200415084758.3.Ia50267a5549392af8b37e67092ca653a59c95886@changeid

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml

[Doug Anderson]

Hi,

On Wed, Apr 15, 2020 at 1:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Douglas,
>
> Thank you for the patch.
>
> On Wed, Apr 15, 2020 at 08:48:39AM -0700, Douglas Anderson wrote:
> > This moves the bindings over, based a lot on toshiba,tc358768.yaml.
> > Unless there's someone known to be better, I've set the maintainer in
> > the yaml as the first person to submit bindings.
>
> You can also add your name ;-)

Sure, though I spend most of my days flitting from subsystem to
subsystem, always a noob everywhere I go.  I'm not sure I'd really be
qualified.  ;-)  If you want, I can add myself though I'd rather not
be solely responsible for this file since I probably won't be the best
at keeping track of it...

I left this as-is for v2 but can change it if you want.


> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  .../bindings/display/bridge/ti,sn65dsi86.txt  |  87 --------
> >  .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++
> >  2 files changed, 188 insertions(+), 87 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> > deleted file mode 100644
> > index 8ec4a7f2623a..000000000000
> > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> > +++ /dev/null
> > @@ -1,87 +0,0 @@
> > -SN65DSI86 DSI to eDP bridge chip
> > ---------------------------------
> > -
> > -This is the binding for Texas Instruments SN65DSI86 bridge.
> > -http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> > -
> > -Required properties:
> > -- compatible: Must be "ti,sn65dsi86"
> > -- reg: i2c address of the chip, 0x2d as per datasheet
> > -- enable-gpios: gpio specification for bridge_en pin (active high)
> > -
> > -- vccio-supply: A 1.8V supply that powers up the digital IOs.
> > -- vpll-supply: A 1.8V supply that powers up the displayport PLL.
> > -- vcca-supply: A 1.2V supply that powers up the analog circuits.
> > -- vcc-supply: A 1.2V supply that powers up the digital core.
> > -
> > -Optional properties:
> > -- interrupts-extended: Specifier for the SN65DSI86 interrupt line.
> > -
> > -- gpio-controller: Marks the device has a GPIO controller.
> > -- #gpio-cells    : Should be two. The first cell is the pin number and
> > -                   the second cell is used to specify flags.
> > -                   See ../../gpio/gpio.txt for more information.
> > -- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of
> > -               the cell formats.
> > -
> > -- clock-names: should be "refclk"
> > -- clocks: Specification for input reference clock. The reference
> > -       clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
> > -
> > -- data-lanes: See ../../media/video-interface.txt
> > -- lane-polarities: See ../../media/video-interface.txt
> > -
> > -- suspend-gpios: specification for GPIO1 pin on bridge (active low)
>
> Where has suspend-gpios gone ? :-)

Oops.  Added it back.


> > -
> > -Required nodes:
> > -This device has two video ports. Their connections are modelled using the
> > -OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> > -
> > -- Video port 0 for DSI input
> > -- Video port 1 for eDP output
> > -
> > -Example
> > --------
> > -
> > -edp-bridge@2d {
> > -     compatible = "ti,sn65dsi86";
> > -     #address-cells = <1>;
> > -     #size-cells = <0>;
> > -     reg = <0x2d>;
> > -
> > -     enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
> > -     suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>;
> > -
> > -     interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>;
> > -
> > -     vccio-supply = <&pm8916_l17>;
> > -     vcca-supply = <&pm8916_l6>;
> > -     vpll-supply = <&pm8916_l17>;
> > -     vcc-supply = <&pm8916_l6>;
> > -
> > -     clock-names = "refclk";
> > -     clocks = <&input_refclk>;
> > -
> > -     ports {
> > -             #address-cells = <1>;
> > -             #size-cells = <0>;
> > -
> > -             port@0 {
> > -                     reg = <0>;
> > -
> > -                     edp_bridge_in: endpoint {
> > -                             remote-endpoint = <&dsi_out>;
> > -                     };
> > -             };
> > -
> > -             port@1 {
> > -                     reg = <1>;
> > -
> > -                     edp_bridge_out: endpoint {
> > -                             data-lanes = <2 1 3 0>;
> > -                             lane-polarities = <0 1 0 1>;
> > -                             remote-endpoint = <&edp_panel_in>;
> > -                     };
> > -             };
> > -     };
> > -}
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > new file mode 100644
> > index 000000000000..8cacc6db33a9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > @@ -0,0 +1,188 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SN65DSI86 DSI to eDP bridge chip
> > +
> > +maintainers:
> > +  - Sandeep Panda <spanda@codeaurora.org>
> > +
> > +description: |
> > +  The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP.
> > +  http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: ti,sn65dsi86
> > +
> > +  reg:
> > +    const: 0x2d
> > +
> > +  enable-gpios:
> > +    maxItems: 1
> > +    description: GPIO specification for bridge_en pin (active high).
> > +
> > +  vccio-supply:
> > +    description: A 1.8V supply that powers up the digital IOs.
> > +
> > +  vpll-supply:
> > +    description: A 1.8V supply that powers up the DisplayPort PLL.
> > +
> > +  vcca-supply:
> > +    description: A 1.2V supply that powers up the analog circuits.
> > +
> > +  vcc-supply:
> > +    description: A 1.2V supply that powers up the digital core.
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description:
> > +      Specification for input reference clock. The reference clock rate must
> > +      be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz.
> > +
> > +  clock-names:
> > +    const: refclk
> > +
> > +  gpio-controller: true
> > +  '#gpio-cells':
> > +    const: 2
> > +    description:
> > +      First cell is pin number, second cell is flags.  GPIO pin numbers are
> > +      1-based to match the datasheet.  See ../../gpio/gpio.txt for more
> > +      information.
> > +
> > +  '#pwm-cells':
> > +    const: 1
> > +    description: See ../../pwm/pwm.yaml for description of the cell formats.
> > +
> > +  data-lanes:
>
> Should this have
>
>         minItems: 1
>         maxItems: 4
>         items:
>           enum:
>             - 0
>             - 1
>             - 2
>             - 3

Interestingly this seemed to be at totally the wrong location in the
old ".txt" file and in my v1.  I moved it to the right place now by
making sure I put the old example back in, not just the example of
what's currently in the tree.


> > +    description: See ../../media/video-interface.txt
> > +
> > +  lane-polarities:
> > +    description: See ../../media/video-interface.txt
>
> And something similar here,
>
>         minItems: 1
>         maxItems: 4
>         items:
>           enum:
>             - 0
>             - 1
>         uniqueItems: false
>
> I'm not entirely sure where uniqueItems should be placed.

I left this out of v2.  "uniqueItems: false" appears to be the default
so it would only be needed if you were trying to override someone who
had already made this "true".  I tested this by setting it to true and
seeing the error, then removing the set to true and seeing the error
gone.


> I'm also not
> sure how to specify that both data-lanes and lane-polarities need to
> have the same number of items, maybe
>
> dependencies:
>   data-lanes: [lane-polarities]

The opposite of that is interesting, that is:

dependencies:
  data-lanes: [lane-polarities]

...that seems to say that specifying "lane-polarities" without
"data-lanes" is an error.

...but that doesn't specify that data-lanes and lane-polarities need
to have the same number of items.  If someone wants to provide the
syntax I'm happy to add it, otherwise it feels like it could be
something to improve in the future.  In general I haven't seen people
get to this level of detail in yaml.


> > +
> > +  ports:
> > +    type: object
> > +
> > +    properties:
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +      port@0:
> > +        type: object
> > +        additionalProperties: false
> > +
> > +        description:
> > +          Video port for MIPI DSI input
> > +
> > +        properties:
> > +          reg:
> > +            const: 0
> > +
> > +        patternProperties:
> > +          endpoint:
>
> If there's a single endpoint, you don't need patternProperties, it can
> be specified in properties.

Done.

Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

[Doug Anderson]

Hi,

On Fri, Apr 17, 2020 at 11:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> As for the hpd-gpios, it should be specified in the DT node of the
> component that provides the HPD signal, and contain a GPIO specifier
> describing what the signal is connected to. When dealing with a physical
> DP connector and external monitor, the HPD signal is provided by the DP
> connector, the hpd-gpios property shall then be specified in the DP
> connector DT node. The display-connector driver already handles that
> property. When dealing with an eDP panel, the HPD signal is provided by
> the panel, the hpd-gpios property shall be specified in the panel DT
> node.

OK, patch posted to add "hpd-gpios" to "panel-common.yaml" which is I
think the summary of what you're saying above.

I _think_ this also means that I need to add support to panel-simple.c
for it so I've posted got a patch for that.  If I followed your whole
description of the future plans it might eventually move somewhere
else but we're not there yet.  If I screwed this up hopefully it's OK
to continue the conversation in v2.  It seemed nice to have code to
talk about.


> As the SN65DSI86 has native HPD detect capability with a dedicated HPD
> input (note that this doesn't make the SN65DSI86 a providder of the HPD
> signal in the sense described above), the bridge driver, in the new
> model, shall implement the HPD-related operations and the .detect()
> operation. The drm_bridge_connector_init() helper will then delegate HPD
> and detection to the ti-sn65dsi86 driver.

I guess this assumes that anyone ever uses it.  Right now the driver
hardcodes HPD to be off and it seems hard for me to imagine anyone
would have a real use for the hardware line given the terrible
debouncing.  Maybe a panel whose hardcoded delay is super bad?


> The new drm_bridge model has support for this use case. It makes a
> difference between the intrinsic capability of a device to provide a
> feature (for instance the SN65DSI86 has the intrinsic capability to
> provide the HPD feature), and the fact that the feature is actually
> provided by that device on a particular system (in the case you describe
> here, the SN65DSI86 intrinsic HPD capability isn't used, as the HPD
> signal isn't connect to the SN65DSI86 HPD input). The former is reported
> by implementing the corresponding drm_bridge_funcs operations, the
> latter is reported by setting DRM_BRIGE_OP_* flags in drm_bridge.ops.
> This mechanism allows bridge drivers to unconditionally set function
> pointers in their drm_bridge_funcs structure (allowing the structure to
> make stored in read-only memory), while exposing, for each device
> instance, whether the feature is actually provided or not.
>
> The drm_bridge_connector_init() helper, to delegate drm_connector
> operations to bridges, will look for the first bridge in the chain,
> starting at the output of the pipeline (connector or panel), that
> supports the corresponding feature. If your DP connector DT node, or
> your eDP connector DT node, specifies that the HPD signal is routed to a
> GPIO (through the hpd-gpios property), then the corresponding bridge
> driver shall reprot the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD
> capabilities. The display-connector driver already supports this, the
> panel bridge driver doesn't and needs to be extended. The
> drm_bridge_connector_init() helper will then detect that the drm_bridge
> for the DP connector or eDP panel supports HPD, and will delegate the
> related drm_connector operations to that bridge. If the HPD signal is
> routed to the HPD pin of the SN65DSI86, the DP connector or eDP panel DT
> node should not contain an hpd-gpios property, the corresponding
> drm_bridge will not set DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD, and
> the drm_bridge_connector_init() will look at the next component in the
> next bridge in the chain, which will be the ti-sn65dsi86. That bridge
> will report support for the HPD-related operations, and will be used.
>
> To be fully correct the ti-sn65dsi86 shouldn't set the
> DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD flags when the HPD signal
> isn't routed to its HPD input pin. As it should not peek into the DT
> node of the DP connector or eDP panel for its output, it should have an
> additional no-hpd DT property in this case. In practice that's may not
> always be required, as if an hpd-gpios property is specified in the DP
> connector or eDP panel DT node, the drm_bridge_connector_init() will not
> look further, but for the case where the HPD signal isn't routed
> anywhere, we need to make sure that the ti-sn65dsi86 driver will not
> incorrectly advertise HPD support.

Sounds like you've thought out a lot of the corner cases!

Right now the 'ti-sn65dsi86' driver is hardcoded not to look at HPD
but its bindings doesn't have the 'no-hpd' property.  Sounds like that
should be OK-ish as long as the panel either has "hpd-gpios" or
"no-hpd" because then nobody will actually query the bridge.  ...but
it would be cleaner to add it.


> > 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be
> > polled.  ...but this is OK.  I'm specifically trying to support the
> > case of a panel that is always connected and I just want HPD to be the
> > signal that the panel is ready for me to talk to it.  Polling is fine.
> > Specifically the bridge driver doesn't try to poll HPD to decide if we
> > have something connected--it always returns
> > 'connector_status_connected'.  ...and this is the correct behavior for
> > eDP because you know the hardware is always there and HPD won't even
> > be asserted until you start to power up the panel.
>
> If you look at bridge/display-connector.c, you will see that it reports
> DRM_BRIDGE_OP_DETECT if there's an hpd-gpios property, and additionally
> reports DRM_BRIDGE_OP_HPD if that GPIO has interrupt capability. If a
> bridge in the pipeline reports DRM_BRIDGE_OP_DETECT but no bridge
> reports DRM_BRIDGE_OP_HPD, drm_bridge_connector_init() creates a
> connector that uses polling. This is another reason why a no-hpd
> property is needed for the ti,sn65dsi86, as otherwise the helper would
> incorrectly consider that the SN65DSI86 will report HPD through an
> interrupt.

Hrm.  I guess technically it breaks bindings compatibility that
"no-hpd" wasn't there before but there's something that will break if
we don't specify it.  ...but it won't break anything until someone
actually tries to add DRM_BRIDGE_OP_HPD to ti-sn65dsi86.  Maybe we're
OK as long as we fix it before then?

I've put this in v2 so we can discuss.


> > 6. My current implementation in patch #3 actually doesn't fully
> > implement a Linux GPIO provider in the bridge driver.  See that patch
> > for justification.  While I could do the work to do this and I'll do
> > it if folks insist, I think the current simpler code is nice.  If
> > there was a separate "edp-connector" driver then presumably I'd have
> > to add the complexity of implementing the GPIO provider API.
>
> This is the only reason why I don't like asking you to change your
> implementation, due to the additional complexity required to expose a
> GPIO provider. However, I think that the new bridge usage model is much
> cleaner than the current one, and this justifies in my opinion
> additional complexity in a small number of places, even if it's
> unfortunate. That being said, if we can put the DT properties where they
> belong for the new model with isolated bridge drivers to only handle the
> features of the hardware they correspond to, I wouldn't be opposed to a
> localized hack (without any derogatory meaning implied) on the driver
> side to ease the implementation. I'm willing to look at you at how this
> could be done, once we complete this discussion about the new model,
> with the hard rule that DT bindings should be designed based on the new
> model.

OK, I managed to implement the GPIO controller.  Let's see how it
looks.  I threw GPIO folks on the series too so hopefully they can
tell me if I'm doing something stupid.


-Doug