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/scdc-helper: Implement parsing and printing HDMI 2.1 fields
Date: Fri, 05 Jun 2026 06:21:29 +1000
Message-ID: <review-patch4-20260604-scdc-link-health-v5-4-11173b0ac3de@collabora.com>
In-Reply-To: <20260604-scdc-link-health-v5-4-11173b0ac3de@collabora.com>
References: <20260604-scdc-link-health-v5-0-11173b0ac3de@collabora.com> <20260604-scdc-link-health-v5-4-11173b0ac3de@collabora.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

**Bug: Missing `else` for `rs_corrections` zeroing.**

```c
if (num_lanes =3D=3D 4 && (buf[SCDC_ERR_DET_RS_H] & SCDC_CHANNEL_VALID))
    state->rs_corrections =3D (buf[SCDC_ERR_DET_RS_H] & ~SCDC_CHANNEL_VALID=
) << 8 |
                            buf[SCDC_ERR_DET_RS_L];
```

When the channel valid bit is not set, `rs_corrections` is left uninitializ=
ed (or carries the value from a prior read). Compare with `counter[3]` whic=
h correctly has an `else counter[3] =3D 0` clause. Should add:
```c
else
    state->rs_corrections =3D 0;
```

**Issue: Training pattern fields parsed but never displayed.**

`drm_scdc_parse_status1_2_flags()` populates `ln[0-3]_training_pattern` in =
the state, but `scdc_status_show()` never prints them. Either add the displ=
ay or defer parsing these fields until a consumer exists.

**Style: Missing parentheses in one condition but present in the equivalent=
 one.**

```c
if (num_lanes =3D=3D 4 && scdc[SCDC_ERR_DET_3_H] & SCDC_CHANNEL_VALID)  // =
no parens
```
vs.
```c
if (num_lanes =3D=3D 4 && (buf[SCDC_ERR_DET_RS_H] & SCDC_CHANNEL_VALID))  /=
/ has parens
```

Both are correct due to C precedence (`&` binds tighter than `&&`), but the=
 inconsistency is confusing and likely to attract compiler warnings. Add pa=
rentheses to the first for clarity and consistency.

**Nit: `__pure` on `drm_scdc_num_frl_lanes`.**

```c
static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate)
```

The v4 changelog said "Drop the __pure attributes on static functions." For=
 a static inline function that only switches on its argument, the compiler =
will determine purity on its own. This is harmless but unnecessary and inco=
nsistent with the stated v4 cleanup.

**Nit: `enum drm_scdc_frl_rate rate : 4` uses an enum bitfield.** Storing a=
n enum in a 4-bit bitfield is implementation-defined in C, though it works =
reliably with GCC/Clang which are the only compilers the kernel supports. W=
orth being aware of.

**Observation: Checksum coverage for 4 lanes.** For the 4-lane case, the ch=
ecksum loop sums registers `0x50=E2=80=930x58`, with the checksum byte itse=
lf at `0x56` (in the middle). This means the original 3-lane checksum byte =
must be recalculated by the sink to cover the two additional lane-3 registe=
rs. This is presumably correct per the HDMI 2.1 spec, but since I can't ver=
ify against the spec, flagging for the author to confirm.

---
Generated by Claude Code Patch Reviewer