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/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver Date: Tue, 10 Mar 2026 12:31:50 +1000 Message-ID: In-Reply-To: <20260309-upstream-6162-v2-2-debdb6c88030@ite.com.tw> References: <20260309-upstream-6162-v2-0-debdb6c88030@ite.com.tw> <20260309-upstream-6162-v2-2-debdb6c88030@ite.com.tw> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Critical bugs:** 1. **Use-before-init of `dsi` variable** in `it6162_attach_dsi()`: ```c if (!it6162->dsi) { ret =3D it6162_load_mipi_pars_from_port(it6162, port); if (ret <=3D 0) return ret; dev_info(dev, "DSI host loaded paras for port %d", port); it6162->dsi =3D dsi; // BUG: 'dsi' is uninitialized here } dsi =3D devm_mipi_dsi_device_register_full(dev, dsi_host, &info); ``` The assignment `it6162->dsi =3D dsi` happens before `dsi` is assigned by `d= evm_mipi_dsi_device_register_full()`. On the first iteration, `dsi` is an u= ninitialized local variable. The `dsi` registration and the `it6162->dsi` a= ssignment need to be reordered. 2. **Missing `DRM_BRIDGE_OP_HDMI` flag**: The bridge sets ops for `DRM_BRID= GE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_MODES` but implements HDM= I-specific callbacks (`hdmi_write_avi_infoframe`, `hdmi_tmds_char_rate_vali= d`, etc.) that require `DRM_BRIDGE_OP_HDMI` to be set: ```c it6162->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_MODES; ``` Without `DRM_BRIDGE_OP_HDMI`, the HDMI connector framework won't use these = callbacks. Similarly, `DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME` is needed for the = SPD infoframe callbacks to be invoked. 3. **Missing `DRM_BRIDGE_OP_HPD` flag**: The driver calls `drm_bridge_hpd_n= otify()` in `it6162_interrupt_handler()` but never sets `DRM_BRIDGE_OP_HPD`= in `bridge.ops`. Without this flag, `drm_bridge_hpd_notify()` will not fun= ction properly. 4. **Sample width mapping is swapped**: ```c case 24: config->sample_width =3D WORD_LENGTH_18BIT; // Should be WORD_LENGTH_2= 4BIT break; case 20: config->sample_width =3D WORD_LENGTH_24BIT; // Should be WORD_LENGTH_2= 0BIT break; ``` The 24-bit and 20-bit cases are assigned wrong enum values. **Significant issues:** 5. **Regulator disable order is wrong**: In `it6162_platform_clear_power()`= , regulators are disabled in the same order they were enabled (`ivdd` =E2= =86=92 `pwr1833` =E2=86=92 `ovdd`). Standard practice is to disable in reve= rse order of enable (`ovdd` =E2=86=92 `pwr1833` =E2=86=92 `ivdd`). Also, if= `pwr1833` disable fails, `ovdd` will be left enabled (no cleanup on partia= l failure). 6. **`readx_poll_timeout` condition bug** in `it6162_infoblock_request_data= ()`: ```c bool val =3D 0; ... status =3D readx_poll_timeout(it6162_infoblock_complete, it6162, val, val <=3D 0 && it6162->data_buf_sts =3D=3D BUF_READY, 50 * 1000, TIMEOUT_INFOBLOCK_MS * 1000); ``` `it6162_infoblock_complete()` returns an `int` but `val` is declared as `bo= ol`. This will truncate the return value and break the `val <=3D 0` conditi= on check. Should be `int val`. 7. **Duplicate register offset definition**: ```c #define OFFSET_VERSION_L 0x03 #define OFFSET_VERSION_M 0x04 #define OFFSET_VERSION_H 0x03 // BUG: same as OFFSET_VERSION_L ``` `OFFSET_VERSION_H` should presumably be `0x05`, not `0x03`. 8. **`it6162_bridge_atomic_check` modifies `adjusted_mode` but doesn't upda= te `htotal`**: When HFP is adjusted, `hsync_start` and `hsync_end` are move= d, but `htotal` is not updated. This means the effective HBP changes implic= itly, and the adjustment may produce inconsistent timing. 9. **Missing `mutex_init` before first use**: `mutex_init(&it6162->lock)` i= s called in `it6162_probe()` after `it6162_detect_devices()`, which calls `= it6162_platform_set_power()` and `it6162_wait_mcu_ready()`. But `it6162_set= _default_config()` (called later via `it6162_poweron()` =E2=86=92 `it6162_r= eset_init()`) uses `guard(mutex)(&it6162->lock)`. The mutex should be initi= alized earlier in the probe sequence, before any function that might acquir= e it. Looking more carefully, `it6162_detect_devices()` doesn't use the mut= ex, but `it6162_poweron()` (called from `it6162_attach_dsi()`) does, and th= e mutex IS initialized before that call. However, it's fragile =E2=80=94 be= tter to initialize it earlier. 10. **`it6162_poweron` / `it6162_poweroff` are marked `__maybe_unused`**: T= hese are called directly from `it6162_attach_dsi()`. The `__maybe_unused` a= nnotation is incorrect and misleading =E2=80=94 these functions are definit= ely used. If the intent was for PM callbacks, they aren't actually wired up= as PM ops. 11. **No power management (runtime PM or system PM)**: The driver powers on= during probe/attach but has no suspend/resume support. `pm_runtime` is inc= luded but never used. 12. **`it6162_platform_clear_power` never called on normal remove**: `it616= 2_remove()` only disables IRQ and cancels work, but never powers down the c= hip. Regulators remain enabled after driver unbind. 13. **Copyright header inconsistency**: The SPDX tag says `GPL-2.0-only` bu= t the comment block says "either version 2 of the License, or (at your opti= on) any later version" which is `GPL-2.0-or-later`. Pick one and remove the= redundant boilerplate text. 14. **Typo**: `"evnet change"` in the debug message at the interrupt handle= r should be `"event change"`. 15. **Excessive `dev_info` logging**: Many `dev_info` calls (chip ID, probe= status, DSI host info, lane config, poweron end) should be `dev_dbg` to av= oid spamming the kernel log during normal operation. 16. **`it6162_hdcp_handler` accesses `cp_status` potentially uninitialized*= *: If `it6162->hdcp_version !=3D NO_HDCP` but `it6162->hdcp_sts =3D=3D hdcp= _sts` and `it6162->hdcp_sts !=3D NO_HDCP_STATE`, the code falls through to = `drm_hdcp_update_content_protection()` with `cp_status` uninitialized (it's= only set inside the inner `if` block). 17. **Infoframe write/clear functions not protected by mutex**: `it6162_wri= te_infoframe()` and `it6162_clear_infoframe()` access registers and call `i= t6162_infoblock_host_set()` but are not wrapped in `guard(mutex)`, unlike o= ther infoblock operations. --- Generated by Claude Code Patch Reviewer