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: Sun, 22 Mar 2026 04:39:32 +1000 Message-ID: In-Reply-To: <20260319-upstream-6162-v4-2-7268483ef03d@ite.com.tw> References: <20260319-upstream-6162-v4-0-7268483ef03d@ite.com.tw> <20260319-upstream-6162-v4-2-7268483ef03d@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 This is a ~1600-line new bridge driver. There are several issues: **Critical / High:** 1. **Uninitialized `video_setting` struct on stack** in `it6162_bridge_atom= ic_enable`: ```c struct it6162_video_settings video_setting; ... it6162_avi_to_video_setting(avi, &video_setting); it6162_display_mode_to_settings(&crtc_state->mode, &video_setting); ``` `it6162_display_mode_to_settings` only conditionally sets `hpol`, `vpol`, a= nd `prog`: ```c if (mode->flags & DRM_MODE_FLAG_PHSYNC) settings->hpol =3D 1; if (mode->flags & DRM_MODE_FLAG_NVSYNC) settings->vpol =3D 1; if (mode->flags & DRM_MODE_FLAG_INTERLACE) settings->prog =3D 0; ``` If none of those flags are set, these fields contain stack garbage. The str= uct should be zero-initialized: `struct it6162_video_settings video_setting= =3D {};` or use `memset`. The `prog` field is particularly problematic =E2= =80=94 for progressive modes (the common case), it's never explicitly set t= o 1. 2. **Device-tree node reference leak** in `it6162_load_mipi_pars_from_port`: ```c endpoint =3D of_graph_get_endpoint_by_regs(of, port, -1); if (!endpoint) return 0; dsi_lanes =3D drm_of_get_data_lanes_count(endpoint, 1, 4); if (dsi_lanes < 0) return dsi_lanes; // <-- leaks endpoint refcount ``` The `of_node_put(endpoint)` at line 1831 is never reached if an early retur= n fires. This needs a cleanup path or use of `__free` cleanup attributes. 3. **Missing locking in infoframe write/clear helpers**: `it6162_write_info= frame` and `it6162_clear_infoframe` call `it6162_infoblock_trigger` and do = regmap writes, but do **not** take `it6162->lock`. All other callers of `it= 6162_infoblock_trigger` take the mutex. These are called from the bridge HD= MI infoframe callbacks which can race with interrupt-driven activity: ```c static inline int it6162_write_infoframe(struct it6162 *it6162, const u8 *buffer, size_t len) { // No lock taken! regmap_bulk_write(it6162->regmap, OFFSET_DATA_BUFFER, buffer, len); regmap_write(it6162->regmap, OFFSET_DATA_TYPE_IDX, len); it6162_infoblock_trigger(it6162, HOST_SET_CEA_INFOFRAME); ... } ``` 4. **Chip ID byte order appears reversed** in `it6162_detect_devices`: ```c regmap_bulk_read(it6162->regmap, OFFSET_CHIP_ID_L, &buf[0], 6); chip_id =3D (buf[0] << 16) | (buf[1] << 8) | (buf[2]); ``` Register 0x00 is `OFFSET_CHIP_ID_L` (low byte), 0x01 is `_M` (mid), 0x02 is= `_H` (high). So `buf[0]` is the low byte, but the code puts it in the most= significant position. This should be: ```c chip_id =3D (buf[2] << 16) | (buf[1] << 8) | buf[0]; ``` Same issue for `version`. (Unless the expected `chip_info` value is byte-sw= apped to match, in which case this is just confusing and should be document= ed.) 5. **Regulator error handling in `it6162_platform_set_power`** =E2=80=94 if= enabling `pwr1833` fails, `ivdd` is left enabled. If enabling `ovdd` fails= , both `ivdd` and `pwr1833` are left enabled: ```c err =3D regulator_enable(it6162->ivdd); if (err) return err; err =3D regulator_enable(it6162->pwr1833); if (err) return err; // ivdd still enabled err =3D regulator_enable(it6162->ovdd); if (err) return err; // ivdd and pwr1833 still enabled ``` Use goto-based unwinding to disable previously enabled regulators on failur= e. **Medium:** 6. **Missing `DRM_BRIDGE_OP_HPD`**: The interrupt handler calls `drm_bridge= _hpd_notify()`, but the bridge ops don't include `DRM_BRIDGE_OP_HPD`: ```c it6162->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_MODES; ``` Without `DRM_BRIDGE_OP_HPD`, the connector infrastructure won't enable HPD = polling/notification support, making the IRQ-based HPD path likely non-func= tional. 7. **`it6162_poweroff` is never called**: The function is defined with `__m= aybe_unused` but is genuinely never called anywhere =E2=80=94 not from `rem= ove`, not from any disable path. This means once powered on, the chip is ne= ver powered down. The `remove` callback only disables the IRQ and cancels t= he work, but doesn't power down: ```c static void it6162_remove(struct i2c_client *client) { struct it6162 *it6162 =3D i2c_get_clientdata(client); disable_irq(client->irq); cancel_delayed_work_sync(&it6162->hdcp_work); mutex_destroy(&it6162->lock); } ``` 8. **Double power-on**: `it6162_detect_devices` calls `it6162_platform_set_= power`, then `it6162_attach_dsi` calls `it6162_poweron` which calls `it6162= _platform_set_power` again. The regulators get `enable` called twice withou= t a matching `disable` in between (unless reference counting saves it, but = the GPIO reset also fires twice). 9. **`__maybe_unused` on `it6162_poweron`**: This function IS used (called = from `it6162_attach_dsi`), so the annotation is wrong. 10. **`it6162_wait_command_complete` error conflation**: The function retur= ns either a negative error (I2C read failure) or the register value. When u= sed with `readx_poll_timeout(..., val, val <=3D 0, ...)`, an I2C read error= (negative value) satisfies the poll condition, but is then treated as time= out rather than a bus error: ```c if (status < 0 || val !=3D 0) { dev_err(dev, "infoblock command timeout: status=3D%d val=3D%d", status, val); return -ETIMEDOUT; } ``` An I2C failure should return the actual error, not `-ETIMEDOUT`. **Low / Style:** 11. **`!!GET_BUFFER_STATUS(int_status)`** =E2=80=94 the double negation is = unnecessary, a simple `GET_BUFFER_STATUS(int_status) !=3D 0` or just `GET_B= UFFER_STATUS(int_status)` would be clearer. 12. **`CACHE_TYPE_NONE`** =E2=80=94 for a register map that's mostly write-= then-poll, this is fine, but worth noting. 13. **`i2c_device_id` table** =E2=80=94 the `{ "it6162", 0 }` style with ex= plicit zero is slightly outdated; newer drivers use `{ "it6162" }`, but thi= s is cosmetic. 14. **Copyright year 2024** for a driver first submitted in 2026 =E2=80=94 = should probably be 2024-2026 or just 2026. --- Generated by Claude Code Patch Reviewer