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: add test pattern generation support
Date: Fri, 27 Feb 2026 11:46:39 +1000
Message-ID: <review-patch3-20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-3-2e15f5a9a6a0@bootlin.com>
In-Reply-To: <20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-3-2e15f5a9a6a0@bootlin.com>
References: <20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-0-2e15f5a9a6a0@bootlin.com> <20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-3-2e15f5a9a6a0@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

**Verdict: Has a bug in the hdisplay halving logic.**

**Bug =E2=80=94 hdisplay halved unconditionally instead of only for dual LV=
DS:**

The commit message says:
> "Active line length needs to be halved for test pattern generation **in d=
ual LVDS output**."

But the code halves `hdisplay` whenever the test pattern is enabled, regard=
less of whether dual LVDS mode is active:

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

On an SN65DSI83 (single LVDS only), enabling the test pattern would incorre=
ctly halve the active line length. This should use the `dual_factor` introd=
uced in patch 2:

```c
le16val =3D cpu_to_le16(mode->hdisplay / (sn65dsi83_test_pattern ? dual_fac=
tor : 1));
```

This way single LVDS (`dual_factor =3D=3D 1`) gets no division, and dual LV=
DS (`dual_factor =3D=3D 2`) gets the correct halving.

**Missing MODULE_PARM_DESC:**

The module parameter has no description:

```c
static bool sn65dsi83_test_pattern;
module_param_named(test_pattern, sn65dsi83_test_pattern, bool, 0644);
```

Adding a `MODULE_PARM_DESC` would help users discover and understand the pa=
rameter (e.g., `MODULE_PARM_DESC(test_pattern, "Enable LVDS test pattern ou=
tput");`).

**Minor =E2=80=94 global state for per-device feature:**

The module parameter is a file-scope `static bool`, meaning it applies to a=
ll SN65DSI83/84 instances in the system. For a debug/bringup feature this i=
s probably acceptable, but worth noting. A per-device debugfs entry would b=
e cleaner if there are multi-bridge use cases.

**Minor =E2=80=94 runtime writability vs. effect:**

The parameter is 0644 (writable at runtime), but changes only take effect o=
n the next `atomic_pre_enable` (i.e., next modeset). This is expected behav=
ior but could surprise users. The `MODULE_PARM_DESC` could note this.

---
Generated by Claude Code Patch Reviewer