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 driver for Synaptics R63455 DSI panel Date: Mon, 25 May 2026 20:00:02 +1000 Message-ID: In-Reply-To: <20260521-sm8650-7-1-bonded-dsi-v4-5-a4dd5e0850f1@linaro.org> References: <20260521-sm8650-7-1-bonded-dsi-v4-0-a4dd5e0850f1@linaro.org> <20260521-sm8650-7-1-bonded-dsi-v4-5-a4dd5e0850f1@linaro.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Concept**: Panel driver managing two R63455-based panels as a single logi= cal display for VR. **Issues:** 1. **Build breakage =E2=80=94 `pulse_offset_rows` does not exist**: The cod= e uses: ```c ctx->backlight->props.pulse_offset_rows =3D GPO1_TES1; ``` and in `r63455_bl_update_status`: ```c bl->props.pulse_offset_rows ``` The field `pulse_offset_rows` does not exist in `struct backlight_proper= ties` in the kernel. This will not compile. It appears to be a custom exten= sion that was never submitted upstream, or a leftover from an out-of-tree t= ree. The brightness-related pulse timing should likely be handled within th= e panel driver directly rather than via backlight properties. 2. **Deprecated API =E2=80=94 `of_device_get_match_data`**: Line: ```c ctx->dsi_init_seq =3D of_device_get_match_data(dev); ``` Should use `device_get_match_data(dev)` instead. `of_device_get_match_da= ta` is deprecated in favor of the bus-agnostic version. 3. **Missing `IS_ERR` check after `devm_drm_panel_alloc`**: The probe funct= ion does: ```c ctx =3D devm_drm_panel_alloc(&dsi->dev, __typeof(*ctx), panel, &r63455_drm_funcs, DRM_MODE_CONNECTOR_DSI); ctx->dsi_init_seq =3D of_device_get_match_data(dev); ``` There's no `IS_ERR(ctx)` check. `devm_drm_panel_alloc` can return `ERR_P= TR(-ENOMEM)`, which would cause a NULL-pointer dereference on the next line. 4. **Unused header**: `#include ` =E2=80=94 with the swi= tch to `device_get_match_data()`, this would be unnecessary. Even with `of_= device_get_match_data`, this header is largely deprecated in favor of ``. 5. **Unused header**: `#include ` =E2=80=94 the d= river doesn't use any pinctrl APIs. 6. **`LE16_BYTE0`/`LE16_BYTE1` and `BE16_BYTE0`/`BE16_BYTE1` macros are con= fusing**: These macros take a `val` that is a plain integer constant (e.g.,= `VBP =3D 100`), then call `le16_to_cpu()` or `be16_to_cpu()` on it. Since = the input is a host-endian constant (not actually little- or big-endian dat= a), this is incorrect usage of endian conversion functions. On little-endia= n ARM, `le16_to_cpu(100)` happens to be a no-op and `be16_to_cpu(100)` byte= -swaps, but the semantic is wrong. These should just use plain shift/mask o= perations: `(val & 0xff)` and `((val >> 8) & 0xff)`. 7. **Error handling in `r63455_unprepare`**: The function continues disabli= ng regulators even after errors, which is reasonable for cleanup, but it on= ly returns the *last* error code. If `reg_lcd_bias_neg` disable fails but `= reg_bl` disable succeeds, the error from `reg_lcd_bias_neg` is lost. 8. **No `remove` or cleanup for `dsi1_device`**: In probe, `mipi_dsi_device= _register_full()` is called to create `dsi1_device`, but there's no corresp= onding `mipi_dsi_device_unregister()` in an error path or remove callback. = Since only `devm_mipi_dsi_attach` is used (which handles detach), the devic= e registration itself leaks on driver unbind. Consider using `devm_mipi_dsi= _device_register_full()` if available, or adding cleanup. 9. **Function name typo**: `r63455_panel_on_boe_vs026c4m_n52_26000` =E2=80= =94 the compatible string is `boe,vs026c4m-n52-6000`, but the function name= has `26000` (an extra `2`). 10. **`of_graph_get_remote_node` returns a reference**: Line: ```c dsi1 =3D of_graph_get_remote_node(dsi->dev.of_node, 1, -1); ``` The `of_node_put(dsi1)` is correctly called a few lines later, but if `= of_find_mipi_dsi_host_by_node` fails and returns `-EPROBE_DEFER`, the funct= ion returns without any issue. This is fine. However, `dsi1` is used as the= node to find the host, but the host's node is the *parent* of the endpoint= =E2=80=94 `of_graph_get_remote_node` returns the remote port's parent devi= ce node, which should be correct for `of_find_mipi_dsi_host_by_node`. This = looks correct. 11. **Mode `.clock` computation**: The clock calculation: ```c .clock =3D (2160 + 24 + 20 + 20) * (2160 + 404 + 1 + 20) * 120 * 2 / 10= 00, ``` The `* 2` is for the doubled horizontal resolution. The result is `(222= 4) * (2585) * 120 * 2 / 1000 =3D 1,380,787.2` =E2=86=92 truncated to `13807= 87` kHz (~1.38 GHz pixel clock). This seems extremely high for a DSI link b= ut may be correct for a compressed/DSC mode. Worth double-checking. --- Generated by Claude Code Patch Reviewer