Re: [PATCH v6 2/2] drm/bridge: add support for lontium lt9211c bridge

[Vishnu Saini]

On Tue, Jul 28, 2026 at 01:01:50PM +0200, Philipp Zabel wrote:
> On So, 2026-07-19 at 21:27 +0530, vishnu.saini@xxxxxxxxxxxxxxxx wrote:
> > From: Yi Zhang <zhanyi@xxxxxxxxxxxxxxxx>
> > 
> > LT9211C is a Single/Dual-Link DSI/LVDS or Single DPI input to
> > Single-Link/Dual-Link DSI/LVDS or Single DPI output bridge chip.
> > Extend the existing lontium-lt9211 driver to support DSI-to-LVDS
> > bridge configuration by detecting and handling both LT9211 and LT9211C
> > variants from a single driver.
> > 
> > Chip detection in lt9211_read_chipid() is extended to identify the
> > LT9211C by its distinct chip ID registers.
> >
> > A separate regmap configuration is introduced for LT9211C to cover
> > its extended register map. The chip_type field added to struct lt9211
> > selects the appropriate regmap and code paths at probe time using
> > of_device_is_compatible().
> 
> This could be done via device match data instead.
Yes, will use device match data in next revision. 
> 
> [...]
> > The LT9211C requires DSI clocks to be running before the bridge can
> > be initialised,
> 
> Is this different from LT9211?
The chip behavior and register sets are different for LT9211C.

> 
> > so lt9211_atomic_enable() queues
> > a delayed workqueue item for LT9211C rather than initialising
> > synchronously as LT9211 does.
> 
> How does this follow? Isn't the DSI clock already enabled before
> atomic_enable is called?

> If the initialization is deferred to a delayed work item, we can't
> correctly enable downstream LVDS panels that require a few frames of
> valid LVDS signal before enabling the panel or backlight. This must be
> synchronous to work correctly.
I did few experiments to check this again, DSI clock is running before atomic_enable,
But when i make it synchronous autodetect format/width/height is failing.
1. Tried ading more delay upto 5s did not help.
2. Tried adding a dummy drm_hook hw_timing_enabled, which is called after enabling the timing engine
with this synchronous approach is working.
Let me know if we can keep delayed work apprach with some improvements, reduce the delay to minimum and wait in autect_rx for correct nonzero format/width/height ?

> > LT9211C uses VIDEO|LPM DSI mode flags, which differ from the
> > sync-pulse mode used by LT9211.
> > 
> > Signed-off-by: Yi Zhang <zhanyi@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Nilesh Laad <nilesh.laad@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Gopi Botlagunta <venkata.botlagunta@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Vishnu Saini <vishnu.saini@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/bridge/lontium-lt9211.c | 796 +++++++++++++++++++++++++++++++-
> >  1 file changed, 779 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c b/drivers/gpu/drm/bridge/lontium-lt9211.c
> > index f39d83a5ae37..36c004df34fc 100644
> > --- a/drivers/gpu/drm/bridge/lontium-lt9211.c
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9211.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/of_graph.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/workqueue.h>
> >  
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > @@ -36,10 +37,21 @@
> >  #define REG_CHIPID2				0x8102
> >  #define REG_CHIPID2_VALUE			0xe3
> >  
> > +/* LT9211C chip ID values */
> > +#define REG_CHIPID0_LT9211C_VALUE		0x21
> > +#define REG_CHIPID1_LT9211C_VALUE		0x03
> > +#define REG_CHIPID2_LT9211C_VALUE		0xe1
> > +
> >  #define REG_DSI_LANE				0xd000
> >  /* DSI lane count - 0 means 4 lanes ; 1, 2, 3 means 1, 2, 3 lanes. */
> >  #define REG_DSI_LANE_COUNT(n)			((n) & 3)
> >  
> > +/* Chip type enum */
> > +enum lt9211_chip_type {
> > +	LT9211,
> > +	LT9211C,
> > +};
> > +
> >  struct lt9211 {
> >  	struct drm_bridge		bridge;
> >  	struct device			*dev;
> > @@ -50,6 +62,14 @@ struct lt9211 {
> >  	struct regulator		*vccio;
> >  	bool				lvds_dual_link;
> >  	bool				lvds_dual_link_even_odd_swap;
> > +	/* LT9211C specific fields */
> 
> This isn't LT9211C specific:
Will correct this. 
> > +	enum lt9211_chip_type		chip_type;
> 
> These won't be needed if the delayed work is dropped:
dropping delayed work is not working for LT9211C.

> > +	struct workqueue_struct		*wq;
> > +	struct delayed_work		lt9211_dw;
> > +	struct drm_display_mode		mode;
> > +	bool				bpp24;
> > +	bool				jeida;
> > +	bool				de;
> >  };
> >  
> >  static const struct regmap_range lt9211_rw_ranges[] = {
> > @@ -93,6 +113,49 @@ static const struct regmap_config lt9211_regmap_config = {
> >  	.max_register = 0xda00,
> >  };
> >  
> > +static const struct regmap_range lt9211c_rw_ranges[] = {
> > +	regmap_reg_range(0xff, 0xff),
> > +	regmap_reg_range(0x8100, 0x8182),
> > +	regmap_reg_range(0x8200, 0x82aa),
> > +	regmap_reg_range(0x8500, 0x85ff),
> > +	regmap_reg_range(0x8600, 0x86a0),
> > +	regmap_reg_range(0x8700, 0x8746),
> > +	regmap_reg_range(0xd000, 0xd0a7),
> > +	regmap_reg_range(0xd400, 0xd42c),
> > +	regmap_reg_range(0xd800, 0xd838),
> > +	regmap_reg_range(0xd9c0, 0xd9d5),
> > +};
> > +
> > +static const struct regmap_access_table lt9211c_rw_table = {
> > +	.yes_ranges = lt9211c_rw_ranges,
> > +	.n_yes_ranges = ARRAY_SIZE(lt9211c_rw_ranges),
> > +};
> > +
> > +static const struct regmap_range_cfg lt9211c_range = {
> > +	.name = "lt9211c",
> > +	.range_min = 0x0000,
> > +	.range_max = 0xda00,
> > +	.selector_reg = REG_PAGE_CONTROL,
> > +	.selector_mask = 0xff,
> > +	.selector_shift = 0,
> > +	.window_start = 0,
> > +	.window_len = 0x100,
> > +};
> > +
> > +static const struct regmap_config lt9211c_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.rd_table = &lt9211c_rw_table,
> > +	.wr_table = &lt9211c_rw_table,
> > +	.volatile_table = &lt9211c_rw_table,
> > +	.ranges = &lt9211c_range,
> > +	.num_ranges = 1,
> > +	.cache_type = REGCACHE_RBTREE,
> 
> Don't use REGCACHE_RBTREE without a good reason, current default is
> REGCACHE_MAPLE. But... if all RW registers are also marked volatile,
> the cache is useless. So, REGCACHE_NONE?
> Do you know anything about which registers may or may not be volatile?
I don't have this list from vendor but REGCACHE_NONE is also working, will use
REGCACHE_NONE in next revision.

> [...]
> > @@ -454,6 +526,651 @@ static int lt9211_configure_tx(struct lt9211 *ctx, bool jeida,
> >  	return 0;
> >  }
> >  
> > +static int lt9211c_configure_rx(struct lt9211 *ctx)
> > +{
> > +	unsigned int pval;
> > 
> [...]
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_rx_phy_seq,
> > +				     ARRAY_SIZE(lt9211c_rx_phy_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_rx_phy_reset_seq,
> > +				     ARRAY_SIZE(lt9211c_rx_phy_reset_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_rx_clk_sel_seq,
> > +				     ARRAY_SIZE(lt9211c_rx_clk_sel_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(ctx->regmap, 0x8180, &pval);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0x8180, ((pval & 0xfc) | 0x03));
> > +	if (ret)
> > +		return ret;
> 
> These two are
> 
> 	ret = regmap_write_bits(ctx->regmap, 0x8180, 0x3, 0x3);
> 
> or even
> 
> 	ret = regmap_set_bits(ctx->regmap, 0x8180, 0x3);

Will use regmap_write_bits in next revision. 
> [...]
> > +	ret = regmap_read(ctx->regmap, 0x8530, &pval);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0xf8) | 0x11));
> > +	if (ret)
> > +		return ret;
> 
> Not masking out bit 0x10 and then setting it is unusual. Was ((pval &
> 0xe8) | 0x11) meant here?
> 
> This corresponds to:
> 
> 	ret = regmap_write_bits(ctx->regmap, 0x8530, 0x17, 0x11);
Will use regmap_write_bits in next revision. 
> > +
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_rx_input_sel_seq,
> > +				     ARRAY_SIZE(lt9211c_rx_input_sel_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_rx_dig_seq,
> > +				     ARRAY_SIZE(lt9211c_rx_dig_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Give the chip time to lock onto RX stream. */
> > +	msleep(100);
> 
> Is there any status register that could be polled to see if the lock
> happened earlier?
Checked with vendor no status register for this, LT9211 is also using msleep(100);
I will try reducing this delay and poll for correct format/width/height in lt9211c_autodetect_rx.

> > +
> > +	return 0;
> > +}
> > +
> > +static int lt9211c_autodetect_rx(struct lt9211 *ctx,
> > +				 const struct drm_display_mode *mode)
> > +{
> > +	u16 width, height;
> > +	u8 buf[5];
> > +	u8 format;
> > +	u8 sot[8];
> > +	int ret;
> > +
> > +	/* Read the SOT from the chip. */
> > +	ret = regmap_bulk_read(ctx->regmap, 0xd088, sot, sizeof(sot));
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(ctx->dev, "Sot Num = 0x%02x, 0x%02x, 0x%02x, 0x%02x\n", sot[0],
> > +		sot[2], sot[4], sot[6]);
> > +
> > +	dev_dbg(ctx->dev, "Sot Data = 0x%02x, 0x%02x, 0x%02x, 0x%02x\n", sot[1],
> > +		sot[3], sot[5], sot[7]);
> > +	/* HS Settle Set */
> > +	if (sot[0] > 0x10 && sot[0] < 0x50)
> > +		regmap_write(ctx->regmap, 0xd002, sot[0] - 5);
> > +	else
> > +		regmap_write(ctx->regmap, 0xd002, 0x08);
> 
> You could prepare the value then call regmap_write() once.
Yes, will correct this in next revision.
 
> [...]
> > +static int lt9211c_configure_timing(struct lt9211 *ctx,
> > +				    const struct drm_display_mode *mode)
> > +{
> > +	const struct reg_sequence lt9211c_timing[] = {
> > +		{ 0xd00d, (mode->vtotal >> 8) & 0xff },
> > +		{ 0xd00e, mode->vtotal & 0xff },
> > +		{ 0xd00f, (mode->vdisplay >> 8) & 0xff },
> > +		{ 0xd010, mode->vdisplay & 0xff },
> > +		{ 0xd011, (mode->htotal >> 8) & 0xff },
> > +		{ 0xd012, mode->htotal & 0xff },
> > +		{ 0xd013, (mode->hdisplay >> 8) & 0xff },
> > +		{ 0xd014, mode->hdisplay & 0xff },
> > +		{ 0xd015, (mode->vsync_end - mode->vsync_start) & 0xff },
> > +		{ 0xd04c, ((mode->hsync_end - mode->hsync_start) >> 8) & 0xff },
> 
> Apart from 16-bit hsync_len this is identical to
> lt9211_configure_timing, maybe that could be reused.
yes, will reuse lt9211_configure_timing

> > +		{ 0xd016, (mode->hsync_end - mode->hsync_start) & 0xff },
> > +		{ 0xd017, ((mode->vsync_start - mode->vdisplay) >> 8) & 0xff },
> > +		{ 0xd018, (mode->vsync_start - mode->vdisplay) & 0xff },
> > +		{ 0xd019, ((mode->hsync_start - mode->hdisplay) >> 8) & 0xff },
> > +		{ 0xd01a, (mode->hsync_start - mode->hdisplay) & 0xff },
> > +	};
> > +
> > +	return regmap_multi_reg_write(ctx->regmap, lt9211c_timing,
> > +				      ARRAY_SIZE(lt9211c_timing));
> > +}
> > +
> > +static int lt9211c_configure_plls(struct lt9211 *ctx,
> > +				  const struct drm_display_mode *mode)
> > +{
> [...]
> > +	if (mode->clock < 22000) {
> > +		ret = regmap_write(ctx->regmap, 0x822f, 0x07);
> > +		ret |= regmap_write(ctx->regmap, 0x822c, 0x01);
> 
> 0x822c is never cleared in the other branches. Would it be possible to
> enable with a mode clock < 22000 and then disable and enable with a
> mode clock >= 22000 but < 44000, ending up with 0x822c still containing
> 0x01 ?
Will fix this in next revision.
 
> > +		div = 16;
> > +	} else if (mode->clock < 44000) {
> > +		ret = regmap_write(ctx->regmap, 0x822f, 0x07);
> > +		div = 16;
> > +	} else if (mode->clock < 88000) {
> > +		ret = regmap_write(ctx->regmap, 0x822f, 0x06);
> > +		div = 8;
> > +	} else if (mode->clock < 176000) {
> > +		ret = regmap_write(ctx->regmap, 0x822f, 0x05);
> > +		div = 4;
> > +	} else {
> > +		ret = regmap_write(ctx->regmap, 0x822f, 0x04);
> > +		div = 2;
> > +	}
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	pcr_m = (mode->clock * div) / 25;
> > +	pcr_k = pcr_m % 1000;
> > +	pcr_m /= 1000;
> > +
> > +	pcr_up = pcr_m + 1;
> > +	pcr_down = pcr_m - 1;
> > +
> > +	pcr_k <<= 14;
> > +
> > +	ret = regmap_write(ctx->regmap, 0xd008, 0x00);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* 0xd026: pcr_m */
> > +	ret = regmap_write(ctx->regmap, 0xd026, (0x80 | (u8)pcr_m) & 0x7f);
> 
> Why set bit 0x80 only to clear it right away?
will simplify to `(u8)pcr_m & 0x7f)` in the next revision. 

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* 0xd027 0xd028 0xd029: pcr_k */
> > +	ret = regmap_write(ctx->regmap, 0xd027, (pcr_k >> 16) & 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0xd028, (pcr_k >> 8) & 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0xd029, pcr_k & 0xff);
> 
> This always writes 0 because of the pcr_k <<= 14 above.
You're right that it's always 0, but this isn't a bug - it's expected given the value ranges.
It's part of one logical 24-bit field. 0xd027:0xd028:0xd029 together encode pcr_k as a single 24-bit fractional value,
so explicitly writing the calculated value.

> [...]
> > +	if (mode->clock < 44000) {
> > +		ret = regmap_write(ctx->regmap, 0xd00c, 0x60);
> > +		ret |= regmap_write(ctx->regmap, 0xd01b, 0x00);
> > +		ret |= regmap_write(ctx->regmap, 0xd01c, 0x60);
> 
> The sashiko bot correctly complained about this. Please fix the error
> handling.
Yes, will fix this in next revision.
 
> > +	} else {
> > +		ret = regmap_write(ctx->regmap, 0xd00c, 0x40);
> > +		ret |= regmap_write(ctx->regmap, 0xd01b, 0x00);
> > +		ret |= regmap_write(ctx->regmap, 0xd01c, 0x40);
> 
> Same as above, you could prepare the 0x60 / 0x40 value in a variable
> and only call one set of regmap_write()s.
yes, will fix this in next revision.
 
> > +	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_pcr_reset_seq,
> > +				     ARRAY_SIZE(lt9211c_pcr_reset_seq));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* PCR stability test takes seconds. */
> > +	ret = regmap_read_poll_timeout(ctx->regmap, 0xd087, pval,
> > +				       ((pval & 0x18) == 0x18), 20000, 3000000);
> > +	if (ret)
> > +		dev_err(ctx->dev, "PCR unstable, ret=%i\n", ret);
> > +
> > +	ret = regmap_write(ctx->regmap, 0x8180, 0x51);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0x863f, 0x00);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0x863f, 0x01);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read_poll_timeout(ctx->regmap, 0x8640, pval,
> > +				       ((pval & 0x01) == 0x01), 50000, 250000);
> > +	if (ret)
> > +		dev_err(ctx->dev, "Video check not stable, ret=%i\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int lt9211c_configure_tx(struct lt9211 *ctx,
> > +				const struct drm_display_mode *mode)
> > +{
> [...]
> > +	ret = regmap_read(ctx->regmap, 0x8530, &pval);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0x3f) | 0x40));
> > +	if (ret)
> > +		return ret;
> 
> 	ret = regmap_write_bits(ctx->regmap, 0x8530, 0xc0, 0x40);
yes, will fix this in next revision.

> > +
> > +	/* [7]0:txpll normal work; txpll ref clk sel pix clk */
> > +	ret = regmap_write(ctx->regmap, 0x8230, 0x00);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (ctx->lvds_dual_link)
> > +		phy_clk = (u32)(mode->clock * 7 / 2);
> > +	else
> > +		phy_clk = (u32)(mode->clock * 7);
> > +
> > +	/* 0x8231: prediv sel */
> > +	if (mode->clock < 20000) {
> > +		val = 0x28;
> > +		pre_div = 1;
> 
> Is this case the same as ...
> 
> > +	} else if (mode->clock < 40000) {
> > +		val = 0x28;
> > +		pre_div = 1;
> 
> .. this case on purpose? If so, why not merge the branches.

Yes, will merge the branches in next revision.
 
> > +	} else if (mode->clock < 80000) {
> > +		val = 0x29;
> > +		pre_div = 2;
> > +	} else if (mode->clock < 160000) {
> > +		val = 0x2a;
> > +		pre_div = 4;
> > +	} else if (mode->clock < 320000) {
> > +		val = 0x2b;
> > +		pre_div = 8;
> > +	} else {
> > +		val = 0x2f;
> > +		pre_div = 16;
> > +	}
> > +	ret = regmap_write(ctx->regmap, 0x8231, val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> [...]
> > +
> > +	return 0;
> > +}
> > +
> > +static void lt9211_delayed_work_func(struct work_struct *work)
> > +{
> > +	struct delayed_work *dw = to_delayed_work(work);
> > +	struct lt9211 *ctx = container_of(dw, struct lt9211, lt9211_dw);
> > +	const struct drm_display_mode *mode = &ctx->mode;
> > +	int ret;
> > +
> > +	if (ctx->chip_type != LT9211C) {
> > +		dev_err(ctx->dev, "LT9211: Delayed work called for non-LT9211C chip\n");
> > +		return;
> > +	}
> > +
> > +	ret = lt9211c_configure_rx(ctx);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = lt9211c_autodetect_rx(ctx, mode);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = lt9211c_configure_timing(ctx, mode);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = lt9211c_configure_plls(ctx, mode);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = lt9211c_configure_tx(ctx, mode);
> > +	if (ret)
> > +		return;
> > +}
> > +
> >  static void lt9211_atomic_enable(struct drm_bridge *bridge,
> >  				 struct drm_atomic_commit *state)
> >  {
> > @@ -523,6 +1240,15 @@ static void lt9211_atomic_enable(struct drm_bridge *bridge,
> >  	if (ret)
> >  		return;
> >  
> > +	if (ctx->chip_type == LT9211C && ctx->wq) {
> > +		drm_mode_copy(&ctx->mode, mode);
> > +		/* LT9211C must enable after mipi clock enable */
> > +		queue_delayed_work(ctx->wq, &ctx->lt9211_dw,
> > +				   msecs_to_jiffies(100));
> 
> Why is this deferred to a delayed work item?
> Can't we just wait here and run all the initialization synchronously?
> 
> As the sashiko bot points out, this calls the delayed work function
> without initializing ctx->jeida, ctx->bpp24 and ctx->de, and both
> atomic_disable and driver remove are missing some kind of
> cancel_delayed_work_sync().
> Further, with the delayed initialization it is impossible to correctly
> enable downstream bridges or panels that require an active LVDS signal
> before (post)enabling.
> All of this wouldn't be a problem if the initialization was done
> synchronously.
WIll correct the uninitialized variabled in next revision but removing delayed work is not working
for this bridge chip as it needs timing engine running.
As mentioned previously i will try improving the delayed work to reduce the delay and poll for correct
format/width/height, if this approach is acceptable.
 
> > +		dev_dbg(ctx->dev, "LT9211C enabled.\n");
> > +		return;
> > +	}
> > +
> >  	ret = lt9211_system_init(ctx);
> >  	if (ret)
> >  		return;
> > @@ -671,11 +1397,6 @@ static int lt9211_parse_dt(struct lt9211 *ctx)
> >  
> >  static int lt9211_host_attach(struct lt9211 *ctx)
> >  {
> > -	const struct mipi_dsi_device_info info = {
> > -		.type = "lt9211",
> > -		.channel = 0,
> > -		.node = NULL,
> > -	};
> >  	struct device *dev = ctx->dev;
> >  	struct device_node *host_node;
> >  	struct device_node *endpoint;
> > @@ -697,7 +1418,22 @@ static int lt9211_host_attach(struct lt9211 *ctx)
> >  	if (dsi_lanes < 0)
> >  		return dsi_lanes;
> >  
> > -	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> > +	if (ctx->chip_type == LT9211C) {
> > +		const struct mipi_dsi_device_info info = {
> > +			.type = "lt9211c",
> > +			.channel = 0,
> > +			.node = NULL,
> > +		};
> 
> Moving mipi_dsi_device_info into device match data would allow to get
> rid of the branching here.
Yes, this will be fixed in next revision.

> > +		dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> > +	} else {
> > +		const struct mipi_dsi_device_info info = {
> > +			.type = "lt9211",
> > +			.channel = 0,
> > +			.node = NULL,
> > +		};
> > +		dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> > +	}
> > +
> >  	if (IS_ERR(dsi))
> >  		return dev_err_probe(dev, PTR_ERR(dsi),
> >  				     "failed to create dsi device\n");
> > @@ -706,10 +1442,16 @@ static int lt9211_host_attach(struct lt9211 *ctx)
> >  
> >  	dsi->lanes = dsi_lanes;
> >  	dsi->format = MIPI_DSI_FMT_RGB888;
> > -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > -			  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_VIDEO_NO_HSA |
> > -			  MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP |
> > -			  MIPI_DSI_MODE_NO_EOT_PACKET;
> > +
> > +	if (ctx->chip_type == LT9211C) {
> > +		dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM;
> 
> This could also be stored in device match data.
Yes, will use device match data in next revision.
 
> > +	} else {
> > +		dsi->mode_flags =
> > +			MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > +			MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_VIDEO_NO_HSA |
> > +			MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP |
> > +			MIPI_DSI_MODE_NO_EOT_PACKET;
> > +	}
> >  
> >  	ret = devm_mipi_dsi_attach(dev, dsi);
> >  	if (ret < 0) {
> > @@ -747,10 +1489,25 @@ static int lt9211_probe(struct i2c_client *client)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ctx->regmap = devm_regmap_init_i2c(client, &lt9211_regmap_config);
> > +	if (of_device_is_compatible(dev->of_node, "lontium,lt9211c")) {
> > +		ctx->chip_type = LT9211C;
> > +		ctx->regmap =
> > +			devm_regmap_init_i2c(client, &lt9211c_regmap_config);
> 
> The branching here could be avoided by getting regmap config from
> device match data (e.g. via i2c_get_match_data from lt9211_id or
> lt9211_match_table).
Yes, will fix this with i2c_get_match_data
 
> > +	} else {
> > +		ctx->chip_type = LT9211;
> > +		ctx->regmap =
> > +			devm_regmap_init_i2c(client, &lt9211_regmap_config);
> > +	}
> >  	if (IS_ERR(ctx->regmap))
> >  		return PTR_ERR(ctx->regmap);
> >  
> > +	if (ctx->chip_type == LT9211C) {
> > +		ctx->wq = create_workqueue("lt9211_work");
> > +		if (!ctx->wq)
> > +			return -ENOMEM;
> > +		INIT_DELAYED_WORK(&ctx->lt9211_dw, lt9211_delayed_work_func);
> > +	}
> 
> I think this should be dropped.
will be required for delayed work, will drop otherwise.
> > +
> >  	dev_set_drvdata(dev, ctx);
> >  	i2c_set_clientdata(client, ctx);
> 
> Not your addition, but these two calls to the same thing.
> This could be dropped with the lt9211_remove function by
> switching to devm_drm_bridge_add().
yes, this will be fixed in next revision 
> 
> regards
> Philipp