Claude review: drm/panel: Add Himax HX83121A panel driver

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

Patch Review

**`devm_drm_panel_alloc` error handling is wrong.** The function returns an ERR_PTR on failure, not NULL. The current code:
```c
ctx = devm_drm_panel_alloc(dev, struct himax, panel, &himax_panel_funcs,
			   DRM_MODE_CONNECTOR_DSI);
if (!ctx)
	return -ENOMEM;
```
Should be:
```c
if (IS_ERR(ctx))
	return PTR_ERR(ctx);
```
Compare with `panel-novatek-nt36523.c:1169` which does it correctly. This is a **bug** — on allocation failure the driver would dereference an ERR_PTR.

**`dsc` field unconditionally copied from `desc->dsc_cfg`.** At line `ctx->dsc = *desc->dsc_cfg;` — this is fine as long as all panel descriptors have a non-NULL `dsc_cfg`, which they currently do. But if a future panel variant doesn't support DSC at all, this would NULL-deref. A guard would be prudent:
```c
if (desc->dsc_cfg)
	ctx->dsc = *desc->dsc_cfg;
```

**`enable_dsc` module parameter is global and not per-panel.** Using a `module_param` bool to toggle DSC at load time is a fragile design. It affects all panel instances simultaneously and cannot be changed at runtime in a meaningful way (the DSI attach already happened). The `dsc` pointer on the DSI device is set once at probe:
```c
ctx->dsi[i]->dsc = enable_dsc ? &ctx->dsc : NULL;
```
If DSC mode is changed after probe, the modes and DSI config won't match. This is likely acceptable for the initial bring-up use case described, but should be documented as a limitation or replaced with a proper DT property or panel mode selection mechanism.

**`ppc357db1_4_dsc_cfg` should be `const`.** The struct is declared as:
```c
static struct drm_dsc_config ppc357db1_4_dsc_cfg = {
```
Since it's used only to initialize `ctx->dsc` by copy, it should be `static const`.

**Missing `backlight_disable` in `himax_unprepare`.** The `himax_prepare` function calls `backlight_enable(ctx->backlight)` at the end, but `himax_unprepare` never calls `backlight_disable`. This means the backlight won't be properly turned off on unprepare. The panel will send sleep-enter but the DCS backlight remains logically enabled. Should add:
```c
backlight_disable(ctx->backlight);
```
at the beginning of `himax_unprepare`.

**Missing `set_display_off` in `himax_off`.** The `himax_off` function only sends `enter_sleep_mode` but does not send `set_display_off` first. The MIPI DCS spec recommends sending Display Off before entering sleep. Many other panel drivers do:
```c
mipi_dsi_dcs_set_display_off_multi(dsi_ctx);
mipi_dsi_msleep(dsi_ctx, 20);
mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx);
```

**Kconfig `select DRM_KMS_HELPER` is likely unnecessary.** Only 4 other panel drivers select it and the trend has been to avoid selecting it for new MIPI DSI panel drivers. The driver doesn't appear to use anything from KMS helper directly. Consider dropping it.

**Kconfig help text indentation mismatch.** The last line of the help text uses spaces instead of a tab:
```
+	  Say Y here if you want to enable support for Himax HX83121A-based
+	  display panels, such as the one found in the HUAWEI Matebook E Go
+          series.
```
The `series.` line uses spaces for indentation instead of a tab+spaces, breaking the indentation pattern.

**`to_himax` helper is unnecessary with `devm_drm_panel_alloc`.** With `devm_drm_panel_alloc`, the idiomatic way to get the container is `to_drm_panel()` already returning the right pointer, but since `container_of` works fine here it's not a real issue — just a style observation.

**Non-standard mode clock calculation.** The mode definitions use inline arithmetic like:
```c
.clock = (800 + 60 + 40 + 40) * 2 * (2560 + 154 + 4 + 18) * 120 / 1000,
```
This is actually a nice pattern for readability. However, for the non-DSC mode, the vfront_porch differs between the mode struct and the clock calculation:
```c
.clock = (800 + 60 + 20 + 40) * 2 * (2560 + 154 + 4 + 18) * 60 / 1000,
...
.vsync_start = 2560 + 168,
```
The clock uses vfp=154 but `vsync_start` uses vfp=168. These should match — either the clock or the vsync_start is wrong. This is a **potential bug** that would cause the actual refresh rate to differ from the intended 60Hz.

**`of_device_get_match_data` vs `device_get_match_data`.** The newer preferred API is `device_get_match_data(dev)` which works with both OF and ACPI. Minor, but worth updating for consistency with modern kernel style.

---
Generated by Claude Code Patch Reviewer