From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/rockchip: vop2: Delay old_{layer|port}_sel updates in setup_layer_mixer()
Date: Tue, 05 May 2026 08:08:45 +1000
Message-ID: <review-patch3-20260504-vop2-layer-cfg-tmout-v1-3-730226a7331e@collabora.com>
In-Reply-To: <20260504-vop2-layer-cfg-tmout-v1-3-730226a7331e@collabora.com>
References: <20260504-vop2-layer-cfg-tmout-v1-0-730226a7331e@collabora.com> <20260504-vop2-layer-cfg-tmout-v1-3-730226a7331e@collabora.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

**Verdict: Clean refactor, correct.**

This removes the `old_layer_sel` and `old_port_sel` locals that were workin=
g around the premature writes to `vop2->old_layer_sel` / `vop2->old_port_se=
l`. After patches 1 and 2 fixed the ordering, the struct members now hold t=
he correct (previous) values throughout the comparison/wait section, so the=
 locals are no longer needed.

Key change: `vop2->old_layer_sel =3D layer_sel` and `vop2->old_port_sel =3D=
 port_sel` are now deferred to just before their corresponding register wri=
tes:

```c
if (port_sel !=3D vop2->old_port_sel) {
    vop2->old_port_sel =3D port_sel;
    vop2_writel(vop2, RK3568_OVL_PORT_SEL, port_sel);
    ...
}

vop2->old_layer_sel =3D layer_sel;
vop2_writel(vop2, RK3568_OVL_LAYER_SEL, layer_sel);
```

This is correct and cleaner. Note that `old_port_sel` is updated inside the=
 `if` (only when changed), while `old_layer_sel` is updated unconditionally=
 =E2=80=94 this matches the behavior since `layer_sel` is always written to=
 the register. This is fine since the shadow register write is unconditiona=
l too.

No issues.

---

---
Generated by Claude Code Patch Reviewer