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 panel driver for Chipone ICNA35XX based panels
Date: Sat, 16 May 2026 10:22:33 +1000
Message-ID: <review-patch2-20260514-icna35xx-v3-2-c304f04c32c4@gmail.com>
In-Reply-To: <20260514-icna35xx-v3-2-c304f04c32c4@gmail.com>
References: <20260514-icna35xx-v3-0-c304f04c32c4@gmail.com> <20260514-icna35xx-v3-2-c304f04c32c4@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the main driver patch. It follows current conventions well but has several issues:

**1. Likely bug: `width_mm` / `height_mm` are swapped (both descriptors)**

Both panels have a native resolution of 1080x1920 (portrait). The physical dimensions should have `width_mm < height_mm` to match:

```c
static struct panel_desc odin2portal_desc = {
	...
	.width_mm = 160,
	.height_mm = 89,
```

For 1080x1920 at ~7", computing from pixel density: width should be ~87-89mm and height ~155-160mm. These values are transposed. It should be:

```c
	.width_mm = 89,
	.height_mm = 160,
```

Same for `thor_top_desc`:
```c
	.width_mm = 136,
	.height_mm = 68,
```
Should likely be `.width_mm = 68, .height_mm = 136`. As-is, userspace DPI calculations will be incorrect.

**2. `panel_desc` structs should be `const`**

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

These are never modified after initialization and should be `static const struct panel_desc`. The `pinfo->desc` member should then become `const struct panel_desc *desc`. The one complication is `pinfo->dsi->dsc = &pinfo->desc->dsc` assigning to a non-const pointer, which can be addressed with a cast or by copying the DSC config into `panel_info` (as `panel-novatek-nt35950` does).

**3. Use `devm_mipi_dsi_attach()` to eliminate the remove callback**

There are now 18+ panel drivers in-tree using `devm_mipi_dsi_attach()`. Since the driver already uses `devm_drm_panel_alloc()`, switching to `devm_mipi_dsi_attach()` would allow removing the `icna35xx_remove()` function entirely:

```c
static void icna35xx_remove(struct mipi_dsi_device *dsi)
{
	struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
	int ret;

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

	drm_panel_remove(&pinfo->panel);
}
```

This entire function can be deleted if `mipi_dsi_attach()` is replaced with `devm_mipi_dsi_attach()` in probe, and the `drm_panel_remove()` error path becomes `return dev_err_probe(...)` since devm handles cleanup.

**4. Unnecessary include**

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

No `of_graph_*` functions are used in this driver. This include should be removed.

**5. `__typeof` usage is non-idiomatic**

```c
	pinfo = devm_drm_panel_alloc(dev, __typeof(*pinfo), panel,
```

All other in-tree callers pass the struct type directly:

```c
	pinfo = devm_drm_panel_alloc(dev, struct panel_info, panel,
```

**6. Indentation issue in `devm_regulator_bulk_get_const` call**

```c
	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(panel_supplies),
	panel_supplies, &pinfo->supplies);
```

The continuation line should be aligned with the opening parenthesis or at least indented further:

```c
	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(panel_supplies),
					    panel_supplies, &pinfo->supplies);
```

**7. `of_device_get_match_data()` cast**

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

The preferred modern API is `device_get_match_data(dev)` which works with both OF and ACPI and returns `const void *`. Using it would also reinforce the constness issue from point 2.

**8. `icna35xx_disable` never restores `MIPI_DSI_MODE_LPM`**

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

The LPM flag is cleared but never restored. In practice this is fine because `prepare()` calls `init_sequence()` which re-sets it via `pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM`, and the full mode_flags are reset from `pinfo->desc->mode_flags` in a new attach cycle. But it would be cleaner to restore it at the end of disable, matching the pattern in the backlight ops.

**9. Minor: `init_sequence` register naming could benefit from defines**

The init sequences use raw vendor register addresses (`0x9F`, `0x48`, `0xB3`, etc.) without any symbolic names. This is common for vendor-derived init sequences and not a blocker, but adding at least a comment for the page-select register (`0x9F`) would help future maintainability.

**10. DSC config completeness**

The DSC configs only populate basic fields (version, slice dimensions, bpc, bpp, block_pred_enable). This matches the pattern in `panel-visionox-r66451.c` and is acceptable for panels where the DDIC handles decompression internally, so this is fine.

**Summary:** The main actionable items are the swapped `width_mm`/`height_mm` (likely bug), switching to `devm_mipi_dsi_attach`, making descriptors `const`, removing the unused `of_graph.h` include, and fixing the indentation/style nits. The overall driver structure and approach are correct.

---
Generated by Claude Code Patch Reviewer