From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver
Date: Sat, 14 Mar 2026 07:20:46 +1000
Message-ID: <review-patch2-20260313-it61620-0714-v7-2-36a16dc036d6@ite.com.tw>
In-Reply-To: <20260313-it61620-0714-v7-2-36a16dc036d6@ite.com.tw>
References: <20260313-it61620-0714-v7-0-36a16dc036d6@ite.com.tw> <20260313-it61620-0714-v7-2-36a16dc036d6@ite.com.tw>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**CRITICAL BUG - Interrupt status checks use register address instead of read value:**

In `it61620_hdmi_interrupt_handler()`, lines 2173-2180 compare the **register address constants** (`TX_REG_INT1`) against the interrupt bit masks instead of the **read values** (`int_status1`):

```c
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);
```

`TX_REG_INT1` is defined as `0x10`, so `TX_REG_INT1 & B_HPD_CHG` (BIT(0)) is always 0 (false), meaning the HPD handler is **never called**. `TX_REG_INT1 & B_RXSEN_CHANGE` (BIT(2)) is always 0 (false). `TX_REG_INT1 & B_INT_AUTH_F` (BIT(3)) is always 0 (false). None of these interrupt handlers will ever fire. The third check for `int_status3` at line 2182 is correct. These must be `int_status1` not `TX_REG_INT1`.

**Medium Issues:**

1. **`sizeof(hdmi_afe)` used incorrectly as array element count** (line 1729):
   ```c
   for (i = 0; i < sizeof(hdmi_afe); i++) {
   ```
   `sizeof(hdmi_afe)` gives the total byte size of the array (which is very large since each element contains a 24-element int array), not the element count (4). This should be `ARRAY_SIZE(hdmi_afe)`. In practice, the `break` will fire before the bogus limit is reached, but `i` will index out of bounds if `clock` is larger than all entries' `clock` values without hitting the `clock == 0` sentinel.

2. **`it61620_hdmi_enable_hdcp()` returns `bool` but declared as `int`** (lines 1941, 1949, 1982, 2003, 2009):
   ```c
   static int it61620_hdmi_enable_hdcp(struct it61620 *it61620)
   ...
       return false;
   ...
       return true;
   ```
   Similarly, `it61620_hdmi_start_hdcp()` (line 2054) is declared `int` but returns `false`/`true`. Pick one type and be consistent.

3. **Vendor infoframe write loop is wrong** (line 2268):
   ```c
   for (i = 3; i < len - 3; i++)
       regmap_write(it61620->tx_regmap, TX_REG_VSIFPKT_PB00 + i, ptr[i]);
   ```
   `ptr` is set to `buffer + 3`, then indexed with `i` starting at 3, so the actual access starts at `buffer[6]` (skipping 3 payload bytes). The loop index should start at 0, or `ptr` should be `buffer`. This likely results in a broken vendor-specific infoframe.

4. **`it61620_pm_bridge_suspend`/`resume` marked `__maybe_unused` but used with `DEFINE_RUNTIME_DEV_PM_OPS`** (lines 2827, 2836):
   The `__maybe_unused` attribute is unnecessary with `DEFINE_RUNTIME_DEV_PM_OPS` and the `inline` qualifier on PM functions is also questionable since they're used as function pointers.

5. **`it61620_parse_dt` returns `unsigned int` but can return negative error codes** (line 2778):
   ```c
   static unsigned int it61620_parse_dt(struct it61620 *it61620)
   ```
   Should return `int`.

**Minor Issues:**

6. **Typo "Intelaced"** (line 1431):
   ```c
   drm_dbg_kms(drm, "Intelaced\n");
   ```
   Should be "Interlaced".

7. **Typo "ststus"** (line 1635):
   ```c
   drm_dbg(drm, "tx ddc ststus %02X\n", val);
   ```
   Should be "status".

8. **Debug message says "ksv valid!" when KSV is actually invalid** (line 2076):
   ```c
   if (!retcheck) {
       drm_dbg(drm, "ksv valid!\n");
       return false;
   }
   ```
   The message should say "ksv invalid" since the function is returning failure.

9. **Bool initialization with `0` instead of `false`** (line 1451):
   ```c
   bool hpol_high = 0, vpol_high = 0;
   ```
   Use `false`.

10. **Hardcoded I2C register `0x7e`** (line 2067):
    ```c
    regmap_read(it61620->tx_regmap, 0x7e, &rx_hdmi_mode);
    ```
    Should use the already-defined `TX_REG_HDCP_BSTS_H` macro (which is `0x7E`), or define a new named constant if the semantics differ.

11. **`it61620_detach_dsi` calls `mipi_dsi_detach` after `devm_mipi_dsi_attach`** (lines 2770-2776):
    Since `devm_mipi_dsi_attach` was used in `it61620_attach_dsi`, the detach is managed by devres. Calling `mipi_dsi_detach` manually in `it61620_remove` will result in a double-detach.

12. **Missing `default` cases in switch statements** (lines 2594-2616, 2618-2628, 2668-2685):
    The `sample_rate`, `sample_width`, and `channels` switch statements have no `default` cases, meaning local variables could be used uninitialized if an unexpected value is passed.

13. **The `it61620_write16` helper** writes timing register pairs to `mipirx_regmap` in the A0 PG timing block (lines 1511-1520), but the `TX_REG_PG_*` constants are in the TX register space (0x150+). Writing TX register addresses to the MIPI regmap will likely write to wrong registers. Compare with line 1522 which correctly uses `tx_regmap` for `TX_REG_PG_POL`.

---
Generated by Claude Code Patch Reviewer