Hello Liu, Maxime,
On Fri Mar 6, 2026 at 8:36 AM CET, Liu Ying wrote:
>> @@ -296,9 +295,7 @@ static const struct drm_bridge_funcs funcs = {
>> static int fsl_ldb_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> - struct device_node *panel_node;
>> struct device_node *remote1, *remote2;
>> - struct drm_panel *panel;
>> struct fsl_ldb *fsl_ldb;
>> int dual_link;
>>
>> @@ -321,36 +318,30 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>> if (IS_ERR(fsl_ldb->regmap))
>> return PTR_ERR(fsl_ldb->regmap);
>>
>> - /* Locate the remote ports and the panel node */
>> + /* Locate the remote ports. */
>> remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
>> remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
>> fsl_ldb->ch0_enabled = (remote1 != NULL);
>> fsl_ldb->ch1_enabled = (remote2 != NULL);
>> - panel_node = of_node_get(remote1 ? remote1 : remote2);
>> of_node_put(remote1);
>> of_node_put(remote2);
>>
>> - if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
>> - of_node_put(panel_node);
>> - return dev_err_probe(dev, -ENXIO, "No panel node found");
>> - }
>> + if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled)
>> + return dev_err_probe(dev, -ENXIO, "No next bridge node found");
>>
>> dev_dbg(dev, "Using %s\n",
>> fsl_ldb_is_dual(fsl_ldb) ? "dual-link mode" :
>> fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
>>
>> - panel = of_drm_find_panel(panel_node);
>> - of_node_put(panel_node);
>> - if (IS_ERR(panel))
>> - return PTR_ERR(panel);
>> -
>> if (of_property_present(dev->of_node,
>> "nxp,enable-termination-resistor"))
>> fsl_ldb->use_termination_resistor = true;
>>
>> - fsl_ldb->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>> - if (IS_ERR(fsl_ldb->panel_bridge))
>> - return PTR_ERR(fsl_ldb->panel_bridge);
>> -
>> + fsl_ldb->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
>> + fsl_ldb->ch0_enabled ? 1
>> : 2,
>> + 0);
>
> Cc'ing Luca.
Thanks Liu!
@Laurentiu: can you please Cc me on the whole series for future iterations?
BTW b4 does that by default, you may consider using it, I find it a great
tool.
> Since commit[1] added next_bridge pointer to struct drm_bridge, can you
> use that pointer instead of fsl_ldb->next_bridge?
> This would be similar to how the in-flight imx93-pdfc.c driver[2] does.
>
> However, after looking at commit[1] closely, I wonder if we need to call
> drm_bridge_get() for the next_bridge returned from devm_drm_of_get_bridge()
> because drm_bridge_put() would be called for the next_bridge when this
> bridge(the next_bridge's previous bridge) is freed in __drm_bridge_free().
> @Luca, can you please comment here? I see your R-b tag on [2] where
> drm_bridge_get() is not called, does it mean that we don't need to call
> drm_bridge_get()?
This is tricky because devm_drm_of_get_bridge() is used. As a matter of
fact, none of the *_of_get_bridge() variants allows proper bridge
refcounting. This is because they could return either a pointer to a
panel_bridge they create on the fly, or a pointer to a pre-existing bridge:
those need different removal actions but the caller does not know which of
the two got returned.
In other words, the *_of_get_bridge() is broken if bridge hotplug is added.
Some discussion here [0], it's a bit outdated but I coundn't find a more
recent one which I think exists.
So, being bridge hotplug not yet supported in the mainline kernel, there is
no visible problem and refcounting does never really come into play, so
using *_of_get_bridge() is OK. I'm adding my Reviewed-by to patches using
it just because there is no alternative currently.
I'm working on having correct refcount handling "everywhere" as a
prerequisite to introducing bridge hotplug (here [1] the steps done and in
progress). Almost all APIs have been converted but *_of_get_bridge() is the
final one and as of now not cleanly doable.
Maxime AFAIK has a plan to rework the panel bridge lifetime, which would
solve this issue at its root. Until that happens, the best we can do is
just ensure no bridge hotplug happens involving driver which use
*_of_get_bridge().
I hope this clarifies the situation a bit.
[0] https://lore.kernel.org/lkml/20250227-macho-convivial-tody-cea7dc@houat/
[1]
https://lore.kernel.org/lkml/20260206-drm-bridge-atomic-vs-remove-clear_and_put-v1-0-6f1a7d03c...@bootlin.com/
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
