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

[Laurentiu Palcu]

Hi Luca,

On Mon, Apr 27, 2026 at 02:59:51PM +0200, Luca Ceresoli wrote:
> Hello Laurentiu,
> 
> On Fri Apr 24, 2026 at 1:07 PM CEST, Laurentiu Palcu wrote:
> > From: Sandor Yu <Sandor.yu at 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 at nxp.com>
> > Co-developed-by: Laurentiu Palcu <laurentiu.palcu at oss.nxp.com>
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu at 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().

ack

> 
> > +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?

The else would be hit when there's another bridge after this one. But I
don't have a practical example since, on the i.MX8MQ, there is no other
bridge after it. However, as reviewers pointed out in earlier patchset
iterations, we needed to make it generic.

> 
> 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.

I believe you are right, I assumed each bridge has only one input
endpoint and one output endpoint... :/

> 
> 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.

I think this is a good idea. I see no reason why having 2 different
compatibles wouldn't work. I'll give it a try.

> 
> 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?

Yes, based on the PHY firmware (which is loaded by the ROM) we can have
HDMI or DP functionality.

> 
> > +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.

Ok, I'll switch to of_drm_find_and_get_bridge().

> 
> 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.

There's no good reason for this. I'll move the check at the beginning.

> 
> Same below for the HDMI version.

will do.

Thank you for your review.

-- 
Thanks,
Laurentiu