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: it66121: Select HDMI or DVI mode based on sink type Date: Sat, 16 May 2026 16:11:55 +1000 Message-ID: In-Reply-To: <20260510191459.90769-4-javierm@redhat.com> References: <20260510191459.90769-1-javierm@redhat.com> <20260510191459.90769-4-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 **Status: One concern about state access in mode_set.** The core logic is correct: checking `conn_state->connector->display_info.is= _hdmi` in `atomic_check` and using that to conditionally skip AVI infoframe= s and write 0 (DVI mode) vs `IT66121_HDMI_MODE_HDMI` to the mode register. The defensive defaulting is good: ```c /* Default to HDMI to preserve legacy behavior. */ state->sink_is_hdmi =3D true; if (conn_state && conn_state->connector) state->sink_is_hdmi =3D conn_state->connector->display_info.is_hdmi; ``` **Concern: State access in `mode_set` via `bridge->base.state`.** ```c struct drm_bridge_state *bridge_state =3D drm_priv_to_bridge_state(bridge->base.state); struct it66121_bridge_state *state =3D to_it66121_bridge_state(bridge_state); ``` The `mode_set` callback has the old-style signature without a `drm_atomic_s= tate *` parameter, so accessing the bridge state through `bridge->base.stat= e` is the only option. This works because `mode_set` is called after the st= ate swap during commit. However, this couples the driver to commit ordering= assumptions. **Suggestion:** Consider converting from the legacy `mode_set` callback to = `atomic_enable` where the `struct drm_atomic_commit *state` parameter is av= ailable. The enable callback already exists =E2=80=94 the mode_set logic co= uld be folded into it, and you'd get the state cleanly via `drm_atomic_get_= new_bridge_state()`. This would eliminate the fragile `bridge->base.state` = access entirely. If that's too much churn for this fix, the current approac= h works but should have a brief comment noting why `bridge->base.state` is = safe here (called post-swap during commit). **Minor nit:** The AVI packet disable path writes 0 to `IT66121_AVI_INFO_PK= T_REG`, which should explicitly clear the AVI info that was previously writ= ten. Depending on hardware behavior, you may also want to zero out the AVI = infoframe data registers (`IT66121_AVIINFO_DB1_REG`) when switching to DVI = mode, to avoid any stale data being accidentally transmitted if the packet = enable bit is ever re-asserted without going through `mode_set` again. This= is a hardening suggestion, not a correctness issue for the current flow. --- Generated by Claude Code Patch Reviewer