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

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

Patch Review

**Issues (by severity):**

#### 1. Firmware data / CRC mismatch (bug)

The CRC is computed on a zero-padded copy of the firmware:

```c
buffer = kmalloc(total_size, GFP_KERNEL);
...
memset(buffer, 0xff, total_size);
memcpy(buffer, fw->data, fw->size);
crc32 = cal_crc32_custom(buffer, total_size);
kfree(buffer);
```

But `lt7911exc_write_data` writes from the original `fw`, not from `buffer`:

```c
ret = lt7911exc_write_data(lt7911exc, fw, 0);
```

Inside `lt7911exc_write_data`, only `fw->size` bytes are written. The flash region from `fw->size` to `total_size` will contain whatever was there after the erase (typically 0xff from a flash erase, which matches the `memset` padding). So this _might_ work in practice if the erase sets bytes to 0xff, but it's fragile — the correctness depends on an implicit assumption about the erase pattern. Either write the padded `buffer` instead of `fw->data`, or compute the CRC only over `fw->size` bytes. As written, this is a latent correctness bug.

#### 2. `regmap_raw_write` vs `regmap_noinc_write` for firmware FIFO writes

```c
ret = regmap_raw_write(lt7911exc->regmap, 0xe05d, &data[offset], page_len);
```

Register `0xe05d` appears to be a FIFO data port (all page bytes go to the same address). The sister driver `lontium-lt9611uxc.c` uses `regmap_noinc_write` for the same pattern. `regmap_raw_write` will auto-increment the register address, which is incorrect for a FIFO. This should be `regmap_noinc_write`. The same issue applies to `lt7911exc_write_crc`:

```c
ret = regmap_raw_write(lt7911exc->regmap, 0xe05d, crc, 4);
```

#### 3. `lt7911exc_prog_init` uses `u64` for a 24-bit address

```c
static int lt7911exc_prog_init(struct lt7911exc *lt7911exc, u64 addr)
```

The address is masked to 24 bits (`addr >> 16 & 0xff`, etc.), so `u32` is sufficient and more appropriate. Similarly, `lt7911exc_write_data` and `lt7911exc_write_crc` use `u64 addr` — all should be `u32`.

#### 4. `lt7911exc_write_data` takes `const struct firmware *` but only needs `data`/`size`

The function signature couples it to the firmware API, yet after the CRC computation the padded buffer is freed and lost. If the intent is to always write the padded image (see issue 1), the function should take the buffer directly.

#### 5. Redundant `dev_set_drvdata` and `i2c_set_clientdata`

In `lt7911exc_probe`:
```c
dev_set_drvdata(dev, lt7911exc);
...
i2c_set_clientdata(client, lt7911exc);
```

For I2C devices, `i2c_set_clientdata(client, data)` calls `dev_set_drvdata(&client->dev, data)`, so these are redundant. Only `i2c_set_clientdata` is needed (it covers both the sysfs `dev_get_drvdata` path and the `i2c_get_clientdata` path in `remove`). The reference driver `lontium-lt8713sx.c` uses only `i2c_set_clientdata`.

#### 6. Version read without MCU halt departs from established pattern

```c
static int lt7911exc_read_version(struct lt7911exc *lt7911exc)
{
	...
	/* no need to halt MCU for this register access */
	ret = regmap_bulk_read(lt7911exc->regmap, 0xe081, buf, ARRAY_SIZE(buf));
```

The sister driver `lontium-lt9611uxc.c` always halts the MCU before reading the version register. The comment claims this is safe but provides no hardware rationale. If the register truly is readable without MCU halt, a brief explanation of why (e.g., "read-only status register not updated by MCU") would be valuable.

#### 7. `lt7911exc_dsi_host_transfer` whitelist does nothing

The transfer function accepts specific DSI write message types via a switch/case whitelist, but then unconditionally returns `msg->tx_len` without actually sending any data:

```c
switch (msg->type) {
case MIPI_DSI_DCS_SHORT_WRITE:
...
break;
default:
return -EOPNOTSUPP;
}
...
return msg->tx_len;
```

The comment says the internal firmware handles DSI commands, so this is essentially a no-op stub. The whitelist filtering adds code without adding value — if nothing is actually transmitted, all types could be silently accepted (or all rejected). The current middle ground is misleading. A single-line comment like `/* firmware handles all panel init; silently accept writes */` before a direct `return msg->tx_len` would be clearer.

#### 8. `lt7911exc_prog_init` redundant `return 0`

```c
ret = regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(seq_write));
if (ret)
    return ret;

return 0;
```

This can be simplified to `return regmap_multi_reg_write(...)`.

#### 9. Kconfig help text formatting

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

Missing space after period: should be `chip. The LT7911EXC`.

#### 10. Comment style

Several C++-style comments remain:

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

Kernel coding style requires `/* */` comments. These should be `/* enable mipi stream */` etc.

#### 11. `lt7911exc_write_data` unchecked return values for partial page handling

```c
if (page_len < LT_PAGE_SIZE) {
    regmap_write(lt7911exc->regmap, 0xe05f, 0x05);
    regmap_write(lt7911exc->regmap, 0xe05f, 0x01);
    ...
}
regmap_write(lt7911exc->regmap, 0xe05f, 0x00);
```

These `regmap_write` calls don't check return values, whereas other similar calls in the function do. Consistency would be good, even if errors here are unlikely.

#### 12. `lt7911exc_write_crc` unchecked return values

Same pattern — several `regmap_write` calls are not checked:

```c
regmap_write(lt7911exc->regmap, 0xe05f, 0x01);
regmap_write(lt7911exc->regmap, 0xe05a, (addr >> 16) & 0xff);
...
```

#### 13. `struct device_node *np` usage

```c
struct device_node *np = dev->of_node;
...
if (!np)
    return -ENODEV;
```

The `np` variable is only used for the NULL check and one assignment (`lt7911exc->bridge.of_node = np`). The OF-matching already guarantees `of_node` is non-NULL when probe runs, so the check is unnecessary. If kept, `dev->of_node` could be used directly without the local variable.

#### 14. `lt7911exc_block_erase` redundantly halts MCU

```c
static int lt7911exc_block_erase(struct lt7911exc *lt7911exc)
{
    ...
    const struct reg_sequence seq_write[] = {
        REG_SEQ0(0xe0ee, 0x01),  /* MCU halt */
```

This function writes 0x01 to the MCU halt register (0xe0ee), but the caller (`lt7911exc_firmware_upgrade_work`) already calls `lt7911exc_hw_mcu_halt()` before `lt7911exc_block_erase()`. This is harmless (idempotent write) but suggests the register sequence was copied verbatim from a vendor reference without cleanup.

---
Generated by Claude Code Patch Reviewer