Claude review: ARM: dts: ti: omap: espresso-common: Add common device tree for Samsung Galaxy Tab 2 series

[Claude Code Review Bot <claude-review@example.com>]

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