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 "color format" DRM property Date: Mon, 25 May 2026 18:48:58 +1000 Message-ID: In-Reply-To: <20260522-color-format-v15-20-21fb136c9df2@collabora.com> References: <20260522-color-format-v15-0-21fb136c9df2@collabora.com> <20260522-color-format-v15-20-21fb136c9df2@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 The patch replaces per-SoC color format defines with unified `RK_COLOR_FMT_= *` values. The old `RK3576_RGB =3D 0x9` is replaced with `RK_COLOR_FMT_RGB = =3D 0x0`. The commit message explains this was a typo in previously unused = code. I verified in the tree that these defines were indeed unused before t= his series, so the correction is safe. The `dw_hdmi_qp_rockchip_get_vop_format` function uses `__free(drm_bridge_p= ut)` for automatic cleanup, which is good. One concern: the `dw_hdmi_qp_rk3576_enc_init` and `dw_hdmi_qp_rk3588_enc_in= it` functions use: ```c if (likely(color >=3D 0)) val |=3D FIELD_PREP_WM16(RK3576_COLOR_FORMAT_MASK, color); ``` The `likely()` hint suggests failure is unexpected, but if `bus_fmt_to_reg`= returns -EINVAL, the color format register is simply not set, leaving what= ever was there before. This silently falls back rather than erroring. Consi= der whether a `drm_warn` would be appropriate when `color < 0`, since it wo= uld indicate a bug in the format selection logic. Note the RK3576 `COLOR_FORMAT_MASK` is `GENMASK(7,4)` while RK3588's is `GE= NMASK(3,0)` =E2=80=94 these are in different bit positions within their res= pective GRF registers, but the actual color format values (0=3DRGB, 1=3D422= , 2=3D444, 3=3D420) are the same for both SoCs, which is why unification wo= rks. The `MEDIA_BUS_FMT_UYVY10_1X20` case is present in the switch but not `MEDI= A_BUS_FMT_UYVY12_1X24` =E2=80=94 12-bit YUV422 is not handled. This matches= the `max_bpc =3D 10` in the plat_data, so it's consistent. --- Generated by Claude Code Patch Reviewer