Claude review: drm/panel: Add panel driver for ChipWealth CH13726A based panels

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

Patch Review

Several issues need attention:

**1. Unnecessary `select DRM_DISPLAY_DP_HELPER` and `select DRM_DISPLAY_HELPER` in Kconfig (important)**

```c
+	select DRM_DISPLAY_DP_HELPER
+	select DRM_DISPLAY_HELPER
```

This is a pure MIPI-DSI panel driver — it has no DisplayPort or DSC functionality. These selects are unnecessary and pull in code the driver doesn't use. Other pure MIPI-DSI panel drivers (e.g., `panel-boe-th101mb31ig002-28a`, `panel-himax-hx8394`) do not select these. Remove both lines.

**2. Redundant `prepared` flag tracking (important)**

```c
+struct ch13726a_panel {
+	...
+	bool prepared;
+};
```

And in `ch13726a_prepare()`:

```c
+	if (ctx->prepared)
+		return 0;
+	...
+	ctx->prepared = true;
```

And in `ch13726a_unprepare()`:

```c
+	if (!ctx->prepared)
+		return 0;
+	...
+	ctx->prepared = false;
```

The DRM panel core already tracks `panel->prepared` and guards against double-prepare/unprepare in `drm_panel_prepare()`/`drm_panel_unprepare()` (see `drm_panel.c` lines 122-134 and 177-199). This driver-level tracking is redundant and should be removed. No current in-tree panel driver uses a private `prepared` flag anymore.

**3. `thor_bottom_desc` should be `const` (minor)**

```c
+static struct ch13726a_desc thor_bottom_desc = {
```

This should be `static const struct ch13726a_desc`. It's immutable data. The struct member should also be `const`:

```c
-	struct ch13726a_desc *desc;
+	const struct ch13726a_desc *desc;
```

This would also eliminate the need for the cast in probe:

```c
+	ctx->desc = (struct ch13726a_desc *)of_device_get_match_data(dev);
```

With `const struct ch13726a_desc *desc`, this becomes simply:

```c
	ctx->desc = of_device_get_match_data(dev);
```

**4. `uint8_t` loop variable (minor, style)**

```c
+	for (uint8_t i = 0; i < ctx->desc->num_modes; i++) {
```

Kernel code uses `u8` rather than `uint8_t`, but for a loop index iterating over a small count, a plain `unsigned int` or `int` is more idiomatic. Declaring the variable inside `for()` is a C99/C11 feature — while it works with modern kernel builds, many panel drivers still declare loop variables at function scope. Use `int i;` declared at the top for consistency with the prevailing kernel style.

**5. Reset GPIO polarity in error path (worth verifying)**

In `ch13726a_prepare()`:

```c
+		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
```

The GPIO is obtained with `GPIOD_OUT_LOW` and the reset sequence asserts high (1), deasserts to low (0), then asserts high (1) again. In the error path after `ch13726a_on()` fails, the driver sets GPIO to 0 (deasserted/inactive), which keeps the panel out of reset. This is inconsistent — typically on error you want to put the panel *into* reset (assert reset) before disabling regulators. If active-high means "reset active", the error path should set the GPIO to 1. However, this depends on the actual hardware semantics. Since the binding says `GPIO_ACTIVE_HIGH` and `reset-gpios` is the name, `gpiod_set_value(1)` means "assert reset" which would be the safer state on error. Worth confirming with the hardware.

Similarly in `ch13726a_unprepare()`:

```c
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
```

This deasserts reset before killing power, which could leave the panel in an undefined state. Most panel drivers assert reset (set to 1) before cutting regulators to ensure clean shutdown. Compare with how `ch13726a_reset()` works — it uses `1` as the active/working state. The unprepare should logically assert reset (1) then cut power, not the other way around.

**6. `mipi_dsi_detach` error checking in `remove()` is unnecessary noise (very minor)**

```c
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
```

`mipi_dsi_detach()` never fails in practice (the host ops->detach can return an error, but no upstream host driver does). Newer panel drivers tend to not check this return. Not a blocker, but the error message adds no value.

**7. Missing `msleep`/delay between display-off and sleep-enter in `ch13726a_disable()` (minor)**

```c
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 50);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
```

There is a 50ms delay between display-off and sleep-enter, which is good. But there is no delay *after* entering sleep mode. Many MIPI panel datasheets require a delay (commonly 120ms) after entering sleep mode before cutting power in `unprepare()`. The `ch13726a_on()` path has a 28ms `msleep` at the end of `prepare()`, but the disable->unprepare transition has no such guard. If the panel requires a post-sleep delay per its datasheet, this could cause issues.

**8. Mode flags manipulation in backlight callback (worth noting)**

```c
+static int ch13726a_bl_update_status(struct backlight_device *bl)
+{
+	...
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+	ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+	...
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```

And in `ch13726a_on()`:

```c
+	ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```

And `ch13726a_disable()`:

```c
+	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
```

The LPM flag toggling is not protected by any locking. If backlight updates can race with disable/prepare calls, the mode_flags could be corrupted. This is a pre-existing pattern in many panel drivers, so not unique to this one, but worth noting.

**Summary of requested changes for v5:**
1. Remove `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER` from Kconfig
2. Remove the private `prepared` bool and the associated checks
3. Make `thor_bottom_desc` and the `desc` pointer `const`, drop the cast
4. Use `int` or `unsigned int` for the loop variable, not `uint8_t`
5. Verify GPIO reset polarity in error/unprepare paths (assert reset before cutting power)

---
Generated by Claude Code Patch Reviewer