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

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

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_atomic_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`, and `prog`:

```c
if (mode->flags & DRM_MODE_FLAG_PHSYNC)
    settings->hpol = 1;
if (mode->flags & DRM_MODE_FLAG_NVSYNC)
    settings->vpol = 1;
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
    settings->prog = 0;
```

If none of those flags are set, these fields contain stack garbage. The struct should be zero-initialized: `struct it6162_video_settings video_setting = {};` or use `memset`. The `prog` field is particularly problematic — for progressive modes (the common case), it's never explicitly set to 1.

2. **Device-tree node reference leak** in `it6162_load_mipi_pars_from_port`:

```c
endpoint = of_graph_get_endpoint_by_regs(of, port, -1);
if (!endpoint)
    return 0;

dsi_lanes = 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 return fires. This needs a cleanup path or use of `__free` cleanup attributes.

3. **Missing locking in infoframe write/clear helpers**: `it6162_write_infoframe` and `it6162_clear_infoframe` call `it6162_infoblock_trigger` and do regmap writes, but do **not** take `it6162->lock`. All other callers of `it6162_infoblock_trigger` take the mutex. These are called from the bridge HDMI 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 = (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 = (buf[2] << 16) | (buf[1] << 8) | buf[0];
```
Same issue for `version`. (Unless the expected `chip_info` value is byte-swapped to match, in which case this is just confusing and should be documented.)

5. **Regulator error handling in `it6162_platform_set_power`** — if enabling `pwr1833` fails, `ivdd` is left enabled. If enabling `ovdd` fails, both `ivdd` and `pwr1833` are left enabled:

```c
err = regulator_enable(it6162->ivdd);
if (err)
    return err;
err = regulator_enable(it6162->pwr1833);
if (err)
    return err;  // ivdd still enabled
err = regulator_enable(it6162->ovdd);
if (err)
    return err;  // ivdd and pwr1833 still enabled
```

Use goto-based unwinding to disable previously enabled regulators on failure.

**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 = 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-functional.

7. **`it6162_poweroff` is never called**: The function is defined with `__maybe_unused` but is genuinely never called anywhere — not from `remove`, not from any disable path. This means once powered on, the chip is never powered down. The `remove` callback only disables the IRQ and cancels the work, but doesn't power down:

```c
static void it6162_remove(struct i2c_client *client)
{
    struct it6162 *it6162 = 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 without 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 returns either a negative error (I2C read failure) or the register value. When used with `readx_poll_timeout(..., val, val <= 0, ...)`, an I2C read error (negative value) satisfies the poll condition, but is then treated as timeout rather than a bus error:

```c
if (status < 0 || val != 0) {
    dev_err(dev, "infoblock command timeout: status=%d val=%d",
            status, val);
    return -ETIMEDOUT;
}
```

An I2C failure should return the actual error, not `-ETIMEDOUT`.

**Low / Style:**

11. **`!!GET_BUFFER_STATUS(int_status)`** — the double negation is unnecessary, a simple `GET_BUFFER_STATUS(int_status) != 0` or just `GET_BUFFER_STATUS(int_status)` would be clearer.

12. **`CACHE_TYPE_NONE`** — for a register map that's mostly write-then-poll, this is fine, but worth noting.

13. **`i2c_device_id` table** — the `{ "it6162", 0 }` style with explicit zero is slightly outdated; newer drivers use `{ "it6162" }`, but this is cosmetic.

14. **Copyright year 2024** for a driver first submitted in 2026 — should probably be 2024-2026 or just 2026.

---
Generated by Claude Code Patch Reviewer