Re: [PATCH v22 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

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

Hello Laurentiu,

On Fri Apr 24, 2026 at 1:07 PM CEST, Laurentiu Palcu wrote:
> From: Sandor Yu <Sandor.yu@nxp.com>
>
> Add a new DRM DisplayPort and HDMI bridge driver for Candence MHDP8501
> used in i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort
> standards according embedded Firmware running in the uCPU.
>
> For iMX8MQ SOC, the DisplayPort/HDMI FW was loaded and activated by
> SOC's ROM code. Bootload binary included respective specific firmware
> is required.
>
> Driver will check display connector type and
> then load the corresponding driver.
>
> Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> Co-developed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

...

> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
...
> +enum drm_connector_status cdns_mhdp8501_detect(struct drm_bridge *bridge,
> +					       struct drm_connector *connector)
> +{
> +	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;

Please don't use driver_private. Write a oneliner function wrapping
container_of(). There are many examples in bridges,
e.g. bridge_to_sn65dsi83().

> +static int cdns_mhdp8501_get_bridge_type(struct device_node *out_ep,
> +					 int *bridge_type)
> +{
> +	struct device_node *incoming_ep, *node, *ep;
> +	int ret = -ENODEV;
> +
> +	incoming_ep = of_graph_get_remote_endpoint(out_ep);
> +	if (!incoming_ep)
> +		return -ENODEV;
> +
> +	node = of_graph_get_port_parent(incoming_ep);
> +	if (!node) {
> +		of_node_put(incoming_ep);
> +		return -ENODEV;
> +	}
> +
> +	if (of_device_is_compatible(node, "hdmi-connector")) {
> +		*bridge_type = DRM_MODE_CONNECTOR_HDMIA;
> +		ret = 0;
> +	} else if (of_device_is_compatible(node, "dp-connector")) {
> +		*bridge_type = DRM_MODE_CONNECTOR_DisplayPort;
> +		ret = 0;
> +	} else {
> +		for_each_endpoint_of_node(node, ep) {
> +			if (ep == incoming_ep)
> +				continue;
> +
> +			ret = cdns_mhdp8501_get_bridge_type(ep, bridge_type);
> +			if (!ret) {
> +				of_node_put(ep);
> +				break;
> +			}
> +		}
> +	}

I don't follow what this logic is doing. Can you provide a practical
example of the "next node" (@node variable) where you fall in the else
case?

Also, while this resursion will probably work in most, if not all,
realistic cases, it could take incorrect decisions. Consider the case there
in the else branch your @node points to some node having two input
endpoints: ep0 is the incoming_ep and ep1 is another input endpoint. In
such case you would recurse on ep1 and return its bridge type, which
however has nothing to to with the output and might be incorrect.

Another question is whether this driver should have two compatible strings,
one for hdmi and one for dp, and set the bridge_type based on that. This
would make it a lot simpler and remove the need for this function.

But if I guess right from the code, this device can output either hdmi or
dp, and the implementation infers the type based on this device tree
walk. Is it the case?

> +static int cdns_mhdp8501_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdns_mhdp8501_device *mhdp;
> +	const struct drm_bridge_funcs *bridge_funcs;
> +	enum phy_mode phy_mode;
> +	struct resource *res;
> +	u32 lane_mapping;
> +	int bridge_type;
> +	u32 reg;
> +	int ret;
> +
> +	ret = cdns_mhdp8501_dt_parse(pdev, &bridge_type, &lane_mapping);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		return ret;
> +
> +	bridge_funcs = (bridge_type == DRM_MODE_CONNECTOR_HDMIA) ?
> +			&cdns_hdmi_bridge_funcs : &cdns_dp_bridge_funcs;
> +
> +	mhdp = devm_drm_bridge_alloc(dev, struct cdns_mhdp8501_device,
> +				     bridge, bridge_funcs);
> +	if (!mhdp)
> +		return -ENOMEM;
> +
> +	mhdp->dev = dev;
> +	mhdp->bridge_type = bridge_type;
> +	mhdp->lane_mapping = lane_mapping;
> +
> +	mhdp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> +	if (IS_ERR(mhdp->next_bridge))
> +		return dev_err_probe(dev, PTR_ERR(mhdp->next_bridge),
> +				     "failed to get next bridge\n");

devm_drm_of_get_bridge() is there to either create a new panel_bridge
wrapping a panel or return an existing bridge. However based on the
cdns_mhdp8501_get_bridge_type() code it seems to me that you will always
have another bridge after this bridge. And so instead of
devm_drm_of_get_bridge() you should use of_drm_find_and_get_bridge(),
which handles bridge refcounting.

When switching to it, you additionally can use the drm_bridge::next_bridge
pointer instead of having your mhdp->next_bridge. This will simplify
putting the bridge reference. An example of its usage is in [0].

[0] https://lore.kernel.org/lkml/20260109-drm-bridge-alloc-getput-drm_of_find_bridge-2-v2-4-8bad3ef90b9f@bootlin.com/

> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
...
> +static int cdns_dp_bridge_attach(struct drm_bridge *bridge,
> +				 struct drm_encoder *encoder,
> +				 enum drm_bridge_attach_flags flags)
> +{
> +	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> +	int ret;
> +
> +	ret = drm_bridge_attach(encoder, mhdp->next_bridge, bridge,
> +				flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		dev_err(mhdp->dev, "do not support creating a drm_connector\n");
> +		return -EINVAL;
> +	}

Any good reason for doing this check after calling drm_bridge_attach()? It
looks to me that you should first check for valid arguments, and if they
pass take any actions.

Same below for the HDMI version.

Luca

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