Claude review: drm/bridge: add support for lontium lt8713sx bridge driver

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

Patch Review

**Issues (in descending severity):**

#### 1. Bug: Redundant assignment of `bridge.funcs` (line 555)

```c
lt8713sx = devm_drm_bridge_alloc(dev, struct lt8713sx, bridge, &lt8713sx_bridge_funcs);
...
lt8713sx->bridge.funcs = &lt8713sx_bridge_funcs;
```

`devm_drm_bridge_alloc()` already sets `bridge->funcs = funcs` (confirmed in `drm_bridge.c:349`). The manual assignment at line 555 is redundant. While not harmful, it suggests a misunderstanding of the API and should be removed to match convention — no other in-tree user of `devm_drm_bridge_alloc()` does this.

#### 2. Firmware data corruption when `fw->size < SZ_64K` (line 128)

```c
memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
```

This unconditionally copies `SZ_64K - 1` bytes from firmware data, but the firmware size is only validated as `<= SZ_256K - 1`. If the firmware is smaller than 64K (which is explicitly handled in `lt8713sx_firmware_upgrade` with `if (lt8713sx->fw->size < SZ_64K)`), this `memcpy` reads past the end of `lt8713sx->fw->data`, causing an out-of-bounds read. The copy size should be `min(lt8713sx->fw->size, (size_t)(SZ_64K - 1))`.

Similarly, when `fw->size < SZ_64K`, the bank firmware section on line 137-139:
```c
memcpy(lt8713sx->fw_buffer + SZ_64K,
       lt8713sx->fw->data + SZ_64K,
       lt8713sx->fw->size - SZ_64K);
```
would produce an underflow in the size argument (`fw->size - SZ_64K` wraps to a huge number since `size_t` is unsigned). This would be catastrophic. The bank firmware copy and bank_num calculation should be guarded by a check that `fw->size > SZ_64K`.

#### 3. `bank_crc_value` array bounds (lines 50, 143-149)

```c
u32 bank_crc_value[17];
...
lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
```

The maximum firmware size is `SZ_256K - 1` (262143 bytes). The bank region starts at `SZ_64K` (65536), so the max bank data is `262143 - 65536 = 196607` bytes. With 12K banks: `ceil(196607 / 12288) = 16`. The array has 17 entries (indices 0-16), so this is just barely sufficient, but the magic number 17 is fragile. A `#define` or computed constant would be safer and clearer. More importantly, there is no bounds check on `bank_num` before using it to index `bank_crc_value[]`.

#### 4. `lt8713sx_block_erase` silently ignores timeout (lines 238-249)

```c
while (1) {
    flash_status = lt8713sx_read_flash_status(lt8713sx);
    if ((flash_status & 0x01) == 0)
        break;
    if (i > 50)
        break;
    i++;
    msleep(50);
}
```

When `i > 50`, the loop breaks silently — the erase timeout is not reported. The function returns `void` so the caller (`lt8713sx_firmware_upgrade`) has no idea the erase failed and proceeds to write firmware to a potentially not-erased flash. This could result in a corrupted firmware on the bridge chip. This function should return an error code on timeout.

#### 5. `lt8713sx_write_data` always returns 0 (line 313)

```c
static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, u64 filesize)
{
    ...
    return 0;
}
```

Despite returning `int`, `lt8713sx_write_data` ignores all `regmap_write()` return values and always returns 0. The callers check `ret < 0` but this can never happen. Either the function should check regmap errors, or the return type should be `void` (and callers adjusted).

#### 6. `i2c_device_id` should not contain commas (line 573)

```c
static struct i2c_device_id lt8713sx_id[] = {
    { "lontium,lt8713sx", 0 },
```

The I2C device ID string should not contain a comma — that's the OF compatible format, not the I2C device name. Compare with other lontium drivers which use e.g. `"lontium,lt9611"`. Wait — I see the other lontium drivers also use the comma format. This appears to be a pre-existing convention in the lontium drivers, though technically it's not correct for I2C device names (they should match the `driver.name` or be plain device names). Additionally, the `0` second field is unnecessary for modern kernels and can be omitted (as `lt9611.c` does).

Also this should be `const`:
```c
static struct i2c_device_id lt8713sx_id[] = {
```
should be:
```c
static const struct i2c_device_id lt8713sx_id[] = {
```

#### 7. `enable_gpio` acquired but not in DT binding (line 475)

```c
lt8713sx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
```

The DT binding in patch 1 has no `enable-gpios` property. If this GPIO is needed, add it to the binding. If it's not needed, remove the code. Having driver code that references undocumented DT properties will cause `dt_binding_check` warnings.

#### 8. Unused includes (lines 10, 13, 18, 19)

Several headers appear unused:
- `<linux/interrupt.h>` — no interrupt handling in this driver
- `<linux/platform_device.h>` — this is an I2C driver
- `<linux/wait.h>` — no wait queues used
- `<linux/workqueue.h>` — no workqueues used

#### 9. `usleep_range(1000, 10000)` has a very wide range (line 446)

```c
usleep_range(1000, 10000);
```

A 10x spread between min and max is unusually large. Typically the range should be tighter, e.g., `usleep_range(1000, 1500)` or `usleep_range(1000, 2000)`, unless the hardware truly doesn't care. This warrants a comment or tightening.

#### 10. Global CRC table (line 30, 560)

```c
DECLARE_CRC8_TABLE(lt8713sx_crc_table);
...
crc8_populate_msb(lt8713sx_crc_table, 0x31);
```

The CRC table is module-global (file-scoped `static`), not per-device. If two instances of this bridge exist, `crc8_populate_msb` would be called twice from two different `probe` calls, which is harmless but wasteful. More importantly, the table population happens in `probe()` but could theoretically be used via the sysfs attribute before a second `probe()` completes, leading to a race. Consider using `once` semantics or making it `__initdata` with a module init function.

#### 11. sysfs attribute naming (line 495)

```c
static DEVICE_ATTR_WO(lt8713sx_firmware);
```

This creates a sysfs attribute named `lt8713sx_firmware`. The convention for sysfs attributes is to not prefix with the driver name — since the attribute is already scoped to the device, just `firmware_update` or `update_firmware` would be clearer. Also, a write-only sysfs attribute that triggers firmware update regardless of what is written is somewhat unusual — the content of `buf` is completely ignored. Consider documenting this in the ABI or at minimum adding a comment.

#### 12. `lt8713sx_firmware_update` cleanup path (lines 410-421)

The `guard(mutex)` at line 387 means the mutex is held until function return. The error label at line 410 always calls `lt8713sx_i2c_disable()`, which writes to regmap — this is fine. However, `kvfree(lt8713sx->fw_buffer)` is called unconditionally. If `lt8713sx_prepare_firmware_data` fails at `request_firmware`, `fw_buffer` is NULL (never allocated), and `kvfree(NULL)` is safe. If it fails at `kvmalloc`, `fw_buffer` is also NULL. So the cleanup is correct, but the `if (lt8713sx->fw)` check on line 418 suggests `fw` might not always be set — the asymmetry between `fw` (conditional free) and `fw_buffer` (unconditional free) is slightly confusing but functionally correct.

#### 13. `u64` types where `u32` would suffice

Several variables use `u64` where `u32` is more than adequate:
- `sz_12k` (line 103): `12 * SZ_1K = 12288`, easily fits in `u32`
- `addr` in `lt8713sx_bank_result_check` (line 348): max value is `0x010000 + 16*0x3000 = 0x40000`, fits in `u32`
- `filesize` parameter in `lt8713sx_write_data` (line 284): max is `SZ_256K`, fits in `u32`

Using `u64` for these is misleading and generates slightly worse code on 32-bit architectures.

---
Generated by Claude Code Patch Reviewer