From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields Date: Wed, 27 May 2026 15:01:45 +1000 Message-ID: In-Reply-To: <20260526-scdc-link-health-v3-4-59e4a4aaead1@collabora.com> References: <20260526-scdc-link-health-v3-0-59e4a4aaead1@collabora.com> <20260526-scdc-link-health-v3-4-59e4a4aaead1@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 This patch extends the HDMI 2.0 code for HDMI 2.1 FRL (Fixed Rate Link) sup= port. Good approach of not requiring a version check since HDMI 2.0 sinks h= ave reserved fields defined as 0. **Error counter checksum with 4 lanes:** For 4-lane mode, the code reads 9 = bytes (0x50-0x58) and checksums all of them: ```c case 4: buf_sz =3D 9; break; ... for (i =3D 0; i < buf_sz; i++) sum =3D wrapping_add(u8, sum, buf[i]); ``` The checksum register is at 0x56 (buf[6]). This assumes the HDMI 2.1 checks= um at 0x56 covers registers 0x50-0x55 *and* 0x57-0x58 (all error detection = registers). Please confirm this matches the CTA-861-H spec. If the checksum= only covers 0x50-0x55 (the HDMI 2.0 range), the validation would fail when= lane 3 has non-zero data, as bytes 0x57-0x58 would be summed but not inclu= ded in the checksum calculation by the sink. **Enum bitfield:** ```c enum drm_scdc_frl_rate rate : 4; u8 ffe_levels : 4; ``` Enum bitfields are a GCC/Clang extension, not standard C. This works in the= kernel build environment but is worth noting. The 4-bit width is sufficien= t for the 0-15 value range since GCC treats this enum's underlying type as = unsigned (all values are non-negative). **`scdc_err_cnt_buf_idx` function:** This works correctly but is more compl= ex than needed. Since the registers are at known offsets, a simpler computa= tion would be `lane * 2` for lanes 0-2 and 7 for lane 3. The switch stateme= nt is clearer about the non-contiguous register layout though (checksum sit= s between lane 2 and lane 3), so it's a reasonable choice. **`drm_scdc_frl_rate_str` has a `default: return NULL` case:** ```c default: return NULL; ``` All 16 possible 4-bit values are covered by the switch cases (0-6 named, 7-= 15 reserved), so the `default` is unreachable. Returning `NULL` from a func= tion whose result is passed directly to `seq_printf` via `scdc_print_str` w= ould cause a NULL dereference if it were ever reached. Consider returning `= "(Unknown)"` instead, or removing the default case entirely. **FRL rate 0 =E2=86=92 3 lanes fallback:** The handling in `drm_scdc_read_s= tate` is correct: ```c num_lanes =3D drm_scdc_num_frl_lanes(state->rate); if (num_lanes < 0) return num_lanes; if (!num_lanes) num_lanes =3D 3; ``` When FRL is off (TMDS mode), defaulting to 3 lanes for the HDMI 2.0 error c= ounters makes sense. **Status register 1 bitfield name:** The field is called `flags` but now co= ntains 4-bit training pattern values rather than individual boolean flags: ```c struct { u8 ln0_training_pattern : 4; u8 ln1_training_pattern : 4; } flags __packed; ``` Consider renaming it to something like `fields` to better reflect the conte= nt, since these are multi-bit values, not flags. --- Generated by Claude Code Patch Reviewer