From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: arm64: dts: qcom: sdm670-google: add common device tree include Date: Sat, 16 May 2026 11:37:15 +1000 Message-ID: In-Reply-To: <20260513172549.1345-2-mailingradian@gmail.com> References: <20260513172549.1345-1-mailingradian@gmail.com> <20260513172549.1345-2-mailingradian@gmail.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 **Overall:** Good refactoring. The split between common and device-specific= properties is logical and complete. **Positive changes:** - The switch from inline `reg` with a computed size to `memory-region =3D <= &fb_mem>` in the framebuffer node is a good improvement. It avoids duplicat= ing the address and references the already-defined reserved memory region, = which is the preferred DT pattern: ``` - framebuffer@9c000000 { + framebuffer: framebuffer@9c000000 { compatible =3D "simple-framebuffer"; - reg =3D <0 0x9c000000 0 (1080 * 2220 * 4)>; + memory-region =3D <&fb_mem>; width =3D <1080>; - height =3D <2220>; + /* height is in specific device trees */ ``` - Adding labels (`framebuffer:`, `fb_mem:`, `rmi4_f12:`, `panel:`) to nodes= that need per-device overrides is the correct mechanism. - The SPDX license change from `GPL-2.0` to `GPL-2.0-only` in the rewritten= sargo.dts is appropriate =E2=80=94 `GPL-2.0` is deprecated as ambiguous. T= he commit message explains the rationale well. **Minor observations (non-blocking):** 1. The comment `/* compatible is provided in specific device trees */` and = `/* height is in specific device trees */` are helpful for readability, sin= ce a panel node without a compatible or a framebuffer without height would = otherwise look like a bug. These are good comments. 2. The `width =3D <1080>` and `stride =3D <(1080 * 4)>` are hardcoded in th= e common dtsi. Both current variants (sargo and bonito-tianma) share 1080px= width, so this is fine. If a future variant has a different width, it woul= d need to override these too, but that's a bridge to cross later. 3. The rewritten `sdm670-google-sargo.dts` is pleasantly minimal =E2=80=94 = just the include and five short override blocks. This validates that the co= mmon/specific split was done well. --- Generated by Claude Code Patch Reviewer