Re: [PATCH v9 08/10] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

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