public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v5 0/2] This series adds LT9211C bridge driver by extending LT9211.
@ 2026-03-23  7:08 Venkata Gopi Nagaraju Botlagunta
  2026-03-23  7:08 ` [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support Venkata Gopi Nagaraju Botlagunta
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Venkata Gopi Nagaraju Botlagunta @ 2026-03-23  7:08 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Marek Vasut
  Cc: dri-devel, devicetree, linux-kernel, Gopi Botlagunta, Nilesh Laad,
	venkata.valluru, jessica.zhang, Yi Zhang

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.
This adds support for DSI to LVDS bridge configuration.By Extending the
existing lontium-lt9211 driver to support DSI-to-LVDS
bridge configuration.

Signed-off-by: Gopi Botlagunta <venkata.botlagunta@oss.qualcomm.com>
Signed-off-by: Nilesh Laad <nilesh.laad@oss.qualcomm.com>
---
Changes in v5:
  - Addressed code formatting in lt9211 driver (no functional or design changes)
  - Addressed v4 comments on lontium-lt9211.yaml.
  - Link to v4: https://lore.kernel.org/r/20251224-add-lt9211c-bridge-v4-0-406e73ec28c5@oss.qualcomm.com

Changes in v4:
  - Removed lontium-lt9211c.yaml.
  - Extended lontium-lt9211.yaml to support LT9211C.
  - Link to v3: https://lore.kernel.org/r/20251218-add-lt9211c-bridge-v3-0-1ee0670a0db2@oss.qualcomm.com

Changes in v3:
  - removed lontium-lt9211c as separate driver
  - Add support to lontium-lt9211c bridge driver by extending the existing lontium-lt9211.c
  - fixed kernel test robot reported build errors
  - Link to v2:https://lore.kernel.org/lkml/20251107-add-lt9211c-bridge-v2-0-b0616e23407c@oss.qualcomm.com/

Changes in v2:
  - Combined driver patch from https://lore.kernel.org/lkml/20250911-lt9211c-bridge-support-v1-1-c221202cbcd5@oss.qualcomm.com/ 
  - Added MODULE_AUTHOR
  - Uppercase to lowercase for hex values
  - Link to v1:https://lore.kernel.org/r/20250910-add-lt9211c-bridge-v1-1-4f23740fe101@oss.qualcomm.com

---
Yi Zhang (2):
      dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support
      drm/bridge: add support for lontium lt9211c bridge

 .../bindings/display/bridge/lontium,lt9211.yaml    |   5 +-
 drivers/gpu/drm/bridge/lontium-lt9211.c            | 846 ++++++++++++++++++++-
 2 files changed, 808 insertions(+), 43 deletions(-)
---
base-commit: f50b969bafafb2810a07f376387350c4c0d72a21
change-id: 20250910-add-lt9211c-bridge-5a21fcb1c803

Best regards,
-- 
Gopi Botlagunta <venkata.botlagunta@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support
  2026-03-23  7:08 [PATCH v5 0/2] This series adds LT9211C bridge driver by extending LT9211 Venkata Gopi Nagaraju Botlagunta
@ 2026-03-23  7:08 ` Venkata Gopi Nagaraju Botlagunta
  2026-03-24 22:04   ` Claude review: " Claude Code Review Bot
  2026-03-23  7:08 ` [PATCH v5 2/2] drm/bridge: add support for lontium lt9211c bridge Venkata Gopi Nagaraju Botlagunta
  2026-03-24 22:04 ` Claude review: This series adds LT9211C bridge driver by extending LT9211 Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Venkata Gopi Nagaraju Botlagunta @ 2026-03-23  7:08 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Marek Vasut
  Cc: dri-devel, devicetree, linux-kernel, Gopi Botlagunta, Nilesh Laad,
	venkata.valluru, jessica.zhang, Yi Zhang

From: Yi Zhang <zhanyi@qti.qualcomm.com>

Add binding for the Lontium LT9211C bridge chip.

Signed-off-by: Yi Zhang <zhanyi@qti.qualcomm.com>
Signed-off-by: Nilesh Laad <nilesh.laad@oss.qualcomm.com>
Signed-off-by: Gopi Botlagunta <venkata.botlagunta@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/display/bridge/lontium,lt9211.yaml | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt9211.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt9211.yaml
index 9a6e9b25d14a..8df90c014064 100644
--- a/Documentation/devicetree/bindings/display/bridge/lontium,lt9211.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt9211.yaml
@@ -4,19 +4,20 @@
 $id: http://devicetree.org/schemas/display/bridge/lontium,lt9211.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Lontium LT9211 DSI/LVDS/DPI to DSI/LVDS/DPI bridge.
+title: Lontium LT9211/LT9211C DSI/LVDS/DPI to DSI/LVDS/DPI bridge.
 
 maintainers:
   - Marek Vasut <marex@denx.de>
 
 description: |
-  The LT9211 are bridge devices which convert Single/Dual-Link DSI/LVDS
+  The LT9211 and LT9211C are bridge devices which convert Single/Dual-Link DSI/LVDS
   or Single DPI to Single/Dual-Link DSI/LVDS or Single DPI.
 
 properties:
   compatible:
     enum:
       - lontium,lt9211
+      - lontium,lt9211c
 
   reg:
     maxItems: 1

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v5 2/2] drm/bridge: add support for lontium lt9211c bridge
  2026-03-23  7:08 [PATCH v5 0/2] This series adds LT9211C bridge driver by extending LT9211 Venkata Gopi Nagaraju Botlagunta
  2026-03-23  7:08 ` [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support Venkata Gopi Nagaraju Botlagunta
@ 2026-03-23  7:08 ` Venkata Gopi Nagaraju Botlagunta
  2026-03-24 22:04   ` Claude review: " Claude Code Review Bot
  2026-03-24 22:04 ` Claude review: This series adds LT9211C bridge driver by extending LT9211 Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Venkata Gopi Nagaraju Botlagunta @ 2026-03-23  7:08 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Marek Vasut
  Cc: dri-devel, devicetree, linux-kernel, Gopi Botlagunta, Nilesh Laad,
	venkata.valluru, jessica.zhang, Yi Zhang

From: Yi Zhang <zhanyi@qti.qualcomm.com>

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.

Signed-off-by: Yi Zhang <zhanyi@qti.qualcomm.com>
Signed-off-by: Nilesh Laad <nilesh.laad@oss.qualcomm.com>
Signed-off-by: Gopi Botlagunta <venkata.botlagunta@oss.qualcomm.com>
---
 drivers/gpu/drm/bridge/lontium-lt9211.c | 846 ++++++++++++++++++++++++++++++--
 1 file changed, 805 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c b/drivers/gpu/drm/bridge/lontium-lt9211.c
index 399fa7eebd49..4a8544eb2192 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,20 +37,39 @@
 #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;
 	struct regmap			*regmap;
-	struct mipi_dsi_device		*dsi;
+	struct mipi_dsi_device	*dsi;
 	struct drm_bridge		*panel_bridge;
 	struct gpio_desc		*reset_gpio;
 	struct regulator		*vccio;
-	bool				lvds_dual_link;
-	bool				lvds_dual_link_even_odd_swap;
+	bool					lvds_dual_link;
+	bool					lvds_dual_link_even_odd_swap;
+	/* LT9211C specific fields */
+	enum lt9211_chip_type	chip_type;
+	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,
+	.max_register = 0xda00,
+};
+
+static void lt9211_delayed_work_func(struct work_struct *work);
+
 static struct lt9211 *bridge_to_lt9211(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct lt9211, bridge);
@@ -120,15 +183,24 @@ static int lt9211_read_chipid(struct lt9211 *ctx)
 		return ret;
 	}
 
-	/* Test for known Chip ID. */
-	if (chipid[0] != REG_CHIPID0_VALUE || chipid[1] != REG_CHIPID1_VALUE ||
-	    chipid[2] != REG_CHIPID2_VALUE) {
-		dev_err(ctx->dev, "Unknown Chip ID: 0x%02x 0x%02x 0x%02x\n",
-			chipid[0], chipid[1], chipid[2]);
-		return -EINVAL;
+	/* Test for LT9211 Chip ID. */
+	if (chipid[0] == REG_CHIPID0_VALUE && chipid[1] == REG_CHIPID1_VALUE &&
+	    chipid[2] == REG_CHIPID2_VALUE) {
+		dev_dbg(ctx->dev, "Detected LT9211 chip\n");
+		return 0;
 	}
 
-	return 0;
+	/* Test for LT9211C Chip ID. */
+	if (chipid[0] == REG_CHIPID0_LT9211C_VALUE &&
+	    chipid[1] == REG_CHIPID1_LT9211C_VALUE &&
+	    chipid[2] == REG_CHIPID2_LT9211C_VALUE) {
+		dev_dbg(ctx->dev, "Detected LT9211C chip\n");
+		return 0;
+	}
+
+	dev_err(ctx->dev, "Unknown Chip ID: 0x%02x 0x%02x 0x%02x\n", chipid[0],
+		chipid[1], chipid[2]);
+	return -EINVAL;
 }
 
 static int lt9211_system_init(struct lt9211 *ctx)
@@ -505,8 +577,8 @@ static void lt9211_atomic_enable(struct drm_bridge *bridge,
 		lvds_format_24bpp = true;
 		lvds_format_jeida = false;
 		dev_warn(ctx->dev,
-			 "Unsupported LVDS bus format 0x%04x, please check output bridge driver. Falling back to SPWG24.\n",
-			 bridge_state->output_bus_cfg.format);
+			"Unsupported LVDS bus format 0x%04x, please check output bridge driver. Falling back to SPWG24.\n",
+			bridge_state->output_bus_cfg.format);
 		break;
 	}
 
@@ -519,35 +591,42 @@ static void lt9211_atomic_enable(struct drm_bridge *bridge,
 	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	mode = &crtc_state->adjusted_mode;
-
 	ret = lt9211_read_chipid(ctx);
 	if (ret)
 		return;
 
-	ret = lt9211_system_init(ctx);
-	if (ret)
+	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));
+		dev_dbg(ctx->dev, "LT9211C enabled.\n");
 		return;
+	}
+		ret = lt9211_system_init(ctx);
+		if (ret)
+			return;
 
-	ret = lt9211_configure_rx(ctx);
-	if (ret)
-		return;
+		ret = lt9211_configure_rx(ctx);
+		if (ret)
+			return;
 
-	ret = lt9211_autodetect_rx(ctx, mode);
-	if (ret)
-		return;
+		ret = lt9211_autodetect_rx(ctx, mode);
+		if (ret)
+			return;
 
-	ret = lt9211_configure_timing(ctx, mode);
-	if (ret)
-		return;
+		ret = lt9211_configure_timing(ctx, mode);
+		if (ret)
+			return;
 
-	ret = lt9211_configure_plls(ctx, mode);
-	if (ret)
-		return;
+		ret = lt9211_configure_plls(ctx, mode);
+		if (ret)
+			return;
 
 	ret = lt9211_configure_tx(ctx, lvds_format_jeida, lvds_format_24bpp,
-				  bus_flags & DRM_BUS_FLAG_DE_HIGH);
-	if (ret)
-		return;
+					  bus_flags & DRM_BUS_FLAG_DE_HIGH);
+		if (ret)
+			return;
 
 	dev_dbg(ctx->dev, "LT9211 enabled.\n");
 }
@@ -672,11 +751,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;
@@ -685,6 +759,8 @@ static int lt9211_host_attach(struct lt9211 *ctx)
 	int dsi_lanes;
 	int ret;
 
+	/* Check if the compatible string matches lt9211c */
+
 	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
 	dsi_lanes = drm_of_get_data_lanes_count(endpoint, 1, 4);
 	host_node = of_graph_get_remote_port_parent(endpoint);
@@ -698,7 +774,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,
+		};
+		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");
@@ -707,10 +798,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;
+	} 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) {
@@ -732,6 +829,7 @@ static int lt9211_probe(struct i2c_client *client)
 		return PTR_ERR(ctx);
 
 	ctx->dev = dev;
+	ctx->chip_type = LT9211;
 
 	/*
 	 * Put the chip in reset, pull nRST line low,
@@ -745,13 +843,29 @@ static int lt9211_probe(struct i2c_client *client)
 	usleep_range(10000, 11000);	/* Very long reset duration. */
 
 	ret = lt9211_parse_dt(ctx);
+
 	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);
+	} 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);
 
+	/* Initialize LT9211C-specific fields */
+	ctx->wq = create_workqueue("lt9211_work");
+	if (!ctx->wq)
+		return -ENOMEM;
+
+	INIT_DELAYED_WORK(&ctx->lt9211_dw, lt9211_delayed_work_func);
+
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
@@ -769,17 +883,667 @@ static void lt9211_remove(struct i2c_client *client)
 {
 	struct lt9211 *ctx = i2c_get_clientdata(client);
 
+	if (ctx->wq)
+		destroy_workqueue(ctx->wq);
+
 	drm_bridge_remove(&ctx->bridge);
 }
 
+static int lt9211c_configure_rx(struct lt9211 *ctx)
+{
+	unsigned int pval;
+
+	const struct reg_sequence lt9211c_rx_phy_seq[] = {
+		{ REG_DSI_LANE, REG_DSI_LANE_COUNT(ctx->dsi->lanes) },
+		{ 0x8201, 0x11 },
+		{ 0x8218, 0x48 },
+		{ 0x8201, 0x91 },
+		{ 0x8202, 0x00 },
+		{ 0x8203, 0xee },
+		{ 0x8209, 0x21 },
+		{ 0x8204, 0x44 },
+		{ 0x8205, 0xc4 },
+		{ 0x8206, 0x44 },
+		{ 0x8213, 0x0c },
+
+		{ 0xd001, 0x00 },
+		{ 0xd002, 0x0e },
+		{ 0xd005, 0x00 },
+		{ 0xd00a, 0x59 },
+		{ 0xd00b, 0x20 },
+	};
+
+	const struct reg_sequence lt9211c_rx_phy_reset_seq[] = {
+		{ 0x8109, 0xde },
+		{ 0x8109, 0xdf },
+	};
+
+	const struct reg_sequence lt9211c_rx_clk_sel_seq[] = {
+		{ 0x85e9, 0x88 },
+		{ 0x8180, 0x51 },
+		{ 0x8181, 0x10 },
+		{ 0x8632, 0x03 },
+	};
+
+	const struct reg_sequence lt9211c_rx_input_sel_seq[] = {
+		{ 0xd004, 0x00 },
+		{ 0xd021, 0x46 },
+	};
+
+	const struct reg_sequence lt9211c_rx_dig_seq[] = {
+		{ 0x853f, 0x08 },
+		{ 0x8540, 0x04 },
+		{ 0x8541, 0x03 },
+		{ 0x8542, 0x02 },
+		{ 0x8543, 0x01 },
+		{ 0x8545, 0x04 },
+		{ 0x8546, 0x03 },
+		{ 0x8547, 0x02 },
+		{ 0x8548, 0x01 },
+		{ 0x8544, 0x00 },
+		{ 0x8549, 0x00 },
+	};
+
+	int ret;
+
+	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;
+
+	ret = regmap_read(ctx->regmap, 0x8680, &pval);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(ctx->regmap, 0x863f, (pval & 0xf8));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(ctx->regmap, 0x863f, 0x05);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(ctx->regmap, 0x8530, &pval);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0xf8) | 0x11));
+	if (ret)
+		return ret;
+
+	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);
+
+	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);
+
+	/* Width/Height/Format Auto-detection */
+	ret = regmap_bulk_read(ctx->regmap, 0xd082, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	width = (buf[0] << 8) | buf[1];
+	height = (buf[3] << 8) | buf[4];
+	format = buf[2] & 0xf;
+
+	if (format == 0x3) { /* YUV422 16bit */
+		width /= 2;
+	} else if (format == 0xa) { /* RGB888 24bit */
+		width /= 3;
+	} else {
+		dev_err(ctx->dev, "Unsupported DSI format 0x%01x\n", format);
+		return -EINVAL;
+	}
+
+	if (width != mode->hdisplay) {
+		dev_err(ctx->dev,
+			"RX: Detected DSI width (%d) does not match mode hdisplay (%d)\n",
+			width, mode->hdisplay);
+		return -EINVAL;
+	}
+
+	if (height != mode->vdisplay) {
+		dev_err(ctx->dev,
+			"RX: Detected DSI height (%d) does not match mode vdisplay (%d)\n",
+			height, mode->vdisplay);
+		return -EINVAL;
+	}
+
+	dev_dbg(ctx->dev, "RX: %dx%d format=0x%01x\n", width, height, format);
+	return 0;
+}
+
+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 },
+		{ 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)
+{
+	const struct reg_sequence lt9211c_dessc_pll_reset[] = {
+		{ 0x8103, 0xfe, 2000 },
+		{ 0x8103, 0xff, 0 },
+	};
+
+	const struct reg_sequence lt9211c_pcr_cali_seq[] = {
+		{ 0xd00a, 0x5f },
+		{ 0xd01e, 0x51 },
+		{ 0xd023, 0x80 },
+		{ 0xd024, 0x70 },
+		{ 0xd025, 0x80 },
+		{ 0xd02a, 0x10 },
+		{ 0xd021, 0x4f },
+		{ 0xd022, 0xf0 },
+		{ 0xd038, 0x04 },
+		{ 0xd039, 0x08 },
+		{ 0xd03a, 0x10 },
+		{ 0xd03b, 0x20 },
+		{ 0xd03f, 0x04 },
+		{ 0xd040, 0x08 },
+		{ 0xd041, 0x10 },
+		{ 0xd042, 0x20 },
+		{ 0xd02b, 0xA0 },
+	};
+
+	const struct reg_sequence lt9211c_pcr_reset_seq[] = {
+		{ 0xd009, 0xdb },
+		{ 0xd009, 0xdf },
+		{ 0xd008, 0x80 },
+		{ 0xd008, 0x00 },
+	};
+
+	unsigned int pval;
+	int ret;
+	u8 div;
+	u32 pcr_m;
+	u32 pcr_k;
+	u32 pcr_up;
+	u32 pcr_down;
+
+	/* DeSSC PLL reference clock is 25 MHz XTal. */
+	ret = regmap_write(ctx->regmap, 0x8226, 0x20);
+	if (ret)
+		return ret;
+
+	/* Prediv = 0 */
+	ret = regmap_write(ctx->regmap, 0x8227, 0x40);
+	if (ret)
+		return ret;
+
+	if (mode->clock < 22000) {
+		ret = regmap_write(ctx->regmap, 0x822f, 0x07);
+		ret |= regmap_write(ctx->regmap, 0x822c, 0x01);
+		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);
+	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);
+	if (ret < 0)
+		return ret;
+
+	/* 0xd02d: pcr_m overflow limit setting */
+	ret = regmap_write(ctx->regmap, 0xd02d, pcr_up);
+	if (ret < 0)
+		return ret;
+
+	/* 0xd031: pcr_m underflow limit setting */
+	ret = regmap_write(ctx->regmap, 0xd031, pcr_down);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_dessc_pll_reset,
+				     ARRAY_SIZE(lt9211c_dessc_pll_reset));
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_pcr_cali_seq,
+				     ARRAY_SIZE(lt9211c_pcr_cali_seq));
+	if (ret)
+		return ret;
+
+	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);
+	} else {
+		ret = regmap_write(ctx->regmap, 0xd00c, 0x40);
+		ret |= regmap_write(ctx->regmap, 0xd01b, 0x00);
+		ret |= regmap_write(ctx->regmap, 0xd01c, 0x40);
+	}
+	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)
+{
+	const struct reg_sequence lt9211c_tx_phy_off_seq[] = {
+		{ 0x8236, 0x00 },
+		{ 0x8237, 0x00 },
+		{ 0x8108, 0x6f },
+		{ 0x8103, 0xbf },
+	};
+
+	const struct reg_sequence lt9211c_tx_phy_seq[] = {
+		{ 0x8236, 0x03 },
+		{ 0x8237, 0x44 },
+		{ 0x8238, 0x14 },
+		{ 0x8239, 0x31 },
+		{ 0x823a, 0xc8 },
+		{ 0x823b, 0x00 },
+		{ 0x823c, 0x0f },
+		{ 0x8246, 0x40 },
+		{ 0x8247, 0x40 },
+		{ 0x8248, 0x40 },
+		{ 0x8249, 0x40 },
+		{ 0x824a, 0x40 },
+		{ 0x824b, 0x40 },
+		{ 0x824c, 0x40 },
+		{ 0x824d, 0x40 },
+		{ 0x824e, 0x40 },
+		{ 0x824f, 0x40 },
+		{ 0x8250, 0x40 },
+		{ 0x8251, 0x40 },
+	};
+
+	const struct reg_sequence lt9211c_tx_mltx_reset[] = {
+		{ 0x8103, 0xbf },
+		{ 0x8103, 0xff },
+	};
+
+	const struct reg_sequence lt9211c_tx_dig_seq[] = {
+		{ 0x854a, 0x01 },
+		{ 0x854b, 0x00 },
+		{ 0x854c, 0x10 },
+		{ 0x854d, 0x20 },
+		{ 0x854e, 0x50 },
+		{ 0x854f, 0x30 },
+		{ 0x8550, 0x46 },
+		{ 0x8551, 0x10 },
+		{ 0x8552, 0x20 },
+		{ 0x8553, 0x50 },
+		{ 0x8554, 0x30 },
+		{ 0x8555, 0x00 },
+		{ 0x8556, 0x20 },
+
+		{ 0x8568, 0x00 },
+		{ 0x856e, 0x10 | (ctx->de ? BIT(6) : 0) },
+		{ 0x856f, 0x81 | (ctx->jeida ? BIT(6) : 0) |
+				  (ctx->lvds_dual_link ? BIT(4) : 0) |
+				  (ctx->bpp24 ? BIT(2) : 0) },
+	};
+
+	const struct reg_sequence lt9211c_tx_ssc_seq[] = {
+		{ 0x8234, 0x00 },
+		{ 0x856e, 0x10 },
+		{ 0x8181, 0x15 },
+		{ 0x871e, 0x00 },
+		{ 0x8717, 0x02 },
+		{ 0x8718, 0x04 },
+		{ 0x8719, 0xd4 },
+		{ 0x871A, 0x00 },
+		{ 0x871B, 0x12 },
+		{ 0x871C, 0x00 },
+		{ 0x871D, 0x24 },
+		{ 0x871F, 0x1c },
+		{ 0x8720, 0x00 },
+		{ 0x8721, 0x00 },
+		{ 0x871e, 0x02 },
+	};
+
+	const struct reg_sequence lt9211c_tx_pll_reset_seq[] = {
+		{ 0x810c, 0xfe, 2000 },
+		{ 0x810c, 0xff, 0 },
+	};
+
+	const struct reg_sequence lt9211c_tx_sw_reset_seq[] = {
+		{ 0x8108, 0x6f, 2000 },
+		{ 0x8108, 0x7f, 0 },
+	};
+
+	unsigned int pval;
+	int ret;
+	u32 phy_clk;
+	u8 pixclk_div;
+	u8 pre_div;
+	u8 div_set;
+	u8 sericlk_div;
+	u8 val;
+
+	dev_info(ctx->dev,
+		 "dual_link=%d,even_odd_swap=%d,bpp24=%d,jeida=%d,de=%d\n",
+		 ctx->lvds_dual_link, ctx->lvds_dual_link_even_odd_swap,
+		 ctx->bpp24, ctx->jeida, ctx->de);
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_phy_off_seq,
+				     ARRAY_SIZE(lt9211c_tx_phy_off_seq));
+	if (ret)
+		return ret;
+
+	ret = regmap_read(ctx->regmap, 0x8530, &pval);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0x3f) | 0x40));
+	if (ret)
+		return ret;
+
+	/* [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;
+	} else if (mode->clock < 40000) {
+		val = 0x28;
+		pre_div = 1;
+	} 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;
+
+	/* 0x8232: serickdiv sel */
+	if (phy_clk < 80000) {
+		val = 0x32;
+		sericlk_div = 16;
+	} else if (phy_clk < 160000) {
+		val = 0x22;
+		sericlk_div = 8;
+	} else if (phy_clk < 320000) {
+		val = 0x12;
+		sericlk_div = 4;
+	} else if (phy_clk < 640000) {
+		val = 0x02;
+		sericlk_div = 2;
+	} else {
+		val = 0x42;
+		sericlk_div = 1;
+	}
+	ret = regmap_write(ctx->regmap, 0x8232, val);
+	if (ret < 0)
+		return ret;
+
+	/* 0x8233: pix_mux sel & pix_div sel
+	 * To avoid floating point operations, The pixclk_div is enlarged by 10 times
+	 */
+	if (mode->clock > 150000) {
+		val = 0x04;
+		pixclk_div = 35;
+	} else {
+		pixclk_div =
+			(u8)((phy_clk * sericlk_div * 10) / (mode->clock * 7));
+		if (pixclk_div <= 10)
+			val = 0x00;
+		else if (pixclk_div <= 20)
+			val = 0x01;
+		else if (pixclk_div <= 40)
+			val = 0x02;
+		else
+			val = 0x03;
+	}
+	ret = regmap_write(ctx->regmap, 0x8233, val);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(ctx->regmap, 0x8234, 0x01);
+	if (ret < 0)
+		return ret;
+
+	/* 0x8235: div set */
+	div_set = (u8)(phy_clk * sericlk_div / mode->clock / pre_div);
+	ret = regmap_write(ctx->regmap, 0x8235, div_set);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_ssc_seq,
+				     ARRAY_SIZE(lt9211c_tx_ssc_seq));
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_pll_reset_seq,
+				     ARRAY_SIZE(lt9211c_tx_pll_reset_seq));
+	if (ret)
+		return ret;
+
+	ret = regmap_read_poll_timeout(ctx->regmap, 0x8739, pval, pval & 0x04,
+				       10000, 1000000);
+	if (ret) {
+		dev_err(ctx->dev, "TX PLL unstable, ret=%i\n", ret);
+		return ret;
+	}
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_phy_seq,
+				     ARRAY_SIZE(lt9211c_tx_phy_seq));
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_mltx_reset,
+				     ARRAY_SIZE(lt9211c_tx_mltx_reset));
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_dig_seq,
+				     ARRAY_SIZE(lt9211c_tx_dig_seq));
+	if (ret)
+		return ret;
+
+	ret = regmap_multi_reg_write(ctx->regmap, lt9211c_tx_sw_reset_seq,
+				     ARRAY_SIZE(lt9211c_tx_sw_reset_seq));
+	if (ret)
+		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);
+	int ret;
+	const struct drm_display_mode *mode = &ctx->mode;
+
+	/* For LT9211C */
+	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 const struct i2c_device_id lt9211_id[] = {
 	{ "lontium,lt9211" },
+	{ "lontium,lt9211c" },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, lt9211_id);
 
 static const struct of_device_id lt9211_match_table[] = {
 	{ .compatible = "lontium,lt9211" },
+	{ .compatible = "lontium,lt9211c" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, lt9211_match_table);

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Claude review: This series adds LT9211C bridge driver by extending LT9211.
  2026-03-23  7:08 [PATCH v5 0/2] This series adds LT9211C bridge driver by extending LT9211 Venkata Gopi Nagaraju Botlagunta
  2026-03-23  7:08 ` [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support Venkata Gopi Nagaraju Botlagunta
  2026-03-23  7:08 ` [PATCH v5 2/2] drm/bridge: add support for lontium lt9211c bridge Venkata Gopi Nagaraju Botlagunta
@ 2026-03-24 22:04 ` Claude Code Review Bot
  2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 22:04 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: This series adds LT9211C bridge driver by extending LT9211.
Author: Venkata Gopi Nagaraju Botlagunta <venkata.botlagunta@oss.qualcomm.com>
Patches: 3
Reviewed: 2026-03-25T08:04:40.526295

---

This v5 series adds LT9211C bridge chip support by extending the existing `lontium-lt9211` driver. The approach of sharing a driver for a chip variant is reasonable, but the implementation has **serious code quality issues** that must be addressed before merging. The most critical problems are broken indentation in `lt9211_atomic_enable()`, improper use of workqueues, gratuitous whitespace changes to existing code, and several logic bugs in the new LT9211C register programming. This is on its 5th revision and still has fundamental structural issues.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Claude review: dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support
  2026-03-23  7:08 ` [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support Venkata Gopi Nagaraju Botlagunta
@ 2026-03-24 22:04   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 22:04 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch is straightforward and looks correct. It adds `lontium,lt9211c` to the compatible enum. One minor issue:

- The description line gets long: `The LT9211 and LT9211C are bridge devices which convert Single/Dual-Link DSI/LVDS`. Consider wrapping or keeping it concise. Not a blocker.

No major issues with this patch.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Claude review: drm/bridge: add support for lontium lt9211c bridge
  2026-03-23  7:08 ` [PATCH v5 2/2] drm/bridge: add support for lontium lt9211c bridge Venkata Gopi Nagaraju Botlagunta
@ 2026-03-24 22:04   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 22:04 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch has numerous problems:

**1. Broken indentation in `lt9211_atomic_enable()` (Critical)**

The LT9211 (non-C) code path has wrong indentation — it looks like it was meant to be inside an `else` block but isn't:

```c
	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));
		dev_dbg(ctx->dev, "LT9211C enabled.\n");
		return;
	}
		ret = lt9211_system_init(ctx);
		if (ret)
			return;

		ret = lt9211_configure_rx(ctx);
		if (ret)
			return;
```

The code after the `if` block is indented with two tabs instead of one. While this still compiles and functions correctly (the `return` in the if-block prevents fall-through being an issue), the indentation is wrong and misleading. It should use single-tab indentation matching the original code. Same problem occurs for `lt9211_configure_tx`:

```c
	ret = lt9211_configure_tx(ctx, lvds_format_jeida, lvds_format_24bpp,
					  bus_flags & DRM_BUS_FLAG_DE_HIGH);
		if (ret)
			return;
```

Here the `if (ret)` is double-indented but `lt9211_configure_tx` is not — this is inconsistent and the `if (ret)` check is at the wrong indentation level.

**2. Same broken indentation in `lt9211_delayed_work_func()` (Critical)**

```c
	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;
```

Code after the guard clause is double-indented. Should be single-tab.

**3. Gratuitous whitespace changes (Style)**

The patch changes alignment of existing struct members and reformats existing `dev_warn` calls that are unrelated to the functional change:

```c
-	struct mipi_dsi_device		*dsi;
+	struct mipi_dsi_device	*dsi;
...
-	bool				lvds_dual_link;
-	bool				lvds_dual_link_even_odd_swap;
+	bool					lvds_dual_link;
+	bool					lvds_dual_link_even_odd_swap;
```

These are inconsistent with each other and with existing code style. Don't change alignment of existing fields.

**4. Workqueue usage is wrong (Design)**

- `create_workqueue()` is deprecated — use `alloc_ordered_workqueue()` or just use `system_wq` with `schedule_delayed_work()`. There's no reason for a private workqueue here.
- The workqueue is created unconditionally for both LT9211 and LT9211C in `lt9211_probe()`, even though only LT9211C uses it. The comment says "Initialize LT9211C-specific fields" but doesn't check `chip_type` first.
- Using a workqueue to delay initialization by 100ms because "LT9211C must enable after mipi clock enable" is a fragile design. This is a race condition — what if 100ms is not enough? There should be a proper synchronization mechanism or at minimum a poll for a ready status.

**5. `lvds_format_jeida`/`lvds_format_24bpp`/`de` not set for LT9211C path (Bug)**

In `lt9211_atomic_enable()`, the LT9211C path returns early before the bus format switch/case determines `lvds_format_jeida` and `lvds_format_24bpp`. The delayed work function uses `ctx->bpp24`, `ctx->jeida`, and `ctx->de` which are never set — they remain at their zero-initialized values. The bus format negotiation result is lost.

**6. `lt9211c_configure_tx` uses `ctx->de` without it being set (Bug)**

```c
	{ 0x856e, 0x10 | (ctx->de ? BIT(6) : 0) },
	{ 0x856f, 0x81 | (ctx->jeida ? BIT(6) : 0) |
			  (ctx->lvds_dual_link ? BIT(4) : 0) |
			  (ctx->bpp24 ? BIT(2) : 0) },
```

None of `ctx->de`, `ctx->jeida`, or `ctx->bpp24` are populated anywhere in the code path. They're always false/0.

**7. Register write logic bug in `lt9211c_configure_rx()`**

```c
	ret = regmap_write(ctx->regmap, 0x863f, (pval & 0xf8));
	...
	ret = regmap_write(ctx->regmap, 0x863f, 0x05);
```

Register `0x863f` is written with a masked value of `pval` from register `0x8680`, then immediately overwritten with `0x05`. The first write is pointless.

**8. Suspicious bitmask in `lt9211c_configure_rx()`**

```c
	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0xf8) | 0x11));
```

`0xf8` masks off bits [2:0], then `0x11` (binary `0001_0001`) sets bits 0 and 4. Bit 4 isn't cleared by the `0xf8` mask, so the OR with `0x11` on bit 4 would only matter if bit 4 was already 0. But bit 0 *is* cleared by the mask and then set — this looks intentional but confusing. The same register is later written differently in `lt9211c_configure_tx()` with `(pval & 0x3f) | 0x40`.

**9. PLL computation bug**

```c
	/* 0xd026: pcr_m */
	ret = regmap_write(ctx->regmap, 0xd026, (0x80 | (u8)pcr_m) & 0x7f);
```

`0x80 | x` sets bit 7, then `& 0x7f` clears bit 7. This is equivalent to just `(u8)pcr_m & 0x7f` — the `0x80 |` is dead code and suggests a copy-paste error from a vendor SDK.

**10. Error handling inconsistency**

The code uses `ret |= regmap_write(...)` pattern in several places within `lt9211c_configure_plls()`:

```c
		ret = regmap_write(ctx->regmap, 0x822f, 0x07);
		ret |= regmap_write(ctx->regmap, 0x822c, 0x01);
```

This ORs error codes together, which loses the original error value and can mask errors. Use proper sequential error checking.

**11. `dev_info` in `lt9211c_configure_tx()` should be `dev_dbg`**

```c
	dev_info(ctx->dev,
		 "dual_link=%d,even_odd_swap=%d,bpp24=%d,jeida=%d,de=%d\n",
```

This prints every time the bridge is enabled. Should be `dev_dbg`.

**12. Stale comment left in `lt9211_host_attach()`**

```c
	/* Check if the compatible string matches lt9211c */

	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
```

This comment describes nothing — it's a leftover with no corresponding code.

**13. Unnecessary duplication in `lt9211_host_attach()`**

The `mipi_dsi_device_info` differs only in `.type`. This could be simplified with a ternary:

```c
	const struct mipi_dsi_device_info info = {
		.type = ctx->chip_type == LT9211C ? "lt9211c" : "lt9211",
		.channel = 0,
		.node = NULL,
	};
```

**14. Blank line removed between mode assignment and chipid read**

```c
	mode = &crtc_state->adjusted_mode;
-
	ret = lt9211_read_chipid(ctx);
```

This removes a logical separator that existed in the original code for readability.

**15. Redundant `chip_type` initialization**

```c
	ctx->chip_type = LT9211;
```

in `lt9211_probe()`, followed later by:

```c
	if (of_device_is_compatible(dev->of_node, "lontium,lt9211c")) {
		ctx->chip_type = LT9211C;
```

The `devm_drm_bridge_alloc` zero-initializes the struct, and `LT9211 = 0`, so the explicit initialization is redundant (though harmless).

**16. `lt9211c_regmap_config` uses `REGCACHE_RBTREE`**

The original LT9211 uses `REGCACHE_MAPLE`. The LT9211C variant should also use `REGCACHE_MAPLE` for consistency (RBTREE is the older, less efficient implementation).

**17. `atomic_disable` doesn't cancel the delayed work**

If `atomic_disable` is called while the delayed work is still pending (within the 100ms window), the work will fire after the chip is put into reset, causing register writes to a reset chip. Need `cancel_delayed_work_sync()` in `lt9211_atomic_disable()`.

**Summary**: This patch needs significant rework. The indentation issues, missing LVDS format propagation to LT9211C, workqueue misuse, and missing work cancellation on disable are the most serious problems.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-03-24 22:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-23  7:08 [PATCH v5 0/2] This series adds LT9211C bridge driver by extending LT9211 Venkata Gopi Nagaraju Botlagunta
2026-03-23  7:08 ` [PATCH v5 1/2] dt-bindings: display: bridge: lontium,lt9211: Add lt9211c support Venkata Gopi Nagaraju Botlagunta
2026-03-24 22:04   ` Claude review: " Claude Code Review Bot
2026-03-23  7:08 ` [PATCH v5 2/2] drm/bridge: add support for lontium lt9211c bridge Venkata Gopi Nagaraju Botlagunta
2026-03-24 22:04   ` Claude review: " Claude Code Review Bot
2026-03-24 22:04 ` Claude review: This series adds LT9211C bridge driver by extending LT9211 Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox