From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dt-bindings: display: rockchip: analogix-dp: Add support for RK3576 Date: Wed, 11 Mar 2026 13:27:10 +1000 Message-ID: In-Reply-To: <20260310105307.309765-2-damon.ding@rock-chips.com> References: <20260310105307.309765-1-damon.ding@rock-chips.com> <20260310105307.309765-2-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 **Clock-names binding change is problematic.** The patch changes the third = clock-name from a fixed `const: grf` to an `enum: [grf, hclk]`: ```yaml - - const: grf + - enum: + - grf + - hclk ``` This is a semantic change that affects **all** existing compatibles, not ju= st the new RK3576 one. The RK3288 uses a `grf` clock, and now the schema wo= uld also allow `hclk` for RK3288, which doesn't make sense. This should ins= tead be handled with a per-compatible conditional (in the `allOf` section),= similar to how the reset constraints are already conditional. For example,= add an `if`/`then` block that constrains clock-names for RK3576 to `[dp, p= clk, hclk]` and keeps the existing behavior for other compatibles. Alternatively, since RK3576 uses `hclk` and RK3288 uses `grf` as the third = clock, consider restructuring the items list to use `oneOf` or per-compatib= le `clock-names` constraints. **The cover letter says "The newly added clock 'hclk' is the video datapath= clock"** =E2=80=94 this description should be in the binding document itse= lf (in the items description or the property description), not just the com= mit message. --- Generated by Claude Code Patch Reviewer