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: Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
Date: Mon, 25 May 2026 22:06:44 +1000
Message-ID: <review-patch1-81944af1-c9e3-4a45-ad42-3426dcab0777@kwiboo.se>
In-Reply-To: <81944af1-c9e3-4a45-ad42-3426dcab0777@kwiboo.se>
References: <81944af1-c9e3-4a45-ad42-3426dcab0777@kwiboo.se> <81944af1-c9e3-4a45-ad42-3426dcab0777@kwiboo.se>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness of the formula:**

The computation mirrors the legacy driver at `dw-hdmi.c:673-675`:
```c
tmp =3D (u64)ftdms * n;
do_div(tmp, 128 * sample_rate);
cts =3D tmp;
```

The patch does the equivalent:
```c
u64 computed =3D (u64)tmds_char_rate * n;
do_div(computed, 128ULL * sample_rate);
cts =3D (unsigned int)computed;
```

This is correct. The `u64` cast prevents overflow since `tmds_char_rate` (u=
p to ~600MHz) times `n` (up to ~10240) can exceed 32 bits.

**Guard condition:**

```c
if (!cts && n) {
```

This is the right check =E2=80=94 only compute when `find_cts()` returned 0=
 (no table entry) and `n` is non-zero (so the division is meaningful). If `=
n` were somehow 0, the computation would produce 0 anyway, so the `n` guard=
 is defensive but harmless.

**Minor observation =E2=80=94 `dw_hdmi_qp_find_cts` can return `-ENOENT`:**

Looking at `dw_hdmi_qp_find_cts()` at line 338:
```c
default:
    return -ENOENT;
```

If an unsupported sample rate is passed (not 32k/44.1k/48k or their multipl=
es), `find_cts()` returns `-ENOENT` (cast to `unsigned int`, a very large v=
alue). In that case `!cts` would be false, so the fallback wouldn't trigger=
 =E2=80=94 which is actually fine, since the CTS value would be garbage and=
 the subsequent `dw_hdmi_qp_set_cts_n()` would enable CTS override with tha=
t garbage. However, this is a pre-existing bug in `dw_hdmi_qp_find_cts()` u=
nrelated to this patch: the same problematic `-ENOENT` return exists for `d=
w_hdmi_qp_match_tmds_n_table()` too, and both pre-date this change. Not som=
ething this patch needs to fix.

**`128ULL` vs `128`:**

The patch uses `128ULL * sample_rate` in the divisor. The legacy driver use=
s `128 * sample_rate`. With `sample_rate` at most 192000, `128 * 192000 =3D=
 24,576,000`, well within 32-bit range, so the `ULL` suffix is unnecessary =
but harmless =E2=80=94 it's slightly more defensive. Fine either way.

**Comment quality:**

The inline comment is longer than strictly necessary (9 lines), but given t=
his is a non-obvious fallback path that compensates for hardware deficiency=
, the explanation of *why* auto-CTS fails and what the fix does is warrante=
d. This is a case where the "why" truly is non-obvious.

**No issues found. The patch is correct and well-tested.**

---
Generated by Claude Code Patch Reviewer