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: Wed, 11 Mar 2026 13:27:11 +1000 Message-ID: In-Reply-To: <20260310105307.309765-5-damon.ding@rock-chips.com> References: <20260310105307.309765-1-damon.ding@rock-chips.com> <20260310105307.309765-5-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 **hclk lifetime concern.** The `hclk` is acquired with `devm_clk_get_option= al_enabled()`: ```c + dp->hclk =3D devm_clk_get_optional_enabled(dev, "hclk"); ``` This means the clock is enabled at probe time and stays enabled for the ent= ire lifetime of the device (until driver removal). The commit message says = "which must be enabled during probing for RK3576 eDP controller," but keepi= ng it permanently enabled may waste power. Consider whether it should inste= ad be enabled/disabled alongside `pclk` in `rockchip_dp_poweron()`/`rockchi= p_dp_powerdown()`. If it truly needs to be always-on, a comment explaining = why would be helpful. That said, `devm_clk_get_optional_enabled` is a valid pattern used elsewher= e in the kernel, and if the clock must be on whenever the device is bound (= e.g., for register access), this is correct. **Missing `edp_mode` GRF field.** The RK3576 chip_data has no `edp_mode` fi= eld set: ```c +static const struct rockchip_dp_chip_data rk3576_edp[] =3D { + { + .chip_type =3D RK3576_EDP, + .reg =3D 0x27dc0000, + }, + { /* sentinel */ } +}; ``` Compare with RK3588 which has: ```c .edp_mode =3D GRF_REG_FIELD(0x0000, 0, 0), ``` The `edp_mode` field is used in `rockchip_dp_poweron()` and `rockchip_dp_po= werdown()` via `rockchip_grf_field_write()`. Since the field's `.valid` wil= l be `false` (zero-initialized), `rockchip_grf_field_write()` will return 0= early =E2=80=94 so this is functionally safe. But the commit message shoul= d clarify whether RK3576 genuinely has no GRF edp_mode bit, or if this is a= n oversight. **No `lcdc_sel` either** =E2=80=94 this is used in `rockchip_dp_drm_encoder= _enable()`. Again, `rockchip_grf_field_write()` will silently no-op due to = `.valid =3D false`. This is consistent with RK3588 which also has no `lcdc_= sel`, so this appears intentional. No blocking issues, but the hclk lifecycle and missing `edp_mode` documenta= tion could use clarification. --- Generated by Claude Code Patch Reviewer