From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH 1/2] dt-bindings: display: verisilicon,dc: generalize for DCUltra Lite variant Date: Sat, 16 May 2026 15:45:51 +1000 Message-ID: In-Reply-To: <177849298427.1066847.3849705835228409800.robh@kernel.org> References: <20260511075142.54752-1-a0987203069@gmail.com> <177849298427.1066847.3849705835228409800.robh@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **1. `additionalProperties: true` is wrong =E2=80=94 should be `unevaluated= Properties: false`** ```yaml -additionalProperties: false +additionalProperties: true ``` Setting `additionalProperties: true` in the base schema means **any** prope= rty name is accepted without validation, defeating the purpose of the schem= a. The standard pattern for DT binding inheritance is to use `unevaluatedPr= operties: false` in the sub-schema (which this patch already does in `nuvot= on,ma35d1-dcu.yaml`) and either remove `additionalProperties` from the base= or also use `unevaluatedProperties: false` there. The base schema should N= OT use `additionalProperties: true`. **2. The `compatible` change from `items` to `contains` weakens DC8000 vali= dation** ```yaml - items: - - enum: - - thead,th1520-dc8200 - - const: verisilicon,dc # DC IPs have discoverable ID/revision regis= ters + contains: + enum: + - verisilicon,dc + - thead,th1520-dc8200 + - nuvoton,ma35d1-dcu ``` The original binding required `compatible =3D "thead,th1520-dc8200", "veris= ilicon,dc"` =E2=80=94 a two-element ordered list where the SoC-specific str= ing comes first and the generic fallback second. The new `contains` schema = merely checks that *at least one* of the listed strings appears somewhere i= n the compatible property. This means `compatible =3D "verisilicon,dc"` alo= ne would now pass validation for DC8000 nodes, losing the requirement for t= he SoC-specific string. The DC8000 sub-schema (or a `thead,th1520-dc8200.ya= ml`) should enforce the ordered two-item compatible, while the base schema = can use a looser constraint. **3. The `select` stanza in the base schema lists `nuvoton,ma35d1-dcu`** ```yaml +select: + properties: + compatible: + contains: + enum: + - verisilicon,dc + - thead,th1520-dc8200 + - nuvoton,ma35d1-dcu ``` The base schema's `select` should not need to enumerate every downstream So= C-specific compatible. Each sub-schema has its own `select`. If the intent = is that the base schema should validate *all* DC-derived nodes, this create= s a maintenance burden =E2=80=94 every new variant requires updating two fi= les. Consider whether the base schema needs a `select` at all, or use a pat= tern match. **4. Removing `ports` from required is correct for the base, but no sub-sch= ema enforces it for DC8000** ```yaml - - ports ``` The `ports` property was required for DC8000 but is removed from the base. = This is fine for the DCUltra Lite (which uses `port` singular), but there's= no `thead,th1520-dc8200.yaml` sub-schema that adds `ports` back as require= d. This means existing DC8000 DT nodes would no longer be validated for hav= ing `ports`. A sub-schema for the TH1520 should be created or the existing = `verisilicon,dc.yaml` should use a conditional (`if/then`) to require `port= s` when the compatible matches DC8000. **5. The `nuvoton,ma35d1-dcu.yaml` sub-schema looks reasonable** The sub-schema properly: - References the base schema via `allOf: - $ref` - Defines `port` (singular) as required - Lists 2 clocks with proper names - Marks resets as optional with a description explaining firmware usage - Has a complete example Minor: The example includes `resets =3D <&sys MA35D1_RESET_DISP>` but the `= required` list does not include `resets`. This is fine (optional reset), bu= t it's worth confirming the driver doesn't try to use the reset on this var= iant (which the driver patch confirms =E2=80=94 it doesn't). --- --- Generated by Claude Code Patch Reviewer