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

Laurentiu Palcu
2026-04-27T14:36:04Z

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@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().

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