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

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

Patch Review

**`module_param` for DSC enable/disable:**

```c
+static bool enable_dsc = false;
+module_param(enable_dsc, bool, 0);
+MODULE_PARM_DESC(enable_dsc, "enable DSC on the panel (default: false)");
```

This is a non-standard approach. No other panel driver in the kernel uses `module_param` to toggle DSC. This is problematic because:
- It's a global variable affecting all instances of the driver simultaneously.
- The DSC state should be a property of the panel/platform, not a user knob. If the SoC DSI controller or the particular board needs DSC, it should be described in devicetree or auto-detected.
- `= false` initialization is redundant for a static bool (it's zero-initialized by default); `checkpatch` may warn about this.
- The mode permission is `0` which means the parameter can't even be read from sysfs, making it unusable for runtime inspection.

If DSC support is experimental/WIP, the DSC paths should simply be removed from this initial submission and added later when properly supported.

**Supply name mismatch with DT binding (as noted above):**

```c
+static const struct regulator_bulk_data himax_supplies[] = {
+	{ .supply = "vddi" },
+	{ .supply = "vsp" },
+	{ .supply = "vsn" },
+};
```

`"vsp"` and `"vsn"` don't match the DT binding's `avdd-supply` / `avee-supply`.

**Missing `bpc` in panel descriptors:**

The `panel_desc` struct defines a `bpc` field and `himax_get_modes` sets it on the connector:
```c
+	connector->display_info.bpc = desc->bpc;
```
But neither `boe_ppc357db1_4_desc` nor `csot_ppc357db1_4_desc` sets `bpc`. This will default to 0, which is wrong for an RGB888 panel (should be 8).

**Kconfig missing DSC helper selections:**

```c
+config DRM_PANEL_HIMAX_HX83121A
+	tristate "Himax HX83121A-based DSI panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_KMS_HELPER
```

The driver includes `<drm/display/drm_dsc_helper.h>` and calls `drm_dsc_pps_payload_pack()`, so it needs:
```
select DRM_DISPLAY_DSC_HELPER
select DRM_DISPLAY_HELPER
```

Also, `select DRM_KMS_HELPER` may not be necessary — the driver doesn't appear to use any KMS helper functions. Check whether this can be dropped.

**Kconfig help text indentation:**

```
+	  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 last line uses spaces instead of a tab+spaces for indentation. This should match the lines above it (tab + 2 spaces).

**`himax_prepare` calls `backlight_update_status` directly:**

```c
+	return backlight_update_status(ctx->backlight);
```

This is unusual. The DRM panel framework normally handles backlight enable/disable through the `drm_panel_enable`/`drm_panel_disable` callbacks. The `prepare` callback should just power on the panel and send init sequences. `backlight_update_status` could return an error here that prevents panel prepare from succeeding, which may not be desired. Consider moving backlight handling to a separate `enable` callback, or removing this entirely and letting the framework handle it.

**`himax_off` missing `set_display_off` before sleep:**

```c
+static int himax_off(struct mipi_dsi_multi_context *dsi_ctx)
+{
+	mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx);
+	mipi_dsi_msleep(dsi_ctx, 120);
+
+	return dsi_ctx->accum_err;
+}
```

The standard MIPI DCS power-off sequence is display_off followed by enter_sleep_mode. Most panel drivers call `mipi_dsi_dcs_set_display_off_multi()` before `enter_sleep_mode`. Omitting it could leave the display in an undefined state during power-down.

**Extra blank line in `himax_get_modes`:**

```c
+static int himax_get_modes(struct drm_panel *panel,
+			   struct drm_connector *connector)
+{
+
+	struct himax *ctx = to_himax(panel);
```

Extra blank line after the opening brace.

**`to_primary_dsi` naming is confusing:**

```c
+static inline struct mipi_dsi_device *to_primary_dsi(struct himax *ctx)
+{
+	/* Sync on DSI1 for dual dsi */
+	return ctx->desc->is_dual_dsi ? ctx->dsi[1] : ctx->dsi[0];
+}
```

This returns `dsi[1]` for dual-DSI, which is counter-intuitive for a function named `to_primary_dsi`. The comment says "Sync on DSI1" but doesn't explain why the secondary DSI interface is considered "primary" for command purposes. This needs a better name or a more detailed comment explaining the rationale.

**`ctx->dsc = *desc->dsc_cfg` unconditional copy:**

```c
+	ctx->dsc = *desc->dsc_cfg;
```

This dereferences `desc->dsc_cfg` unconditionally during probe. If a future panel descriptor doesn't set `dsc_cfg`, this will NULL-dereference. Consider guarding this with a NULL check.

**`ppc357db1_4_dsc_cfg` should be `const`:**

```c
+static struct drm_dsc_config ppc357db1_4_dsc_cfg = {
```

This can and should be declared `const` since it's only used as a source for copying into `ctx->dsc`.

**MODULE_AUTHOR email mismatch:**

```c
+MODULE_AUTHOR("Pengyu Luo <mitltlatltl0@gmail.com>");
```

The author email has a trailing `0` (`mitltlatltl0@gmail.com`) but the Signed-off-by and From use `mitltlatltl@gmail.com` (without the `0`). One of these is wrong.

**Non-DSC mode only provides 60Hz:**

The `ppc357db1_4_modes[]` array only has a single 60Hz mode, while the DSC modes array has both 120Hz and 60Hz. The comment in the CSOT DSC init sequence mentions 120Hz without DSC is possible via register 0xE2. If the panel supports 120Hz without DSC, it should be represented in the non-DSC modes as well. Otherwise, this asymmetry should be documented.

**`vsync_start` inconsistency in non-DSC mode:**

In `ppc357db1_4_modes`:
```c
+		.vsync_start = 2560 + 168,
```
But in the DSC 120Hz mode:
```c
+		.vsync_start = 2560 + 154,
```
The vertical front porch differs (168 vs 154). This may be intentional for the different modes, but it would benefit from a comment explaining why.

---
Generated by Claude Code Patch Reviewer