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: Thu, 28 May 2026 12:16:29 +1000 Message-ID: In-Reply-To: <20260527-scdc-link-health-v4-4-622ea40a1f59@collabora.com> References: <20260527-scdc-link-health-v4-0-622ea40a1f59@collabora.com> <20260527-scdc-link-health-v4-4-622ea40a1f59@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Verdict: A couple of issues.** The HDMI 2.1 extension is well done overall. The `scdc_err_cnt_buf_idx` helper correctly handles the register gap (lane 3 at 0x57-0x58, after checksum at 0x56). The FRL rate enum and helper are clean. **1. Training patterns are read but never displayed** (moderate): `drm_scdc_read_status1_2_flags` reads `ln{0,1,2,3}_training_pattern` from status registers 1 and 2, and `drm_scdc_read_state` calls it when `SCDC_FLT_UPDATE` is set. However, `scdc_status_show` never prints these training pattern values. If the fields are worth reading and storing, they should be displayed in the debugfs output. Otherwise, there's no way for users to see them. **2. `__pure` still present on `drm_scdc_num_frl_lanes`** despite the v4 changelog stating "Drop the __pure attributes on static functions": ```c static inline __pure int drm_scdc_num_frl_lanes(enum drm_scdc_frl_rate rate) ``` The `__pure` attribute on a `static inline` function whose body is fully visible to the compiler is redundant -- GCC can already see it's a pure function. This is harmless but inconsistent with the stated v4 changes. **3. Enum bitfield**: `enum drm_scdc_frl_rate rate : 4` in `drm_scdc_state` -- the signedness of enum bitfields is implementation-defined in C. With values 0-15 in the enum, a signed 4-bit field would misrepresent values 8-15. GCC infers unsigned for enums whose range fits in unsigned, so this works in practice, but it's a portability subtlety worth being aware of. **4. `drm_scdc_frl_rate_str` returns `NULL` for the `default` case**: Since all 16 possible 4-bit values are covered by explicit cases (0-6 valid, 7-15 reserved), the `default` branch returning `NULL` is unreachable. If somehow hit, passing NULL to `seq_printf`'s `%s` would trigger a kernel warning. Consider returning `"(Unknown)"` instead of `NULL` for defensive safety, or drop the `default` case entirely. --- Generated by Claude Code Patch Reviewer