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

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

Patch Review

**Critical / Functional Issues:**

1. **`DRM_BRIDGE_OP_MODES` set without `get_modes` callback** (line 1572 in driver, corresponding to mbox line ~1783):
   ```c
   it6162->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
                        DRM_BRIDGE_OP_MODES;
   ```
   The `DRM_BRIDGE_OP_MODES` flag requires implementing the `get_modes` callback per the bridge documentation (`include/drm/drm_bridge.h:1027`). The driver doesn't implement `get_modes` in `it6162_bridge_funcs`. This flag should either be removed (since `DRM_BRIDGE_OP_EDID` is sufficient to enumerate modes from EDID), or a `get_modes` callback should be added.

2. **`DRM_BRIDGE_OP_HPD` not set**: The driver calls `drm_bridge_hpd_notify()` from the interrupt handler (line 694) but doesn't set `DRM_BRIDGE_OP_HPD` in `bridge.ops`. The HPD notification path may not work correctly without this flag, as the connector infrastructure checks for it.

3. **`it6162_poweroff` is dead code** (line 842):
   ```c
   static int __maybe_unused it6162_poweroff(struct it6162 *it6162)
   ```
   `it6162_poweroff` is never called anywhere. The `__maybe_unused` annotation hides the warning, but the driver has no power-off path — not in `remove`, not in any PM ops, nowhere. This means once powered on, the chip is never cleanly powered down. The `remove` callback only disables the IRQ and cancels the HDCP work.

4. **`it6162_platform_set_power` missing regulator cleanup on error** (lines 753-776): If enabling `pwr1833` fails, `ivdd` is left enabled. If enabling `ovdd` fails, both `ivdd` and `pwr1833` are left enabled. Should use `goto` error unwind or `regulator_bulk_*` APIs.

5. **`it6162_detect_devices` powers on but never powers off** (lines 799-822): This is called during probe to verify the chip ID. It calls `it6162_platform_set_power()` but if the chip ID matches, it returns 0 without powering back down. Later, `it6162_poweron()` in `it6162_attach_dsi` calls `it6162_platform_set_power()` again, enabling the regulators a second time (bumping their reference counts), which means a single disable won't actually turn them off.

**Moderate Issues:**

6. **`it6162_bridge_hdmi_audio_prepare` ignores error from `it6162_audio_update_hw_params`** (lines 1388-1393):
   ```c
   it6162_audio_update_hw_params(it6162, &config, fmt, params);
   it6162_enable_audio(it6162, &config);
   ```
   The return value of `it6162_audio_update_hw_params` is discarded. If it returns `-EINVAL` (unsupported rate/width/format), the driver proceeds with uninitialized `config` fields.

7. **`it6162_display_mode_to_settings` uses conditional sets without zeroing first** (lines 1208-1230): The function checks `DRM_MODE_FLAG_PHSYNC` to set `hpol=1` and `DRM_MODE_FLAG_NVSYNC` to set `vpol=1`, but never sets them to 0 for the opposite case. The `video_setting` struct on the stack in `it6162_bridge_atomic_enable` is not zeroed (no `= {}` or `memset`). The `prog` field is never set at all — it defaults to whatever garbage is on the stack, but should be 1 for progressive.

8. **`it6162_write_infoframe` and `it6162_clear_infoframe` not holding the lock** (lines 1422-1441): These functions write to the regmap and call `it6162_infoblock_trigger` but don't take `it6162->lock`, unlike similar functions (`it6162_enable_audio`, `it6162_mipi_enable`, etc.). This risks racing with other infoblock operations.

9. **`!!GET_BUFFER_STATUS(int_status)` in interrupt handler** (line 672):
   ```c
   if (!!GET_BUFFER_STATUS(int_status)) {
   ```
   The double negation is unnecessary. Just `if (GET_BUFFER_STATUS(int_status))` is clearer.

10. **`regmap_read` return values mostly unchecked**: Many `regmap_read` calls have their return value ignored (e.g., in `it6162_tx_hdcp_enable` line 380, `it6162_hdcp_handler` line 618, `it6162_detect` line 883). While regmap errors are relatively uncommon on I2C, inconsistent error checking is poor practice — the driver checks in some places but not others.

**Minor / Style Issues:**

11. **`video/videomode.h` included but unused**: The header `<video/videomode.h>` is included at line 26 but no videomode types or functions are used in the driver.

12. **`drm_bridge_connector.h` included but unused**: `<drm/drm_bridge_connector.h>` is included but the driver doesn't use `drm_bridge_connector_*` APIs directly.

13. **`of_irq.h` included but unused**: `<linux/of_irq.h>` is included but not needed; IRQ is obtained via `client->irq`.

14. **`pm_runtime.h` included but unused**: `<linux/pm_runtime.h>` is included but no PM runtime APIs are used.

15. **`it6162_i2c_regmap_init` is `inline` unnecessarily** (line 275): This function is called once from probe; `inline` is pointless and the compiler will decide on its own.

16. **`it6162_avi_to_video_setting` and `it6162_display_mode_to_settings` are `inline` unnecessarily** (lines 1174, 1206): Same as above — these are called once from `atomic_enable`.

17. **Chip ID read byte ordering** (lines 815-816):
    ```c
    chip_id = (buf[0] << 16) | (buf[1] << 8) | (buf[2]);
    ```
    The register names suggest `buf[0]` is `CHIP_ID_L` (low byte) and `buf[2]` is `CHIP_ID_H` (high byte), but the code places `buf[0]` in the MSB position. This is either an endianness confusion or the register naming is non-standard. Either way, a comment would help. Same for `version`.

18. **`content_protection` field stored but never read back**: The driver stores `conn_state->content_protection` in `it6162->content_protection` (line 1256) but never reads it.

19. **Kconfig help text style**: The help text (lines 185-189) is a bit terse and inconsistent with the capitalization of "iTE" vs "ITE" used elsewhere.

---
Generated by Claude Code Patch Reviewer