Claude review: drm/panel: Add panel driver for Chipone ICNA35XX based panels

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

Patch Review

This is the main driver. Several issues:

**1. `connector` pointer stash is fragile (medium)**

The driver stores a `struct drm_connector *connector` in `panel_info`:

```c
struct panel_info {
	struct drm_panel panel;
	struct drm_connector *connector;
	...
};
```

It's set in `icna35xx_get_modes()`:
```c
	pinfo->connector = connector;
```

Then accessed in `icna35xx_get_current_mode()` (called from `prepare()` → `init_sequence()`):
```c
	struct drm_connector *connector = pinfo->connector;
	...
	if (!connector->state || !connector->state->crtc)
		return 0;

	crtc_state = connector->state->crtc->state;
```

Modern panel drivers do not store the connector pointer. This pattern is fragile — if `prepare()` were ever called before `get_modes()`, `pinfo->connector` would be NULL and the NULL check on `connector->state` would crash (it checks `connector->state`, not `connector` itself). The lifetime of the connector is also not guaranteed to match the panel.

While this anti-pattern exists in some older drivers, it should be avoided in new ones. If multi-mode init sequences are truly needed, consider passing mode information through `drm_panel`'s `enable()` path or documenting the assumption clearly.

**2. `panel_desc` structs should be `const` (minor/style)**

```c
static struct panel_desc odin2portal_desc = {
```
```c
static struct panel_desc thor_top_desc = {
```

These should be `static const struct panel_desc`. All other modern panel drivers in the tree (e.g., `panel-ilitek-ili9882t.c`, `panel-boe-himax8279d.c`) declare their descriptors as `const`. Correspondingly, `pinfo->desc` should be `const struct panel_desc *desc`.

**3. Casting away const from `of_device_get_match_data` (minor)**

```c
	pinfo->desc = (struct panel_desc *)of_device_get_match_data(dev);
```

`of_device_get_match_data()` returns `const void *`. The explicit cast drops const. If the `desc` pointer and structs are made `const` (per point 2), this cast becomes unnecessary and the line becomes:
```c
	pinfo->desc = of_device_get_match_data(dev);
```

**4. Unnecessary include (nit)**

```c
#include <linux/of_graph.h>
```

The driver doesn't use any `of_graph_*()` functions. This include should be removed.

**5. `icna35xx_disable` clears LPM before sending commands (question)**

```c
static int icna35xx_disable(struct drm_panel *panel)
{
	...
	pinfo->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
	...
```

Display off and sleep entry commands are typically sent in LP mode. Clearing `MIPI_DSI_MODE_LPM` to send them in HS mode is unusual. If this is required by the hardware, a brief comment would help future maintainers understand why. The same pattern appears in the backlight ops — is this intentional or an error?

**6. `icna3512_init_sequence` sets LPM on `mode_flags` directly (minor)**

```c
	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```

The init sequence modifies `dsi->mode_flags` directly. This is fine for `prepare()`, but note that `MIPI_DSI_MODE_LPM` is already included in the `mode_flags` set in `probe()`:

```c
	.mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS |
			MIPI_DSI_MODE_LPM,
```

So the OR in init_sequence is redundant on the first call. After `disable()` clears it, `prepare()` would restore it. The interplay between `disable()` clearing LPM and `prepare()` restoring it works but is subtle — worth checking this is intentional.

**7. `icna35xx_reset` sequence (ok)**

```c
	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
	usleep_range(20000, 21000);
	gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
	usleep_range(20000, 21000);
	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
	usleep_range(20000, 21000);
```

With `GPIOD_OUT_LOW` at probe and `GPIO_ACTIVE_LOW` in DT, this is a valid deassert→assert→deassert pulse sequence. Consistent with other panel drivers. No issue here.

**8. `drm_panel_remove` / `mipi_dsi_detach` in remove callback (ok)**

The `remove()` callback calls both `mipi_dsi_detach()` and `drm_panel_remove()`. This is the correct pattern when using `devm_drm_panel_alloc()` — devm handles the panel allocation lifetime, but `drm_panel_remove()` is still needed to unregister from the panel list.

**9. DSC config is incomplete (question)**

The DSC configurations only set a few fields:

```c
	.dsc = {
		.dsc_version_major = 0x1,
		.dsc_version_minor = 0x1,
		.slice_height = 20,
		.slice_width = 540,
		.slice_count = 2,
		.bits_per_component = 8,
		.bits_per_pixel = 8 << 4,
		.block_pred_enable = true,
	},
```

Many DSC fields like `line_buf_depth`, `rc_model_size`, `rc_range_parameters`, `initial_xmit_delay`, `initial_dec_delay`, `mux_word_size`, etc. are left at zero/default. Is the DSI host driver expected to compute these? If so, this should be verified against the DSI controller driver being used (likely `msm` for Qualcomm QCS8550). If the DSI host doesn't fill in defaults, the DSC encoding could produce garbage.

**Summary:** The main actionable items are making `panel_desc` const (items 2-3), removing the unused include (item 4), and ideally rethinking the connector stash pattern (item 1). Items 5-6 and 9 deserve clarification from the author.

---
Generated by Claude Code Patch Reviewer