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: Move .mode_set logic to .atomic_enable Date: Mon, 25 May 2026 17:50:13 +1000 Message-ID: In-Reply-To: <20260523-it66121-fix-dvi-mode-v5-v5-2-33b4468162f9@redhat.com> References: <20260523-it66121-fix-dvi-mode-v5-v5-0-33b4468162f9@redhat.com> <20260523-it66121-fix-dvi-mode-v5-v5-2-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 **Clean refactor.** The mode_set logic is correctly extracted into `it66121= _set_mode()` and called from `atomic_enable`. The `ctx->connector` field is= removed in favor of a local variable, which is the right approach. **Mode lookup through atomic state is correct:** ```c conn_state =3D drm_atomic_get_new_connector_state(state, connector); crtc =3D conn_state->crtc; crtc_state =3D drm_atomic_get_new_crtc_state(state, crtc); mode =3D &crtc_state->adjusted_mode; ``` Each step correctly chains through the state to get the adjusted mode. The = `WARN_ON` checks for NULL at each stage are defensive and appropriate. **Enable ordering:** ```c drm_atomic_helper_connector_hdmi_update_infoframes(connector, state); it66121_set_mode(ctx, connector, state); it66121_set_mute(ctx, false); ``` Infoframes are written before the TX mode is configured. This works because= the infoframe data is just stored in hardware registers =E2=80=94 actual t= ransmission starts when the TX mode is enabled. However, the more natural o= rder would be to configure the TX mode first, then enable infoframes. If th= ere's a reason the IT66121 requires this order, a comment would help. **The `connector =3D NULL` cleanup in `bridge_disable` is correctly removed= **, since `connector` is now a local variable. --- Generated by Claude Code Patch Reviewer