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/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops Date: Mon, 25 May 2026 21:15:07 +1000 Message-ID: In-Reply-To: <20260520-dw-hdmi-qp-scramb-v6-20-24b74603b782@collabora.com> References: <20260520-dw-hdmi-qp-scramb-v6-0-24b74603b782@collabora.com> <20260520-dw-hdmi-qp-scramb-v6-20-24b74603b782@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 This is a significant refactoring of the interrupt handling code. **Key observation on RK3588 enable_hpd vs old setup_hpd**: The original `se= tup_hpd` wrote both CLR and MSK=3D0 in a single register write. The new `en= able_hpd` does exactly the same =E2=80=94 clear and unmask. Good. **RK3576 `enable_hpd` does NOT write `0xa404`**: The old `setup_hpd` for RK= 3576 wrote to `0xa404` (a magic register). The new code moves this write to= `io_init()`, which is correct since it's a one-time hardware initializatio= n. **Interrupt clear in threaded handler**: The old RK3588 threaded handler ex= plicitly cleared the interrupt before re-enabling it. The new code calls `e= nable_hpd()`, which clears+unmasks atomically. The old RK3576 threaded hand= ler similarly cleared then unmasked. Now both call their respective `enable= _hpd()` which does both. This is functionally equivalent and cleaner. **One subtle issue**: In `dw_hdmi_qp_rk3588_irq` (the threaded handler), th= e old code did: ```c val =3D FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_CLR, 1); // CLR only regmap_write(..., val); // ... val |=3D FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 0); // CLR+MSK regmap_write(..., val); ``` It cleared first, then later wrote CLR+MSK=3D0 in a second write. The new `= enable_hpd` does both in one write. Since these are write-mask-16 registers= (upper 16 bits are write-enable, lower 16 bits are the value), combining C= LR and MSK into a single write is fine and may even be slightly better to a= void a transient state. --- Generated by Claude Code Patch Reviewer