[PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Marek Vasut]

The DT binding for this bridge describe register offsets for the LDB,
parse the register offsets from DT instead of hard-coding them in the
driver. No functional change.

Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: Abel Vesa <abelvesa@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Liu Ying <victor.liu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
---
V2: - Switch to of_property_read_reg()
    - Parse single-register LDB variants from DT too
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 58 ++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 5c3cf37200bce..2357cb2fbbe39 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -8,6 +8,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -61,24 +62,13 @@ enum fsl_ldb_devtype {
 };
 
 struct fsl_ldb_devdata {
-	u32 ldb_ctrl;
-	u32 lvds_ctrl;
 	bool lvds_en_bit;
-	bool single_ctrl_reg;
 };
 
 static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
-	[IMX6SX_LDB] = {
-		.ldb_ctrl = 0x18,
-		.single_ctrl_reg = true,
-	},
-	[IMX8MP_LDB] = {
-		.ldb_ctrl = 0x5c,
-		.lvds_ctrl = 0x128,
-	},
+	[IMX6SX_LDB] = { },
+	[IMX8MP_LDB] = { },
 	[IMX93_LDB] = {
-		.ldb_ctrl = 0x20,
-		.lvds_ctrl = 0x24,
 		.lvds_en_bit = true,
 	},
 };
@@ -90,8 +80,11 @@ struct fsl_ldb {
 	struct clk *clk;
 	struct regmap *regmap;
 	const struct fsl_ldb_devdata *devdata;
+	u64 ldb_ctrl;
+	u64 lvds_ctrl;
 	bool ch0_enabled;
 	bool ch1_enabled;
+	bool single_ctrl_reg;
 };
 
 static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
@@ -204,15 +197,15 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 		reg |=	(fsl_ldb->ch0_enabled ? LDB_CTRL_DI0_VSYNC_POLARITY : 0) |
 			(fsl_ldb->ch1_enabled ? LDB_CTRL_DI1_VSYNC_POLARITY : 0);
 
-	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, reg);
+	regmap_write(fsl_ldb->regmap, fsl_ldb->ldb_ctrl, reg);
 
-	if (fsl_ldb->devdata->single_ctrl_reg)
+	if (fsl_ldb->single_ctrl_reg)
 		return;
 
 	/* Program LVDS_CTRL */
 	reg = LVDS_CTRL_CC_ADJ(2) | LVDS_CTRL_PRE_EMPH_EN |
 	      LVDS_CTRL_PRE_EMPH_ADJ(3) | LVDS_CTRL_VBG_EN;
-	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
+	regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, reg);
 
 	/* Wait for VBG to stabilize. */
 	usleep_range(15, 20);
@@ -220,7 +213,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 	reg |=	(fsl_ldb->ch0_enabled ? LVDS_CTRL_CH0_EN : 0) |
 		(fsl_ldb->ch1_enabled ? LVDS_CTRL_CH1_EN : 0);
 
-	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
+	regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, reg);
 }
 
 static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
@@ -231,12 +224,12 @@ static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
 	/* Stop channel(s). */
 	if (fsl_ldb->devdata->lvds_en_bit)
 		/* Set LVDS_CTRL_LVDS_EN bit to disable. */
-		regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl,
+		regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl,
 			     LVDS_CTRL_LVDS_EN);
 	else
-		if (!fsl_ldb->devdata->single_ctrl_reg)
-			regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, 0);
-	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, 0);
+		if (!fsl_ldb->single_ctrl_reg)
+			regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, 0);
+	regmap_write(fsl_ldb->regmap, fsl_ldb->ldb_ctrl, 0);
 
 	clk_disable_unprepare(fsl_ldb->clk);
 }
@@ -296,7 +289,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	struct device_node *remote1, *remote2;
 	struct drm_panel *panel;
 	struct fsl_ldb *fsl_ldb;
-	int dual_link;
+	int dual_link, idx, ret;
 
 	fsl_ldb = devm_drm_bridge_alloc(dev, struct fsl_ldb, bridge, &funcs);
 	if (IS_ERR(fsl_ldb))
@@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	fsl_ldb->dev = &pdev->dev;
 	fsl_ldb->bridge.of_node = dev->of_node;
 
+	/* No "reg-names" property likely means single-register LDB */
+	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
+	if (idx < 0) {
+		fsl_ldb->single_ctrl_reg = true;
+		idx = 0;
+	}
+
+	ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->ldb_ctrl, NULL);
+	if (ret)
+		return ret;
+
+	if (!fsl_ldb->single_ctrl_reg) {
+		idx = of_property_match_string(dev->of_node, "reg-names", "lvds");
+		if (idx < 0)
+			return idx;
+
+		ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->lvds_ctrl, NULL);
+		if (ret)
+			return ret;
+	}
+
 	fsl_ldb->clk = devm_clk_get(dev, "ldb");
 	if (IS_ERR(fsl_ldb->clk))
 		return PTR_ERR(fsl_ldb->clk);
-- 
2.51.0

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Luca Ceresoli]

Hello Marek,

+Laurentiu Palcu

On Sun Nov 2, 2025 at 6:02 PM CET, Marek Vasut wrote:
> The DT binding for this bridge describe register offsets for the LDB,
> parse the register offsets from DT instead of hard-coding them in the
> driver. No functional change.
>
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>

I was initially a bit skeptical because normally register offsets are
derived from the compatible string, not from device tree. But then I
realized this is about the LDB which has its two registers in the
MEDIA_BLK. This means all in all this looks somewhat like an integration
aspect (the LDB component uses two resources of the MEDIA_CLK component)
and your patch mekse sense.

So my only remark is that the above may be in the commit message, to make
the "why" clear from the beginning. It took a bit of research for me to
find out.

Laurentiu's patch adding i.MX94 support will conflict with this
one. Whichever gets applied after the other will have to be adapted
accordingly.

[0] https://lore.kernel.org/dri-devel/20251103-dcif-upstreaming-v6-3-76fcecfda919@oss.nxp.com/

> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	fsl_ldb->dev = &pdev->dev;
>  	fsl_ldb->bridge.of_node = dev->of_node;
>
> +	/* No "reg-names" property likely means single-register LDB */

Uh? If it is "likely" it means we are not sure this code is not introducing
regressions, and that would be bad.

> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
> +	if (idx < 0) {
> +		fsl_ldb->single_ctrl_reg = true;
> +		idx = 0;
> +	}

From the bindings I understand that having two 'reg' values and no
'reg-names' at all is legal. Your patch implies differently. Who's right
here?

> +	ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->ldb_ctrl, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (!fsl_ldb->single_ctrl_reg) {
> +		idx = of_property_match_string(dev->of_node, "reg-names", "lvds");
> +		if (idx < 0)
> +			return idx;
> +
> +		ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->lvds_ctrl, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +

Best regards,
Luca

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

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Marek Vasut]

On 11/3/25 4:55 PM, Luca Ceresoli wrote:

Hello Luca,

> On Sun Nov 2, 2025 at 6:02 PM CET, Marek Vasut wrote:
>> The DT binding for this bridge describe register offsets for the LDB,
>> parse the register offsets from DT instead of hard-coding them in the
>> driver. No functional change.
>>
>> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> 
> I was initially a bit skeptical because normally register offsets are
> derived from the compatible string, not from device tree. But then I
> realized this is about the LDB which has its two registers in the
> MEDIA_BLK. This means all in all this looks somewhat like an integration
> aspect (the LDB component uses two resources of the MEDIA_CLK component)
> and your patch mekse sense.
> 
> So my only remark is that the above may be in the commit message, to make
> the "why" clear from the beginning. It took a bit of research for me to
> find out.

Actually, the LDB was always meant to parse the 'reg' offsets out of the 
DT, it then went somewhat ... wrong ... and we ended up with hard-coded 
reg<->compatible mapping. It was never intended to be that way. That is 
all there is to it, there isn't any deeper reason behind it.

What would you add into the commit message ?

> Laurentiu's patch adding i.MX94 support will conflict with this
> one. Whichever gets applied after the other will have to be adapted
> accordingly.

It would be good to clean this driver up before we add more functionality.

> [0] https://lore.kernel.org/dri-devel/20251103-dcif-upstreaming-v6-3-76fcecfda919@oss.nxp.com/
> 
>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>   	fsl_ldb->dev = &pdev->dev;
>>   	fsl_ldb->bridge.of_node = dev->of_node;
>>
>> +	/* No "reg-names" property likely means single-register LDB */
> 
> Uh? If it is "likely" it means we are not sure this code is not introducing
> regressions, and that would be bad.

I can drop the 'likely' part.

>> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
>> +	if (idx < 0) {
>> +		fsl_ldb->single_ctrl_reg = true;
>> +		idx = 0;
>> +	}
> 
>  From the bindings I understand that having two 'reg' values and no
> 'reg-names' at all is legal. Your patch implies differently. Who's right
> here?
I think if you have two two reg values, you should have reg-names , so 
the binding should be updated ?

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Liu Ying]

On 11/3/25 01:02, Marek Vasut wrote:
> The DT binding for this bridge describe register offsets for the LDB,

I'm repeating my comment on v1:
s/describe/describes/

> parse the register offsets from DT instead of hard-coding them in the
> driver. No functional change.
> 
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Liu Ying <victor.liu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
> V2: - Switch to of_property_read_reg()
>     - Parse single-register LDB variants from DT too
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 58 ++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 5c3cf37200bce..2357cb2fbbe39 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -8,6 +8,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> @@ -61,24 +62,13 @@ enum fsl_ldb_devtype {
>  };
>  
>  struct fsl_ldb_devdata {
> -	u32 ldb_ctrl;
> -	u32 lvds_ctrl;
>  	bool lvds_en_bit;
> -	bool single_ctrl_reg;
>  };
>  
>  static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {

As I pointed out in v1 comment, this patch should remove struct
fsl_ldb_devdata.

> -	[IMX6SX_LDB] = {
> -		.ldb_ctrl = 0x18,
> -		.single_ctrl_reg = true,
> -	},
> -	[IMX8MP_LDB] = {
> -		.ldb_ctrl = 0x5c,
> -		.lvds_ctrl = 0x128,
> -	},
> +	[IMX6SX_LDB] = { },
> +	[IMX8MP_LDB] = { },
>  	[IMX93_LDB] = {
> -		.ldb_ctrl = 0x20,
> -		.lvds_ctrl = 0x24,
>  		.lvds_en_bit = true,
>  	},
>  };
> @@ -90,8 +80,11 @@ struct fsl_ldb {
>  	struct clk *clk;
>  	struct regmap *regmap;
>  	const struct fsl_ldb_devdata *devdata;
> +	u64 ldb_ctrl;
> +	u64 lvds_ctrl;
>  	bool ch0_enabled;
>  	bool ch1_enabled;
> +	bool single_ctrl_reg;
>  };
>  
>  static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
> @@ -204,15 +197,15 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>  		reg |=	(fsl_ldb->ch0_enabled ? LDB_CTRL_DI0_VSYNC_POLARITY : 0) |
>  			(fsl_ldb->ch1_enabled ? LDB_CTRL_DI1_VSYNC_POLARITY : 0);
>  
> -	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, reg);
> +	regmap_write(fsl_ldb->regmap, fsl_ldb->ldb_ctrl, reg);
>  
> -	if (fsl_ldb->devdata->single_ctrl_reg)
> +	if (fsl_ldb->single_ctrl_reg)
>  		return;
>  
>  	/* Program LVDS_CTRL */
>  	reg = LVDS_CTRL_CC_ADJ(2) | LVDS_CTRL_PRE_EMPH_EN |
>  	      LVDS_CTRL_PRE_EMPH_ADJ(3) | LVDS_CTRL_VBG_EN;
> -	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
> +	regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, reg);
>  
>  	/* Wait for VBG to stabilize. */
>  	usleep_range(15, 20);
> @@ -220,7 +213,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>  	reg |=	(fsl_ldb->ch0_enabled ? LVDS_CTRL_CH0_EN : 0) |
>  		(fsl_ldb->ch1_enabled ? LVDS_CTRL_CH1_EN : 0);
>  
> -	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
> +	regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, reg);
>  }
>  
>  static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
> @@ -231,12 +224,12 @@ static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
>  	/* Stop channel(s). */
>  	if (fsl_ldb->devdata->lvds_en_bit)
>  		/* Set LVDS_CTRL_LVDS_EN bit to disable. */
> -		regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl,
> +		regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl,
>  			     LVDS_CTRL_LVDS_EN);
>  	else
> -		if (!fsl_ldb->devdata->single_ctrl_reg)
> -			regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, 0);
> -	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, 0);
> +		if (!fsl_ldb->single_ctrl_reg)
> +			regmap_write(fsl_ldb->regmap, fsl_ldb->lvds_ctrl, 0);
> +	regmap_write(fsl_ldb->regmap, fsl_ldb->ldb_ctrl, 0);
>  
>  	clk_disable_unprepare(fsl_ldb->clk);
>  }
> @@ -296,7 +289,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	struct device_node *remote1, *remote2;
>  	struct drm_panel *panel;
>  	struct fsl_ldb *fsl_ldb;
> -	int dual_link;
> +	int dual_link, idx, ret;
>  
>  	fsl_ldb = devm_drm_bridge_alloc(dev, struct fsl_ldb, bridge, &funcs);
>  	if (IS_ERR(fsl_ldb))
> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>  	fsl_ldb->dev = &pdev->dev;
>  	fsl_ldb->bridge.of_node = dev->of_node;
>  
> +	/* No "reg-names" property likely means single-register LDB */
> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");

You don't need to match reg-names.  Instead, just call of_property_read_reg()
twice to get the first reg and the second reg by passing indexes 0 and 1 to it.
If the second reg is not found, then set fsl_ldb->single_ctrl_reg to true.

It would be good to take this chance to clean up reg and reg-names properties
in fsl,ldb.yaml.  See this complaint:

DTC [C] arch/arm/boot/dts/nxp/imx/imx6sx-sdb.dtb
arch/arm/boot/dts/nxp/imx/imx6sx-sdb.dtb: bridge@18 (fsl,imx6sx-ldb): reg: [[24, 4]] is too short
	from schema $id: http://devicetree.org/schemas/display/bridge/fsl,ldb.yaml#


> +	if (idx < 0) {
> +		fsl_ldb->single_ctrl_reg = true;
> +		idx = 0;
> +	}
> +
> +	ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->ldb_ctrl, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (!fsl_ldb->single_ctrl_reg) {
> +		idx = of_property_match_string(dev->of_node, "reg-names", "lvds");
> +		if (idx < 0)
> +			return idx;
> +
> +		ret = of_property_read_reg(dev->of_node, idx, &fsl_ldb->lvds_ctrl, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	fsl_ldb->clk = devm_clk_get(dev, "ldb");
>  	if (IS_ERR(fsl_ldb->clk))
>  		return PTR_ERR(fsl_ldb->clk);


-- 
Regards,
Liu Ying

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Marek Vasut]

On 11/4/25 3:39 AM, Liu Ying wrote:

Hello Liu,

>> @@ -61,24 +62,13 @@ enum fsl_ldb_devtype {
>>   };
>>   
>>   struct fsl_ldb_devdata {
>> -	u32 ldb_ctrl;
>> -	u32 lvds_ctrl;
>>   	bool lvds_en_bit;
>> -	bool single_ctrl_reg;
>>   };
>>   
>>   static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> 
> As I pointed out in v1 comment, this patch should remove struct
> fsl_ldb_devdata.

The lvds_en_bit is still needed , and I plan to add MX95 support here, 
which would extend this again. Going back and forth makes little sense.

[...]

>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>   	fsl_ldb->dev = &pdev->dev;
>>   	fsl_ldb->bridge.of_node = dev->of_node;
>>   
>> +	/* No "reg-names" property likely means single-register LDB */
>> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
> 
> You don't need to match reg-names.  Instead, just call of_property_read_reg()
> twice to get the first reg and the second reg by passing indexes 0 and 1 to it.
> If the second reg is not found, then set fsl_ldb->single_ctrl_reg to true.

This wouldn't work if the two entries were ordered the other way around 
in DT, i.e. first "ldb" second "lvds" and vice-versa. That's why 
properties with multiple values also have the -names property that goes 
with them.

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Liu Ying]

On 11/04/2025, Marek Vasut wrote:
> On 11/4/25 3:39 AM, Liu Ying wrote:
> 
> Hello Liu,

Hello Marek,

> 
>>> @@ -61,24 +62,13 @@ enum fsl_ldb_devtype {
>>>   };
>>>     struct fsl_ldb_devdata {
>>> -    u32 ldb_ctrl;
>>> -    u32 lvds_ctrl;
>>>       bool lvds_en_bit;
>>> -    bool single_ctrl_reg;
>>>   };
>>>     static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
>>
>> As I pointed out in v1 comment, this patch should remove struct
>> fsl_ldb_devdata.
> 
> The lvds_en_bit is still needed , and I plan to add MX95 support here,
> which would extend this again. Going back and forth makes little sense.

lvds_en_bit is indeed needed and it doesn't have to be in struct
fsl_ldb_devdata.  Given that it's not clear if i.MX95 LDB support would
be in fsl-ldb.c or not and it's trivial to add struct fsl_ldb_devdata
back if needed, I think this patch should remove struct fsl_ldb_devdata.

> 
> [...]
> 
>>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>>       fsl_ldb->dev = &pdev->dev;
>>>       fsl_ldb->bridge.of_node = dev->of_node;
>>>   +    /* No "reg-names" property likely means single-register LDB */
>>> +    idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
>>
>> You don't need to match reg-names.  Instead, just call of_property_read_reg()
>> twice to get the first reg and the second reg by passing indexes 0 and 1 to it.
>> If the second reg is not found, then set fsl_ldb->single_ctrl_reg to true.
> 
> This wouldn't work if the two entries were ordered the other way around in,
> DT, i.e. first "ldb" second "lvds" and vice-versa. That's why properties
> with multiple values also have the -names property that goes with them.

Isn't the order checked by schema fsl,ldb.yaml?  Let the schema do it's job.
"ldb" is the first item and "lvds" is the second one.  If you swap the two
for the example in the schema, dt_binding_check would complain:

DTC [C] Documentation/devicetree/bindings/display/bridge/fsl,ldb.example.dtb
Documentation/devicetree/bindings/display/bridge/fsl,ldb.example.dtb: bridge@5c (fsl,imx8mp-ldb): reg-names:0: 'ldb' was expected
	from schema $id: http://devicetree.org/schemas/display/bridge/fsl,ldb.yaml#
Documentation/devicetree/bindings/display/bridge/fsl,ldb.example.dtb: bridge@5c (fsl,imx8mp-ldb): reg-names:1: 'lvds' was expected
	from schema $id: http://devicetree.org/schemas/display/bridge/fsl,ldb.yaml#

-- 
Regards,
Liu Ying

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Luca Ceresoli]

Hi Marek,

On Tue Nov 4, 2025 at 12:08 AM CET, Marek Vasut wrote:
> On 11/3/25 4:55 PM, Luca Ceresoli wrote:
>
> Hello Luca,
>
>> On Sun Nov 2, 2025 at 6:02 PM CET, Marek Vasut wrote:
>>> The DT binding for this bridge describe register offsets for the LDB,
>>> parse the register offsets from DT instead of hard-coding them in the
>>> driver. No functional change.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
>>
>> I was initially a bit skeptical because normally register offsets are
>> derived from the compatible string, not from device tree. But then I
>> realized this is about the LDB which has its two registers in the
>> MEDIA_BLK. This means all in all this looks somewhat like an integration
>> aspect (the LDB component uses two resources of the MEDIA_CLK component)
>> and your patch mekse sense.
>>
>> So my only remark is that the above may be in the commit message, to make
>> the "why" clear from the beginning. It took a bit of research for me to
>> find out.
>
> Actually, the LDB was always meant to parse the 'reg' offsets out of the
> DT, it then went somewhat ... wrong ... and we ended up with hard-coded
> reg<->compatible mapping. It was never intended to be that way. That is
> all there is to it, there isn't any deeper reason behind it.
>
> What would you add into the commit message ?

The above paragraph is a good draft of what I woudl add.

>> [0] https://lore.kernel.org/dri-devel/20251103-dcif-upstreaming-v6-3-76fcecfda919@oss.nxp.com/
>>
>>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>>   	fsl_ldb->dev = &pdev->dev;
>>>   	fsl_ldb->bridge.of_node = dev->of_node;
>>>
>>> +	/* No "reg-names" property likely means single-register LDB */
>>
>> Uh? If it is "likely" it means we are not sure this code is not introducing
>> regressions, and that would be bad.
>
> I can drop the 'likely' part.

If you are sure it's not "likely" but "sure", then OK. However it all
depends on the bindings, which leads to the below question.

>>> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
>>> +	if (idx < 0) {
>>> +		fsl_ldb->single_ctrl_reg = true;
>>> +		idx = 0;
>>> +	}
>>
>>  From the bindings I understand that having two 'reg' values and no
>> 'reg-names' at all is legal. Your patch implies differently. Who's right
>> here?
> I think if you have two two reg values, you should have reg-names , so
> the binding should be updated ?

If the bindings are unclear or ambiguous (or wrong) then they should be
fixed in the first place. With bad bindings we can either have a bad but
compliant implementation or a good but non-compliant implementation.

Best regards,
Luca

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

Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

[Marek Vasut]

On 11/5/25 7:03 PM, Luca Ceresoli wrote:

Hello Luca,

sorry for the late reply.

>>> On Sun Nov 2, 2025 at 6:02 PM CET, Marek Vasut wrote:
>>>> The DT binding for this bridge describe register offsets for the LDB,
>>>> parse the register offsets from DT instead of hard-coding them in the
>>>> driver. No functional change.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
>>>
>>> I was initially a bit skeptical because normally register offsets are
>>> derived from the compatible string, not from device tree. But then I
>>> realized this is about the LDB which has its two registers in the
>>> MEDIA_BLK. This means all in all this looks somewhat like an integration
>>> aspect (the LDB component uses two resources of the MEDIA_CLK component)
>>> and your patch mekse sense.
>>>
>>> So my only remark is that the above may be in the commit message, to make
>>> the "why" clear from the beginning. It took a bit of research for me to
>>> find out.
>>
>> Actually, the LDB was always meant to parse the 'reg' offsets out of the
>> DT, it then went somewhat ... wrong ... and we ended up with hard-coded
>> reg<->compatible mapping. It was never intended to be that way. That is
>> all there is to it, there isn't any deeper reason behind it.
>>
>> What would you add into the commit message ?
> 
> The above paragraph is a good draft of what I woudl add.
> 
>>> [0] https://lore.kernel.org/dri-devel/20251103-dcif-upstreaming-v6-3-76fcecfda919@oss.nxp.com/
>>>
>>>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>>>    	fsl_ldb->dev = &pdev->dev;
>>>>    	fsl_ldb->bridge.of_node = dev->of_node;
>>>>
>>>> +	/* No "reg-names" property likely means single-register LDB */
>>>
>>> Uh? If it is "likely" it means we are not sure this code is not introducing
>>> regressions, and that would be bad.
>>
>> I can drop the 'likely' part.
> 
> If you are sure it's not "likely" but "sure", then OK. However it all
> depends on the bindings, which leads to the below question.

Fixed in V3

>>>> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
>>>> +	if (idx < 0) {
>>>> +		fsl_ldb->single_ctrl_reg = true;
>>>> +		idx = 0;
>>>> +	}
>>>
>>>   From the bindings I understand that having two 'reg' values and no
>>> 'reg-names' at all is legal. Your patch implies differently. Who's right
>>> here?
>> I think if you have two two reg values, you should have reg-names , so
>> the binding should be updated ?
> 
> If the bindings are unclear or ambiguous (or wrong) then they should be
> fixed in the first place. With bad bindings we can either have a bad but
> compliant implementation or a good but non-compliant implementation.
Binding update sent:

https://lore.kernel.org/dri-devel/20260104213457.128734-1-marek.vasut@mailbox.org/