From: Chaoyi Chen <chaoyi.chen@rock-chips.com>

The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
the CDN-DP can be switched to output to one of the PHYs. If both ports
are plugged into DP, DP will select the first port for output.

This patch adds support for multiple bridges, enabling users to flexibly
select the output port. For each PHY port, a separate encoder and bridge
are registered.

The change is based on the DRM AUX HPD bridge, rather than the
extcon approach. This requires the DT to correctly describe the
connections between the first bridge in bridge chain and DP
controller. For example, the bridge chain may be like this:

PHY aux birdge -> fsa4480 analog audio switch bridge ->
onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge

In this case, the connection relationships among the PHY aux bridge
and the DP contorller need to be described in DT.

In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
determines whether to register one or two bridges.

Since there is only one DP controller, only one of the PHY ports can
output at a time. The key is how to switch between different PHYs,
which is handled by cdn_dp_switch_port() and cdn_dp_enable().

There are two cases:

1. Neither bridge is enabled. In this case, both bridges can
independently read the EDID, and the PHY port may switch before
reading the EDID.

2. One bridge is already enabled. In this case, other bridges are not
allowed to read the EDID. So we will try to return the cached EDID.

Since the scenario of two ports plug in at the same time is rare,
I don't have a board which support two TypeC connector to test this.
Therefore, I tested forced switching on a single PHY port, as well as
output using a fake PHY port alongside a real PHY port.

Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>
---

Changes in v9:
- Select DRM_AUX_HPD_BRIDGE when using DP driver.

(no changes since v7)

Changes in v6:
- Rename some variable names.
- Attach the DP bridge to the next bridge.

Changes in v5:
- By parsing the HPD bridge chain, set the connector's of_node to the
of_node corresponding to the USB-C connector.
- Return EDID cache when other port is already enabled.

 drivers/gpu/drm/rockchip/Kconfig       |   1 +
 drivers/gpu/drm/rockchip/cdn-dp-core.c | 329 ++++++++++++++++++++-----
 drivers/gpu/drm/rockchip/cdn-dp-core.h |  24 +-
 3 files changed, 297 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index b7b025814e72..10d9f29a3d44 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -56,6 +56,7 @@ config ROCKCHIP_CDN_DP
 	select DRM_DISPLAY_HELPER
 	select DRM_BRIDGE_CONNECTOR
 	select DRM_DISPLAY_DP_HELPER
+	select DRM_AUX_HPD_BRIDGE
 	help
 	  This selects support for Rockchip SoC specific extensions
 	  for the cdn DP driver. If you want to enable Dp on
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 1e27301584a4..5c0e5209d3b8 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -27,16 +27,17 @@
 #include "cdn-dp-core.h"
 #include "cdn-dp-reg.h"
 
-static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge)
+static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
+			      struct cdn_dp_port *port);
+
+static inline struct cdn_dp_bridge *bridge_to_dp_bridge(struct drm_bridge *bridge)
 {
-	return container_of(bridge, struct cdn_dp_device, bridge);
+	return container_of(bridge, struct cdn_dp_bridge, bridge);
 }
 
-static inline struct cdn_dp_device *encoder_to_dp(struct drm_encoder *encoder)
+static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge)
 {
-	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
-
-	return container_of(rkencoder, struct cdn_dp_device, encoder);
+	return bridge_to_dp_bridge(bridge)->parent;
 }
 
 #define GRF_SOC_CON9		0x6224
@@ -191,14 +192,27 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp, u8 *sink_count)
 static struct cdn_dp_port *cdn_dp_connected_port(struct cdn_dp_device *dp)
 {
 	struct cdn_dp_port *port;
-	int i, lanes;
+	int i, lanes[MAX_PHY];
 
 	for (i = 0; i < dp->ports; i++) {
 		port = dp->port[i];
-		lanes = cdn_dp_get_port_lanes(port);
-		if (lanes)
+		lanes[i] = cdn_dp_get_port_lanes(port);
+		if (!dp->next_bridge_valid)
 			return port;
 	}
+
+	if (dp->next_bridge_valid) {
+		/* If more than one port is available, pick the last active port */
+		if (dp->active_port > 0 && lanes[dp->active_port])
+			return dp->port[dp->active_port];
+
+		/* If the last active port is not available, pick an available port in order */
+		for (i = 0; i < dp->bridge_count; i++) {
+			if (lanes[i])
+				return dp->port[i];
+		}
+	}
+
 	return NULL;
 }
 
@@ -253,12 +267,45 @@ static const struct drm_edid *
 cdn_dp_bridge_edid_read(struct drm_bridge *bridge, struct drm_connector *connector)
 {
 	struct cdn_dp_device *dp = bridge_to_dp(bridge);
-	const struct drm_edid *drm_edid;
+	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
+	struct cdn_dp_port *port = dp->port[dp_bridge->id];
+	struct cdn_dp_port *prev_port;
+	const struct drm_edid *drm_edid = NULL;
+	int i, ret;
 
 	mutex_lock(&dp->lock);
+
+	/* More than one port is available */
+	if (dp->bridge_count > 1 && !port->phy_enabled) {
+		for (i = 0; i < dp->bridge_count; i++) {
+			/* Another port already enable */
+			if (dp->bridge_list[i] != dp_bridge && dp->bridge_list[i]->enabled)
+				goto get_cache;
+			/* Find already enabled port */
+			if (dp->port[i]->phy_enabled)
+				prev_port = dp->port[i];
+		}
+
+		/* Switch to current port */
+		if (prev_port) {
+			ret = cdn_dp_switch_port(dp, prev_port, port);
+			if (ret)
+				goto get_cache;
+		}
+	}
+
 	drm_edid = drm_edid_read_custom(connector, cdn_dp_get_edid_block, dp);
+	/* replace edid cache */
+	if (dp->edid_cache[dp_bridge->id])
+		drm_edid_free(dp->edid_cache[dp_bridge->id]);
+	dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid);
+
 	mutex_unlock(&dp->lock);
+	return drm_edid;
 
+get_cache:
+	drm_edid = drm_edid_dup(dp->edid_cache[dp_bridge->id]);
+	mutex_unlock(&dp->lock);
 	return drm_edid;
 }
 
@@ -267,12 +314,13 @@ cdn_dp_bridge_mode_valid(struct drm_bridge *bridge,
 			 const struct drm_display_info *display_info,
 			 const struct drm_display_mode *mode)
 {
+	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
 	struct cdn_dp_device *dp = bridge_to_dp(bridge);
 	u32 requested, actual, rate, sink_max, source_max = 0;
 	u8 lanes, bpc;
 
 	/* If DP is disconnected, every mode is invalid */
-	if (!dp->connected)
+	if (!dp_bridge->connected || !dp->connected)
 		return MODE_BAD;
 
 	switch (display_info->bpc) {
@@ -550,6 +598,54 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
 	return drm_dp_channel_eq_ok(link_status, min(port->lanes, sink_lanes));
 }
 
+static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
+			      struct cdn_dp_port *port)
+{
+	int ret;
+
+	if (dp->active)
+		return 0;
+
+	ret = cdn_dp_disable_phy(dp, prev_port);
+	if (ret)
+		goto out;
+	ret = cdn_dp_enable_phy(dp, port);
+	if (ret)
+		goto out;
+
+	ret = cdn_dp_get_sink_capability(dp);
+	if (ret) {
+		cdn_dp_disable_phy(dp, port);
+		goto out;
+	}
+
+	dp->active = true;
+	dp->lanes = port->lanes;
+
+	if (!cdn_dp_check_link_status(dp)) {
+		dev_info(dp->dev, "Connected with sink; re-train link\n");
+
+		ret = cdn_dp_train_link(dp);
+		if (ret) {
+			dev_err(dp->dev, "Training link failed: %d\n", ret);
+			goto out;
+		}
+
+		ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
+		if (ret) {
+			dev_err(dp->dev, "Failed to idle video %d\n", ret);
+			goto out;
+		}
+
+		ret = cdn_dp_config_video(dp);
+		if (ret)
+			dev_err(dp->dev, "Failed to configure video: %d\n", ret);
+	}
+
+out:
+	return ret;
+}
+
 static void cdn_dp_display_info_update(struct cdn_dp_device *dp,
 				       struct drm_display_info *display_info)
 {
@@ -571,6 +667,7 @@ static void cdn_dp_display_info_update(struct cdn_dp_device *dp,
 static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_atomic_state *state)
 {
 	struct cdn_dp_device *dp = bridge_to_dp(bridge);
+	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
 	struct drm_connector *connector;
 	int ret, val;
 
@@ -580,7 +677,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
 
 	cdn_dp_display_info_update(dp, &connector->display_info);
 
-	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, &dp->encoder.encoder);
+	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, &dp_bridge->encoder.encoder);
 	if (ret < 0) {
 		DRM_DEV_ERROR(dp->dev, "Could not get vop id, %d", ret);
 		return;
@@ -599,6 +696,9 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
 
 	mutex_lock(&dp->lock);
 
+	if (dp->next_bridge_valid)
+		dp->active_port = dp_bridge->id;
+
 	ret = cdn_dp_enable(dp);
 	if (ret) {
 		DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n",
@@ -631,6 +731,7 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
 		goto out;
 	}
 
+	dp_bridge->enabled = true;
 out:
 	mutex_unlock(&dp->lock);
 }
@@ -638,9 +739,11 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
 static void cdn_dp_bridge_atomic_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
 {
 	struct cdn_dp_device *dp = bridge_to_dp(bridge);
+	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
 	int ret;
 
 	mutex_lock(&dp->lock);
+	dp_bridge->enabled = false;
 
 	if (dp->active) {
 		ret = cdn_dp_disable(dp);
@@ -827,6 +930,16 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge *bridge,
 	return ret;
 }
 
+static void cdn_dp_bridge_hpd_notify(struct drm_bridge *bridge,
+			   enum drm_connector_status status)
+{
+	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
+	struct cdn_dp_device *dp = bridge_to_dp(bridge);
+
+	dp->bridge_list[dp_bridge->id]->connected = status == connector_status_connected;
+	schedule_work(&dp->event_work);
+}
+
 static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -837,6 +950,7 @@ static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
 	.atomic_disable = cdn_dp_bridge_atomic_disable,
 	.mode_valid = cdn_dp_bridge_mode_valid,
 	.mode_set = cdn_dp_bridge_mode_set,
+	.hpd_notify = cdn_dp_bridge_hpd_notify,
 
 	.dp_audio_prepare = cdn_dp_audio_prepare,
 	.dp_audio_mute_stream = cdn_dp_audio_mute_stream,
@@ -885,7 +999,8 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
 {
 	struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device,
 						event_work);
-	int ret;
+	bool connected;
+	int i, ret;
 
 	mutex_lock(&dp->lock);
 
@@ -944,9 +1059,12 @@ static void cdn_dp_pd_event_work(struct work_struct *work)
 
 out:
 	mutex_unlock(&dp->lock);
-	drm_bridge_hpd_notify(&dp->bridge,
-			      dp->connected ? connector_status_connected
-					    : connector_status_disconnected);
+	for (i = 0; i < dp->bridge_count; i++) {
+		connected = dp->connected && dp->bridge_list[i]->connected;
+		drm_bridge_hpd_notify(&dp->bridge_list[i]->bridge,
+				      connected ? connector_status_connected
+						: connector_status_disconnected);
+	}
 }
 
 static int cdn_dp_pd_event(struct notifier_block *nb,
@@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
+static int cdn_bridge_add(struct device *dev,
+			  struct drm_bridge *bridge,
+			  struct drm_bridge *next_bridge,
+			  struct drm_encoder *encoder)
 {
 	struct cdn_dp_device *dp = dev_get_drvdata(dev);
-	struct drm_encoder *encoder;
+	struct drm_device *drm_dev = dp->drm_dev;
+	struct drm_bridge *last_bridge = NULL;
 	struct drm_connector *connector;
-	struct cdn_dp_port *port;
-	struct drm_device *drm_dev = data;
-	int ret, i;
-
-	ret = cdn_dp_parse_dt(dp);
-	if (ret < 0)
-		return ret;
-
-	dp->drm_dev = drm_dev;
-	dp->connected = false;
-	dp->active = false;
-	dp->active_port = -1;
-	dp->fw_loaded = false;
-
-	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
-
-	encoder = &dp->encoder.encoder;
+	int ret;
 
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
 							     dev->of_node);
@@ -1002,26 +1108,38 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 
 	drm_encoder_helper_add(encoder, &cdn_dp_encoder_helper_funcs);
 
-	dp->bridge.ops =
-			DRM_BRIDGE_OP_DETECT |
-			DRM_BRIDGE_OP_EDID |
-			DRM_BRIDGE_OP_HPD |
-			DRM_BRIDGE_OP_DP_AUDIO;
-	dp->bridge.of_node = dp->dev->of_node;
-	dp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
-	dp->bridge.hdmi_audio_dev = dp->dev;
-	dp->bridge.hdmi_audio_max_i2s_playback_channels = 8;
-	dp->bridge.hdmi_audio_spdif_playback = 1;
-	dp->bridge.hdmi_audio_dai_port = -1;
-
-	ret = devm_drm_bridge_add(dev, &dp->bridge);
+
+	bridge->ops =
+		DRM_BRIDGE_OP_DETECT |
+		DRM_BRIDGE_OP_EDID |
+		DRM_BRIDGE_OP_HPD |
+		DRM_BRIDGE_OP_DP_AUDIO;
+	bridge->of_node = dp->dev->of_node;
+	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
+	bridge->hdmi_audio_dev = dp->dev;
+	bridge->hdmi_audio_max_i2s_playback_channels = 8;
+	bridge->hdmi_audio_spdif_playback = 1;
+	bridge->hdmi_audio_dai_port = -1;
+
+	ret = devm_drm_bridge_add(dev, bridge);
 	if (ret)
 		return ret;
 
-	ret = drm_bridge_attach(encoder, &dp->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret)
 		return ret;
 
+	if (next_bridge) {
+		ret = drm_bridge_attach(encoder, next_bridge, bridge,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (ret)
+			return ret;
+
+		last_bridge = next_bridge;
+		while (drm_bridge_get_next_bridge(last_bridge))
+			last_bridge = drm_bridge_get_next_bridge(last_bridge);
+	}
+
 	connector = drm_bridge_connector_init(drm_dev, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
@@ -1029,8 +1147,102 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
+	if (last_bridge)
+		connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
+
 	drm_connector_attach_encoder(connector, encoder);
 
+	return 0;
+}
+
+static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
+{
+	struct device_node *np = dp->dev->of_node;
+	struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
+	struct drm_bridge *bridge;
+	int count = 0;
+	int ret = 0;
+	int i;
+
+	/* If device use extcon, do not use hpd bridge */
+	for (i = 0; i < dp->ports; i++) {
+		if (dp->port[i]->extcon) {
+			dp->bridge_count = 1;
+			return 0;
+		}
+	}
+
+
+	/* One endpoint may correspond to one next bridge. */
+	for_each_of_graph_port_endpoint(port, dp_ep) {
+		struct device_node *next_bridge_node __free(device_node) =
+			of_graph_get_remote_port_parent(dp_ep);
+
+		bridge = of_drm_find_bridge(next_bridge_node);
+		if (!bridge) {
+			ret = -EPROBE_DEFER;
+			goto out;
+		}
+
+		dp->next_bridge_valid = true;
+		dp->next_bridge_list[count].bridge = bridge;
+		dp->next_bridge_list[count].parent = dp;
+		dp->next_bridge_list[count].id = count;
+		count++;
+	}
+
+out:
+	dp->bridge_count = count ? count : 1;
+	return ret;
+}
+
+static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
+{
+	struct cdn_dp_device *dp = dev_get_drvdata(dev);
+	struct drm_bridge *bridge, *next_bridge;
+	struct drm_encoder *encoder;
+	struct cdn_dp_port *port;
+	struct drm_device *drm_dev = data;
+	struct cdn_dp_bridge *dp_bridge;
+	int ret, i;
+
+	ret = cdn_dp_parse_dt(dp);
+	if (ret < 0)
+		return ret;
+
+	ret = cdn_dp_parse_next_bridge_dt(dp);
+	if (ret)
+		return ret;
+
+	dp->drm_dev = drm_dev;
+	dp->connected = false;
+	dp->active = false;
+	dp->active_port = -1;
+	dp->fw_loaded = false;
+
+	for (i = 0; i < dp->bridge_count; i++) {
+		dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, bridge,
+						    &cdn_dp_bridge_funcs);
+		if (IS_ERR(dp_bridge))
+			return PTR_ERR(dp_bridge);
+		dp_bridge->id = i;
+		dp_bridge->parent = dp;
+		if (!dp->next_bridge_valid)
+			dp_bridge->connected = true;
+		dp->bridge_list[i] = dp_bridge;
+	}
+
+	for (i = 0; i < dp->bridge_count; i++) {
+		encoder = &dp->bridge_list[i]->encoder.encoder;
+		bridge = &dp->bridge_list[i]->bridge;
+		next_bridge = dp->next_bridge_list[i].bridge;
+		ret = cdn_bridge_add(dev, bridge, next_bridge, encoder);
+		if (ret)
+			return ret;
+	}
+
+	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
+
 	for (i = 0; i < dp->ports; i++) {
 		port = dp->port[i];
 
@@ -1058,10 +1270,17 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct cdn_dp_device *dp = dev_get_drvdata(dev);
-	struct drm_encoder *encoder = &dp->encoder.encoder;
+	struct drm_encoder *encoder;
+	int i;
 
 	cancel_work_sync(&dp->event_work);
-	encoder->funcs->destroy(encoder);
+	for (i = 0; i < dp->bridge_count; i++) {
+		encoder = &dp->bridge_list[i]->encoder.encoder;
+		encoder->funcs->destroy(encoder);
+	}
+
+	for (i = 0; i < MAX_PHY; i++)
+		drm_edid_free(dp->edid_cache[i]);
 
 	pm_runtime_disable(dev);
 	if (dp->fw_loaded)
@@ -1112,10 +1331,10 @@ static int cdn_dp_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	dp = devm_drm_bridge_alloc(dev, struct cdn_dp_device, bridge,
-				   &cdn_dp_bridge_funcs);
-	if (IS_ERR(dp))
-		return PTR_ERR(dp);
+	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
+	if (!dp)
+		return -ENOMEM;
+
 	dp->dev = dev;
 
 	match = of_match_node(cdn_dp_dt_ids, pdev->dev.of_node);
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h b/drivers/gpu/drm/rockchip/cdn-dp-core.h
index e9c30b9fd543..ce1707a5c746 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
@@ -38,6 +38,8 @@ enum vic_pxl_encoding_format {
 	Y_ONLY = 0x10,
 };
 
+struct cdn_dp_device;
+
 struct video_info {
 	bool h_sync_polarity;
 	bool v_sync_polarity;
@@ -63,16 +65,34 @@ struct cdn_dp_port {
 	u8 id;
 };
 
+struct cdn_dp_bridge {
+	struct cdn_dp_device *parent;
+	struct drm_bridge bridge;
+	struct rockchip_encoder encoder;
+	bool connected;
+	bool enabled;
+	int id;
+};
+
+struct cdn_dp_next_bridge {
+	struct cdn_dp_device *parent;
+	struct drm_bridge *bridge;
+	int id;
+};
+
 struct cdn_dp_device {
 	struct device *dev;
 	struct drm_device *drm_dev;
-	struct drm_bridge bridge;
-	struct rockchip_encoder encoder;
+	int bridge_count;
+	struct cdn_dp_bridge *bridge_list[MAX_PHY];
+	struct cdn_dp_next_bridge next_bridge_list[MAX_PHY];
+	const struct drm_edid *edid_cache[MAX_PHY];
 	struct drm_display_mode mode;
 	struct platform_device *audio_pdev;
 	struct work_struct event_work;
 
 	struct mutex lock;
+	bool next_bridge_valid;
 	bool connected;
 	bool active;
 	bool suspended;
-- 
2.51.1

Hello Chaoyi,

On Tue Nov 11, 2025 at 11:50 AM CET, Chaoyi Chen wrote:
> From: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>
> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
> the CDN-DP can be switched to output to one of the PHYs. If both ports
> are plugged into DP, DP will select the first port for output.
>
> This patch adds support for multiple bridges, enabling users to flexibly
> select the output port. For each PHY port, a separate encoder and bridge
> are registered.
>
> The change is based on the DRM AUX HPD bridge, rather than the
> extcon approach. This requires the DT to correctly describe the
> connections between the first bridge in bridge chain and DP
> controller. For example, the bridge chain may be like this:
>
> PHY aux birdge -> fsa4480 analog audio switch bridge ->
> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge
>
> In this case, the connection relationships among the PHY aux bridge
> and the DP contorller need to be described in DT.
>
> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
> determines whether to register one or two bridges.
>
> Since there is only one DP controller, only one of the PHY ports can
> output at a time. The key is how to switch between different PHYs,
> which is handled by cdn_dp_switch_port() and cdn_dp_enable().
>
> There are two cases:
>
> 1. Neither bridge is enabled. In this case, both bridges can
> independently read the EDID, and the PHY port may switch before
> reading the EDID.
>
> 2. One bridge is already enabled. In this case, other bridges are not
> allowed to read the EDID. So we will try to return the cached EDID.
>
> Since the scenario of two ports plug in at the same time is rare,
> I don't have a board which support two TypeC connector to test this.
> Therefore, I tested forced switching on a single PHY port, as well as
> output using a fake PHY port alongside a real PHY port.
>
> Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>

[...]

> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>
> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +static int cdn_bridge_add(struct device *dev,
> +			  struct drm_bridge *bridge,
> +			  struct drm_bridge *next_bridge,
> +			  struct drm_encoder *encoder)
>  {
>  	struct cdn_dp_device *dp = dev_get_drvdata(dev);
> -	struct drm_encoder *encoder;
> +	struct drm_device *drm_dev = dp->drm_dev;
> +	struct drm_bridge *last_bridge = NULL;
>  	struct drm_connector *connector;
> -	struct cdn_dp_port *port;
> -	struct drm_device *drm_dev = data;
> -	int ret, i;

[...]

> +	if (next_bridge) {
> +		ret = drm_bridge_attach(encoder, next_bridge, bridge,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +		if (ret)
> +			return ret;
> +
> +		last_bridge = next_bridge;
> +		while (drm_bridge_get_next_bridge(last_bridge))
> +			last_bridge = drm_bridge_get_next_bridge(last_bridge);

DRM bridges are now refcounted, and you are not calling drm_bridge_get()
and drm_bridge_put() here. But here you can use
drm_bridge_chain_get_last_bridge() which will simplify your job.

Don't forget to call drm_bridge_put() on the returned bridge when the
bridge is not referenced anymore. This should be as easy as adding a
cleanup action on the variable declaration above:

-	struct drm_bridge *last_bridge = NULL;
+	struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;

> @@ -1029,8 +1147,102 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>
> +	if (last_bridge)
> +		connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
> +
>  	drm_connector_attach_encoder(connector, encoder);
>
> +	return 0;
> +}
> +
> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
> +{
> +	struct device_node *np = dp->dev->of_node;
> +	struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
> +	struct drm_bridge *bridge;
> +	int count = 0;
> +	int ret = 0;
> +	int i;
> +
> +	/* If device use extcon, do not use hpd bridge */
> +	for (i = 0; i < dp->ports; i++) {
> +		if (dp->port[i]->extcon) {
> +			dp->bridge_count = 1;
> +			return 0;
> +		}
> +	}
> +
> +
> +	/* One endpoint may correspond to one next bridge. */
> +	for_each_of_graph_port_endpoint(port, dp_ep) {
> +		struct device_node *next_bridge_node __free(device_node) =
> +			of_graph_get_remote_port_parent(dp_ep);
> +
> +		bridge = of_drm_find_bridge(next_bridge_node);
> +		if (!bridge) {
> +			ret = -EPROBE_DEFER;
> +			goto out;
> +		}
> +
> +		dp->next_bridge_valid = true;
> +		dp->next_bridge_list[count].bridge = bridge;

You are storing a reference to a drm_bridge, so have to increment the
refcount:

		dp->next_bridge_list[count].bridge = drm_bridge_get(bridge);
		                                     ^^^^^^^^^^^^^^

FYI there is a plan to replace of_drm_find_bridge() with a function that
increases the bridge refcount before returning the bridge, but it's not
there yet. When that will happen, the explicit drm_bridge_get() won't be
needed anymore and this code can be updated accordingly.

Also you have to call drm_bridge_put() to release that reference when the
pointer goes away. I guess that should happen in cdn_dp_unbind().

> +static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +{

In this function you do:
...(see below)...

> +	struct cdn_dp_device *dp = dev_get_drvdata(dev);
> +	struct drm_bridge *bridge, *next_bridge;
> +	struct drm_encoder *encoder;
> +	struct cdn_dp_port *port;
> +	struct drm_device *drm_dev = data;
> +	struct cdn_dp_bridge *dp_bridge;
> +	int ret, i;
> +
> +	ret = cdn_dp_parse_dt(dp);
> +	if (ret < 0)
.> +		return ret;
> +
> +	ret = cdn_dp_parse_next_bridge_dt(dp);

1. compute the next bridges and store them in dp->next_bridge_list[]
...

> +	if (ret)
> +		return ret;
> +
> +	dp->drm_dev = drm_dev;
> +	dp->connected = false;
> +	dp->active = false;
> +	dp->active_port = -1;
> +	dp->fw_loaded = false;
> +
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, bridge,
> +						    &cdn_dp_bridge_funcs);
> +		if (IS_ERR(dp_bridge))
> +			return PTR_ERR(dp_bridge);
> +		dp_bridge->id = i;
> +		dp_bridge->parent = dp;
> +		if (!dp->next_bridge_valid)
> +			dp_bridge->connected = true;
> +		dp->bridge_list[i] = dp_bridge;
> +	}
> +
> +	for (i = 0; i < dp->bridge_count; i++) {
> +		encoder = &dp->bridge_list[i]->encoder.encoder;
> +		bridge = &dp->bridge_list[i]->bridge;
> +		next_bridge = dp->next_bridge_list[i].bridge;
> +		ret = cdn_bridge_add(dev, bridge, next_bridge, encoder);

...
2. pass the dp->next_bridge_list[i].bridge to cdn_bridge_add
3. not use  dp->next_bridge_list[i] elsewhere

So you may want to change this function to parse into a local array, with
function scope. If you do this, the drm_bridge_get/put() I mentioned above
should still exist, but would be localized to this function, thus even
easier to handle.

Even better, you can parse the DT one bridge at a time inside the for loop,
so you don't need to store any next_bridge pointer array. This will need a
bit of rework of cdn_dp_parse_next_bridge_dt() though, and I haven't
checked in detail so it might be not worth.

[...]

> +struct cdn_dp_bridge {
> +	struct cdn_dp_device *parent;
> +	struct drm_bridge bridge;
> +	struct rockchip_encoder encoder;
> +	bool connected;
> +	bool enabled;
> +	int id;
> +};
> +
> +struct cdn_dp_next_bridge {
> +	struct cdn_dp_device *parent;
> +	struct drm_bridge *bridge;
> +	int id;

The @parent and @id fields are unused if I'm not mistaken.

If it is the case then you can... (see below)

>  struct cdn_dp_device {
>  	struct device *dev;
>  	struct drm_device *drm_dev;
> -	struct drm_bridge bridge;
> -	struct rockchip_encoder encoder;
> +	int bridge_count;
> +	struct cdn_dp_bridge *bridge_list[MAX_PHY];
> +	struct cdn_dp_next_bridge next_bridge_list[MAX_PHY];

...replace this line with:
	struct drm_bridge *next_bridge[MAX_PHY];

Unless of course you just don't store the next_bridge at all, as I
suggested above, and which looks way easier and more efficient.

Best regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Hello Luca,

On 11/11/2025 11:14 PM, Luca Ceresoli wrote:
> Hello Chaoyi,
>
> On Tue Nov 11, 2025 at 11:50 AM CET, Chaoyi Chen wrote:
>> From: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>>
>> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
>> the CDN-DP can be switched to output to one of the PHYs. If both ports
>> are plugged into DP, DP will select the first port for output.
>>
>> This patch adds support for multiple bridges, enabling users to flexibly
>> select the output port. For each PHY port, a separate encoder and bridge
>> are registered.
>>
>> The change is based on the DRM AUX HPD bridge, rather than the
>> extcon approach. This requires the DT to correctly describe the
>> connections between the first bridge in bridge chain and DP
>> controller. For example, the bridge chain may be like this:
>>
>> PHY aux birdge -> fsa4480 analog audio switch bridge ->
>> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge
>>
>> In this case, the connection relationships among the PHY aux bridge
>> and the DP contorller need to be described in DT.
>>
>> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
>> determines whether to register one or two bridges.
>>
>> Since there is only one DP controller, only one of the PHY ports can
>> output at a time. The key is how to switch between different PHYs,
>> which is handled by cdn_dp_switch_port() and cdn_dp_enable().
>>
>> There are two cases:
>>
>> 1. Neither bridge is enabled. In this case, both bridges can
>> independently read the EDID, and the PHY port may switch before
>> reading the EDID.
>>
>> 2. One bridge is already enabled. In this case, other bridges are not
>> allowed to read the EDID. So we will try to return the cached EDID.
>>
>> Since the scenario of two ports plug in at the same time is rare,
>> I don't have a board which support two TypeC connector to test this.
>> Therefore, I tested forced switching on a single PHY port, as well as
>> output using a fake PHY port alongside a real PHY port.
>>
>> Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>
> [...]
>
>> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>>   	return NOTIFY_DONE;
>>   }
>>
>> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>> +static int cdn_bridge_add(struct device *dev,
>> +			  struct drm_bridge *bridge,
>> +			  struct drm_bridge *next_bridge,
>> +			  struct drm_encoder *encoder)
>>   {
>>   	struct cdn_dp_device *dp = dev_get_drvdata(dev);
>> -	struct drm_encoder *encoder;
>> +	struct drm_device *drm_dev = dp->drm_dev;
>> +	struct drm_bridge *last_bridge = NULL;
>>   	struct drm_connector *connector;
>> -	struct cdn_dp_port *port;
>> -	struct drm_device *drm_dev = data;
>> -	int ret, i;
> [...]
>
>> +	if (next_bridge) {
>> +		ret = drm_bridge_attach(encoder, next_bridge, bridge,
>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +		if (ret)
>> +			return ret;
>> +
>> +		last_bridge = next_bridge;
>> +		while (drm_bridge_get_next_bridge(last_bridge))
>> +			last_bridge = drm_bridge_get_next_bridge(last_bridge);
> DRM bridges are now refcounted, and you are not calling drm_bridge_get()
> and drm_bridge_put() here. But here you can use
> drm_bridge_chain_get_last_bridge() which will simplify your job.
>
> Don't forget to call drm_bridge_put() on the returned bridge when the
> bridge is not referenced anymore. This should be as easy as adding a
> cleanup action on the variable declaration above:
>
> -	struct drm_bridge *last_bridge = NULL;
> +	struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;

Ah, I have seen your patch about this. Thank you for the reminder, I will fix this in v10.

>
>> @@ -1029,8 +1147,102 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>   		return ret;
>>   	}
>>
>> +	if (last_bridge)
>> +		connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
>> +
>>   	drm_connector_attach_encoder(connector, encoder);
>>
>> +	return 0;
>> +}
>> +
>> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
>> +{
>> +	struct device_node *np = dp->dev->of_node;
>> +	struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
>> +	struct drm_bridge *bridge;
>> +	int count = 0;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	/* If device use extcon, do not use hpd bridge */
>> +	for (i = 0; i < dp->ports; i++) {
>> +		if (dp->port[i]->extcon) {
>> +			dp->bridge_count = 1;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +
>> +	/* One endpoint may correspond to one next bridge. */
>> +	for_each_of_graph_port_endpoint(port, dp_ep) {
>> +		struct device_node *next_bridge_node __free(device_node) =
>> +			of_graph_get_remote_port_parent(dp_ep);
>> +
>> +		bridge = of_drm_find_bridge(next_bridge_node);
>> +		if (!bridge) {
>> +			ret = -EPROBE_DEFER;
>> +			goto out;
>> +		}
>> +
>> +		dp->next_bridge_valid = true;
>> +		dp->next_bridge_list[count].bridge = bridge;
> You are storing a reference to a drm_bridge, so have to increment the
> refcount:
>
> 		dp->next_bridge_list[count].bridge = drm_bridge_get(bridge);
> 		                                     ^^^^^^^^^^^^^^
>
> FYI there is a plan to replace of_drm_find_bridge() with a function that
> increases the bridge refcount before returning the bridge, but it's not
> there yet. When that will happen, the explicit drm_bridge_get() won't be
> needed anymore and this code can be updated accordingly.
>
> Also you have to call drm_bridge_put() to release that reference when the
> pointer goes away. I guess that should happen in cdn_dp_unbind().

You're right, this is indeed a pitfall. I will fix it in v10.

>
>> +static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>> +{
> In this function you do:
> ...(see below)...
>
>> +	struct cdn_dp_device *dp = dev_get_drvdata(dev);
>> +	struct drm_bridge *bridge, *next_bridge;
>> +	struct drm_encoder *encoder;
>> +	struct cdn_dp_port *port;
>> +	struct drm_device *drm_dev = data;
>> +	struct cdn_dp_bridge *dp_bridge;
>> +	int ret, i;
>> +
>> +	ret = cdn_dp_parse_dt(dp);
>> +	if (ret < 0)
> .> +		return ret;
>> +
>> +	ret = cdn_dp_parse_next_bridge_dt(dp);
> 1. compute the next bridges and store them in dp->next_bridge_list[]
> ...
>
>> +	if (ret)
>> +		return ret;
>> +
>> +	dp->drm_dev = drm_dev;
>> +	dp->connected = false;
>> +	dp->active = false;
>> +	dp->active_port = -1;
>> +	dp->fw_loaded = false;
>> +
>> +	for (i = 0; i < dp->bridge_count; i++) {
>> +		dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, bridge,
>> +						    &cdn_dp_bridge_funcs);
>> +		if (IS_ERR(dp_bridge))
>> +			return PTR_ERR(dp_bridge);
>> +		dp_bridge->id = i;
>> +		dp_bridge->parent = dp;
>> +		if (!dp->next_bridge_valid)
>> +			dp_bridge->connected = true;
>> +		dp->bridge_list[i] = dp_bridge;
>> +	}
>> +
>> +	for (i = 0; i < dp->bridge_count; i++) {
>> +		encoder = &dp->bridge_list[i]->encoder.encoder;
>> +		bridge = &dp->bridge_list[i]->bridge;
>> +		next_bridge = dp->next_bridge_list[i].bridge;
>> +		ret = cdn_bridge_add(dev, bridge, next_bridge, encoder);
> ...
> 2. pass the dp->next_bridge_list[i].bridge to cdn_bridge_add
> 3. not use  dp->next_bridge_list[i] elsewhere
>
> So you may want to change this function to parse into a local array, with
> function scope. If you do this, the drm_bridge_get/put() I mentioned above
> should still exist, but would be localized to this function, thus even
> easier to handle.
>
> Even better, you can parse the DT one bridge at a time inside the for loop,
> so you don't need to store any next_bridge pointer array. This will need a
> bit of rework of cdn_dp_parse_next_bridge_dt() though, and I haven't
> checked in detail so it might be not worth.

The next_bridge is obtained through of_drm_find_bridge(). According to your description just now, drm_bridge_put() needs to be called in cdn_dp_unbind(). So if I understand correctly, we do need to store this pointer.


>
> [...]
>
>> +struct cdn_dp_bridge {
>> +	struct cdn_dp_device *parent;
>> +	struct drm_bridge bridge;
>> +	struct rockchip_encoder encoder;
>> +	bool connected;
>> +	bool enabled;
>> +	int id;
>> +};
>> +
>> +struct cdn_dp_next_bridge {
>> +	struct cdn_dp_device *parent;
>> +	struct drm_bridge *bridge;
>> +	int id;
> The @parent and @id fields are unused if I'm not mistaken.
>
> If it is the case then you can... (see below)
>
>>   struct cdn_dp_device {
>>   	struct device *dev;
>>   	struct drm_device *drm_dev;
>> -	struct drm_bridge bridge;
>> -	struct rockchip_encoder encoder;
>> +	int bridge_count;
>> +	struct cdn_dp_bridge *bridge_list[MAX_PHY];
>> +	struct cdn_dp_next_bridge next_bridge_list[MAX_PHY];
> ...replace this line with:
> 	struct drm_bridge *next_bridge[MAX_PHY];
>
> Unless of course you just don't store the next_bridge at all, as I
> suggested above, and which looks way easier and more efficient.

Yes, the structure cdn_dp_next_bridge is only reserved for future expansion. I will replace it with a simpler struct drm_bridge in v10.

-- 
Best,
Chaoyi

Hi Luca,

On 11/12/2025 9:37 AM, Chaoyi Chen wrote:
> Hello Luca,
>
> On 11/11/2025 11:14 PM, Luca Ceresoli wrote:
>> Hello Chaoyi,
>>
>> On Tue Nov 11, 2025 at 11:50 AM CET, Chaoyi Chen wrote:
>>> From: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>>>
>>> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
>>> the CDN-DP can be switched to output to one of the PHYs. If both ports
>>> are plugged into DP, DP will select the first port for output.
>>>
>>> This patch adds support for multiple bridges, enabling users to flexibly
>>> select the output port. For each PHY port, a separate encoder and bridge
>>> are registered.
>>>
>>> The change is based on the DRM AUX HPD bridge, rather than the
>>> extcon approach. This requires the DT to correctly describe the
>>> connections between the first bridge in bridge chain and DP
>>> controller. For example, the bridge chain may be like this:
>>>
>>> PHY aux birdge -> fsa4480 analog audio switch bridge ->
>>> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge
>>>
>>> In this case, the connection relationships among the PHY aux bridge
>>> and the DP contorller need to be described in DT.
>>>
>>> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and
>>> determines whether to register one or two bridges.
>>>
>>> Since there is only one DP controller, only one of the PHY ports can
>>> output at a time. The key is how to switch between different PHYs,
>>> which is handled by cdn_dp_switch_port() and cdn_dp_enable().
>>>
>>> There are two cases:
>>>
>>> 1. Neither bridge is enabled. In this case, both bridges can
>>> independently read the EDID, and the PHY port may switch before
>>> reading the EDID.
>>>
>>> 2. One bridge is already enabled. In this case, other bridges are not
>>> allowed to read the EDID. So we will try to return the cached EDID.
>>>
>>> Since the scenario of two ports plug in at the same time is rare,
>>> I don't have a board which support two TypeC connector to test this.
>>> Therefore, I tested forced switching on a single PHY port, as well as
>>> output using a fake PHY port alongside a real PHY port.
>>>
>>> Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>
>> [...]
>>
>>> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>>>       return NOTIFY_DONE;
>>>   }
>>>
>>> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>> +static int cdn_bridge_add(struct device *dev,
>>> +              struct drm_bridge *bridge,
>>> +              struct drm_bridge *next_bridge,
>>> +              struct drm_encoder *encoder)
>>>   {
>>>       struct cdn_dp_device *dp = dev_get_drvdata(dev);
>>> -    struct drm_encoder *encoder;
>>> +    struct drm_device *drm_dev = dp->drm_dev;
>>> +    struct drm_bridge *last_bridge = NULL;
>>>       struct drm_connector *connector;
>>> -    struct cdn_dp_port *port;
>>> -    struct drm_device *drm_dev = data;
>>> -    int ret, i;
>> [...]
>>
>>> +    if (next_bridge) {
>>> +        ret = drm_bridge_attach(encoder, next_bridge, bridge,
>>> +                    DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        last_bridge = next_bridge;
>>> +        while (drm_bridge_get_next_bridge(last_bridge))
>>> +            last_bridge = drm_bridge_get_next_bridge(last_bridge);
>> DRM bridges are now refcounted, and you are not calling drm_bridge_get()
>> and drm_bridge_put() here. But here you can use
>> drm_bridge_chain_get_last_bridge() which will simplify your job.
>>
>> Don't forget to call drm_bridge_put() on the returned bridge when the
>> bridge is not referenced anymore. This should be as easy as adding a
>> cleanup action on the variable declaration above:
>>
>> -    struct drm_bridge *last_bridge = NULL;
>> +    struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
>
> Ah, I have seen your patch about this. Thank you for the reminder, I will fix this in v10.
>
>>
>>> @@ -1029,8 +1147,102 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>>>           return ret;
>>>       }
>>>
>>> +    if (last_bridge)
>>> +        connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
>>> +
>>>       drm_connector_attach_encoder(connector, encoder);
>>>
>>> +    return 0;
>>> +}
>>> +
>>> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
>>> +{
>>> +    struct device_node *np = dp->dev->of_node;
>>> +    struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
>>> +    struct drm_bridge *bridge;
>>> +    int count = 0;
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    /* If device use extcon, do not use hpd bridge */
>>> +    for (i = 0; i < dp->ports; i++) {
>>> +        if (dp->port[i]->extcon) {
>>> +            dp->bridge_count = 1;
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +
>>> +    /* One endpoint may correspond to one next bridge. */
>>> +    for_each_of_graph_port_endpoint(port, dp_ep) {
>>> +        struct device_node *next_bridge_node __free(device_node) =
>>> +            of_graph_get_remote_port_parent(dp_ep);
>>> +
>>> +        bridge = of_drm_find_bridge(next_bridge_node);
>>> +        if (!bridge) {
>>> +            ret = -EPROBE_DEFER;
>>> +            goto out;
>>> +        }
>>> +
>>> +        dp->next_bridge_valid = true;
>>> +        dp->next_bridge_list[count].bridge = bridge;
>> You are storing a reference to a drm_bridge, so have to increment the
>> refcount:
>>
>>         dp->next_bridge_list[count].bridge = drm_bridge_get(bridge);
>>                                              ^^^^^^^^^^^^^^
>>
>> FYI there is a plan to replace of_drm_find_bridge() with a function that
>> increases the bridge refcount before returning the bridge, but it's not
>> there yet. When that will happen, the explicit drm_bridge_get() won't be
>> needed anymore and this code can be updated accordingly.

Out of curiosity, I checked the callers of of_drm_find_bridge(), and it seems that the vast majority of them do not pay attention to the increase or decrease of reference counts. Does this mean that even if we add reference counting in of_drm_find_bridge(), we still need to modify the corresponding functions of their callers and decrease the reference count at the appropriate time? Thank you.


>>
>> Also you have to call drm_bridge_put() to release that reference when the
>> pointer goes away. I guess that should happen in cdn_dp_unbind().
>
> You're right, this is indeed a pitfall. I will fix it in v10.
>
>
-- 
Best,
Chaoyi

Hello Chaoyi,

On Mon Nov 17, 2025 at 2:33 AM CET, Chaoyi Chen wrote:
...
>>>> +    /* One endpoint may correspond to one next bridge. */
>>>> +    for_each_of_graph_port_endpoint(port, dp_ep) {
>>>> +        struct device_node *next_bridge_node __free(device_node) =
>>>> +            of_graph_get_remote_port_parent(dp_ep);
>>>> +
>>>> +        bridge = of_drm_find_bridge(next_bridge_node);
>>>> +        if (!bridge) {
>>>> +            ret = -EPROBE_DEFER;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        dp->next_bridge_valid = true;
>>>> +        dp->next_bridge_list[count].bridge = bridge;
>>> You are storing a reference to a drm_bridge, so have to increment the
>>> refcount:
>>>
>>>         dp->next_bridge_list[count].bridge = drm_bridge_get(bridge);
>>>                                              ^^^^^^^^^^^^^^
>>>
>>> FYI there is a plan to replace of_drm_find_bridge() with a function that
>>> increases the bridge refcount before returning the bridge, but it's not
>>> there yet. When that will happen, the explicit drm_bridge_get() won't be
>>> needed anymore and this code can be updated accordingly.
>
> Out of curiosity, I checked the callers of of_drm_find_bridge(), and it
> seems that the vast majority of them do not pay attention to the increase
> or decrease of reference counts.

They do not call drm_bridge_put() to decrease the refcount, and that's
"correct" because of_drm_find_bridge() does not increase it. This was
totally correct in the past because DRM bridge refcounting did not exist.

Refcounting has been added to support hot-pluggable bridges. If you want
more info, this presentation I gave at ELCE 2025 is a good summary, with
links to relevant patches:

 * Video (start at 19:30): https://www.youtube.com/watch?v=msmBQBSyZZ4
 * Slides (start at slide 27):
   https://bootlin.com/pub/conferences/2025/elce/ceresoli-hotplug-status.pdf

> Does this mean that even if we add reference counting in
> of_drm_find_bridge(), we still need to modify the corresponding functions
> of their callers and decrease the reference count at the appropriate
> time?

Exactly. I have explored that approach but it turned out being not
reasonably doable due to the large number of (direct + indirect) callers of
of_drm_find_bridge() as well as the trickiness of those involvingthe
panel_bridge. So Maxime proposed a different approach [0]: deprecate
of_drm_find_bridge() and replace it with a function that increments the
refcount, then let the various callers move to the new function over time.

Earlier today I sent a series doing that, and converting lots of users
[1]. If/when that approach will be accepted, you can update your driver to
use the new drm_of_fund_bridge() and take care of putting the reference
when appropriate. But you don't need to do anything until then.

[0] https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/
[1] https://lore.kernel.org/lkml/20251119-drm-bridge-alloc-getput-drm_of_find_bridge-v1-0-0db98a7fe474@bootlin.com/

> Thank you.

You're welcome. I hope it was a clear explanation.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com