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: Tue, 10 Mar 2026 11:42:23 +1000 Message-ID: In-Reply-To: <20260310002606.16413-5-mailingradian@gmail.com> References: <20260310002606.16413-1-mailingradian@gmail.com> <20260310002606.16413-5-mailingradian@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Good refactoring, one concern.** The factoring approach is correct. The common dtsi removes device-specific properties (`model`, `compatible`, `charge-full-design-microamp-hours`, `height`, `touchscreen-x-mm`, `touchscreen-y-mm`, and panel `compatible`) and adds labels (`framebuffer:`, `fb_mem:`, `panel:`, `rmi4_f12:`) for overriding in device-specific files. 1. **Framebuffer `height` comment**: The common dtsi replaces the `height` property with a comment: ``` + /* height is in specific device trees */ ``` This is fine for documentation, but it means the common framebuffer node lacks a required `height` property for `simple-framebuffer`. If used standalone (which it isn't, as a `.dtsi`), this would be invalid. This is acceptable since the `.dtsi` is always included by a `.dts` that provides `height`, but the DT compiler may not warn if a `.dts` forgets to set it. 2. **Memory-region change**: The framebuffer changes from `reg` to `memory-region`: ``` - reg = <0 0x9c000000 0 (1080 * 2220 * 4)>; + memory-region = <&fb_mem>; ``` This is a good change since it references the reserved memory region properly, but it changes the binding usage for the existing sargo board. The `fb_mem` region is `0x2400000` bytes (about 37.7 MB) which is larger than `1080 * 2220 * 4 = 9,590,400` bytes. This should still work since `simple-framebuffer` only reads the needed portion, but the semantics are slightly different. 3. **License change**: The SPDX change from `GPL-2.0` to `GPL-2.0-only` is well-explained in the commit message. --- Generated by Claude Code Patch Reviewer