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: analogix_dp: Add support for RK3576 Date: Mon, 25 May 2026 20:40:14 +1000 Message-ID: In-Reply-To: <20260521080835.1362416-11-damon.ding@rock-chips.com> References: <20260521080835.1362416-1-damon.ding@rock-chips.com> <20260521080835.1362416-11-damon.ding@rock-chips.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 ```c +static const struct rockchip_dp_chip_data rk3576_edp[] =3D { + { + .chip_type =3D RK3576_EDP, + .reg =3D 0x27dc0000, + }, + { /* sentinel */ } +}; ``` **Question/concern**: The RK3576 chip data does not set `edp_mode`, unlike = RK3588 which has `.edp_mode =3D GRF_REG_FIELD(0x0000, 0, 0)`. This is *func= tionally safe* =E2=80=94 `rockchip_grf_field_write()` checks `!field->valid= ` and returns 0 for unset fields (line 107 in `analogix_dp-rockchip.c`). Ho= wever, the commit message states RK3576 "fully match[es] the proven RK3588 = design." If the hardware really does match RK3588, does RK3576 not have an = eDP mode selection bit in its VO0 GRF? If it does, it should be populated h= ere. If it genuinely doesn't need one, a brief comment or commit message no= te explaining why would be helpful. **Also**: The single-entry array (vs RK3588's two entries) correctly reflec= ts that RK3576 has only one eDP controller. The DT match table addition maintains alphabetical ordering: ```c + {.compatible =3D "rockchip,rk3576-edp", .data =3D &rk3576_edp }, ``` Overall this patch is fine, but the missing `edp_mode` relative to RK3588 d= eserves confirmation from the author. --- Generated by Claude Code Patch Reviewer