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 Himax HX83121A panel driver Date: Mon, 16 Mar 2026 11:51:07 +1000 Message-ID: In-Reply-To: <20260315144536.515032-3-mitltlatltl@gmail.com> References: <20260315144536.515032-1-mitltlatltl@gmail.com> <20260315144536.515032-3-mitltlatltl@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 **`devm_drm_panel_alloc` error handling is wrong.** The function returns an= ERR_PTR on failure, not NULL. The current code: ```c ctx =3D devm_drm_panel_alloc(dev, struct himax, panel, &himax_panel_funcs, DRM_MODE_CONNECTOR_DSI); if (!ctx) return -ENOMEM; ``` Should be: ```c if (IS_ERR(ctx)) return PTR_ERR(ctx); ``` Compare with `panel-novatek-nt36523.c:1169` which does it correctly. This i= s a **bug** =E2=80=94 on allocation failure the driver would dereference an= ERR_PTR. **`dsc` field unconditionally copied from `desc->dsc_cfg`.** At line `ctx->= dsc =3D *desc->dsc_cfg;` =E2=80=94 this is fine as long as all panel descri= ptors have a non-NULL `dsc_cfg`, which they currently do. But if a future p= anel variant doesn't support DSC at all, this would NULL-deref. A guard wou= ld be prudent: ```c if (desc->dsc_cfg) ctx->dsc =3D *desc->dsc_cfg; ``` **`enable_dsc` module parameter is global and not per-panel.** Using a `mod= ule_param` bool to toggle DSC at load time is a fragile design. It affects = all panel instances simultaneously and cannot be changed at runtime in a me= aningful way (the DSI attach already happened). The `dsc` pointer on the DS= I device is set once at probe: ```c ctx->dsi[i]->dsc =3D enable_dsc ? &ctx->dsc : NULL; ``` If DSC mode is changed after probe, the modes and DSI config won't match. T= his is likely acceptable for the initial bring-up use case described, but s= hould be documented as a limitation or replaced with a proper DT property o= r panel mode selection mechanism. **`ppc357db1_4_dsc_cfg` should be `const`.** The struct is declared as: ```c static struct drm_dsc_config ppc357db1_4_dsc_cfg =3D { ``` Since it's used only to initialize `ctx->dsc` by copy, it should be `static= const`. **Missing `backlight_disable` in `himax_unprepare`.** The `himax_prepare` f= unction calls `backlight_enable(ctx->backlight)` at the end, but `himax_unp= repare` never calls `backlight_disable`. This means the backlight won't be = properly turned off on unprepare. The panel will send sleep-enter but the D= CS backlight remains logically enabled. Should add: ```c backlight_disable(ctx->backlight); ``` at the beginning of `himax_unprepare`. **Missing `set_display_off` in `himax_off`.** The `himax_off` function only= sends `enter_sleep_mode` but does not send `set_display_off` first. The MI= PI DCS spec recommends sending Display Off before entering sleep. Many othe= r panel drivers do: ```c mipi_dsi_dcs_set_display_off_multi(dsi_ctx); mipi_dsi_msleep(dsi_ctx, 20); mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx); ``` **Kconfig `select DRM_KMS_HELPER` is likely unnecessary.** Only 4 other pan= el drivers select it and the trend has been to avoid selecting it for new M= IPI DSI panel drivers. The driver doesn't appear to use anything from KMS h= elper directly. Consider dropping it. **Kconfig help text indentation mismatch.** The last line of the help text = uses spaces instead of a tab: ``` + 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 `series.` line uses spaces for indentation instead of a tab+spaces, bre= aking the indentation pattern. **`to_himax` helper is unnecessary with `devm_drm_panel_alloc`.** With `dev= m_drm_panel_alloc`, the idiomatic way to get the container is `to_drm_panel= ()` already returning the right pointer, but since `container_of` works fin= e here it's not a real issue =E2=80=94 just a style observation. **Non-standard mode clock calculation.** The mode definitions use inline ar= ithmetic like: ```c .clock =3D (800 + 60 + 40 + 40) * 2 * (2560 + 154 + 4 + 18) * 120 / 1000, ``` This is actually a nice pattern for readability. However, for the non-DSC m= ode, the vfront_porch differs between the mode struct and the clock calcula= tion: ```c .clock =3D (800 + 60 + 20 + 40) * 2 * (2560 + 154 + 4 + 18) * 60 / 1000, ... .vsync_start =3D 2560 + 168, ``` The clock uses vfp=3D154 but `vsync_start` uses vfp=3D168. These should mat= ch =E2=80=94 either the clock or the vsync_start is wrong. This is a **pote= ntial bug** that would cause the actual refresh rate to differ from the int= ended 60Hz. **`of_device_get_match_data` vs `device_get_match_data`.** The newer prefer= red API is `device_get_match_data(dev)` which works with both OF and ACPI. = Minor, but worth updating for consistency with modern kernel style. --- Generated by Claude Code Patch Reviewer