[PATCH v2] drm/bridge: ti-sn65dsi83: two fixes + add test pattern

[PATCH v2] drm/bridge: ti-sn65dsi83: two fixes + add test pattern

[Luca Ceresoli]

This series used to have two bug fixes, now applied to drm-misc-fuxes, and
a patch adding support for enabling the test pattern output from userspace,
which is at its v2 after fixing a small issue in v1.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v2:
- Removed patches now on drm-misc-fixes
- Fixes potential inconsistend values in the test_pattern patch
- Link to v1: https://lore.kernel.org/r/20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-0-2e15f5a9a6a0@bootlin.com

---
Luca Ceresoli (1):
      drm/bridge: ti-sn65dsi83: add test pattern generation support

 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
---
base-commit: bfb18fd193e2413f02ad934e46887f415f0ce4ec
change-id: 20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-f5ff67e1900c

Best regards,
--  
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v2] drm/bridge: ti-sn65dsi83: add test pattern generation support

[Luca Ceresoli]

Generation of a test pattern output is a useful tool for panel bringup and
debugging, and very simple to support with this chip.

The value of REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW needs to be divided by two
for the test pattern to work in dual LVDS mode. While not clearly stated in
the datasheet, this is needed according to the DSI Tuner [0] output. And
some dual-LVDS panels refuse to show any picture without this division by
two.

[0] https://www.ti.com/tool/DSI-TUNER

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v2:
 - added local variable to avoid potential race condition leading to
   inconsistent settings
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index f6736b4457bb..1936080e6a1a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -114,6 +114,7 @@
 #define REG_VID_CHA_HORIZONTAL_FRONT_PORCH	0x38
 #define REG_VID_CHA_VERTICAL_FRONT_PORCH	0x3a
 #define REG_VID_CHA_TEST_PATTERN		0x3c
+#define  REG_VID_CHA_TEST_PATTERN_EN		BIT(4)
 /* IRQ registers */
 #define REG_IRQ_GLOBAL				0xe0
 #define  REG_IRQ_GLOBAL_IRQ_EN			BIT(0)
@@ -134,6 +135,9 @@
 #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
 #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
 
+static bool sn65dsi83_test_pattern;
+module_param_named(test_pattern, sn65dsi83_test_pattern, bool, 0644);
+
 enum sn65dsi83_channel {
 	CHANNEL_A,
 	CHANNEL_B
@@ -522,6 +526,7 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	const struct drm_display_mode *mode;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
+	bool test_pattern = sn65dsi83_test_pattern;
 	bool lvds_format_24bpp;
 	bool lvds_format_jeida;
 	unsigned int pval;
@@ -644,7 +649,11 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
 	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
 
-	le16val = cpu_to_le16(mode->hdisplay);
+	/*
+	 * Active line length needs to be halved for test pattern
+	 * generation in dual LVDS output.
+	 */
+	le16val = cpu_to_le16(mode->hdisplay / (test_pattern ? 2 : 1));
 	regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
 			  &le16val, 2);
 	le16val = cpu_to_le16(mode->vdisplay);
@@ -667,7 +676,8 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		     mode->hsync_start - mode->hdisplay);
 	regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
 		     mode->vsync_start - mode->vdisplay);
-	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
+	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN,
+		     test_pattern ? REG_VID_CHA_TEST_PATTERN_EN : 0);
 
 	/* Enable PLL */
 	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);

-- 
2.53.0

Claude review: drm/bridge: ti-sn65dsi83: two fixes + add test pattern

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ti-sn65dsi83: two fixes + add test pattern
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>
Patches: 2
Reviewed: 2026-03-10T11:48:29.150205

---

This is a single-patch series (v2) that adds test pattern generation support to the TI SN65DSI83 DSI-to-LVDS bridge driver. The feature is controlled via a module parameter. The patch is simple, well-scoped, and addresses a real debugging/bringup need. There are a few design concerns worth discussing.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ti-sn65dsi83: two fixes + add test pattern

[Claude Code Review Bot]

Patch Review

**Functionality**: The patch adds a `test_pattern` module parameter (bool, 0644) that, when set, enables the chip's built-in test pattern generator and halves the active line length for dual LVDS mode compatibility.

**Issues and observations:**

1. **The hdisplay halving is not conditional on dual-link mode (medium concern).**

   ```c
   le16val = cpu_to_le16(mode->hdisplay / (test_pattern ? 2 : 1));
   ```

   The comment says:
   ```
   /* Active line length needs to be halved for test pattern
    * generation in dual LVDS output. */
   ```

   But the code halves `hdisplay` whenever `test_pattern` is true, regardless of whether `ctx->lvds_dual_link` is set. For single-LVDS configurations with test pattern enabled, this would incorrectly halve the active line length. This should likely be:
   ```c
   le16val = cpu_to_le16(mode->hdisplay / ((test_pattern && ctx->lvds_dual_link) ? 2 : 1));
   ```

2. **Module parameter is a global static, shared across all instances (minor concern).** If there are multiple SN65DSI83 chips in a system, the test pattern is all-or-nothing. For a debug/bringup tool this is acceptable, but it's worth noting. The v2 changelog mentions a local variable copy to avoid race conditions:

   ```c
   bool test_pattern = sn65dsi83_test_pattern;
   ```

   This is a reasonable approach — it snapshots the value at the start of `sn65dsi83_atomic_pre_enable` so both the hdisplay division and the test pattern register write use a consistent value.

3. **Missing MODULE_PARM_DESC (minor).** The module parameter has no description string, which means `modinfo` won't show any help text for it. Adding:
   ```c
   MODULE_PARM_DESC(test_pattern, "Enable test pattern output for debugging");
   ```
   would improve usability.

4. **Module parameter vs. debugfs (design question).** Module parameters are persistent and affect all instances. For a debug feature like this, a per-bridge debugfs file might be more appropriate, especially since the parameter is writable at runtime (0644). However, module parameters are simpler to implement and the existing driver doesn't use debugfs, so this is a reasonable choice for a debug tool.

5. **The test pattern register define looks correct.** Cross-referencing with the datasheet convention used in the driver, `REG_VID_CHA_TEST_PATTERN_EN` at `BIT(4)` of register `0x3c` is consistent with the TI SN65DSI83/SN65DSI84 documentation.

**Summary**: The patch is clean and well-motivated. The main issue is **#1** — the hdisplay halving should be conditional on dual-link mode, not just on test pattern being enabled, otherwise single-link LVDS setups would get an incorrect active line length when using the test pattern.

---
Generated by Claude Code Patch Reviewer