Claude review: drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

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

Patch Review

**Critical / Functional Issues:**

1. **`lt7911exc_write_data` does not advance the address for `prog_init`** — the `addr` parameter passed into `lt7911exc_prog_init` is always `0` (the initial value from the caller). Only the local `addr` at the bottom of the loop is incremented. But `lt7911exc_prog_init` receives the function parameter `addr`, not a local tracking variable:

```c
static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct firmware *fw, u64 addr)
{
    ...
    for (num = 0; num < page; num++) {
        offset = num * LT_PAGE_SIZE;
        ...
        ret = lt7911exc_prog_init(lt7911exc, addr);
        ...
        addr += LT_PAGE_SIZE;
    }
```

Wait — re-reading more carefully, `addr` *is* the function parameter and it is modified in the loop (`addr += LT_PAGE_SIZE`). So this is actually correct since C passes by value and `addr` is used as a local accumulator. My mistake — this is fine.

2. **Goto jumps skip `out_unlock` label on early exits** — In `lt7911exc_firmware_upgrade_work`, the `goto out_release_fw` path (firmware too large) and `goto out_clear_status` (request_firmware failure) both skip the `out_unlock` label. This is correct since `upgrade_lock` is only acquired after those checks. However, the `buffer` allocation failure path (`goto out_release_fw`) also skips `out_clear_status` — actually no, the labels are ordered so `out_release_fw` falls through to `out_clear_status`. Let me re-examine:

```c
out_unlock:
    ...
    mutex_unlock(&lt7911exc->upgrade_lock);
    if (lt7911exc->bridge.dev)
        drm_kms_helper_hotplug_event(lt7911exc->bridge.dev);

out_release_fw:
    release_firmware(fw);

out_clear_status:
    mutex_lock(&lt7911exc->ocm_lock);
    if (!lt7911exc->removed)
        lt7911exc->upgrade = false;
    mutex_unlock(&lt7911exc->ocm_lock);
```

This is correct — the labels cascade. Good.

3. **`kvfree(buffer)` is called before the firmware data is fully consumed** — The buffer is freed immediately after computing the CRC, but then `lt7911exc_write_data(lt7911exc, fw, 0)` uses `fw->data` directly, not the padded buffer. This means the CRC is computed over the padded data (firmware + 0xff padding to `FW_SIZE - 4`), but only the raw firmware bytes are written to flash. The hardware CRC verification in `lt7911exc_upgrade_result` reads back what was written. **If the flash does not already contain 0xff in the padded region, the CRC will mismatch.** This is probably fine because `lt7911exc_block_erase` erases the entire block first (erased flash is typically 0xff), but the logic is fragile and implicit. Worth a comment at minimum.

**Medium Issues:**

4. **Unchecked `regmap_write` return values in multiple places** — Several functions call `regmap_write` without checking the return value:

    - `lt7911exc_write_crc`: First four `regmap_write` calls and last three are unchecked.
    - `lt7911exc_upgrade_result`: `regmap_write` calls to 0xe0ee, 0xe07b are unchecked.
    - `lt7911exc_write_data`: The `regmap_write` calls for the short-page handling and the final `0xe05f` write are unchecked.

    While regmap-over-I2C errors are rare, silently continuing after a failed register write during firmware programming could corrupt the flash.

5. **`lt7911exc_write_data` uses `u64` for sizes/addresses but firmware is max 64KB** — The `addr` parameter and `size`/`offset` locals are all `u64`, but `FW_SIZE` is `64 * 1024`. Using `u32` or even `unsigned int` would be more appropriate and avoids confusion. Similarly, `lt7911exc_prog_init` takes `u64 addr`. The register writes only use the low 24 bits anyway (`(addr >> 16) & 0xff` etc.), so a `u64` is misleading.

6. **`lt7911exc_read_version` returns a negative error code or a positive version packed into `int`** — The version is `(buf[0] << 16) | (buf[1] << 8) | buf[2]`, which could set bit 23 if `buf[0] >= 0x80`, producing a negative value indistinguishable from an error code. This would cause a spurious probe failure. Consider using `unsigned int` for the version, or masking to ensure positivity.

7. **`dev_set_drvdata` and `i2c_set_clientdata` are both called in probe** — For an I2C driver, `dev_set_drvdata(&client->dev, ...)` and `i2c_set_clientdata(client, ...)` are the same thing. One of them is redundant. The remove path uses `i2c_get_clientdata`, so keep that one and drop `dev_set_drvdata`. (Actually, the sysfs show/store callbacks use `dev_get_drvdata` on the I2C client's dev, so they are equivalent — but having both is confusing.)

8. **Kconfig help text formatting** — Missing space after period:

```
chip.The LT7911EXC converts eDP input to MIPI
```

Should be `chip. The LT7911EXC...`.

**Minor / Style Issues:**

9. **Comment style** — A couple of `//` (C++) style comments remain:

```c
//enable mipi stream
//disable mipi stream
//hardware requires delay
```

The kernel coding style requires `/* ... */` comments. These should be converted.

10. **`lt7911exc_prog_init` has a redundant return pattern:**

```c
ret = regmap_multi_reg_write(...);
if (ret)
    return ret;

return 0;
```

This can be simply `return regmap_multi_reg_write(...)`.

11. **`struct device_node *np` usage in probe** — `np` is only used for the NULL check and assigning `bridge.of_node`. Since this is an OF-dependent driver (`depends on OF`), the `if (!np) return -ENODEV` check is redundant. And `bridge.of_node = np` could be `bridge.of_node = dev->of_node` directly, eliminating the variable.

12. **The `lt7911exc_dsi_host_transfer` accepted message types have no indentation in the switch body:**

```c
case MIPI_DSI_DCS_SHORT_WRITE:
case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
...
break;
default:
return -EOPNOTSUPP;
```

The `break` and the `default` body should be indented one level under their case labels per kernel coding style.

13. **The `cal_crc32_custom` function name** — "cal" is an unusual abbreviation. Consider `lt7911exc_calc_crc32` to be consistent with driver namespace and clearer naming.

14. **`platform_device.h` is included but not used** — This is an I2C driver, not a platform driver. The include should be removed.

**Summary:** The driver is close but needs attention to unchecked regmap writes in the firmware programming path, the `u64`/`u32` type confusion, the version-sign issue, and the coding style issues (C++ comments, switch indentation, Kconfig typo). The overall locking design is workable but complex for a v12 submission — it would benefit from clearer documentation of which lock protects what invariant.

---
Generated by Claude Code Patch Reviewer