From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
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: Wed, 04 Mar 2026 06:45:50 +1000
Message-ID: <review-patch5-20260303203017.511-6-bavishimithil@gmail.com>
In-Reply-To: <20260303203017.511-6-bavishimithil@gmail.com>
References: <20260303203017.511-1-bavishimithil@gmail.com> <20260303203017.511-6-bavishimithil@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest patch (744 lines) and has several issues:

**Bug - magnetometer interrupts property:**
```
magnetometer@2e {
    compatible =3D "yamaha,yas530";
    ...
    interrupts =3D <&gpio6 10 IRQ_TYPE_EDGE_RISING>;
};
```
The `interrupts` property takes plain interrupt specifiers, not phandle ref=
erences. This should be either:
```
interrupt-parent =3D <&gpio6>;
interrupts =3D <10 IRQ_TYPE_EDGE_RISING>;
```
or:
```
interrupts-extended =3D <&gpio6 10 IRQ_TYPE_EDGE_RISING>;
```

**Issue - reserved-memory size hardcoded for 7" variant only:**
```
continuous_splash: framebuffer@bef00000{
    reg =3D <0xbef00000 (1024 * 600 * 4)>;
    no-map;
};
```
The size `1024 * 600 * 4` corresponds to the 7" panel (1024x600). The 10" p=
anel is 1280x800, which requires `1280 * 800 * 4` bytes. This is in the **c=
ommon** dtsi but the size is variant-specific. Either this should be moved =
to the per-variant dts files, or the size should be large enough for the la=
rgest panel, or the 10" variant should override it.

**Style - missing space before brace:**
```
continuous_splash: framebuffer@bef00000{
```
Should be `framebuffer@bef00000 {` (space before `{`).

**Typo in node name:**
```
ledir_pins: pimux-ledir-pins {
```
Should be `pinmux-ledir-pins` (missing `n` in `pinmux`).

**Inconsistent include syntax:**
```
#include "dt-bindings/gpio/gpio.h"
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/input/input.h>
```
The first include uses double quotes while the others use angle brackets. S=
hould be consistent =E2=80=94 prefer angle brackets (`<...>`) as used elsew=
here in the kernel DTS files.

**Inconsistent hex prefix case in OMAP4_IOPAD:**
```
OMAP4_IOPAD(0X0D8, PIN_OUTPUT | PIN_OFF_OUTPUT_LOW | MUX_MODE3)
OMAP4_IOPAD(0X0D6, PIN_OUTPUT | PIN_OFF_OUTPUT_LOW | MUX_MODE1)
OMAP4_IOPAD(0X136, PIN_OUTPUT | MUX_MODE3)
```
These use uppercase `0X` prefix while all other entries use lowercase `0x`.=
 Should be consistent (lowercase).

**Questionable interrupt flags on accelerometer:**
```
accelerometer@18 {
    ...
    interrupts =3D <25 (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING)>,
        <26 (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING)>;
};
```
Combining `IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_EDGE_RISING` (value 5) is unusual=
. Typically an interrupt is either level-triggered or edge-triggered, not b=
oth. Please verify this is correct for the BMA254 accelerometer.

**Unnecessary `#address-cells` in chosen:**
```
chosen {
    stdout-path =3D &uart3;
    #address-cells =3D <1>;
};
```
The `#address-cells` in the `chosen` node is unnecessary if there are no ch=
ild nodes. Can be dropped.

**Minor - `ti,non-removable` in mmc2:**
```
&mmc2 {
    ...
    ti,non-removable;
    bus-width =3D <8>;
};
```
This is consistent with other OMAP DTS files that use the same property, so=
 it's fine. (Though some newer DTS files use standard `non-removable`.)

---
Generated by Claude Code Patch Reviewer