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: Thu, 28 May 2026 12:16:29 +1000 [thread overview]
Message-ID: <review-patch4-20260527-scdc-link-health-v4-4-622ea40a1f59@collabora.com> (raw)
In-Reply-To: <20260527-scdc-link-health-v4-4-622ea40a1f59@collabora.com>
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
next prev parent reply other threads:[~2026-05-28 2:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-27 14:03 [PATCH v4 0/4] Add SCDC information to connector debugfs Nicolas Frattaroli
2026-05-27 14:03 ` [PATCH v4 1/4] drm/scdc-helper: Don't use ssize_t return type for scdc_read/write Nicolas Frattaroli
2026-05-28 2:16 ` Claude review: " Claude Code Review Bot
2026-05-27 14:03 ` [PATCH v4 2/4] drm/scdc-helper: Add scdc_status debugfs entry Nicolas Frattaroli
2026-05-28 2:16 ` Claude review: " Claude Code Review Bot
2026-05-27 14:03 ` [PATCH v4 3/4] drm/display: bridge_connector: init scdc debugfs for HDMI Nicolas Frattaroli
2026-05-28 2:16 ` Claude review: " Claude Code Review Bot
2026-05-27 14:03 ` [PATCH v4 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields Nicolas Frattaroli
2026-05-28 2:16 ` Claude Code Review Bot [this message]
2026-05-28 2:16 ` Claude review: Add SCDC information to connector debugfs Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-05-26 10:19 [PATCH v3 0/4] " Nicolas Frattaroli
2026-05-26 10:20 ` [PATCH v3 4/4] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields Nicolas Frattaroli
2026-05-27 5:01 ` Claude review: " Claude Code Review Bot
2026-05-20 13:35 [PATCH v2 0/3] Add SCDC information to connector debugfs Nicolas Frattaroli
2026-05-20 13:35 ` [PATCH v2 3/3] drm/scdc-helper: Implement parsing and printing HDMI 2.1 fields Nicolas Frattaroli
2026-05-25 11:49 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch4-20260527-scdc-link-health-v4-4-622ea40a1f59@collabora.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox