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, adding the shared common dtsi. Several items to note:

> +#include "dt-bindings/gpio/gpio.h"
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/input/input.h>

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 <dt-bindings/gpio/gpio.h>` 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