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: Mon, 23 Feb 2026 06:16:28 +1000 Message-ID: In-Reply-To: <20260219210408.5451-6-bavishimithil@gmail.com> References: <20260219210408.5451-1-bavishimithil@gmail.com> <20260219210408.5451-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 and has several issues. > + 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 here contains a phandle (`&gpio6`) which is not valid for this property. The `interrupts` property expects only the interrupt specifier (numeric cells), with the interrupt controller specified separately via `interrupt-parent`. To include an inline phandle reference, this should be `interrupts-extended = <&gpio6 10 IRQ_TYPE_EDGE_RISING>;`. As written, the DT compiler may not flag this, but the kernel will misparse the interrupt specifier. > + continuous_splash: framebuffer@bef00000{ > + reg = <0xbef00000 (1024 * 600 * 4)>; > + no-map; > + }; Two issues here. First, missing space before `{` (`framebuffer@bef00000{` should be `framebuffer@bef00000 {`). Second, this is in the common dtsi but the framebuffer size `(1024 * 600 * 4)` is specific to the 7" variant's resolution. The 10" variant uses 1280x800, which would need `(1280 * 800 * 4)` -- nearly double the size. If the bootloader sets up a splash screen on the 10" variant, this reservation is too small and memory could be corrupted. This should either be overridden in the variant-specific dts files, or use the larger size of the two variants, or not be in the common dtsi at all. Additionally, the `continuous_splash` label is never referenced anywhere. > + ledir_pins: pimux-ledir-pins { Typo: `pimux-ledir-pins` should be `pinmux-ledir-pins`. > + chosen { > + stdout-path = &uart3; > + #address-cells = <1>; > + }; Having `#address-cells` in `chosen` without `#size-cells` is unusual. Is this intentionally here for something specific (e.g., a bootloader convention), or is it leftover? > +&omap4_pmx_wkup { > + gpio_keys: gpio-keys-pins { > + pinctrl-single,pins = < > + OMAP4_IOPAD(0x046, WAKEUP_EN | PIN_INPUT | MUX_MODE3) > + /* sim_cd.gpio_wk3 - EXT_WAKEUP */ > + OMAP4_IOPAD(0x056, WAKEUP_EN | PIN_INPUT | MUX_MODE3) > + /* fref_clk3_req.gpio_wk30 - VOL_UP */ > + OMAP4_IOPAD(0x05C, WAKEUP_EN | PIN_INPUT | MUX_MODE3) > + /* fref_clk4_out.gpio_wk8 - VOL_DN */ > + >; > + }; > + > + prox_irq: prox-irq-pins { > + pinctrl-single,pins = < > + OMAP4_IOPAD(0x042, WAKEUP_EN | PIN_INPUT_PULLUP | MUX_MODE3) > + /* sim_clk.gpio_wk1 - PS_VOUT */ > + >; > + }; The v6 changelog states "Remove references to WAKEUP_EN (drivers dont support interrupts-extended)" but WAKEUP_EN is still present in these pin definitions. Andreas Kemnade also pointed out in his review that WAKEUP_EN is not needed for the gpio1 controller since it's always active per the OMAP4 TRM Table 3-327. The commit message could also use improvement: "Let's create a common tree for all the variants first, later we can device specific trees based on their screen sizes" appears to be missing the word "add" or "create" after "we can." --- Generated by Claude Code Patch Reviewer