Hello Chaoyi,
On Tue Nov 11, 2025 at 11:50 AM CET, Chaoyi Chen wrote:
> From: Chaoyi Chen <[email protected]>
>
> 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 <[email protected]>
[...]
> @@ -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
