From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: ti-sn65dsi83: two fixes + add test pattern
Date: Tue, 10 Mar 2026 11:48:29 +1000
Message-ID: <review-patch1-20260309-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v2-0-e6aaa7e1d181@bootlin.com>
In-Reply-To: <20260309-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v2-0-e6aaa7e1d181@bootlin.com>
References: <20260309-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v2-0-e6aaa7e1d181@bootlin.com> <20260309-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v2-0-e6aaa7e1d181@bootlin.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 an=
d 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 conc=
ern).**

   ```c
   le16val =3D 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, regardle=
ss of whether `ctx->lvds_dual_link` is set. For single-LVDS configurations =
with test pattern enabled, this would incorrectly halve the active line len=
gth. This should likely be:
   ```c
   le16val =3D cpu_to_le16(mode->hdisplay / ((test_pattern && ctx->lvds_dua=
l_link) ? 2 : 1));
   ```

2. **Module parameter is a global static, shared across all instances (mino=
r concern).** If there are multiple SN65DSI83 chips in a system, the test p=
attern 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 =3D sn65dsi83_test_pattern;
   ```

   This is a reasonable approach =E2=80=94 it snapshots the value at the st=
art 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 descri=
ption 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 ar=
e persistent and affect all instances. For a debug feature like this, a per=
-bridge debugfs file might be more appropriate, especially since the parame=
ter is writable at runtime (0644). However, module parameters are simpler t=
o implement and the existing driver doesn't use debugfs, so this is a reaso=
nable choice for a debug tool.

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

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

---
Generated by Claude Code Patch Reviewer