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 logic .mode_set setup to .atomic_enable Date: Sat, 16 May 2026 09:32:44 +1000 Message-ID: In-Reply-To: <20260515090220.809830-3-javierm@redhat.com> References: <20260515090220.809830-1-javierm@redhat.com> <20260515090220.809830-3-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 moves from the deprecated `.mode_set` to `.atomic_enable`. - The new `it66121_set_mode` properly extracts the mode from the atomic sta= te through the connector =E2=86=92 CRTC =E2=86=92 adjusted_mode chain. **Issue =E2=80=94 ordering of `it66121_set_mode` vs `drm_atomic_helper_conn= ector_hdmi_update_infoframes`:** In the resulting `atomic_enable`: ```c drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state); it66121_set_mode(ctx, state); it66121_set_mute(ctx, false); ``` The infoframes are written *before* the hardware is configured (TX mode, cl= ock bank, AFE, etc.). In the original code, the infoframe write happened in= `mode_set` which runs before `atomic_enable`, so the ordering was the same= . But logically, writing infoframes before the TX mode is set seems backwar= ds =E2=80=94 the hardware should probably be configured first, then infofra= mes sent. Looking at the register programming sequence (power off TX clock = =E2=86=92 configure input =E2=86=92 configure AFE =E2=86=92 power on TX clo= ck =E2=86=92 set HDMI mode), infoframes should come *after* the TX path is = set up. Consider swapping the order so `it66121_set_mode` runs before the i= nfoframe update. However, if this matches the tested behavior on real hardw= are, it may be intentional. **Observation =E2=80=94 no error propagation from `it66121_set_mode`:** The function silently swallows errors via `goto unlock`. This isn't new (th= e old `mode_set` did the same), but now that it's called from `atomic_enabl= e`, it might be worth at least a `dev_err()` on failure so issues are visib= le in dmesg. --- Generated by Claude Code Patch Reviewer