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 at mailbox.org>
> ---
> Cc: Abel Vesa <abelvesa at kernel.org>
> Cc: Conor Dooley <conor+dt at kernel.org>
> Cc: Fabio Estevam <festevam at gmail.com>
> Cc: Krzysztof Kozlowski <krzk+dt at kernel.org>
> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> Cc: Liu Ying <victor.liu at nxp.com>
> Cc: Lucas Stach <l.stach at pengutronix.de>
> Cc: Peng Fan <peng.fan at nxp.com>
> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> Cc: Rob Herring <robh at kernel.org>
> Cc: Shawn Guo <shawnguo at kernel.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: devicetree at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: imx at lists.linux.dev
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-clk at 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 at 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