From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: ARM: dts: ti: omap: espresso-common: Add common device tree for Samsung Galaxy Tab 2 series Date: Tue, 24 Feb 2026 10:31:42 +1000 Message-ID: In-Reply-To: <20260223055113.1288-6-bavishimithil@gmail.com> References: <20260223055113.1288-1-bavishimithil@gmail.com> <20260223055113.1288-6-bavishimithil@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 This is the largest patch, adding the shared common dtsi. Several items to note: > +#include "dt-bindings/gpio/gpio.h" > +#include > +#include The first include uses quotes while the other two use angle brackets. Every other file in the OMAP DT directory uses angle brackets for dt-bindings includes. This should be `#include ` for consistency. > + continuous_splash: framebuffer@bef00000{ > + reg = <0xbef00000 (1024 * 600 * 4)>; > + no-map; > + }; Two issues here. First, there's a missing space before `{`. Second, and more substantively, this reserves `1024 * 600 * 4` bytes (about 2.4 MB), which is sized for the 7" panel. The espresso10 variant uses a 1280x800 panel, which would need `1280 * 800 * 4` bytes (about 3.9 MB). If the bootloader renders a splash screen at the native 10" resolution, this reservation is too small and the framebuffer would overlap with non-reserved memory. Should this size be overridden in the espresso10 DTS, or should the common dtsi use the larger of the two sizes? > + chosen { > + stdout-path = &uart3; > + #address-cells = <1>; > + }; The `#address-cells` property in `chosen` without a corresponding `#size-cells` seems like a leftover. The v4 changelog mentions "Removed address/size-cells in chosen" but `#address-cells` is still present. Does the `chosen` node actually need this? > + magnetometer@2e { > + compatible = "yamaha,yas530"; > + reg = <0x2e>; > + vdd-supply = <&ldo4>; > + iovdd-supply = <&ldo5>; > + reset-gpios = <&gpio5 29 GPIO_ACTIVE_LOW>; > + interrupts = <&gpio6 10 IRQ_TYPE_EDGE_RISING>; > + }; The `interrupts` property does not accept phandle references -- it only takes interrupt specifier cells relative to the node's interrupt parent. This should be either `interrupts-extended = <&gpio6 10 IRQ_TYPE_EDGE_RISING>;` or split into `interrupt-parent = <&gpio6>;` plus `interrupts = <10 IRQ_TYPE_EDGE_RISING>;`. As written, dtc should flag this. > + ledir_pins: pimux-ledir-pins { Typo: `pimux` should be `pinmux`. > + lvds_pins: pinmux-lvds-pins { > + pinctrl-single,pins = < > + OMAP4_IOPAD(0X136, PIN_OUTPUT | MUX_MODE3) A few OMAP4_IOPAD calls use uppercase `0X` for the hex prefix (also `0X0D8`, `0X0D6` in other pin groups). The rest of the file and the tree generally use lowercase `0x`. Minor inconsistency. > + pinctrl-names = "default"; > + pinctrl-0 = < > + &twl6030_pins > + &twl6030_wkup_pins > + >; These pinctrl properties on the `twl` node are redundant since `twl6030_omap4.dtsi` (included a few lines later) sets the same properties on `&twl`. You could drop these from the inline node definition and rely on the include. --- Generated by Claude Code Patch Reviewer