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

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

Patch Review

#### Bug: Firmware leak and goto label ordering error

The `lt7911exc_firmware_upgrade_work` function has a critical goto ordering problem. When the firmware upgrade fails after `mutex_lock(&lt7911exc->upgrade_lock)`, the error path jumps to `out_unlock`, which falls through to `out_release_fw` and then `out_clear_status`. But when `fw->size > total_size` or `kvmalloc` fails, the code jumps to `out_release_fw` — and then falls through to `out_clear_status`. However, the `out_unlock` label also falls through to `out_release_fw`. The problem is this sequence:

```c
out_unlock:
	mutex_lock(&lt7911exc->ocm_lock);
	lt7911exc_hw_mcu_run(lt7911exc);
	lt7911exc->fw_version = lt7911exc_read_version(lt7911exc);
	mutex_unlock(&lt7911exc->ocm_lock);

	mutex_unlock(&lt7911exc->upgrade_lock);
	/* Notify DRM framework that hardware state changed/needs a modeset */
	if (lt7911exc->bridge.dev)
		drm_kms_helper_hotplug_event(lt7911exc->bridge.dev);

out_release_fw:
	release_firmware(fw);

out_clear_status:
```

When `lt7911exc_block_erase` or `lt7911exc_write_data` fails and we goto `out_unlock`, this is correct. But the `out_release_fw` label is positioned **after** `out_unlock`, so when we goto `out_release_fw` directly (from the size check or kvmalloc failure), `release_firmware` is called but the `upgrade_lock` is never unlocked — the lock was never taken in that path, so that's fine. Wait — actually, looking more carefully, the `upgrade_lock` is taken **after** the `buffer` work:

```c
	mutex_lock(&lt7911exc->upgrade_lock);
	...
	ret = lt7911exc_block_erase(lt7911exc);
	if (ret) {
		...
		goto out_unlock;
	}
```

The early errors (`fw->size > total_size`, kvmalloc failure) goto `out_release_fw` which does NOT unlock `upgrade_lock` because it was never taken — this is correct. But the real bug is that the `out_unlock` path runs `lt7911exc_hw_mcu_run` and `lt7911exc_read_version` even on error, then does `drm_kms_helper_hotplug_event` after a failed upgrade, which generates a spurious hotplug event. The hotplug event is misleading but not a crash.

**However**, there is a subtle but real issue: the `out_clear_status` block accesses `lt7911exc` fields after `release_firmware(fw)` — the firmware pointer `fw` is unrelated to `lt7911exc`, so this is fine for memory safety. But the **actual bug** is: if `request_firmware` fails, we jump to `out_clear_status` directly, and the `upgrade` flag is correctly cleared. Good.

#### Bug: Firmware data vs CRC mismatch

The firmware upgrade writes a CRC computed from a zero-padded copy:

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

But the actual write to hardware uses the raw firmware data:

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

Inside `lt7911exc_write_data`, only `fw->size` bytes are written. The CRC is computed over the full `total_size` (64K - 4) with 0xFF padding, but the hardware only receives `fw->size` bytes. Either the full padded buffer should be written, or the CRC should be computed over just `fw->data` with `fw->size`. As-is, the CRC verification at the end (`lt7911exc_upgrade_result`) will always fail if `fw->size < total_size`, because the hardware CRC won't match the padded CRC.

#### Issue: Unchecked regmap writes in flash programming

Throughout the flash programming functions, many `regmap_write` calls have their return values silently discarded:

```c
static int lt7911exc_write_crc(struct lt7911exc *lt7911exc, u32 crc32, u64 addr)
{
	...
	regmap_write(lt7911exc->regmap, 0xe05f, 0x01);
	regmap_write(lt7911exc->regmap, 0xe05a, (addr >> 16) & 0xff);
	regmap_write(lt7911exc->regmap, 0xe05b, (addr >> 8) & 0xff);
	regmap_write(lt7911exc->regmap, 0xe05c, addr & 0xff);
```

Similarly in `lt7911exc_write_data`:

```c
		regmap_write(lt7911exc->regmap, 0xe05f, 0x05);
		regmap_write(lt7911exc->regmap, 0xe05f, 0x01);
```

And in `lt7911exc_upgrade_result`:

```c
	regmap_write(lt7911exc->regmap, 0xe0ee, 0x01);
	regmap_write(lt7911exc->regmap, 0xe07b, 0x60);
	regmap_write(lt7911exc->regmap, 0xe07b, 0x40);
```

If any of these I2C writes fail during a flash erase/program operation, the firmware upgrade will silently produce corrupted results. These should at minimum be checked and cause an early return on error.

#### Issue: Locking design — upgrade_lock released too early in sysfs path

In `lt7911exc_firmware_store`, the `upgrade_lock` is taken with `mutex_trylock`, then the `upgrade` flag is set, and **both locks are released**:

```c
	if (!mutex_trylock(&lt7911exc->upgrade_lock))
		return -EBUSY;

	mutex_lock(&lt7911exc->ocm_lock);
	...
	lt7911exc->upgrade = true;
	mutex_unlock(&lt7911exc->ocm_lock);
	mutex_unlock(&lt7911exc->upgrade_lock);

	schedule_work(&lt7911exc->work);
```

The `upgrade_lock` is released before `schedule_work`. Then in the worker, `upgrade_lock` is re-acquired. But between the sysfs `store` releasing `upgrade_lock` and the worker acquiring it, a second `store` could `mutex_trylock(&upgrade_lock)` successfully and also see `upgrade == true` → return `-EBUSY`. This is actually safe because of the `upgrade` flag check, but it means the `upgrade_lock` in `firmware_store` serves no real purpose — the `ocm_lock` + `upgrade` flag alone would be sufficient. The two-lock scheme is confusing and should be simplified.

#### Issue: Sysfs attribute naming

```c
static DEVICE_ATTR_RW(lt7911exc_firmware);
```

This creates an attribute named `lt7911exc_firmware`. Kernel sysfs conventions say the attribute should not include the driver name as a prefix — it should just be `firmware` (or `firmware_version` / `firmware_update`). The device is already scoped to this driver instance. Also, having a single attribute for both read (version) and write (trigger upgrade) is confusing — consider splitting into separate `firmware_version` (read-only) and `firmware_update` (write-only) attributes.

#### Issue: Kconfig help text formatting

```c
	help
	  DRM driver for the Lontium LT7911EXC bridge
	  chip.The LT7911EXC converts eDP input to MIPI
```

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

#### Issue: Comment style

Several comments use C++ style which, while accepted, is inconsistent with typical kernel bridge driver style:

```c
		//hardware requires delay
		usleep_range(1000, 2000);
```

```c
	//enable mipi stream
```

```c
	//disable mipi stream
```

Should use `/* ... */` style per kernel coding conventions.

#### Issue: `lt7911exc_dsi_host_transfer` is a no-op

The transfer callback accepts write commands but does nothing with them:

```c
	guard(mutex)(&lt7911exc->ocm_lock);

	if (lt7911exc->upgrade)
		return -EBUSY;

	return msg->tx_len;
```

The comment explains this is intentional (firmware handles DSI init), which is fine. But the function acquires the `ocm_lock` and checks the `upgrade` flag for writes that are immediately discarded — this is unnecessary overhead. If the firmware truly handles everything, consider whether this callback is needed at all, or document more clearly why the lock/flag check is still needed (e.g., to prevent panel init during upgrade).

#### Issue: `lt7911exc_prog_init` has an unnecessary pattern

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

	return 0;
```

Should just be `return regmap_multi_reg_write(...)`.

#### Nit: `u64` type for addresses that fit in `u32`

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

```c
static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct firmware *fw, u64 addr)
```

```c
static int lt7911exc_write_crc(struct lt7911exc *lt7911exc, u32 crc32, u64 addr)
```

The flash addresses are clearly within a 64KB space (`FW_SIZE = 64 * 1024`), and the register writes only use the lower 24 bits. Using `u64` for these addresses is misleading — `u32` would be appropriate.

#### Nit: `cal_crc32_custom` naming

The function name `cal_crc32_custom` is vague. Something like `lt7911exc_crc32_be_byteswap` would better describe that it computes a CRC32-BE over byte-reversed 32-bit words. Also, the `length` parameter should be `size_t`, not `u64`.

#### Nit: `dev_set_drvdata` and `i2c_set_clientdata` redundancy

In probe:

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

`i2c_set_clientdata(client, lt7911exc)` calls `dev_set_drvdata(&client->dev, lt7911exc)` internally. Since `dev = &client->dev`, these are redundant. Remove the `dev_set_drvdata` call.

#### Nit: `of_node` check is unnecessary

```c
	if (!np)
		return -ENODEV;
```

The driver has `depends on OF` in Kconfig and uses OF match table, so `of_node` will always be set when the driver probes. This check is superfluous.

#### Missing: `drm_panel_bridge_remove` leak on detach

In `lt7911exc_dsi_host_detach`, if a panel bridge was created via `drm_panel_bridge_add_typed` in the attach path, the detach only clears `output.dev` but does not call `drm_panel_bridge_remove`:

```c
static int lt7911exc_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *dev)
{
	struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);

	if (!lt7911exc->output.dev)
		return 0;

	lt7911exc->output.dev = NULL;

	return 0;
}
```

The panel bridge is only cleaned up in `lt7911exc_remove`. If a DSI device attaches, detaches, and attaches again, `drm_panel_bridge_add_typed` would be called a second time, leaking the first bridge. The detach callback should clean up `output.bridge` and `output.panel` as well.

---
Generated by Claude Code Patch Reviewer