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/bridge: ite-it66121: Switch to the HDMI connector helpers Date: Mon, 25 May 2026 17:50:13 +1000 Message-ID: In-Reply-To: <20260523-it66121-fix-dvi-mode-v5-v5-1-33b4468162f9@redhat.com> References: <20260523-it66121-fix-dvi-mode-v5-v5-0-33b4468162f9@redhat.com> <20260523-it66121-fix-dvi-mode-v5-v5-1-33b4468162f9@redhat.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 **Build error =E2=80=94 undefined symbol:** The `it66121_bridge_hdmi_tmds_char_rate_valid()` function uses `HDMI_TMDS_C= HAR_RATE_MIN_HZ`: ```c if (tmds_rate < HDMI_TMDS_CHAR_RATE_MIN_HZ) return MODE_CLOCK_LOW; ``` This macro is not defined anywhere in the kernel tree. The original code us= ed `mode->clock < 25000` (25 MHz in kHz). This needs to be either defined l= ocally (e.g., `#define HDMI_TMDS_CHAR_RATE_MIN_HZ 25000000ULL`) or replaced= with a literal `25000000ULL`. The sister driver `ite-it6263.c` defines its= own `MAX_HDMI_TMDS_CHAR_RATE_HZ` locally, suggesting local definition is t= he expected pattern. **VSIF write =E2=80=94 no length bounds check:** The VSIF write function writes the full buffer directly to registers starti= ng at `IT66121_PKT_NULL_HB(0)` (0x138): ```c ret =3D regmap_bulk_write(ctx->regmap, IT66121_PKT_NULL_HB(0), buffer, len); ``` There is no validation that `len` fits within the available null packet reg= ister space. The AVI infoframe write is careful to write exactly `HDMI_AVI_= INFOFRAME_SIZE` bytes, and the audio infoframe uses `min_t(size_t, len - HD= MI_INFOFRAME_HEADER_SIZE, 5)` to clamp. The VSIF write should have similar = protection, or at least a comment documenting the maximum supported size. I= n practice the framework sends well-formed packets, so this is a minor robu= stness concern rather than a functional bug. **Locking asymmetry between clear and write callbacks:** The `write_*` callbacks all acquire `ctx->lock`, but the `clear_*` callback= s do not: ```c static int it66121_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridg= e) { struct it66121_ctx *ctx =3D container_of(bridge, struct it66121_ctx, br= idge); return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0); } ``` Since these are all called from the serialized atomic commit path, this is = safe in practice. However, the inconsistency is worth noting =E2=80=94 if `= ctx->lock` protects register access, the clears should arguably also take i= t for defensive consistency. **Rate conversion correctness:** The max rate conversion from kHz to Hz is correct: - Old: `max_clock =3D (ctx->bus_width =3D=3D 12) ? 74250 : 148500` (kHz, co= mpared against `mode->clock`) - New: `max_rate =3D (ctx->bus_width =3D=3D 12) ? 74250000ULL : 148500000UL= L` (Hz, compared against `tmds_rate`) **Audio infoframe data clamped to 5 bytes:** ```c min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5) ``` This is correct for the IT66121 register layout where `IT66121_AUD_INFO_DB1= _REG` (0x168) through 0x16C provides 5 data registers, with the checksum at= 0x16D. **Bridge configuration in probe looks correct:** ```c ctx->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HDMI; ctx->bridge.vendor =3D "ITE"; ctx->bridge.product =3D "IT66121"; ``` Adding `DRM_BRIDGE_OP_HDMI` and the vendor/product strings are required for= the HDMI helper framework. **Calling `drm_atomic_helper_connector_hdmi_update_audio_infoframe` outside= the lock in audio_prepare:** ```c out: mutex_unlock(&ctx->lock); if (!ret) ret =3D drm_atomic_helper_connector_hdmi_update_audio_infoframe(con= nector, ¶= ms->cea); return ret; ``` This is correct =E2=80=94 the helper will call back into `hdmi_write_audio_= infoframe` which acquires the lock itself, so calling it outside the outer = lock avoids deadlock. --- Generated by Claude Code Patch Reviewer