From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot 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:37:00 +1000 Message-ID: In-Reply-To: <20260514-icna35xx-v2-2-45acd1dfa566@gmail.com> References: <20260514-icna35xx-v2-0-45acd1dfa566@gmail.com> <20260514-icna35xx-v2-2-45acd1dfa566@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 =3D connector; ``` Then accessed in `icna35xx_get_current_mode()` (called from `prepare()` =E2= =86=92 `init_sequence()`): ```c struct drm_connector *connector =3D pinfo->connector; ... if (!connector->state || !connector->state->crtc) return 0; crtc_state =3D connector->state->crtc->state; ``` Modern panel drivers do not store the connector pointer. This pattern is fr= agile =E2=80=94 if `prepare()` were ever called before `get_modes()`, `pinf= o->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 passin= g 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 =3D { ``` ```c static struct panel_desc thor_top_desc =3D { ``` These should be `static const struct panel_desc`. All other modern panel dr= ivers in the tree (e.g., `panel-ilitek-ili9882t.c`, `panel-boe-himax8279d.c= `) declare their descriptors as `const`. Correspondingly, `pinfo->desc` sho= uld be `const struct panel_desc *desc`. **3. Casting away const from `of_device_get_match_data` (minor)** ```c pinfo->desc =3D (struct panel_desc *)of_device_get_match_data(dev); ``` `of_device_get_match_data()` returns `const void *`. The explicit cast drop= s const. If the `desc` pointer and structs are made `const` (per point 2), = this cast becomes unnecessary and the line becomes: ```c pinfo->desc =3D of_device_get_match_data(dev); ``` **4. Unnecessary include (nit)** ```c #include ``` 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 &=3D ~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. Clearin= g `MIPI_DSI_MODE_LPM` to send them in HS mode is unusual. If this is requir= ed by the hardware, a brief comment would help future maintainers understan= d why. The same pattern appears in the backlight ops =E2=80=94 is this inte= ntional or an error? **6. `icna3512_init_sequence` sets LPM on `mode_flags` directly (minor)** ```c pinfo->dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; ``` The init sequence modifies `dsi->mode_flags` directly. This is fine for `pr= epare()`, but note that `MIPI_DSI_MODE_LPM` is already included in the `mod= e_flags` set in `probe()`: ```c .mode_flags =3D MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOU= S | 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 =E2=80=94 w= orth 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=E2=86=92assert=E2=86=92deassert pulse sequence. Consistent with ot= her 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_remov= e()`. This is the correct pattern when using `devm_drm_panel_alloc()` =E2= =80=94 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 =3D { .dsc_version_major =3D 0x1, .dsc_version_minor =3D 0x1, .slice_height =3D 20, .slice_width =3D 540, .slice_count =3D 2, .bits_per_component =3D 8, .bits_per_pixel =3D 8 << 4, .block_pred_enable =3D true, }, ``` Many DSC fields like `line_buf_depth`, `rc_model_size`, `rc_range_parameter= s`, `initial_xmit_delay`, `initial_dec_delay`, `mux_word_size`, etc. are le= ft 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 (lik= ely `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 con= nector stash pattern (item 1). Items 5-6 and 9 deserve clarification from t= he author. --- Generated by Claude Code Patch Reviewer