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 IT61620 MIPI DSI to HDMI bridge driver Date: Sun, 22 Mar 2026 03:53:20 +1000 Message-ID: In-Reply-To: <20260320-it61620-0714-v8-2-0e70271cf5a9@ite.com.tw> References: <20260320-it61620-0714-v8-0-0e70271cf5a9@ite.com.tw> <20260320-it61620-0714-v8-2-0e70271cf5a9@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 BUG: Interrupt status checks use register addresses instead of r= ead values** In `it61620_hdmi_interrupt_handler()`, the status register values are read = into local variables but then never actually used for the first three check= s: ```c + regmap_read(it61620->tx_regmap, TX_REG_INT1, &int_status1); + ... + if (TX_REG_INT1 & B_HPD_CHG) + it61620_hdmi_irq_hpd(it61620); + + if (TX_REG_INT1 & B_RXSEN_CHANGE) + it61620_hdmi_irq_rxsen_chg(it61620); + + if (TX_REG_INT1 & B_INT_AUTH_F) + it61620_hdmi_irq_hdcp_auth_fail(it61620); ``` These compare the **register address constant** `TX_REG_INT1` (0x10) agains= t bit masks, not the actual read value `int_status1`. Since `TX_REG_INT1 = =3D 0x10`, and `B_HPD_CHG =3D BIT(0)`, `B_RXSEN_CHANGE =3D BIT(2)`, `B_INT_= AUTH_F =3D BIT(3)` =E2=80=94 the expression `0x10 & BIT(0)` is 0, so the HP= D handler **never fires**. Meanwhile `0x10 & BIT(4)` would be true, which h= appens to be `B_INT_AUTH_D` =E2=80=94 so the auth-fail handler fires uncond= itionally. This is clearly wrong and must be `int_status1 & B_HPD_CHG` etc.= Note that the `int_status3` check on the last condition is correct. **BUG: `sizeof(hdmi_afe)` iterates over bytes, not array entries** ```c + for (i =3D 0; i < sizeof(hdmi_afe); i++) { ``` `sizeof(hdmi_afe)` returns the total size in bytes of the entire array (4 = =C3=97 sizeof(struct it61620_hdmi_afe_setting)), not the number of elements= . This should be `ARRAY_SIZE(hdmi_afe)`. **BUG: `it61620_hdmi_enable_hdcp` returns `bool` but declared as `int`** ```c +static int it61620_hdmi_enable_hdcp(struct it61620 *it61620) +{ + ... + if ((sts & B_TMDS_STABLE) !=3D B_TMDS_STABLE) { + drm_dbg(drm, "TMDS not stable, stop hdcp %x", sts); + return false; + } + ... + return true; ``` Returns `true`/`false` but is declared as returning `int`. The caller `it61= 620_hdmi_start_hdcp()` also returns `int` but uses `return false;` and `ret= urn it61620_hdmi_enable_hdcp(...)`. The semantics work by accident (true=3D= 1, false=3D0) but the types should be consistent =E2=80=94 either use `int`= with 0/-errno or `bool`. **Issue: `it61620_hdmi_start_hdcp` returns `false` with misleading debug me= ssage** ```c + retcheck =3D it61620_hdmi_hdcp_is_ksv_valid(bksv); + if (!retcheck) { + drm_dbg(drm, "ksv valid!\n"); + return false; + } ``` When `retcheck` is false (KSV is NOT valid), the debug message says "ksv va= lid!" =E2=80=94 this is misleading. It should say "ksv invalid" or similar. **Issue: Vendor infoframe write loop appears incorrect** ```c +static void it61620_hdmi_vendor_infoframe_set(struct it61620 *it61620, + const u8 *buffer, size_t len) +{ + ... + ptr =3D buffer + 3; + for (i =3D 3; i < len - 3; i++) + regmap_write(it61620->tx_regmap, TX_REG_VSIFPKT_PB00 + i, ptr[i]); ``` `ptr` is already offset by 3 from `buffer`, but then the loop starts at `i = =3D 3` and indexes `ptr[i]`, which means it actually starts reading at `buf= fer[6]`, skipping 3 bytes of payload. The loop should likely start at `i = =3D 0` (like the SPD infoframe handler does), or `ptr` should just be `buff= er`. **Issue: `__maybe_unused` with `DEFINE_RUNTIME_DEV_PM_OPS`** ```c +static inline int __maybe_unused it61620_pm_bridge_suspend(struct device *= dev) +... +static inline int __maybe_unused it61620_pm_bridge_resume(struct device *d= ev) ``` `DEFINE_RUNTIME_DEV_PM_OPS` already handles the `__maybe_unused` case via `= pm_ptr()`. The `__maybe_unused` annotations and `inline` on these functions= are unnecessary. **Minor: `it61620_parse_dt` returns `unsigned int` but can return negative = error** ```c +static unsigned int it61620_parse_dt(struct it61620 *it61620) +{ + ... + return -EINVAL; ``` The return type should be `int`, not `unsigned int`. **Minor: Typo in debug message** ```c + drm_dbg(drm, "tx ddc ststus %02X\n", val); ``` "ststus" should be "status". **Minor: Typo in debug output** ```c + if (mode->flags & DRM_MODE_FLAG_INTERLACE) + drm_dbg_kms(drm, "Intelaced\n"); ``` "Intelaced" should be "Interlaced". **Minor: `it61620_detach_dsi` is called in remove but `devm_mipi_dsi_attach= ` should handle cleanup** Since `devm_mipi_dsi_attach()` is used in `it61620_attach_dsi()`, the detac= h should be handled automatically by devres. The explicit `it61620_detach_d= si()` call in `it61620_remove()` and the function itself appear unnecessary= and could lead to a double-detach. **Nit: Inconsistent naming** The struct is named `it6162_chip_info` but this is for the IT61620. While I= T61620 is a variant of IT6162, using `it61620_chip_info` would be more cons= istent with the rest of the driver naming. **Nit: Magic numbers** Many register writes use raw hex values without documenting what the bits m= ean (e.g., `regmap_write(it61620->tx_regmap, TX_REG_CR_1_CTRL, 0x09)`). Thi= s is common in bridge drivers with vendor-provided init sequences, but make= s the code hard to review and maintain. --- Generated by Claude Code Patch Reviewer