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: Wed, 04 Mar 2026 07:28:32 +1000 Message-ID: In-Reply-To: <20260303115730.9580-3-mitltlatltl@gmail.com> References: <20260303115730.9580-1-mitltlatltl@gmail.com> <20260303115730.9580-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 **`module_param` for DSC enable/disable:** ```c +static bool enable_dsc =3D false; +module_param(enable_dsc, bool, 0); +MODULE_PARM_DESC(enable_dsc, "enable DSC on the panel (default: false)"); ``` This is a non-standard approach. No other panel driver in the kernel uses `= module_param` to toggle DSC. This is problematic because: - It's a global variable affecting all instances of the driver simultaneous= ly. - The DSC state should be a property of the panel/platform, not a user knob= . If the SoC DSI controller or the particular board needs DSC, it should be= described in devicetree or auto-detected. - `=3D false` initialization is redundant for a static bool (it's zero-init= ialized by default); `checkpatch` may warn about this. - The mode permission is `0` which means the parameter can't even be read f= rom sysfs, making it unusable for runtime inspection. If DSC support is experimental/WIP, the DSC paths should simply be removed = from this initial submission and added later when properly supported. **Supply name mismatch with DT binding (as noted above):** ```c +static const struct regulator_bulk_data himax_supplies[] =3D { + { .supply =3D "vddi" }, + { .supply =3D "vsp" }, + { .supply =3D "vsn" }, +}; ``` `"vsp"` and `"vsn"` don't match the DT binding's `avdd-supply` / `avee-supp= ly`. **Missing `bpc` in panel descriptors:** The `panel_desc` struct defines a `bpc` field and `himax_get_modes` sets it= on the connector: ```c + connector->display_info.bpc =3D desc->bpc; ``` But neither `boe_ppc357db1_4_desc` nor `csot_ppc357db1_4_desc` sets `bpc`. = This will default to 0, which is wrong for an RGB888 panel (should be 8). **Kconfig missing DSC helper selections:** ```c +config DRM_PANEL_HIMAX_HX83121A + tristate "Himax HX83121A-based DSI panel" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + select DRM_KMS_HELPER ``` The driver includes `` and calls `drm_dsc_pps= _payload_pack()`, so it needs: ``` select DRM_DISPLAY_DSC_HELPER select DRM_DISPLAY_HELPER ``` Also, `select DRM_KMS_HELPER` may not be necessary =E2=80=94 the driver doe= sn't appear to use any KMS helper functions. Check whether this can be drop= ped. **Kconfig help text indentation:** ``` + 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 last line uses spaces instead of a tab+spaces for indentation. This sho= uld match the lines above it (tab + 2 spaces). **`himax_prepare` calls `backlight_update_status` directly:** ```c + return backlight_update_status(ctx->backlight); ``` This is unusual. The DRM panel framework normally handles backlight enable/= disable through the `drm_panel_enable`/`drm_panel_disable` callbacks. The `= prepare` callback should just power on the panel and send init sequences. `= backlight_update_status` could return an error here that prevents panel pre= pare from succeeding, which may not be desired. Consider moving backlight h= andling to a separate `enable` callback, or removing this entirely and lett= ing the framework handle it. **`himax_off` missing `set_display_off` before sleep:** ```c +static int himax_off(struct mipi_dsi_multi_context *dsi_ctx) +{ + mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx); + mipi_dsi_msleep(dsi_ctx, 120); + + return dsi_ctx->accum_err; +} ``` The standard MIPI DCS power-off sequence is display_off followed by enter_s= leep_mode. Most panel drivers call `mipi_dsi_dcs_set_display_off_multi()` b= efore `enter_sleep_mode`. Omitting it could leave the display in an undefin= ed state during power-down. **Extra blank line in `himax_get_modes`:** ```c +static int himax_get_modes(struct drm_panel *panel, + struct drm_connector *connector) +{ + + struct himax *ctx =3D to_himax(panel); ``` Extra blank line after the opening brace. **`to_primary_dsi` naming is confusing:** ```c +static inline struct mipi_dsi_device *to_primary_dsi(struct himax *ctx) +{ + /* Sync on DSI1 for dual dsi */ + return ctx->desc->is_dual_dsi ? ctx->dsi[1] : ctx->dsi[0]; +} ``` This returns `dsi[1]` for dual-DSI, which is counter-intuitive for a functi= on named `to_primary_dsi`. The comment says "Sync on DSI1" but doesn't expl= ain why the secondary DSI interface is considered "primary" for command pur= poses. This needs a better name or a more detailed comment explaining the r= ationale. **`ctx->dsc =3D *desc->dsc_cfg` unconditional copy:** ```c + ctx->dsc =3D *desc->dsc_cfg; ``` This dereferences `desc->dsc_cfg` unconditionally during probe. If a future= panel descriptor doesn't set `dsc_cfg`, this will NULL-dereference. Consid= er guarding this with a NULL check. **`ppc357db1_4_dsc_cfg` should be `const`:** ```c +static struct drm_dsc_config ppc357db1_4_dsc_cfg =3D { ``` This can and should be declared `const` since it's only used as a source fo= r copying into `ctx->dsc`. **MODULE_AUTHOR email mismatch:** ```c +MODULE_AUTHOR("Pengyu Luo "); ``` The author email has a trailing `0` (`mitltlatltl0@gmail.com`) but the Sign= ed-off-by and From use `mitltlatltl@gmail.com` (without the `0`). One of th= ese is wrong. **Non-DSC mode only provides 60Hz:** The `ppc357db1_4_modes[]` array only has a single 60Hz mode, while the DSC = modes array has both 120Hz and 60Hz. The comment in the CSOT DSC init seque= nce mentions 120Hz without DSC is possible via register 0xE2. If the panel = supports 120Hz without DSC, it should be represented in the non-DSC modes a= s well. Otherwise, this asymmetry should be documented. **`vsync_start` inconsistency in non-DSC mode:** In `ppc357db1_4_modes`: ```c + .vsync_start =3D 2560 + 168, ``` But in the DSC 120Hz mode: ```c + .vsync_start =3D 2560 + 154, ``` The vertical front porch differs (168 vs 154). This may be intentional for = the different modes, but it would benefit from a comment explaining why. --- Generated by Claude Code Patch Reviewer