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: Sat, 14 Mar 2026 07:27:22 +1000 Message-ID: In-Reply-To: <20260313-upstream-6162-v3-2-8497fb7c4406@ite.com.tw> References: <20260313-upstream-6162-v3-0-8497fb7c4406@ite.com.tw> <20260313-upstream-6162-v3-2-8497fb7c4406@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 / Functional Issues:** 1. **`DRM_BRIDGE_OP_MODES` set without `get_modes` callback** (line 1572 in= driver, corresponding to mbox line ~1783): ```c it6162->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_MODES; ``` The `DRM_BRIDGE_OP_MODES` flag requires implementing the `get_modes` cal= lback per the bridge documentation (`include/drm/drm_bridge.h:1027`). The d= river doesn't implement `get_modes` in `it6162_bridge_funcs`. This flag sho= uld either be removed (since `DRM_BRIDGE_OP_EDID` is sufficient to enumerat= e modes from EDID), or a `get_modes` callback should be added. 2. **`DRM_BRIDGE_OP_HPD` not set**: The driver calls `drm_bridge_hpd_notify= ()` from the interrupt handler (line 694) but doesn't set `DRM_BRIDGE_OP_HP= D` in `bridge.ops`. The HPD notification path may not work correctly withou= t this flag, as the connector infrastructure checks for it. 3. **`it6162_poweroff` is dead code** (line 842): ```c static int __maybe_unused it6162_poweroff(struct it6162 *it6162) ``` `it6162_poweroff` is never called anywhere. The `__maybe_unused` annotat= ion hides the warning, but the driver has no power-off path =E2=80=94 not i= n `remove`, not in any PM ops, nowhere. This means once powered on, the chi= p is never cleanly powered down. The `remove` callback only disables the IR= Q and cancels the HDCP work. 4. **`it6162_platform_set_power` missing regulator cleanup on error** (line= s 753-776): If enabling `pwr1833` fails, `ivdd` is left enabled. If enablin= g `ovdd` fails, both `ivdd` and `pwr1833` are left enabled. Should use `got= o` error unwind or `regulator_bulk_*` APIs. 5. **`it6162_detect_devices` powers on but never powers off** (lines 799-82= 2): This is called during probe to verify the chip ID. It calls `it6162_pla= tform_set_power()` but if the chip ID matches, it returns 0 without powerin= g back down. Later, `it6162_poweron()` in `it6162_attach_dsi` calls `it6162= _platform_set_power()` again, enabling the regulators a second time (bumpin= g their reference counts), which means a single disable won't actually turn= them off. **Moderate Issues:** 6. **`it6162_bridge_hdmi_audio_prepare` ignores error from `it6162_audio_up= date_hw_params`** (lines 1388-1393): ```c it6162_audio_update_hw_params(it6162, &config, fmt, params); it6162_enable_audio(it6162, &config); ``` The return value of `it6162_audio_update_hw_params` is discarded. If it = returns `-EINVAL` (unsupported rate/width/format), the driver proceeds with= uninitialized `config` fields. 7. **`it6162_display_mode_to_settings` uses conditional sets without zeroin= g first** (lines 1208-1230): The function checks `DRM_MODE_FLAG_PHSYNC` to = set `hpol=3D1` and `DRM_MODE_FLAG_NVSYNC` to set `vpol=3D1`, but never sets= them to 0 for the opposite case. The `video_setting` struct on the stack i= n `it6162_bridge_atomic_enable` is not zeroed (no `=3D {}` or `memset`). Th= e `prog` field is never set at all =E2=80=94 it defaults to whatever garbag= e is on the stack, but should be 1 for progressive. 8. **`it6162_write_infoframe` and `it6162_clear_infoframe` not holding the = lock** (lines 1422-1441): These functions write to the regmap and call `it6= 162_infoblock_trigger` but don't take `it6162->lock`, unlike similar functi= ons (`it6162_enable_audio`, `it6162_mipi_enable`, etc.). This risks racing = with other infoblock operations. 9. **`!!GET_BUFFER_STATUS(int_status)` in interrupt handler** (line 672): ```c if (!!GET_BUFFER_STATUS(int_status)) { ``` The double negation is unnecessary. Just `if (GET_BUFFER_STATUS(int_stat= us))` is clearer. 10. **`regmap_read` return values mostly unchecked**: Many `regmap_read` ca= lls have their return value ignored (e.g., in `it6162_tx_hdcp_enable` line = 380, `it6162_hdcp_handler` line 618, `it6162_detect` line 883). While regma= p errors are relatively uncommon on I2C, inconsistent error checking is poo= r practice =E2=80=94 the driver checks in some places but not others. **Minor / Style Issues:** 11. **`video/videomode.h` included but unused**: The header `