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

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

Patch Review

**`bridge.funcs` assigned redundantly:**
`devm_drm_bridge_alloc()` already sets `bridge->funcs = funcs` internally. The manual assignment at line is unnecessary and misleading:

```c
+	lt8713sx->bridge.funcs = &lt8713sx_bridge_funcs;
```

Remove this line.

**`enable_gpio` acquired but never used:**
The driver requests an "enable" GPIO and stores it, but never reads or toggles it after acquisition:

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

If the purpose is just to drive it high at probe time, that side effect relies on the `GPIOD_OUT_HIGH` flag, which is subtle and undocumented. This should either be used explicitly (e.g., in enable/disable callbacks) or removed. It's also not documented in the DT binding.

**`bridge_attach` signature may be wrong:**
On the current drm-next tree, the attach callback signature includes the encoder parameter:
```c
int (*attach)(struct drm_bridge *bridge, struct drm_encoder *encoder,
              enum drm_bridge_attach_flags flags);
```
The patch matches this, but the driver only passes through to `drm_bridge_attach`. This is fine but should be verified against the exact tree it's targeting.

**No bridge `ops` set — driver does nothing as a bridge:**
The bridge has no `ops` flags set (no `DRM_BRIDGE_OP_DETECT`, `DRM_BRIDGE_OP_EDID`, etc.) and the only bridge function is `attach`. This means the bridge does nothing in the display pipeline — it just chains to `next_bridge`. This raises the question of whether this needs to be a `drm_bridge` driver at all, or whether a simpler i2c driver would suffice for firmware loading.

**Sysfs attribute naming and approach:**
```c
+static DEVICE_ATTR_WO(lt8713sx_firmware);
```

This creates a sysfs attribute named `lt8713sx_firmware`. The naming convention is unusual — sysfs attributes typically use shorter generic names (e.g., `firmware_update` or `update_fw`). More importantly, using a raw sysfs write-triggered firmware flash is not the preferred approach. The kernel has the firmware upload API (`firmware_upload`) in `include/linux/firmware.h` designed exactly for this use case, or firmware can be loaded automatically at probe time.

The `store` function ignores the buffer content entirely:
```c
+static ssize_t lt8713sx_firmware_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	struct lt8713sx *lt8713sx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = lt8713sx_firmware_update(lt8713sx);
```

Writing any value triggers a firmware flash. This is fragile — there's no validation, no idempotency check.

**Integer type issues with `sz_12k`:**
```c
+	u64 sz_12k = 12 * SZ_1K;
```

This is `u64` for a value of 12288 — completely unnecessary. Just use a `#define` or plain `unsigned int`. The subsequent division by `sz_12k` then does a 64-bit divide which is expensive on 32-bit architectures (and may need `do_div`):

```c
+	lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
```

This will break on 32-bit builds because it does a 64-bit divide. Use `DIV_ROUND_UP()` with appropriate types.

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

The function never checks `regmap_write()` return values and always returns success. If any register write fails during firmware flashing, the failure is silently ignored. All the `regmap_write` calls in this driver are unchecked — this is a pervasive issue throughout the driver.

**`lt8713sx_block_erase` has unbounded-ish busy wait:**
```c
+		while (1) {
+			flash_status = lt8713sx_read_flash_status(lt8713sx);
+			if ((flash_status & 0x01) == 0)
+				break;
+			if (i > 50)
+				break;
+			i++;
+			msleep(50);
+		}
```

The loop breaks after 50 iterations but doesn't report an error if the flash never becomes ready. A block erase that fails silently will lead to corrupted firmware.

**Firmware size check is off-by-one:**
```c
+	if (lt8713sx->fw->size > SZ_256K - 1) {
```

This allows firmware of size exactly `SZ_256K - 1` (262143 bytes). If the intent is to allow up to 256KB, use `>= SZ_256K` or `> SZ_256K`. The `- 1` suggests confusion.

**Main firmware copy is also off-by-one:**
```c
+	memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
```

This copies 65535 bytes instead of 65536. This means the last byte of the first 64K is not copied from the firmware file — it's left as 0xFF (from the memset). The CRC is then computed over 65535 bytes and stored at offset `SZ_64K - 1`. This seems intentional (the CRC replaces the last byte), but it means the firmware format requires the last byte of the first 64K block to be reserved. This should be documented.

**`bank_crc_value` array size is 17 but not bounds-checked:**
```c
+	u32 bank_crc_value[17];
+	...
+	lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
+	...
+	for (int i = 0; i < lt8713sx->bank_num; i++) {
+		lt8713sx->bank_crc_value[i] = ...
```

The maximum number of 12K banks in a 192KB region (256K - 64K) is `(192*1024) / (12*1024) = 16`, so `bank_crc_value[17]` is sufficient. However, there is no explicit bounds check on `bank_num` before indexing into the array. If the firmware size calculation yields a `bank_num > 17`, this is a buffer overflow.

**`i2c_device_id` should be `const`:**
```c
+static struct i2c_device_id lt8713sx_id[] = {
```

Should be:
```c
+static const struct i2c_device_id lt8713sx_id[] = {
```

**`i2c_device_id` entry should not include the vendor prefix:**
```c
+	{ "lontium,lt8713sx", 0 },
```

The `i2c_device_id` `.name` field should be just `"lt8713sx"`, not `"lontium,lt8713sx"`. The compatible string in `of_device_id` uses the vendor prefix, but the I2C ID table does not. Also, the trailing `, 0` is unnecessary with modern kernel conventions.

**`crc8_populate_msb` called on a global table in probe:**
```c
+DECLARE_CRC8_TABLE(lt8713sx_crc_table);
+...
+	crc8_populate_msb(lt8713sx_crc_table, 0x31);
```

This is a module-level static array populated in `probe()`. If multiple devices probe (unlikely but possible), there's a race on table initialization. This should be done in `module_init` or with a `once` guard. Alternatively, use a `CRC8_INIT_TABLE` or populate it at compile time.

**`drm_bridge_remove` in remove but no unplug support:**
```c
+static void lt8713sx_remove(struct i2c_client *client)
+{
+	struct lt8713sx *lt8713sx = i2c_get_clientdata(client);
+	drm_bridge_remove(&lt8713sx->bridge);
+}
```

Modern bridge drivers using `devm_drm_bridge_alloc` should use `devm_drm_bridge_add` instead of the manual `drm_bridge_add`/`drm_bridge_remove` pair, which would eliminate the need for a `remove` callback entirely.

**`lt8713sx_load_bank_fw_to_sram` takes `u64 addr` unnecessarily:**
```c
+static void lt8713sx_load_bank_fw_to_sram(struct lt8713sx *lt8713sx, u64 addr)
```

The address fits in 24 bits (max 0x040000). Using `u64` is misleading and wasteful. Use `u32`.

**No `DRM_BRIDGE_ATTACH_NO_CONNECTOR` handling:**
The `attach` callback doesn't check for or pass through `DRM_BRIDGE_ATTACH_NO_CONNECTOR` flags. Modern bridge drivers should support this flag.

---
Generated by Claude Code Patch Reviewer