[PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors

[PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors

[Kieran Bingham]

Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
provide IRQ based hotplug detect to identify when the connector is
present.

no-hpd is extended to be the default behaviour for non DisplayPort
connectors.

This series is based on top of José Expósito's patch [0] "drm/bridge:
ti-sn65dsi86: switch to devm_drm_of_get_bridge" and Nikita Yushchenko's
patch [1] "drm/bridge_connector: enable HPD by default if supported".

[0] https://lore.kernel.org/all/20220228183955.25508-1-jose.exposito89@gmail.com/
[1] https://lore.kernel.org/all/20211225063151.2110878-1-nikita.yoush@cogentembedded.com/

Kieran Bingham (1):
  drm/bridge: ti-sn65dsi86: Support hotplug detection

Laurent Pinchart (3):
  drm/bridge: ti-sn65dsi86: Implement bridge connector operations
  drm/bridge: ti-sn65dsi86: Make connector creation optional
  drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 178 ++++++++++++++++++++++----
 1 file changed, 156 insertions(+), 22 deletions(-)

-- 
2.32.0

[PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors

[Kieran Bingham]

Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
provide IRQ based hotplug detect to identify when the connector is
present.

no-hpd is extended to be the default behaviour for non DisplayPort
connectors.

This series is based on top of José Expósito's patch [0] "drm/bridge:
ti-sn65dsi86: switch to devm_drm_of_get_bridge" and Nikita Yushchenko's
patch [1] "drm/bridge_connector: enable HPD by default if supported".

[0] https://lore.kernel.org/all/20220228183955.25508-1-jose.exposito89@gmail.com/
[1] https://lore.kernel.org/all/20211225063151.2110878-1-nikita.yoush@cogentembedded.com/

Kieran Bingham (1):
  drm/bridge: ti-sn65dsi86: Support hotplug detection

Laurent Pinchart (3):
  drm/bridge: ti-sn65dsi86: Implement bridge connector operations
  drm/bridge: ti-sn65dsi86: Make connector creation optional
  drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 178 ++++++++++++++++++++++----
 1 file changed, 156 insertions(+), 22 deletions(-)

-- 
2.32.0

[PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Implement the bridge connector-related .get_edid() operation, and report
the related bridge capabilities and type.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:

- The connector .get_modes() operation doesn't rely on EDID anymore,
  __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
  together

Notes from Kieran:

RB Tags collected from:
 https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/

However this was over a year ago, so let me know if other patches now
superceed this one or otherwise invalidate this update.

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c55848588123..ffb6c04f6c46 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+					  struct drm_connector *connector)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	struct edid *edid;
+
+	pm_runtime_get_sync(pdata->dev);
+	edid = drm_get_edid(connector, &pdata->aux.ddc);
+	pm_runtime_put_autosuspend(pdata->dev);
+
+	return edid;
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
@@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
 	.post_disable = ti_sn_bridge_post_disable,
+	.get_edid = ti_sn_bridge_get_edid,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
@@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
+	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.32.0

[PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Implement the bridge connector-related .get_edid() operation, and report
the related bridge capabilities and type.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:

- The connector .get_modes() operation doesn't rely on EDID anymore,
  __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
  together

Notes from Kieran:

RB Tags collected from:
 https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/

However this was over a year ago, so let me know if other patches now
superceed this one or otherwise invalidate this update.

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c55848588123..ffb6c04f6c46 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+					  struct drm_connector *connector)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	struct edid *edid;
+
+	pm_runtime_get_sync(pdata->dev);
+	edid = drm_get_edid(connector, &pdata->aux.ddc);
+	pm_runtime_put_autosuspend(pdata->dev);
+
+	return edid;
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
@@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
 	.post_disable = ti_sn_bridge_post_disable,
+	.get_edid = ti_sn_bridge_get_edid,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
@@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
+	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.32.0

[PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Now that the driver supports the connector-related bridge operations,
make the connector creation optional. This enables usage of the
sn65dsi86 with the DRM bridge connector helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
Changes since v1:
 - None

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ffb6c04f6c46..29f5f7123ed9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -745,11 +745,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
 	pdata->aux.drm_dev = bridge->dev;
 	ret = drm_dp_aux_register(&pdata->aux);
 	if (ret < 0) {
@@ -757,12 +752,14 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
-	ret = ti_sn_bridge_connector_init(pdata);
-	if (ret < 0)
-		goto err_conn_init;
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		ret = ti_sn_bridge_connector_init(pdata);
+		if (ret < 0)
+			goto err_conn_init;
 
-	/* We never want the next bridge to *also* create a connector: */
-	flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+		/* We never want the next bridge to *also* create a connector: */
+		flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+	}
 
 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
-- 
2.32.0

[PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Now that the driver supports the connector-related bridge operations,
make the connector creation optional. This enables usage of the
sn65dsi86 with the DRM bridge connector helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
Changes since v1:
 - None

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ffb6c04f6c46..29f5f7123ed9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -745,11 +745,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
 	pdata->aux.drm_dev = bridge->dev;
 	ret = drm_dp_aux_register(&pdata->aux);
 	if (ret < 0) {
@@ -757,12 +752,14 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
-	ret = ti_sn_bridge_connector_init(pdata);
-	if (ret < 0)
-		goto err_conn_init;
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		ret = ti_sn_bridge_connector_init(pdata);
+		if (ret < 0)
+			goto err_conn_init;
 
-	/* We never want the next bridge to *also* create a connector: */
-	flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+		/* We never want the next bridge to *also* create a connector: */
+		flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+	}
 
 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
-- 
2.32.0

[PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is detected as a DisplayPort
connector, and disable eDP features in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reworked to set bridge type based on the next bridge/connector.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
--
Changes since v1/RFC:
 - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
   devm_drm_of_get_bridge"
 - eDP/DP mode determined from the next bridge connector type.

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 29f5f7123ed9..461f963faa0b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -60,6 +60,7 @@
 #define SN_LN_ASSIGN_REG			0x59
 #define  LN_ASSIGN_WIDTH			2
 #define SN_ENH_FRAME_REG			0x5A
+#define  ASSR_CONTROL				BIT(0)
 #define  VSTREAM_ENABLE				BIT(3)
 #define  LN_POLRS_OFFSET			4
 #define  LN_POLRS_MASK				0xf0
@@ -91,6 +92,8 @@
 #define SN_DATARATE_CONFIG_REG			0x94
 #define  DP_DATARATE_MASK			GENMASK(7, 5)
 #define  DP_DATARATE(x)				((x) << 5)
+#define SN_TRAINING_SETTING_REG			0x95
+#define  SCRAMBLE_DISABLE			BIT(4)
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
@@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
 			   DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
 
+	/* For DisplayPort, use the standard DP scrambler seed. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+				   ASSR_CONTROL, 0);
+
 	/* enable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
 
@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 		goto exit;
 	}
 
+	/* For DisplayPort, disable scrambling mode. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+				   SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+
 	/*
 	 * We'll try to link train several times.  As part of link training
 	 * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
@@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
 	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
-	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
+	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
+			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.32.0

[PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is detected as a DisplayPort
connector, and disable eDP features in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reworked to set bridge type based on the next bridge/connector.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
--
Changes since v1/RFC:
 - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
   devm_drm_of_get_bridge"
 - eDP/DP mode determined from the next bridge connector type.

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 29f5f7123ed9..461f963faa0b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -60,6 +60,7 @@
 #define SN_LN_ASSIGN_REG			0x59
 #define  LN_ASSIGN_WIDTH			2
 #define SN_ENH_FRAME_REG			0x5A
+#define  ASSR_CONTROL				BIT(0)
 #define  VSTREAM_ENABLE				BIT(3)
 #define  LN_POLRS_OFFSET			4
 #define  LN_POLRS_MASK				0xf0
@@ -91,6 +92,8 @@
 #define SN_DATARATE_CONFIG_REG			0x94
 #define  DP_DATARATE_MASK			GENMASK(7, 5)
 #define  DP_DATARATE(x)				((x) << 5)
+#define SN_TRAINING_SETTING_REG			0x95
+#define  SCRAMBLE_DISABLE			BIT(4)
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
@@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
 			   DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
 
+	/* For DisplayPort, use the standard DP scrambler seed. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+				   ASSR_CONTROL, 0);
+
 	/* enable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
 
@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 		goto exit;
 	}
 
+	/* For DisplayPort, disable scrambling mode. */
+	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+				   SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+
 	/*
 	 * We'll try to link train several times.  As part of link training
 	 * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
@@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
 	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
-	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
+	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
+			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.32.0

[PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Kieran Bingham]

When the SN65DSI86 is used in DisplayPort mode, its output is likely
routed to a DisplayPort connector, which can benefit from hotplug
detection. Support it in such cases, with polling mode only for now.

The implementation is limited to the bridge operations, as the connector
operations are legacy and new users should use
DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:

- Document the no_hpd field
- Rely on the SN_HPD_DISABLE_REG default value in the HPD case
- Add a TODO comment regarding IRQ support
[Kieran]
- Fix spelling s/assrted/asserted/
- Only enable HPD on DisplayPort connector.
- Support IRQ based hotplug detect
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 133 +++++++++++++++++++++++---
 1 file changed, 120 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 461f963faa0b..febb4e672ece 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -68,6 +68,7 @@
 #define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
+#define  HPD_DEBOUNCED_STATE			BIT(4)
 #define SN_GPIO_IO_REG				0x5E
 #define  SN_GPIO_INPUT_SHIFT			4
 #define  SN_GPIO_OUTPUT_SHIFT			0
@@ -104,10 +105,24 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+#define SN_IRQ_HPD_REG				0xE6
+#define  IRQ_HPD_EN				BIT(0)
+#define  IRQ_HPD_INSERTION_EN			BIT(1)
+#define  IRQ_HPD_REMOVAL_EN			BIT(2)
+#define  IRQ_HPD_REPLUG_EN			BIT(3)
+#define  IRQ_HPD_PLL_UNLOCK_EN			BIT(5)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_HPD_STATUS_REG			0xF5
+#define  IRQ_HPD_STATUS				BIT(0)
+#define  IRQ_HPD_INSERTION_STATUS		BIT(1)
+#define  IRQ_HPD_REMOVAL_STATUS			BIT(2)
+#define  IRQ_HPD_REPLUG_STATUS			BIT(3)
+#define  IRQ_PLL_UNLOCK				BIT(5)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -166,6 +181,11 @@
  * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
  * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
  * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ *
+ * @no_hpd:       Disable hot-plug detection as instructed by device tree (used
+ *                for instance for eDP panels whose HPD signal won't be asserted
+ *                until the panel is turned on, and is thus not usable for
+ *                downstream device detection).
  */
 struct ti_sn65dsi86 {
 	struct auxiliary_device		bridge_aux;
@@ -200,6 +220,8 @@ struct ti_sn65dsi86 {
 	atomic_t			pwm_pin_busy;
 #endif
 	unsigned int			pwm_refclk_freq;
+
+	bool				no_hpd;
 };
 
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -314,23 +336,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
 	ti_sn_bridge_set_refclk_freq(pdata);
 
 	/*
-	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
-	 * so the HPD is an internal signal that's only there to signal that
-	 * the panel is done powering up.  ...but the bridge chip debounces
-	 * this signal by between 100 ms and 400 ms (depending on process,
-	 * voltage, and temperate--I measured it at about 200 ms).  One
+	 * As this is an eDP bridge, the output will be connected to a fixed
+	 * panel in most systems. HPD is in that case only an internal signal
+	 * to signal that the panel is done powering up. The bridge chip
+	 * debounces this signal by between 100 ms and 400 ms (depending on
+	 * process, voltage, and temperate--I measured it at about 200 ms). One
 	 * particular panel asserted HPD 84 ms after it was powered on meaning
 	 * that we saw HPD 284 ms after power on.  ...but the same panel said
 	 * that instead of looking at HPD you could just hardcode a delay of
-	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
-	 * delay in its prepare and always disable HPD.
+	 * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
+	 * assume that the panel driver will have the hardcoded delay in its
+	 * prepare and always disable HPD.
 	 *
-	 * If HPD somehow makes sense on some future panel we'll have to
-	 * change this to be conditional on someone specifying that HPD should
-	 * be used.
+	 * However, on some systems, the output is connected to a DisplayPort
+	 * connector. HPD is needed in such cases. To accommodate both use
+	 * cases, enable HPD only when requested.
 	 */
-	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
-			   HPD_DISABLE);
+	if (pdata->no_hpd)
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+				   HPD_DISABLE, HPD_DISABLE);
 
 	pdata->comms_enabled = true;
 
@@ -1164,6 +1188,36 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	int val;
+
+	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+
+	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+					 : connector_status_disconnected;
+}
+
+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	/* The device must remain active for HPD to function */
+	pm_runtime_get_sync(pdata->dev);
+	regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
+		     IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
+		     IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
+	pm_runtime_put_autosuspend(pdata->dev);
+}
+
 static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
 					  struct drm_connector *connector)
 {
@@ -1185,6 +1239,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
 	.post_disable = ti_sn_bridge_post_disable,
+	.detect = ti_sn_bridge_detect,
+	.hpd_enable = ti_sn_bridge_hpd_enable,
+	.hpd_disable = ti_sn_bridge_hpd_disable,
 	.get_edid = ti_sn_bridge_get_edid,
 };
 
@@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 		return PTR_ERR(pdata->next_bridge);
 	}
 
+	pdata->no_hpd = of_property_read_bool(np, "no-hpd");
+	if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
+	    !pdata->no_hpd) {
+		dev_warn(pdata->dev,
+			 "HPD support requires a DisplayPort connector\n");
+		pdata->no_hpd = true;
+	}
+
 	ti_sn_bridge_parse_lanes(pdata, np);
 
 	ret = ti_sn_bridge_parse_dsi_host(pdata);
@@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
-	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
+			  | DRM_BRIDGE_OP_EDID;
 	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
 			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
 				       pdata->supplies);
 }
 
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
+{
+	struct ti_sn65dsi86 *pdata = arg;
+	int ret;
+	int hpd;
+
+	ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
+	if (ret || !hpd)
+		return IRQ_NONE;
+
+	if (hpd & IRQ_HPD_INSERTION_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
+
+	if (hpd & IRQ_HPD_REMOVAL_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+	/* When replugged, ensure we trigger a detect to update the display */
+	if (hpd & IRQ_HPD_REPLUG_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+	/* reset the status registers */
+	regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
+		     IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
+		     IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return dev_err_probe(dev, PTR_ERR(pdata->refclk),
 				     "failed to get reference clock\n");
 
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						ti_sn65dsi86_irq_handler,
+						IRQF_ONESHOT, "sn65dsi86-irq",
+						pdata);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to register DP interrupt\n");
+	}
+
 	pm_runtime_enable(dev);
 	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
 	if (ret)
@@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	pm_runtime_set_autosuspend_delay(pdata->dev, 500);
 	pm_runtime_use_autosuspend(pdata->dev);
 
+	/* Enable interrupt handling */
+	regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
+
 	ti_sn65dsi86_debugfs_init(pdata);
 
 	/*
-- 
2.32.0

[PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Kieran Bingham]

When the SN65DSI86 is used in DisplayPort mode, its output is likely
routed to a DisplayPort connector, which can benefit from hotplug
detection. Support it in such cases, with polling mode only for now.

The implementation is limited to the bridge operations, as the connector
operations are legacy and new users should use
DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:

- Document the no_hpd field
- Rely on the SN_HPD_DISABLE_REG default value in the HPD case
- Add a TODO comment regarding IRQ support
[Kieran]
- Fix spelling s/assrted/asserted/
- Only enable HPD on DisplayPort connector.
- Support IRQ based hotplug detect
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 133 +++++++++++++++++++++++---
 1 file changed, 120 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 461f963faa0b..febb4e672ece 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -68,6 +68,7 @@
 #define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
+#define  HPD_DEBOUNCED_STATE			BIT(4)
 #define SN_GPIO_IO_REG				0x5E
 #define  SN_GPIO_INPUT_SHIFT			4
 #define  SN_GPIO_OUTPUT_SHIFT			0
@@ -104,10 +105,24 @@
 #define SN_PWM_EN_INV_REG			0xA5
 #define  SN_PWM_INV_MASK			BIT(0)
 #define  SN_PWM_EN_MASK				BIT(1)
+#define SN_IRQ_EN_REG				0xE0
+#define  IRQ_EN					BIT(0)
+#define SN_IRQ_HPD_REG				0xE6
+#define  IRQ_HPD_EN				BIT(0)
+#define  IRQ_HPD_INSERTION_EN			BIT(1)
+#define  IRQ_HPD_REMOVAL_EN			BIT(2)
+#define  IRQ_HPD_REPLUG_EN			BIT(3)
+#define  IRQ_HPD_PLL_UNLOCK_EN			BIT(5)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
+#define SN_IRQ_HPD_STATUS_REG			0xF5
+#define  IRQ_HPD_STATUS				BIT(0)
+#define  IRQ_HPD_INSERTION_STATUS		BIT(1)
+#define  IRQ_HPD_REMOVAL_STATUS			BIT(2)
+#define  IRQ_HPD_REPLUG_STATUS			BIT(3)
+#define  IRQ_PLL_UNLOCK				BIT(5)
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -166,6 +181,11 @@
  * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
  * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
  * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ *
+ * @no_hpd:       Disable hot-plug detection as instructed by device tree (used
+ *                for instance for eDP panels whose HPD signal won't be asserted
+ *                until the panel is turned on, and is thus not usable for
+ *                downstream device detection).
  */
 struct ti_sn65dsi86 {
 	struct auxiliary_device		bridge_aux;
@@ -200,6 +220,8 @@ struct ti_sn65dsi86 {
 	atomic_t			pwm_pin_busy;
 #endif
 	unsigned int			pwm_refclk_freq;
+
+	bool				no_hpd;
 };
 
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -314,23 +336,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
 	ti_sn_bridge_set_refclk_freq(pdata);
 
 	/*
-	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
-	 * so the HPD is an internal signal that's only there to signal that
-	 * the panel is done powering up.  ...but the bridge chip debounces
-	 * this signal by between 100 ms and 400 ms (depending on process,
-	 * voltage, and temperate--I measured it at about 200 ms).  One
+	 * As this is an eDP bridge, the output will be connected to a fixed
+	 * panel in most systems. HPD is in that case only an internal signal
+	 * to signal that the panel is done powering up. The bridge chip
+	 * debounces this signal by between 100 ms and 400 ms (depending on
+	 * process, voltage, and temperate--I measured it at about 200 ms). One
 	 * particular panel asserted HPD 84 ms after it was powered on meaning
 	 * that we saw HPD 284 ms after power on.  ...but the same panel said
 	 * that instead of looking at HPD you could just hardcode a delay of
-	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
-	 * delay in its prepare and always disable HPD.
+	 * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
+	 * assume that the panel driver will have the hardcoded delay in its
+	 * prepare and always disable HPD.
 	 *
-	 * If HPD somehow makes sense on some future panel we'll have to
-	 * change this to be conditional on someone specifying that HPD should
-	 * be used.
+	 * However, on some systems, the output is connected to a DisplayPort
+	 * connector. HPD is needed in such cases. To accommodate both use
+	 * cases, enable HPD only when requested.
 	 */
-	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
-			   HPD_DISABLE);
+	if (pdata->no_hpd)
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+				   HPD_DISABLE, HPD_DISABLE);
 
 	pdata->comms_enabled = true;
 
@@ -1164,6 +1188,36 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	int val;
+
+	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+
+	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+					 : connector_status_disconnected;
+}
+
+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	/* The device must remain active for HPD to function */
+	pm_runtime_get_sync(pdata->dev);
+	regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
+		     IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
+		     IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
+	pm_runtime_put_autosuspend(pdata->dev);
+}
+
 static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
 					  struct drm_connector *connector)
 {
@@ -1185,6 +1239,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
 	.post_disable = ti_sn_bridge_post_disable,
+	.detect = ti_sn_bridge_detect,
+	.hpd_enable = ti_sn_bridge_hpd_enable,
+	.hpd_disable = ti_sn_bridge_hpd_disable,
 	.get_edid = ti_sn_bridge_get_edid,
 };
 
@@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 		return PTR_ERR(pdata->next_bridge);
 	}
 
+	pdata->no_hpd = of_property_read_bool(np, "no-hpd");
+	if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
+	    !pdata->no_hpd) {
+		dev_warn(pdata->dev,
+			 "HPD support requires a DisplayPort connector\n");
+		pdata->no_hpd = true;
+	}
+
 	ti_sn_bridge_parse_lanes(pdata, np);
 
 	ret = ti_sn_bridge_parse_dsi_host(pdata);
@@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = np;
-	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
+			  | DRM_BRIDGE_OP_EDID;
 	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
 			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
 				       pdata->supplies);
 }
 
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
+{
+	struct ti_sn65dsi86 *pdata = arg;
+	int ret;
+	int hpd;
+
+	ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
+	if (ret || !hpd)
+		return IRQ_NONE;
+
+	if (hpd & IRQ_HPD_INSERTION_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
+
+	if (hpd & IRQ_HPD_REMOVAL_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+	/* When replugged, ensure we trigger a detect to update the display */
+	if (hpd & IRQ_HPD_REPLUG_STATUS)
+		drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+	/* reset the status registers */
+	regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
+		     IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
+		     IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
+
+	return IRQ_HANDLED;
+}
+
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return dev_err_probe(dev, PTR_ERR(pdata->refclk),
 				     "failed to get reference clock\n");
 
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						ti_sn65dsi86_irq_handler,
+						IRQF_ONESHOT, "sn65dsi86-irq",
+						pdata);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to register DP interrupt\n");
+	}
+
 	pm_runtime_enable(dev);
 	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
 	if (ret)
@@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 	pm_runtime_set_autosuspend_delay(pdata->dev, 500);
 	pm_runtime_use_autosuspend(pdata->dev);
 
+	/* Enable interrupt handling */
+	regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
+
 	ti_sn65dsi86_debugfs_init(pdata);
 
 	/*
-- 
2.32.0

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
>   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
>   together
>
> Notes from Kieran:
>
> RB Tags collected from:
>  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
>
> However this was over a year ago, so let me know if other patches now
> superceed this one or otherwise invalidate this update.
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c55848588123..ffb6c04f6c46 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> +                                         struct drm_connector *connector)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       struct edid *edid;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> +       pm_runtime_put_autosuspend(pdata->dev);
> +
> +       return edid;
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
> @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .enable = ti_sn_bridge_enable,
>         .disable = ti_sn_bridge_disable,
>         .post_disable = ti_sn_bridge_post_disable,
> +       .get_edid = ti_sn_bridge_get_edid,
>  };
>
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
> +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;

This doesn't look right to me. In the eDP case the EDID reading is
driven by the panel.

-Doug

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
>   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
>   together
>
> Notes from Kieran:
>
> RB Tags collected from:
>  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
>
> However this was over a year ago, so let me know if other patches now
> superceed this one or otherwise invalidate this update.
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c55848588123..ffb6c04f6c46 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> +                                         struct drm_connector *connector)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       struct edid *edid;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> +       pm_runtime_put_autosuspend(pdata->dev);
> +
> +       return edid;
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
> @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .enable = ti_sn_bridge_enable,
>         .disable = ti_sn_bridge_disable,
>         .post_disable = ti_sn_bridge_post_disable,
> +       .get_edid = ti_sn_bridge_get_edid,
>  };
>
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
> +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;

This doesn't look right to me. In the eDP case the EDID reading is
driven by the panel.

-Doug

Re: [PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Now that the driver supports the connector-related bridge operations,
> make the connector creation optional. This enables usage of the
> sn65dsi86 with the DRM bridge connector helper.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> ---
> Changes since v1:
>  - None
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Can you please CC me on this series next time you send it out? I was
pretty involved in previous reviews here. Luckily I got CCed on one of
the patches so I knew to look, but since that one is (probably)
getting dropped it'd be good to make sure I was on the others. It's
also pretty important to include +Sam Ravnborg since there's a lot of
overlap with his series [1]:


> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ffb6c04f6c46..29f5f7123ed9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         int ret;
>
> -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -               DRM_ERROR("Fix bridge driver to make connector optional!");
> -               return -EINVAL;
> -       }

That won't work without some other fixes, sorry!

The problem is that you'll break ti_sn_bridge_get_bpp(). That
absolutely relies on having our own connector so you need to fix that
_before_ making the connector optional.

Rob Clark made an attempt on this [2] but Laurent didn't like the
solution he came up with. Sam's series that I mentioned [1] also made
an attempt at this, specifically in patch 5 ("drm/bridge:
ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
Unfortunately, it didn't work because the "output_bus_cfg.format"
wasn't set to anything. Personally I wouldn't mind Rob's solution as a
stopgap if Laurent removes his NAK. Then we're not stuck while someone
tries to find time to fix this correctly.

One last problem here is that your patch doesn't actually apply to
drm-misc/drm-misc-next, which is likely where it would land. I believe
it conflicts with my recent commit e283820cbf80 ("drm/bridge:
ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
series on mainline, but you should really be targeting drm-misc-next.

-Doug

[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
[2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/

Re: [PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Now that the driver supports the connector-related bridge operations,
> make the connector creation optional. This enables usage of the
> sn65dsi86 with the DRM bridge connector helper.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> ---
> Changes since v1:
>  - None
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Can you please CC me on this series next time you send it out? I was
pretty involved in previous reviews here. Luckily I got CCed on one of
the patches so I knew to look, but since that one is (probably)
getting dropped it'd be good to make sure I was on the others. It's
also pretty important to include +Sam Ravnborg since there's a lot of
overlap with his series [1]:


> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ffb6c04f6c46..29f5f7123ed9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         int ret;
>
> -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -               DRM_ERROR("Fix bridge driver to make connector optional!");
> -               return -EINVAL;
> -       }

That won't work without some other fixes, sorry!

The problem is that you'll break ti_sn_bridge_get_bpp(). That
absolutely relies on having our own connector so you need to fix that
_before_ making the connector optional.

Rob Clark made an attempt on this [2] but Laurent didn't like the
solution he came up with. Sam's series that I mentioned [1] also made
an attempt at this, specifically in patch 5 ("drm/bridge:
ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
Unfortunately, it didn't work because the "output_bus_cfg.format"
wasn't set to anything. Personally I wouldn't mind Rob's solution as a
stopgap if Laurent removes his NAK. Then we're not stuck while someone
tries to find time to fix this correctly.

One last problem here is that your patch doesn't actually apply to
drm-misc/drm-misc-next, which is likely where it would land. I believe
it conflicts with my recent commit e283820cbf80 ("drm/bridge:
ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
series on mainline, but you should really be targeting drm-misc-next.

-Doug

[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
[2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
>  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
>    devm_drm_of_get_bridge"
>  - eDP/DP mode determined from the next bridge connector type.
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 29f5f7123ed9..461f963faa0b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -60,6 +60,7 @@
>  #define SN_LN_ASSIGN_REG                       0x59
>  #define  LN_ASSIGN_WIDTH                       2
>  #define SN_ENH_FRAME_REG                       0x5A
> +#define  ASSR_CONTROL                          BIT(0)
>  #define  VSTREAM_ENABLE                                BIT(3)
>  #define  LN_POLRS_OFFSET                       4
>  #define  LN_POLRS_MASK                         0xf0
> @@ -91,6 +92,8 @@
>  #define SN_DATARATE_CONFIG_REG                 0x94
>  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
>  #define  DP_DATARATE(x)                                ((x) << 5)
> +#define SN_TRAINING_SETTING_REG                        0x95
> +#define  SCRAMBLE_DISABLE                      BIT(4)
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
>
> +       /* For DisplayPort, use the standard DP scrambler seed. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +                                  ASSR_CONTROL, 0);

I thought it was agreed that this hunk wasn't doing anything and
should be removed? I did some research previously [1] and the manual
said that this bit is "read only" unless "TEST2" is pulled high. In
the previous discussion Laurent said that it wasn't. I also pointed
out that this conflicts with the bit of code in ti_sn_bridge_enable()
which tells the sink to enable the alternate scrambler.


>         /* enable DP PLL */
>         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>
> @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>                 goto exit;
>         }
>
> +       /* For DisplayPort, disable scrambling mode. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);

In my previous review I asked for some comments to include the "why"
we disabling scrambling mode for DP. Can you please add that?

I also presume that for DP it's probably a good idea to avoid the code
in ti_sn_bridge_enable() that tells the sink to use the alternate
scrambler.


[1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0g@mail.gmail.com

-Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
>  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
>    devm_drm_of_get_bridge"
>  - eDP/DP mode determined from the next bridge connector type.
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 29f5f7123ed9..461f963faa0b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -60,6 +60,7 @@
>  #define SN_LN_ASSIGN_REG                       0x59
>  #define  LN_ASSIGN_WIDTH                       2
>  #define SN_ENH_FRAME_REG                       0x5A
> +#define  ASSR_CONTROL                          BIT(0)
>  #define  VSTREAM_ENABLE                                BIT(3)
>  #define  LN_POLRS_OFFSET                       4
>  #define  LN_POLRS_MASK                         0xf0
> @@ -91,6 +92,8 @@
>  #define SN_DATARATE_CONFIG_REG                 0x94
>  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
>  #define  DP_DATARATE(x)                                ((x) << 5)
> +#define SN_TRAINING_SETTING_REG                        0x95
> +#define  SCRAMBLE_DISABLE                      BIT(4)
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
>
> +       /* For DisplayPort, use the standard DP scrambler seed. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +                                  ASSR_CONTROL, 0);

I thought it was agreed that this hunk wasn't doing anything and
should be removed? I did some research previously [1] and the manual
said that this bit is "read only" unless "TEST2" is pulled high. In
the previous discussion Laurent said that it wasn't. I also pointed
out that this conflicts with the bit of code in ti_sn_bridge_enable()
which tells the sink to enable the alternate scrambler.


>         /* enable DP PLL */
>         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>
> @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>                 goto exit;
>         }
>
> +       /* For DisplayPort, disable scrambling mode. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);

In my previous review I asked for some comments to include the "why"
we disabling scrambling mode for DP. Can you please add that?

I also presume that for DP it's probably a good idea to avoid the code
in ti_sn_bridge_enable() that tells the sink to use the alternate
scrambler.


[1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0g@mail.gmail.com

-Doug

Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> @@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>                 return PTR_ERR(pdata->next_bridge);
>         }
>
> +       pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> +       if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> +           !pdata->no_hpd) {
> +               dev_warn(pdata->dev,
> +                        "HPD support requires a DisplayPort connector\n");

Maybe "HPD support only implemented for full DP connectors".

Plausibly someone could come up with a reason to hook HPD up for eDP
one day, but so far we haven't seen it.


> @@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
> -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
> +                         | DRM_BRIDGE_OP_EDID;

Seems like "OP_HPD" ought to be dependent on whether the IRQ was
provided, right? AKA you could have "detect" because HPD is plumbed
through to the bridge but not "HPD" if the IRQ from the bridge isn't
hooked up to the main processor...


> @@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
>                                        pdata->supplies);
>  }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> +       struct ti_sn65dsi86 *pdata = arg;
> +       int ret;
> +       int hpd;

`hpd` should be an `unsigned int` to match the prototype of regmap-read.


> @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
>                                      "failed to get reference clock\n");
>
> +       if (client->irq > 0) {
> +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +                                               ti_sn65dsi86_irq_handler,
> +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> +                                               pdata);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to register DP interrupt\n");
> +       }

Is this the right place to request the IRQ, or should it be in
ti_sn_bridge_probe(). As soon as you request it the interrupt can go
off, but you're relying on a bunch of bridge stuff to have been
initted, right?


> @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
>         pm_runtime_use_autosuspend(pdata->dev);
>
> +       /* Enable interrupt handling */
> +       regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);

Shouldn't we only enable interrupt handling if client->irq > 0? AKA
combine this with the "if" statement?



-Doug

Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> @@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>                 return PTR_ERR(pdata->next_bridge);
>         }
>
> +       pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> +       if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> +           !pdata->no_hpd) {
> +               dev_warn(pdata->dev,
> +                        "HPD support requires a DisplayPort connector\n");

Maybe "HPD support only implemented for full DP connectors".

Plausibly someone could come up with a reason to hook HPD up for eDP
one day, but so far we haven't seen it.


> @@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
> -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
> +                         | DRM_BRIDGE_OP_EDID;

Seems like "OP_HPD" ought to be dependent on whether the IRQ was
provided, right? AKA you could have "detect" because HPD is plumbed
through to the bridge but not "HPD" if the IRQ from the bridge isn't
hooked up to the main processor...


> @@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
>                                        pdata->supplies);
>  }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> +       struct ti_sn65dsi86 *pdata = arg;
> +       int ret;
> +       int hpd;

`hpd` should be an `unsigned int` to match the prototype of regmap-read.


> @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
>                                      "failed to get reference clock\n");
>
> +       if (client->irq > 0) {
> +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +                                               ti_sn65dsi86_irq_handler,
> +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> +                                               pdata);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to register DP interrupt\n");
> +       }

Is this the right place to request the IRQ, or should it be in
ti_sn_bridge_probe(). As soon as you request it the interrupt can go
off, but you're relying on a bunch of bridge stuff to have been
initted, right?


> @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
>         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
>         pm_runtime_use_autosuspend(pdata->dev);
>
> +       /* Enable interrupt handling */
> +       regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);

Shouldn't we only enable interrupt handling if client->irq > 0? AKA
combine this with the "if" statement?



-Doug

Re: [PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Kieran Bingham]

Hi Doug,

Quoting Doug Anderson (2022-03-07 23:21:28)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Now that the driver supports the connector-related bridge operations,
> > make the connector creation optional. This enables usage of the
> > sn65dsi86 with the DRM bridge connector helper.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > ---
> > Changes since v1:
> >  - None
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> Can you please CC me on this series next time you send it out? I was
> pretty involved in previous reviews here. Luckily I got CCed on one of
> the patches so I knew to look, but since that one is (probably)
> getting dropped it'd be good to make sure I was on the others. It's
> also pretty important to include +Sam Ravnborg since there's a lot of
> overlap with his series [1]:

Absolutely - I was assuming you were the main target for reviews. I'm
sorry - I also assumed get_maintainer.pl had / would add you - and I
didn't check getting the patches out last night.

I'll be sure to manually add you.

> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index ffb6c04f6c46..29f5f7123ed9 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
> >         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >         int ret;
> >
> > -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -               DRM_ERROR("Fix bridge driver to make connector optional!");
> > -               return -EINVAL;
> > -       }
> 
> That won't work without some other fixes, sorry!

Ok ;-)

To be clear, my goal here has been to get the IRQ HPD working - so
my main focus is there. I guess I now have to handle custodianship of
these dependencies somehow too though.


> The problem is that you'll break ti_sn_bridge_get_bpp(). That
> absolutely relies on having our own connector so you need to fix that
> _before_ making the connector optional.
> 
> Rob Clark made an attempt on this [2] but Laurent didn't like the
> solution he came up with. Sam's series that I mentioned [1] also made
> an attempt at this, specifically in patch 5 ("drm/bridge:
> ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
> Unfortunately, it didn't work because the "output_bus_cfg.format"
> wasn't set to anything. Personally I wouldn't mind Rob's solution as a
> stopgap if Laurent removes his NAK. Then we're not stuck while someone
> tries to find time to fix this correctly.

Ok - all of that is going to take some time for me to review and
process.
 
> One last problem here is that your patch doesn't actually apply to
> drm-misc/drm-misc-next, which is likely where it would land. I believe
> it conflicts with my recent commit e283820cbf80 ("drm/bridge:
> ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
> series on mainline, but you should really be targeting drm-misc-next.

Ah sorry - I thought I had merged in drm-misc-next, but it was a week
ago or so so I guess I'm already outdated.

I'll cleanup for a new base now.


> -Doug
> 
> [1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
> [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/

Re: [PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Kieran Bingham]

Hi Doug,

Quoting Doug Anderson (2022-03-07 23:21:28)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Now that the driver supports the connector-related bridge operations,
> > make the connector creation optional. This enables usage of the
> > sn65dsi86 with the DRM bridge connector helper.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > ---
> > Changes since v1:
> >  - None
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> Can you please CC me on this series next time you send it out? I was
> pretty involved in previous reviews here. Luckily I got CCed on one of
> the patches so I knew to look, but since that one is (probably)
> getting dropped it'd be good to make sure I was on the others. It's
> also pretty important to include +Sam Ravnborg since there's a lot of
> overlap with his series [1]:

Absolutely - I was assuming you were the main target for reviews. I'm
sorry - I also assumed get_maintainer.pl had / would add you - and I
didn't check getting the patches out last night.

I'll be sure to manually add you.

> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index ffb6c04f6c46..29f5f7123ed9 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
> >         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >         int ret;
> >
> > -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -               DRM_ERROR("Fix bridge driver to make connector optional!");
> > -               return -EINVAL;
> > -       }
> 
> That won't work without some other fixes, sorry!

Ok ;-)

To be clear, my goal here has been to get the IRQ HPD working - so
my main focus is there. I guess I now have to handle custodianship of
these dependencies somehow too though.


> The problem is that you'll break ti_sn_bridge_get_bpp(). That
> absolutely relies on having our own connector so you need to fix that
> _before_ making the connector optional.
> 
> Rob Clark made an attempt on this [2] but Laurent didn't like the
> solution he came up with. Sam's series that I mentioned [1] also made
> an attempt at this, specifically in patch 5 ("drm/bridge:
> ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
> Unfortunately, it didn't work because the "output_bus_cfg.format"
> wasn't set to anything. Personally I wouldn't mind Rob's solution as a
> stopgap if Laurent removes his NAK. Then we're not stuck while someone
> tries to find time to fix this correctly.

Ok - all of that is going to take some time for me to review and
process.
 
> One last problem here is that your patch doesn't actually apply to
> drm-misc/drm-misc-next, which is likely where it would land. I believe
> it conflicts with my recent commit e283820cbf80 ("drm/bridge:
> ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
> series on mainline, but you should really be targeting drm-misc-next.

Ah sorry - I thought I had merged in drm-misc-next, but it was a week
ago or so so I guess I'm already outdated.

I'll cleanup for a new base now.


> -Doug
> 
> [1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
> [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/

Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Kieran Bingham]

Quoting Doug Anderson (2022-03-07 23:22:17)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > @@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >                 return PTR_ERR(pdata->next_bridge);
> >         }
> >
> > +       pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> > +       if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +           !pdata->no_hpd) {
> > +               dev_warn(pdata->dev,
> > +                        "HPD support requires a DisplayPort connector\n");
> 
> Maybe "HPD support only implemented for full DP connectors".
> 
> Plausibly someone could come up with a reason to hook HPD up for eDP
> one day, but so far we haven't seen it.
> 

Ok, updated.


> 
> > @@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> > -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
> > +                         | DRM_BRIDGE_OP_EDID;
> 
> Seems like "OP_HPD" ought to be dependent on whether the IRQ was
> provided, right? AKA you could have "detect" because HPD is plumbed
> through to the bridge but not "HPD" if the IRQ from the bridge isn't
> hooked up to the main processor...

Yes, I think that's right. If there's no IRQ, then OP_HPD shouldn't be
set, and it will fall back to polling.

I'll fix that up.

> > @@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
> >                                        pdata->supplies);
> >  }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > +       struct ti_sn65dsi86 *pdata = arg;
> > +       int ret;
> > +       int hpd;
> 
> `hpd` should be an `unsigned int` to match the prototype of regmap-read.

Agreed, and updated.

> > @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
> >                                      "failed to get reference clock\n");
> >
> > +       if (client->irq > 0) {
> > +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                               ti_sn65dsi86_irq_handler,
> > +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> > +                                               pdata);
> > +               if (ret)
> > +                       return dev_err_probe(dev, ret,
> > +                                            "Failed to register DP interrupt\n");
> > +       }
> 
> Is this the right place to request the IRQ, or should it be in
> ti_sn_bridge_probe(). As soon as you request it the interrupt can go
> off, but you're relying on a bunch of bridge stuff to have been
> initted, right?

Indeed, it will be relying upon the bridge to have been set up.

You're right I believe, ti_sn_bridge_probe() sounds reasonable. And only
after that should we enable the interrupts.

Fixing ... (But getting stuck/blocked by the connector changes, so ..
I'll keep plowing through).

> > @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
> >         pm_runtime_use_autosuspend(pdata->dev);
> >
> > +       /* Enable interrupt handling */
> > +       regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> 
> Shouldn't we only enable interrupt handling if client->irq > 0? AKA
> combine this with the "if" statement?

Agreed.

> -Doug

Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi86: Support hotplug detection

[Kieran Bingham]

Quoting Doug Anderson (2022-03-07 23:22:17)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > @@ -1264,6 +1321,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >                 return PTR_ERR(pdata->next_bridge);
> >         }
> >
> > +       pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> > +       if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +           !pdata->no_hpd) {
> > +               dev_warn(pdata->dev,
> > +                        "HPD support requires a DisplayPort connector\n");
> 
> Maybe "HPD support only implemented for full DP connectors".
> 
> Plausibly someone could come up with a reason to hook HPD up for eDP
> one day, but so far we haven't seen it.
> 

Ok, updated.


> 
> > @@ -1272,7 +1337,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> > -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)
> > +                         | DRM_BRIDGE_OP_EDID;
> 
> Seems like "OP_HPD" ought to be dependent on whether the IRQ was
> provided, right? AKA you could have "detect" because HPD is plumbed
> through to the bridge but not "HPD" if the IRQ from the bridge isn't
> hooked up to the main processor...

Yes, I think that's right. If there's no IRQ, then OP_HPD shouldn't be
set, and it will fall back to polling.

I'll fix that up.

> > @@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
> >                                        pdata->supplies);
> >  }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > +       struct ti_sn65dsi86 *pdata = arg;
> > +       int ret;
> > +       int hpd;
> 
> `hpd` should be an `unsigned int` to match the prototype of regmap-read.

Agreed, and updated.

> > @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >                 return dev_err_probe(dev, PTR_ERR(pdata->refclk),
> >                                      "failed to get reference clock\n");
> >
> > +       if (client->irq > 0) {
> > +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +                                               ti_sn65dsi86_irq_handler,
> > +                                               IRQF_ONESHOT, "sn65dsi86-irq",
> > +                                               pdata);
> > +               if (ret)
> > +                       return dev_err_probe(dev, ret,
> > +                                            "Failed to register DP interrupt\n");
> > +       }
> 
> Is this the right place to request the IRQ, or should it be in
> ti_sn_bridge_probe(). As soon as you request it the interrupt can go
> off, but you're relying on a bunch of bridge stuff to have been
> initted, right?

Indeed, it will be relying upon the bridge to have been set up.

You're right I believe, ti_sn_bridge_probe() sounds reasonable. And only
after that should we enable the interrupts.

Fixing ... (But getting stuck/blocked by the connector changes, so ..
I'll keep plowing through).

> > @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> >         pm_runtime_set_autosuspend_delay(pdata->dev, 500);
> >         pm_runtime_use_autosuspend(pdata->dev);
> >
> > +       /* Enable interrupt handling */
> > +       regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> 
> Shouldn't we only enable interrupt handling if client->irq > 0? AKA
> combine this with the "if" statement?

Agreed.

> -Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Doug Anderson (2022-03-07 23:22:00)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> > routed to a DisplayPort connector. Enable DisplayPort mode when the next
> > component in the display pipeline is detected as a DisplayPort
> > connector, and disable eDP features in that case.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reworked to set bridge type based on the next bridge/connector.
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > --
> > Changes since v1/RFC:
> >  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> >    devm_drm_of_get_bridge"
> >  - eDP/DP mode determined from the next bridge connector type.
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 29f5f7123ed9..461f963faa0b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -60,6 +60,7 @@
> >  #define SN_LN_ASSIGN_REG                       0x59
> >  #define  LN_ASSIGN_WIDTH                       2
> >  #define SN_ENH_FRAME_REG                       0x5A
> > +#define  ASSR_CONTROL                          BIT(0)
> >  #define  VSTREAM_ENABLE                                BIT(3)
> >  #define  LN_POLRS_OFFSET                       4
> >  #define  LN_POLRS_MASK                         0xf0
> > @@ -91,6 +92,8 @@
> >  #define SN_DATARATE_CONFIG_REG                 0x94
> >  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
> >  #define  DP_DATARATE(x)                                ((x) << 5)
> > +#define SN_TRAINING_SETTING_REG                        0x95
> > +#define  SCRAMBLE_DISABLE                      BIT(4)
> >  #define SN_ML_TX_MODE_REG                      0x96
> >  #define  ML_TX_MAIN_LINK_OFF                   0
> >  #define  ML_TX_NORMAL_MODE                     BIT(0)
> > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
> >                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
> >
> > +       /* For DisplayPort, use the standard DP scrambler seed. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> > +                                  ASSR_CONTROL, 0);
> 
> I thought it was agreed that this hunk wasn't doing anything and
> should be removed? I did some research previously [1] and the manual
> said that this bit is "read only" unless "TEST2" is pulled high. In
> the previous discussion Laurent said that it wasn't. I also pointed
> out that this conflicts with the bit of code in ti_sn_bridge_enable()
> which tells the sink to enable the alternate scrambler.

Sorry - I had misremembered indeed, and looking back I confirmed that
this hunk was not required. I had incorrectly remembered that the second
part was required (below) and assumed that had meant both were.

Of course if we're disabling the scrambling mode, then it likely doesn't
matter what the seed is!.

Looking at the datasheet though, register 0x5a, clearly says the default
is 01 (ASSR) which we're not using. So the datasheet implies we want the
DP scrambler seed (if it were enabled?)

Reading the 0x5a register here shows we have 0x05. Indeed, re-reading
after attempting to write '0' to it still shows 0x05. So it's read-only
and not relevant regardless.

I've removed this hunk now.

> >         /* enable DP PLL */
> >         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
> >
> > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >                 goto exit;
> >         }
> >
> > +       /* For DisplayPort, disable scrambling mode. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> > +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> 
> In my previous review I asked for some comments to include the "why"
> we disabling scrambling mode for DP. Can you please add that?
> 
> I also presume that for DP it's probably a good idea to avoid the code
> in ti_sn_bridge_enable() that tells the sink to use the alternate
> scrambler.

looking at it yes. I've added a check to avoid that in my DP connector
case, and there doesn't seem to be any effect on the output. But I'll
add it to the patch for the next version.



>> I think it was done for DRM purpose, to prevent signals meant for a
>> panel to be connected to a device that could capture video from a DP
>> source.

Is this better:

	/*
	 * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR)
	 * are supported by the SN65DSI86. For DisplayPort, disable scrambling
	 * mode.
	 */

I don't know if it answers your 'why' ... and I don't think adding
 "We think it is for DRM protection"

really suits the comment.

 
> [1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0g@mail.gmail.com
> 
> -Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Doug Anderson (2022-03-07 23:22:00)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> > routed to a DisplayPort connector. Enable DisplayPort mode when the next
> > component in the display pipeline is detected as a DisplayPort
> > connector, and disable eDP features in that case.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reworked to set bridge type based on the next bridge/connector.
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > --
> > Changes since v1/RFC:
> >  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> >    devm_drm_of_get_bridge"
> >  - eDP/DP mode determined from the next bridge connector type.
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 29f5f7123ed9..461f963faa0b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -60,6 +60,7 @@
> >  #define SN_LN_ASSIGN_REG                       0x59
> >  #define  LN_ASSIGN_WIDTH                       2
> >  #define SN_ENH_FRAME_REG                       0x5A
> > +#define  ASSR_CONTROL                          BIT(0)
> >  #define  VSTREAM_ENABLE                                BIT(3)
> >  #define  LN_POLRS_OFFSET                       4
> >  #define  LN_POLRS_MASK                         0xf0
> > @@ -91,6 +92,8 @@
> >  #define SN_DATARATE_CONFIG_REG                 0x94
> >  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
> >  #define  DP_DATARATE(x)                                ((x) << 5)
> > +#define SN_TRAINING_SETTING_REG                        0x95
> > +#define  SCRAMBLE_DISABLE                      BIT(4)
> >  #define SN_ML_TX_MODE_REG                      0x96
> >  #define  ML_TX_MAIN_LINK_OFF                   0
> >  #define  ML_TX_NORMAL_MODE                     BIT(0)
> > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
> >                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
> >
> > +       /* For DisplayPort, use the standard DP scrambler seed. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> > +                                  ASSR_CONTROL, 0);
> 
> I thought it was agreed that this hunk wasn't doing anything and
> should be removed? I did some research previously [1] and the manual
> said that this bit is "read only" unless "TEST2" is pulled high. In
> the previous discussion Laurent said that it wasn't. I also pointed
> out that this conflicts with the bit of code in ti_sn_bridge_enable()
> which tells the sink to enable the alternate scrambler.

Sorry - I had misremembered indeed, and looking back I confirmed that
this hunk was not required. I had incorrectly remembered that the second
part was required (below) and assumed that had meant both were.

Of course if we're disabling the scrambling mode, then it likely doesn't
matter what the seed is!.

Looking at the datasheet though, register 0x5a, clearly says the default
is 01 (ASSR) which we're not using. So the datasheet implies we want the
DP scrambler seed (if it were enabled?)

Reading the 0x5a register here shows we have 0x05. Indeed, re-reading
after attempting to write '0' to it still shows 0x05. So it's read-only
and not relevant regardless.

I've removed this hunk now.

> >         /* enable DP PLL */
> >         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
> >
> > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >                 goto exit;
> >         }
> >
> > +       /* For DisplayPort, disable scrambling mode. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> > +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> 
> In my previous review I asked for some comments to include the "why"
> we disabling scrambling mode for DP. Can you please add that?
> 
> I also presume that for DP it's probably a good idea to avoid the code
> in ti_sn_bridge_enable() that tells the sink to use the alternate
> scrambler.

looking at it yes. I've added a check to avoid that in my DP connector
case, and there doesn't seem to be any effect on the output. But I'll
add it to the patch for the next version.



>> I think it was done for DRM purpose, to prevent signals meant for a
>> panel to be connected to a device that could capture video from a DP
>> source.

Is this better:

	/*
	 * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR)
	 * are supported by the SN65DSI86. For DisplayPort, disable scrambling
	 * mode.
	 */

I don't know if it answers your 'why' ... and I don't think adding
 "We think it is for DRM protection"

really suits the comment.

 
> [1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0g@mail.gmail.com
> 
> -Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Doug Anderson]

Hi,

On Wed, Mar 9, 2022 at 9:05 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> >> I think it was done for DRM purpose, to prevent signals meant for a
> >> panel to be connected to a device that could capture video from a DP
> >> source.
>
> Is this better:
>
>         /*
>          * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR)

s/pannels/panels

>          * are supported by the SN65DSI86. For DisplayPort, disable scrambling
>          * mode.
>          */
>
> I don't know if it answers your 'why' ... and I don't think adding
>  "We think it is for DRM protection"
>
> really suits the comment.

Yeah, that looks pretty good. ...or even:

eDP panels use an Alternate Scrambler Seed compared to displays hooked
up via a full DisplayPort connector. SN65DSI86 only supports the
alternate scrambler seed, not the normal one, so the only way we can
support full DisplayPort displays is by fully turning off the
scrambler.


-Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Doug Anderson]

Hi,

On Wed, Mar 9, 2022 at 9:05 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> >> I think it was done for DRM purpose, to prevent signals meant for a
> >> panel to be connected to a device that could capture video from a DP
> >> source.
>
> Is this better:
>
>         /*
>          * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR)

s/pannels/panels

>          * are supported by the SN65DSI86. For DisplayPort, disable scrambling
>          * mode.
>          */
>
> I don't know if it answers your 'why' ... and I don't think adding
>  "We think it is for DRM protection"
>
> really suits the comment.

Yeah, that looks pretty good. ...or even:

eDP panels use an Alternate Scrambler Seed compared to displays hooked
up via a full DisplayPort connector. SN65DSI86 only supports the
alternate scrambler seed, not the normal one, so the only way we can
support full DisplayPort displays is by fully turning off the
scrambler.


-Doug

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Kieran Bingham]

Hi Doug,

Quoting Doug Anderson (2022-03-07 19:52:08)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> >   together
> >
> > Notes from Kieran:
> >
> > RB Tags collected from:
> >  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
> >
> > However this was over a year ago, so let me know if other patches now
> > superceed this one or otherwise invalidate this update.
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index c55848588123..ffb6c04f6c46 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> >         pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                         struct drm_connector *connector)
> > +{
> > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +       struct edid *edid;
> > +
> > +       pm_runtime_get_sync(pdata->dev);
> > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > +       pm_runtime_put_autosuspend(pdata->dev);
> > +
> > +       return edid;
> > +}
> > +
> >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >         .attach = ti_sn_bridge_attach,
> >         .detach = ti_sn_bridge_detach,
> > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >         .enable = ti_sn_bridge_enable,
> >         .disable = ti_sn_bridge_disable,
> >         .post_disable = ti_sn_bridge_post_disable,
> > +       .get_edid = ti_sn_bridge_get_edid,
> >  };
> >
> >  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> > +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> 
> This doesn't look right to me. In the eDP case the EDID reading is
> driven by the panel.

Now that I have the optional connector working based on Sam's series I
think this is the last issue to solve before reposting the DP/HPD
support.

Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID
when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?

--
Regards

Kieran


> 
> -Doug

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Kieran Bingham]

Hi Doug,

Quoting Doug Anderson (2022-03-07 19:52:08)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> >   together
> >
> > Notes from Kieran:
> >
> > RB Tags collected from:
> >  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
> >
> > However this was over a year ago, so let me know if other patches now
> > superceed this one or otherwise invalidate this update.
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index c55848588123..ffb6c04f6c46 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> >         pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                         struct drm_connector *connector)
> > +{
> > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +       struct edid *edid;
> > +
> > +       pm_runtime_get_sync(pdata->dev);
> > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > +       pm_runtime_put_autosuspend(pdata->dev);
> > +
> > +       return edid;
> > +}
> > +
> >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >         .attach = ti_sn_bridge_attach,
> >         .detach = ti_sn_bridge_detach,
> > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >         .enable = ti_sn_bridge_enable,
> >         .disable = ti_sn_bridge_disable,
> >         .post_disable = ti_sn_bridge_post_disable,
> > +       .get_edid = ti_sn_bridge_get_edid,
> >  };
> >
> >  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> > +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> 
> This doesn't look right to me. In the eDP case the EDID reading is
> driven by the panel.

Now that I have the optional connector working based on Sam's series I
think this is the last issue to solve before reposting the DP/HPD
support.

Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID
when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?

--
Regards

Kieran


> 
> -Doug

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Kieran Bingham (2022-03-07 17:59:54)
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
>  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
>    devm_drm_of_get_bridge"
>  - eDP/DP mode determined from the next bridge connector type.
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 29f5f7123ed9..461f963faa0b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -60,6 +60,7 @@
>  #define SN_LN_ASSIGN_REG                       0x59
>  #define  LN_ASSIGN_WIDTH                       2
>  #define SN_ENH_FRAME_REG                       0x5A
> +#define  ASSR_CONTROL                          BIT(0)
>  #define  VSTREAM_ENABLE                                BIT(3)
>  #define  LN_POLRS_OFFSET                       4
>  #define  LN_POLRS_MASK                         0xf0
> @@ -91,6 +92,8 @@
>  #define SN_DATARATE_CONFIG_REG                 0x94
>  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
>  #define  DP_DATARATE(x)                                ((x) << 5)
> +#define SN_TRAINING_SETTING_REG                        0x95
> +#define  SCRAMBLE_DISABLE                      BIT(4)
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
>  
> +       /* For DisplayPort, use the standard DP scrambler seed. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +                                  ASSR_CONTROL, 0);
> +
>         /* enable DP PLL */
>         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>  
> @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>                 goto exit;
>         }
>  
> +       /* For DisplayPort, disable scrambling mode. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> +
>         /*
>          * We'll try to link train several times.  As part of link training
>          * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
> @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
>         pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> -       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> +       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> +                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;

Anyone know if there's any expectation of other types here? Or is it
just as safe to do:

	pdata->bridge.type = pdata->next_bridge->type;

To achieve the same effect?

--
Kieran


>  
>         drm_bridge_add(&pdata->bridge);
>  
> -- 
> 2.32.0
>

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Kieran Bingham (2022-03-07 17:59:54)
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
>  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
>    devm_drm_of_get_bridge"
>  - eDP/DP mode determined from the next bridge connector type.
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 29f5f7123ed9..461f963faa0b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -60,6 +60,7 @@
>  #define SN_LN_ASSIGN_REG                       0x59
>  #define  LN_ASSIGN_WIDTH                       2
>  #define SN_ENH_FRAME_REG                       0x5A
> +#define  ASSR_CONTROL                          BIT(0)
>  #define  VSTREAM_ENABLE                                BIT(3)
>  #define  LN_POLRS_OFFSET                       4
>  #define  LN_POLRS_MASK                         0xf0
> @@ -91,6 +92,8 @@
>  #define SN_DATARATE_CONFIG_REG                 0x94
>  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
>  #define  DP_DATARATE(x)                                ((x) << 5)
> +#define SN_TRAINING_SETTING_REG                        0x95
> +#define  SCRAMBLE_DISABLE                      BIT(4)
>  #define SN_ML_TX_MODE_REG                      0x96
>  #define  ML_TX_MAIN_LINK_OFF                   0
>  #define  ML_TX_NORMAL_MODE                     BIT(0)
> @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
>  
> +       /* For DisplayPort, use the standard DP scrambler seed. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +                                  ASSR_CONTROL, 0);
> +
>         /* enable DP PLL */
>         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>  
> @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>                 goto exit;
>         }
>  
> +       /* For DisplayPort, disable scrambling mode. */
> +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> +
>         /*
>          * We'll try to link train several times.  As part of link training
>          * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
> @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = np;
>         pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> -       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> +       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> +                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;

Anyone know if there's any expectation of other types here? Or is it
just as safe to do:

	pdata->bridge.type = pdata->next_bridge->type;

To achieve the same effect?

--
Kieran


>  
>         drm_bridge_add(&pdata->bridge);
>  
> -- 
> 2.32.0
>

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Kieran Bingham (2022-03-10 14:04:09)
> Quoting Kieran Bingham (2022-03-07 17:59:54)
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> > routed to a DisplayPort connector. Enable DisplayPort mode when the next
> > component in the display pipeline is detected as a DisplayPort
> > connector, and disable eDP features in that case.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reworked to set bridge type based on the next bridge/connector.
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > --
> > Changes since v1/RFC:
> >  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> >    devm_drm_of_get_bridge"
> >  - eDP/DP mode determined from the next bridge connector type.
> > 
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 29f5f7123ed9..461f963faa0b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -60,6 +60,7 @@
> >  #define SN_LN_ASSIGN_REG                       0x59
> >  #define  LN_ASSIGN_WIDTH                       2
> >  #define SN_ENH_FRAME_REG                       0x5A
> > +#define  ASSR_CONTROL                          BIT(0)
> >  #define  VSTREAM_ENABLE                                BIT(3)
> >  #define  LN_POLRS_OFFSET                       4
> >  #define  LN_POLRS_MASK                         0xf0
> > @@ -91,6 +92,8 @@
> >  #define SN_DATARATE_CONFIG_REG                 0x94
> >  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
> >  #define  DP_DATARATE(x)                                ((x) << 5)
> > +#define SN_TRAINING_SETTING_REG                        0x95
> > +#define  SCRAMBLE_DISABLE                      BIT(4)
> >  #define SN_ML_TX_MODE_REG                      0x96
> >  #define  ML_TX_MAIN_LINK_OFF                   0
> >  #define  ML_TX_NORMAL_MODE                     BIT(0)
> > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
> >                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
> >  
> > +       /* For DisplayPort, use the standard DP scrambler seed. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> > +                                  ASSR_CONTROL, 0);
> > +
> >         /* enable DP PLL */
> >         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
> >  
> > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >                 goto exit;
> >         }
> >  
> > +       /* For DisplayPort, disable scrambling mode. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> > +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> > +
> >         /*
> >          * We'll try to link train several times.  As part of link training
> >          * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
> > @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> >         pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > -       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> > +       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > +                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> 
> Anyone know if there's any expectation of other types here? Or is it
> just as safe to do:
> 
>         pdata->bridge.type = pdata->next_bridge->type;
> 
> To achieve the same effect?

Unfortunately, it would create nicer code - but I don't think it's
exactly right. It leaves the definition of our bridge type to the
connector type of the panel. While the SN65DSI86 can /only/ be either
eDP or DisplayPort.

So I'll keep this conditional on if the next connector is
DRM_MODE_CONNECTOR_DisplayPort.


> 
> --
> Kieran
> 
> 
> >  
> >         drm_bridge_add(&pdata->bridge);
> >  
> > -- 
> > 2.32.0
> >

Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode

[Kieran Bingham]

Quoting Kieran Bingham (2022-03-10 14:04:09)
> Quoting Kieran Bingham (2022-03-07 17:59:54)
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> > routed to a DisplayPort connector. Enable DisplayPort mode when the next
> > component in the display pipeline is detected as a DisplayPort
> > connector, and disable eDP features in that case.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reworked to set bridge type based on the next bridge/connector.
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > --
> > Changes since v1/RFC:
> >  - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> >    devm_drm_of_get_bridge"
> >  - eDP/DP mode determined from the next bridge connector type.
> > 
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 29f5f7123ed9..461f963faa0b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -60,6 +60,7 @@
> >  #define SN_LN_ASSIGN_REG                       0x59
> >  #define  LN_ASSIGN_WIDTH                       2
> >  #define SN_ENH_FRAME_REG                       0x5A
> > +#define  ASSR_CONTROL                          BIT(0)
> >  #define  VSTREAM_ENABLE                                BIT(3)
> >  #define  LN_POLRS_OFFSET                       4
> >  #define  LN_POLRS_MASK                         0xf0
> > @@ -91,6 +92,8 @@
> >  #define SN_DATARATE_CONFIG_REG                 0x94
> >  #define  DP_DATARATE_MASK                      GENMASK(7, 5)
> >  #define  DP_DATARATE(x)                                ((x) << 5)
> > +#define SN_TRAINING_SETTING_REG                        0x95
> > +#define  SCRAMBLE_DISABLE                      BIT(4)
> >  #define SN_ML_TX_MODE_REG                      0x96
> >  #define  ML_TX_MAIN_LINK_OFF                   0
> >  #define  ML_TX_NORMAL_MODE                     BIT(0)
> > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
> >                            DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
> >  
> > +       /* For DisplayPort, use the standard DP scrambler seed. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> > +                                  ASSR_CONTROL, 0);
> > +
> >         /* enable DP PLL */
> >         regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
> >  
> > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> >                 goto exit;
> >         }
> >  
> > +       /* For DisplayPort, disable scrambling mode. */
> > +       if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > +               regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> > +                                  SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> > +
> >         /*
> >          * We'll try to link train several times.  As part of link training
> >          * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER.  If
> > @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = np;
> >         pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > -       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> > +       pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > +                          ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> 
> Anyone know if there's any expectation of other types here? Or is it
> just as safe to do:
> 
>         pdata->bridge.type = pdata->next_bridge->type;
> 
> To achieve the same effect?

Unfortunately, it would create nicer code - but I don't think it's
exactly right. It leaves the definition of our bridge type to the
connector type of the panel. While the SN65DSI86 can /only/ be either
eDP or DisplayPort.

So I'll keep this conditional on if the next connector is
DRM_MODE_CONNECTOR_DisplayPort.


> 
> --
> Kieran
> 
> 
> >  
> >         drm_bridge_add(&pdata->bridge);
> >  
> > -- 
> > 2.32.0
> >

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Thu, Mar 10, 2022 at 3:43 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> Hi Doug,
>
> Quoting Doug Anderson (2022-03-07 19:52:08)
> > Hi,
> >
> > On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> > <kieran.bingham+renesas@ideasonboard.com> wrote:
> > >
> > > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > Implement the bridge connector-related .get_edid() operation, and report
> > > the related bridge capabilities and type.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - The connector .get_modes() operation doesn't rely on EDID anymore,
> > >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> > >   together
> > >
> > > Notes from Kieran:
> > >
> > > RB Tags collected from:
> > >  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
> > >
> > > However this was over a year ago, so let me know if other patches now
> > > superceed this one or otherwise invalidate this update.
> > >
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index c55848588123..ffb6c04f6c46 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > >         pm_runtime_put_sync(pdata->dev);
> > >  }
> > >
> > > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > > +                                         struct drm_connector *connector)
> > > +{
> > > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > > +       struct edid *edid;
> > > +
> > > +       pm_runtime_get_sync(pdata->dev);
> > > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > > +       pm_runtime_put_autosuspend(pdata->dev);
> > > +
> > > +       return edid;
> > > +}
> > > +
> > >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > >         .attach = ti_sn_bridge_attach,
> > >         .detach = ti_sn_bridge_detach,
> > > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > >         .enable = ti_sn_bridge_enable,
> > >         .disable = ti_sn_bridge_disable,
> > >         .post_disable = ti_sn_bridge_post_disable,
> > > +       .get_edid = ti_sn_bridge_get_edid,
> > >  };
> > >
> > >  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > >
> > >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> > >         pdata->bridge.of_node = np;
> > > +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > > +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> >
> > This doesn't look right to me. In the eDP case the EDID reading is
> > driven by the panel.
>
> Now that I have the optional connector working based on Sam's series I
> think this is the last issue to solve before reposting the DP/HPD
> support.
>
> Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID
> when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?

Yes.

-Doug

Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Doug Anderson]

Hi,

On Thu, Mar 10, 2022 at 3:43 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> Hi Doug,
>
> Quoting Doug Anderson (2022-03-07 19:52:08)
> > Hi,
> >
> > On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> > <kieran.bingham+renesas@ideasonboard.com> wrote:
> > >
> > > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > Implement the bridge connector-related .get_edid() operation, and report
> > > the related bridge capabilities and type.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - The connector .get_modes() operation doesn't rely on EDID anymore,
> > >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> > >   together
> > >
> > > Notes from Kieran:
> > >
> > > RB Tags collected from:
> > >  https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com/
> > >
> > > However this was over a year ago, so let me know if other patches now
> > > superceed this one or otherwise invalidate this update.
> > >
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index c55848588123..ffb6c04f6c46 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > >         pm_runtime_put_sync(pdata->dev);
> > >  }
> > >
> > > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > > +                                         struct drm_connector *connector)
> > > +{
> > > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > > +       struct edid *edid;
> > > +
> > > +       pm_runtime_get_sync(pdata->dev);
> > > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > > +       pm_runtime_put_autosuspend(pdata->dev);
> > > +
> > > +       return edid;
> > > +}
> > > +
> > >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > >         .attach = ti_sn_bridge_attach,
> > >         .detach = ti_sn_bridge_detach,
> > > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > >         .enable = ti_sn_bridge_enable,
> > >         .disable = ti_sn_bridge_disable,
> > >         .post_disable = ti_sn_bridge_post_disable,
> > > +       .get_edid = ti_sn_bridge_get_edid,
> > >  };
> > >
> > >  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > >
> > >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> > >         pdata->bridge.of_node = np;
> > > +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > > +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> >
> > This doesn't look right to me. In the eDP case the EDID reading is
> > driven by the panel.
>
> Now that I have the optional connector working based on Sam's series I
> think this is the last issue to solve before reposting the DP/HPD
> support.
>
> Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID
> when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?

Yes.

-Doug