From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: dt-bindings: display: msm: Fix reg ranges and clocks on Glymur
Date: Wed, 04 Mar 2026 07:43:56 +1000
Message-ID: <review-patch1-20260303-glymur-fix-dp-bindings-reg-clocks-v4-1-1ebd9c7c2cee@oss.qualcomm.com>
In-Reply-To: <20260303-glymur-fix-dp-bindings-reg-clocks-v4-1-1ebd9c7c2cee@oss.qualcomm.com>
References: <20260303-glymur-fix-dp-bindings-reg-clocks-v4-1-1ebd9c7c2cee@oss.qualcomm.com> <20260303-glymur-fix-dp-bindings-reg-clocks-v4-1-1ebd9c7c2cee@oss.qualcomm.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

**Commit message**: Clear and well-written. Good explanation of the hardwar=
e topology (four controllers, three with 2 streams, one SST). The `Fixes:` =
and `Cc: stable` tags are appropriate.

**dp-controller.yaml changes**:

The removal of `qcom,glymur-dp` from the existing 2-stream group and additi=
on of a dedicated clause is the right approach. The new constraints match t=
he hardware:
- `reg: minItems: 9, maxItems: 9` =E2=80=94 correct for all 9 register bloc=
ks (ahb, aux, link, p0=E2=80=93p3, mst2link, mst3link), consistent with `qc=
om,sa8775p-dp` which has a similar 4-port topology.
- `clocks: minItems: 5, maxItems: 6` =E2=80=94 correct for allowing SST (5 =
clocks) or 2-stream MST (6 clocks, with `stream_1_pixel`).

**Issue =E2=80=94 `clocks-names` typo**:

```yaml
+        clocks-names:
+          minItems: 5
+          maxItems: 6
```

The property is defined as `clock-names` (line 101 of the file) and listed =
in the `required:` section as `clock-names` (line 184). The `clocks-names` =
used here (with the extra 's') doesn't match any defined property, so this =
constraint is silently ignored by the schema validator. This is a **pre-exi=
sting bug** =E2=80=94 the same typo appears in all the other `allOf` clause=
s in this file =E2=80=94 but since you're adding new code, this would be a =
good opportunity to use the correct name. At minimum it should be `clock-na=
mes:` in the new clause. Ideally a follow-up (or inclusion in this patch) w=
ould fix the other clauses too.

**qcom,glymur-mdss.yaml changes**:

The example update looks correct:

```yaml
-            displayport-controller@ae90000 {
+            displayport-controller@af54000 {
                 compatible =3D "qcom,glymur-dp";
-                reg =3D <0xae90000 0x200>,
-                      <0xae90200 0x200>,
-                      <0xae90400 0x600>,
-                      <0xae91000 0x400>,
-                      <0xae91400 0x400>;
+                reg =3D <0xaf54000 0x200>,
+                      <0xaf54200 0x200>,
+                      <0xaf55000 0xc00>,
+                      <0xaf56000 0x400>,
+                      <0xaf57000 0x400>,
+                      <0xaf58000 0x400>,
+                      <0xaf59000 0x400>,
+                      <0xaf5a000 0x600>,
+                      <0xaf5b000 0x600>;
```

- Addresses updated from made-up values to actual first-controller addresse=
s.
- Link register block size corrected from `0x600` to `0xc00` (matching the =
sc7180 example and presumably the actual hardware).
- Four new register blocks added (p2, p3, mst2link, mst3link) to reach the =
required 9 entries.
- The `@` unit address in the node name matches the first reg entry. Good.

**Summary**: The fix is correct. The only issue is the `clocks-names` =E2=
=86=92 `clock-names` typo that should be corrected before merging.

---
Generated by Claude Code Patch Reviewer