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: Sat, 16 May 2026 09:32:44 +1000 Message-ID: In-Reply-To: <20260515090220.809830-2-javierm@redhat.com> References: <20260515090220.809830-1-javierm@redhat.com> <20260515090220.809830-2-javierm@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 **Good:** - Correctly adds `DRM_BRIDGE_OP_HDMI` to bridge ops and provides the requir= ed `vendor`/`product` fields. - The infoframe write/clear callbacks properly replicate the existing regis= ter programming. - Adding `DRM_DISPLAY_HDMI_STATE_HELPER` and `DRM_DISPLAY_HELPER` to Kconfi= g is correct. - The NULL check added for `ctx->connector` in `it66121_bridge_enable` is a= good defensive addition. **Issue =E2=80=94 `hdmi_tmds_char_rate_valid` ignores `tmds_rate`:** The new callback signature receives a `tmds_rate` parameter but the impleme= ntation only validates `mode->clock`: ```c static enum drm_mode_status it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge, const struct drm_display_mode *mode, unsigned long long tmds_rate) { ... max_clock =3D (ctx->bus_width =3D=3D 12) ? 74250 : 148500; if (mode->clock > max_clock) return MODE_CLOCK_HIGH; if (mode->clock < 25000) return MODE_CLOCK_LOW; return MODE_OK; } ``` The purpose of this callback is specifically to validate the TMDS character= rate, not the pixel clock. The `tmds_rate` value may differ from the pixel= clock (e.g., deep color modes multiply the rate). Looking at the reference= `it6263` driver, it validates *both* `mode->clock` and `tmds_rate`. The `m= ax_clock` values here (74250/148500 kHz) are pixel clock limits, while `tmd= s_rate` is in Hz. Consider converting the max clock limits to a TMDS rate c= heck against `tmds_rate` as well, or at minimum validating `tmds_rate` agai= nst the maximum TMDS rate the hardware supports. As-is, this could allow mo= des that exceed the hardware's TMDS capability when deep color is in use. N= ot a regression since the old `mode_valid` had the same limitation, but thi= s is the right time to fix it. **Minor =E2=80=94 no-op HDMI infoframe callbacks:** ```c static int it66121_bridge_hdmi_clear_hdmi_infoframe(struct drm_bridge *brid= ge) { return 0; } static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *brid= ge, const u8 *buffer, size_t len) { return 0; } ``` These are stubs that silently succeed. If the hardware genuinely can't send= Vendor Specific InfoFrames, this is fine, but a brief comment explaining w= hy would help future readers understand this is intentional rather than a T= ODO. **Minor =E2=80=94 locking in `it66121_bridge_hdmi_write_avi_infoframe`:** The write callback takes `ctx->lock` internally. This is fine for now, but = worth noting that `drm_atomic_helper_connector_hdmi_update_infoframes` (cal= led from `atomic_enable`) will call both clear and write =E2=80=94 the clea= r callback does *not* take the lock. This asymmetry is harmless since the c= lear only does a single `regmap_write` (which has its own lock), but it's w= orth being aware of. --- Generated by Claude Code Patch Reviewer