Claude review: drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**CRITICAL BUG: Interrupt status checks use register addresses instead of read values**

In `it61620_hdmi_interrupt_handler()`, the status register values are read into local variables but then never actually used for the first three checks:

```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) against bit masks, not the actual read value `int_status1`. Since `TX_REG_INT1 = 0x10`, and `B_HPD_CHG = BIT(0)`, `B_RXSEN_CHANGE = BIT(2)`, `B_INT_AUTH_F = BIT(3)` — the expression `0x10 & BIT(0)` is 0, so the HPD handler **never fires**. Meanwhile `0x10 & BIT(4)` would be true, which happens to be `B_INT_AUTH_D` — so the auth-fail handler fires unconditionally. 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 = 0; i < sizeof(hdmi_afe); i++) {
```

`sizeof(hdmi_afe)` returns the total size in bytes of the entire array (4 × 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) != 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 `it61620_hdmi_start_hdcp()` also returns `int` but uses `return false;` and `return it61620_hdmi_enable_hdcp(...)`. The semantics work by accident (true=1, false=0) but the types should be consistent — either use `int` with 0/-errno or `bool`.

**Issue: `it61620_hdmi_start_hdcp` returns `false` with misleading debug message**

```c
+	retcheck = 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 valid!" — 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 = buffer + 3;
+	for (i = 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 = 3` and indexes `ptr[i]`, which means it actually starts reading at `buffer[6]`, skipping 3 bytes of payload. The loop should likely start at `i = 0` (like the SPD infoframe handler does), or `ptr` should just be `buffer`.

**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 *dev)
```

`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 detach should be handled automatically by devres. The explicit `it61620_detach_dsi()` 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 IT61620 is a variant of IT6162, using `it61620_chip_info` would be more consistent with the rest of the driver naming.

**Nit: Magic numbers**

Many register writes use raw hex values without documenting what the bits mean (e.g., `regmap_write(it61620->tx_regmap, TX_REG_CR_1_CTRL, 0x09)`). This is common in bridge drivers with vendor-provided init sequences, but makes the code hard to review and maintain.

---
Generated by Claude Code Patch Reviewer