Claude review: drm/bridge: ite-it66121: Switch to the HDMI connector helpers

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

Patch Review

**Build error — undefined symbol:**

The `it66121_bridge_hdmi_tmds_char_rate_valid()` function uses `HDMI_TMDS_CHAR_RATE_MIN_HZ`:
```c
if (tmds_rate < HDMI_TMDS_CHAR_RATE_MIN_HZ)
    return MODE_CLOCK_LOW;
```
This macro is not defined anywhere in the kernel tree. The original code used `mode->clock < 25000` (25 MHz in kHz). This needs to be either defined locally (e.g., `#define HDMI_TMDS_CHAR_RATE_MIN_HZ 25000000ULL`) or replaced with a literal `25000000ULL`. The sister driver `ite-it6263.c` defines its own `MAX_HDMI_TMDS_CHAR_RATE_HZ` locally, suggesting local definition is the expected pattern.

**VSIF write — no length bounds check:**

The VSIF write function writes the full buffer directly to registers starting at `IT66121_PKT_NULL_HB(0)` (0x138):
```c
ret = regmap_bulk_write(ctx->regmap, IT66121_PKT_NULL_HB(0), buffer, len);
```
There is no validation that `len` fits within the available null packet register space. The AVI infoframe write is careful to write exactly `HDMI_AVI_INFOFRAME_SIZE` bytes, and the audio infoframe uses `min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5)` to clamp. The VSIF write should have similar protection, or at least a comment documenting the maximum supported size. In practice the framework sends well-formed packets, so this is a minor robustness concern rather than a functional bug.

**Locking asymmetry between clear and write callbacks:**

The `write_*` callbacks all acquire `ctx->lock`, but the `clear_*` callbacks do not:
```c
static int it66121_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridge)
{
    struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
    return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0);
}
```
Since these are all called from the serialized atomic commit path, this is safe in practice. However, the inconsistency is worth noting — if `ctx->lock` protects register access, the clears should arguably also take it for defensive consistency.

**Rate conversion correctness:**

The max rate conversion from kHz to Hz is correct:
- Old: `max_clock = (ctx->bus_width == 12) ? 74250 : 148500` (kHz, compared against `mode->clock`)
- New: `max_rate = (ctx->bus_width == 12) ? 74250000ULL : 148500000ULL` (Hz, compared against `tmds_rate`)

**Audio infoframe data clamped to 5 bytes:**
```c
min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5)
```
This is correct for the IT66121 register layout where `IT66121_AUD_INFO_DB1_REG` (0x168) through 0x16C provides 5 data registers, with the checksum at 0x16D.

**Bridge configuration in probe looks correct:**
```c
ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
                  DRM_BRIDGE_OP_HDMI;
ctx->bridge.vendor = "ITE";
ctx->bridge.product = "IT66121";
```
Adding `DRM_BRIDGE_OP_HDMI` and the vendor/product strings are required for the HDMI helper framework.

**Calling `drm_atomic_helper_connector_hdmi_update_audio_infoframe` outside the lock in audio_prepare:**
```c
out:
    mutex_unlock(&ctx->lock);

    if (!ret)
        ret = drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
                                                                      &params->cea);
    return ret;
```
This is correct — the helper will call back into `hdmi_write_audio_infoframe` which acquires the lock itself, so calling it outside the outer lock avoids deadlock.

---
Generated by Claude Code Patch Reviewer