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: drm/panel: Add Ilitek ILI9488 controller driver
Date: Thu, 28 May 2026 12:40:41 +1000
Message-ID: <review-patch2-20260527062300.88928-3-igor@reznichenko.net>
In-Reply-To: <20260527062300.88928-3-igor@reznichenko.net>
References: <20260527062300.88928-1-igor@reznichenko.net> <20260527062300.88928-3-igor@reznichenko.net>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Structure and patterns =E2=80=94 good:**
- Uses `devm_drm_panel_alloc()` (modern pattern, matches ILI9805).
- Uses `mipi_dsi_multi_context` for the init sequence (correct modern appro=
ach).
- Uses `drm_connector_helper_get_modes_fixed()` instead of manual `drm_mode=
_duplicate` + `drm_mode_probed_add` (cleaner than ILI9805).
- Panel descriptor pattern allows future ILI9488-based panels with differen=
t timings to be added easily.

**Mode timing =E2=80=94 correct:**
```c
.clock =3D 14400,
.htotal =3D 320 + 60 + 20 + 42,   /* =3D 442 */
.vtotal =3D 480 + 20 + 10 + 33,   /* =3D 543 */
```
14400000 / (442 =C3=97 543) =3D ~59.97 Hz. Sensible for a 60 Hz panel.

**Init sequence =E2=80=94 minor observation on command 0x21:**
```c
mipi_dsi_dcs_write_seq_multi(ctx, 0x21, 0x00);
```
DCS command 0x21 (`set_display_inversion_on`) is defined as a no-parameter =
command per the MIPI DCS spec. The extra `0x00` data byte is likely harmles=
s (the panel probably ignores it), but could be simplified to just the comm=
and byte. This is likely copied from the vendor's reference init code, so i=
t's fine to leave as-is.

**Reset sequence =E2=80=94 correct:**
```c
gpiod_set_value_cansleep(ili->reset, 0);   /* deassert */
usleep_range(1000, 5000);
gpiod_set_value_cansleep(ili->reset, 1);   /* assert reset */
usleep_range(1000, 5000);
gpiod_set_value_cansleep(ili->reset, 0);   /* deassert */
usleep_range(5000, 10000);
```
Standard reset pulse pattern =E2=80=94 deassert, pulse assert, deassert. Us=
es `_cansleep` variant which is safer than the ILI9805's non-cansleep versi=
on (since the GPIO controller might be on an I2C/SPI bus).

**Unprepare =E2=80=94 deactivate error ignored:**
```c
static int ili9488_unprepare(struct drm_panel *panel)
{
	struct ili9488 *ili =3D panel_to_ili9488(panel);
	struct mipi_dsi_device *dsi =3D ili->dsi;
	int ret;

	ili9488_deactivate(ili);
	ret =3D ili9488_power_off(ili);
```
The return value from `ili9488_deactivate()` is silently discarded. This is=
 reasonable =E2=80=94 you still want to power off even if the display-off/s=
leep-in commands fail =E2=80=94 but a `dev_warn` on failure would be helpfu=
l for debugging. Not a blocking issue.

**Probe ordering =E2=80=94 correct:**
```c
ili->panel.prepare_prev_first =3D true;
drm_panel_add(&ili->panel);

ret =3D mipi_dsi_attach(dsi);
if (ret < 0) {
	dev_err_probe(dev, ret, "failed to attach to DSI host\n");
	drm_panel_remove(&ili->panel);
	return ret;
}
```
The `drm_panel_add` before `mipi_dsi_attach` with rollback on failure is th=
e correct ordering.

**MAINTAINERS entry =E2=80=94 correct:**
```
+DRM DRIVER FOR ILITEK ILI9488 PANELS
+M:	Igor Reznichenko <igor@reznichenko.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/display/panel/ilitek,ili9488.yaml
+F:	drivers/gpu/drm/panel/panel-ilitek-ili9488.c
```
Missing a `T:` (git tree) line, but this is common for new drivers that go =
through drm-misc. Not required.

**Overall for patch 2:** Clean driver, no correctness bugs found. Ready for=
 merge with the minor nits above optionally addressed.

---
Generated by Claude Code Patch Reviewer