From: Liu Ying <victor.liu@nxp.com>
The next bridge in bridge chain could be a panel bridge or a non-panel
bridge. Use devm_drm_of_get_bridge() to replace the combination
function calls of of_drm_find_panel() and devm_drm_panel_bridge_add()
to get either a panel bridge or a non-panel bridge, instead of getting
a panel bridge only.
Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Francesco Valla <francesco@valla.it>
Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
drivers/gpu/drm/bridge/fsl-ldb.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 7b71cde173e0c..d59f26016de26 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -15,7 +15,6 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
#include <drm/drm_of.h>
-#include <drm/drm_panel.h>
#define LDB_CTRL_CH0_ENABLE BIT(0)
#define LDB_CTRL_CH0_DI_SELECT BIT(1)
@@ -86,7 +85,7 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
struct fsl_ldb {
struct device *dev;
struct drm_bridge bridge;
- struct drm_bridge *panel_bridge;
+ struct drm_bridge *next_bridge;
struct clk *clk;
struct regmap *regmap;
const struct fsl_ldb_devdata *devdata;
@@ -119,7 +118,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
{
struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
- return drm_bridge_attach(encoder, fsl_ldb->panel_bridge,
+ return drm_bridge_attach(encoder, fsl_ldb->next_bridge,
bridge, flags);
}
@@ -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);
+ if (IS_ERR(fsl_ldb->next_bridge))
+ return dev_err_probe(dev, PTR_ERR(fsl_ldb->next_bridge),
+ "failed to get next bridge\n");
if (fsl_ldb_is_dual(fsl_ldb)) {
struct device_node *port1, *port2;
--
2.51.0On Wed, Mar 04, 2026 at 11:34:11AM +0000, Laurentiu Palcu wrote:
> From: Liu Ying <victor.liu@nxp.com>
>
> The next bridge in bridge chain could be a panel bridge or a non-panel
> bridge. Use devm_drm_of_get_bridge() to replace the combination
> function calls of of_drm_find_panel() and devm_drm_panel_bridge_add()
> to get either a panel bridge or a non-panel bridge, instead of getting
> a panel bridge only.
>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Reviewed-by: Francesco Valla <francesco@valla.it>
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
> drivers/gpu/drm/bridge/fsl-ldb.c | 31 +++++++++++--------------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 7b71cde173e0c..d59f26016de26 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -15,7 +15,6 @@
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> #include <drm/drm_of.h>
> -#include <drm/drm_panel.h>
>
> #define LDB_CTRL_CH0_ENABLE BIT(0)
> #define LDB_CTRL_CH0_DI_SELECT BIT(1)
> @@ -86,7 +85,7 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> struct fsl_ldb {
> struct device *dev;
> struct drm_bridge bridge;
> - struct drm_bridge *panel_bridge;
> + struct drm_bridge *next_bridge;
> struct clk *clk;
> struct regmap *regmap;
> const struct fsl_ldb_devdata *devdata;
> @@ -119,7 +118,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> {
> struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>
> - return drm_bridge_attach(encoder, fsl_ldb->panel_bridge,
> + return drm_bridge_attach(encoder, fsl_ldb->next_bridge,
> bridge, flags);
> }
>
> @@ -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.
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()?
[1] 3fdeae134ba9 drm/bridge: add next_bridge pointer to struct drm_bridge
[2] https://lore.kernel.org/all/20260303-v6-18-topic-imx93-parallel-display-v11-2-1b03733c8461@pengutronix.de/
> + if (IS_ERR(fsl_ldb->next_bridge))
> + return dev_err_probe(dev, PTR_ERR(fsl_ldb->next_bridge),
> + "failed to get next bridge\n");
>
> if (fsl_ldb_is_dual(fsl_ldb)) {
> struct device_node *port1, *port2;
>
--
Regards,
Liu Ying
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-6f1a7d03c45f@bootlin.com/
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello again,
On Tue Mar 10, 2026 at 12:34 PM CET, Luca Ceresoli wrote:
> 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-6f1a7d03c45f@bootlin.com/
All of that said, afer double checking devm_drm_of_get_bridge() I agree
drm_get_bridge() whoudl be called on the returned pointer when assigning
it:
next_bridge = devm_drm_of_get_bridge(...);
if (IS_ERR(next_bridge))
return (after cleanup acrtions if applicable)
fsl_ldb->next_bridge = drm_get_bridge(next_bridge);
At least this will avoid use-after-free in case the bridge is removed. It
might lead to a memory leak in some cases, not sure, but it's way better
than use-after-free especially as hotplug is not currently supported.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
© 2016 - 2026 Red Hat, Inc.