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 driver for Novatek NT35532
Date: Mon, 09 Mar 2026 07:46:26 +1000
Message-ID: <review-patch2-20260308-rimob-new-features-v1-2-aa2c330572c0@protonmail.com>
In-Reply-To: <20260308-rimob-new-features-v1-2-aa2c330572c0@protonmail.com>
References: <20260308-rimob-new-features-v1-0-aa2c330572c0@protonmail.com> <20260308-rimob-new-features-v1-2-aa2c330572c0@protonmail.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

**Init sequence uses generic writes for standard DCS commands.** At the end=
 of `nt35532_on()`:

```c
mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x11, 0x00);
mipi_dsi_msleep(&dsi_ctx, 120);
mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x29, 0x00);
mipi_dsi_msleep(&dsi_ctx, 50);
```

`0x11` is `MIPI_DCS_EXIT_SLEEP_MODE` and `0x29` is `MIPI_DCS_SET_DISPLAY_ON=
`. These should use the proper DCS helpers, consistent with how `nt35532_of=
f()` already uses them:

```c
mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
mipi_dsi_msleep(&dsi_ctx, 120);
mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
mipi_dsi_msleep(&dsi_ctx, 50);
```

This is a common artifact of auto-generated drivers, but reviewers will ask=
 for it to be fixed.

**`#include <drm/drm_probe_helper.h>`** is included for `drm_connector_help=
er_get_modes_fixed()`. This is correct, but the corresponding `select DRM_K=
MS_HELPER` in Kconfig may be unnecessary =E2=80=94 check whether `DRM_MIPI_=
DSI` already selects it transitively. Other recent panel drivers don't alwa=
ys add this select.

**Structure and APIs are good:**
- Uses `devm_drm_panel_alloc()` (the modern allocation API)=20
- Uses `devm_regulator_bulk_get_const()` with a static supply array
- Uses `mipi_dsi_multi_context` for the init sequence
- Uses `drm_connector_helper_get_modes_fixed()` for mode reporting
- `prepare_prev_first =3D true` is set correctly

**Reset sequence looks correct** given `GPIO_ACTIVE_LOW` in the DTS:

```c
gpiod_set_value_cansleep(ctx->reset_gpio, 0);  // de-assert
usleep_range(10000, 11000);
gpiod_set_value_cansleep(ctx->reset_gpio, 1);  // assert (reset pulse)
usleep_range(5000, 6000);
gpiod_set_value_cansleep(ctx->reset_gpio, 0);  // de-assert
usleep_range(10000, 11000);
```

**Minor:** The `MODULE_AUTHOR()` macro is missing. Not strictly required bu=
t conventional.

---
Generated by Claude Code Patch Reviewer