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

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

Patch Review

**Kconfig issues:**

1. Missing space in help text:
```c
	  chip.The LT7911EXC converts eDP input to MIPI
```
Should be `chip. The LT7911EXC...`.

2. Consider adding `select DRM_MIPI_DSI` — the driver implements a `mipi_dsi_host` and uses MIPI DSI APIs.

**Struct formatting:**

3. Inconsistent closing brace indentation in `lt7911exc_dsi_output`:
```c
struct lt7911exc_dsi_output {
	struct mipi_dsi_device *dev;
	struct drm_panel *panel;
	struct drm_bridge *bridge;
	};
```
The `};` has an extra tab. Should be flush left.

**`fw_version` sysfs display bug:**

4. `lt7911exc_read_version` returns a 24-bit value:
```c
	return (buf[0] << 16) | (buf[1] << 8) | buf[2];
```
But `lt7911exc_firmware_show` formats it as:
```c
	return sysfs_emit(buf, "0x%04x\n", lt7911exc->fw_version);
```
`%04x` pads to 4 hex digits minimum but a 24-bit version (e.g., `0x010203`) needs 6 hex digits. Use `"0x%06x\n"` to match the 3-byte version, or `"0x%x\n"` if there's no fixed width convention.

**Unnecessary `u64` types:**

5. Several functions use `u64` for addresses and sizes that are at most 64KB:
```c
static u32 cal_crc32_custom(const u8 *data, u64 length)
static int lt7911exc_prog_init(struct lt7911exc *lt7911exc, u64 addr)
static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct firmware *fw, u64 addr)
```
These should be `size_t` for lengths and `u32` for addresses. Firmware is capped at `FW_SIZE` (64KB), so `u64` is misleading. This also causes mixed-type arithmetic in `lt7911exc_write_data`:
```c
	u64 size, offset;
	...
	page = (size + LT_PAGE_SIZE - 1) / LT_PAGE_SIZE;
```
Here `page` is `int` but the RHS is `u64`, resulting in silent truncation.

**`cal_crc32_custom` fragility:**

6. The function accesses `data[i+3]` without checking that `length` is a multiple of 4:
```c
	for (i = 0; i < length; i += 4) {
		buf[0] = data[i + 3];
```
Currently all callers pass `FW_SIZE - 4 = 65532` which is divisible by 4, so there's no actual bug. But the function is fragile — consider adding a `WARN_ON(length % 4)` or a comment documenting the requirement.

**Unchecked `regmap_write` return values:**

7. Several `regmap_write` calls in `lt7911exc_write_crc` ignore return values:
```c
	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);
```
Same issue in `lt7911exc_write_data` for the short-page handling path:
```c
		if (page_len < LT_PAGE_SIZE) {
			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);
```
These should check return values or at minimum use `regmap_multi_reg_write` as done in the erase function for consistency.

**`lt7911exc_lock`/`lt7911exc_unlock` vs. internal OCM register writes:**

8. `lt7911exc_lock` writes `0xe0ee = 0x01` to stop the on-chip MCU, but `lt7911exc_block_erase` and `lt7911exc_prog_init` also write `0xe0ee = 0x01` via their `seq_write` arrays:
```c
	const struct reg_sequence seq_write[] = {
		REG_SEQ0(0xe0ee, 0x01),   /* redundant when caller holds lock */
		REG_SEQ0(0xe054, 0x01),
```
This is harmless but confusing — the OCM lock is managed at two different layers. Consider removing the redundant `0xe0ee` writes from the firmware functions since the caller always holds the lock, or document that these functions must not be called without the lock held.

**Redundant `dev_set_drvdata` / `i2c_set_clientdata`:**

9. In probe:
```c
	dev_set_drvdata(dev, lt7911exc);
	...
	i2c_set_clientdata(client, lt7911exc);
```
`i2c_set_clientdata` calls `dev_set_drvdata(&client->dev, data)` internally, so this is redundant. One call to `i2c_set_clientdata` is sufficient (and is what `lt7911exc_remove` uses to retrieve it).

**Comment style:**

10. Missing spaces after `/*` in comments:
```c
	/*3. calculate crc32 */
	/*4. firmware upgrade */
	/*5. check upgrade of result */
```
Should be `/* 3.` etc.

**`lt7911exc_prog_init` unnecessary early return:**

11. The function ends with:
```c
	ret = regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(seq_write));
	if (ret)
		return ret;

	return 0;
```
This is just `return regmap_multi_reg_write(...)`.

**Firmware upgrade safety:**

12. `lt7911exc_firmware_store` can be triggered while the display pipeline is active. The function takes `ocm_lock`, resets the chip, erases flash, and reprograms firmware — all while a display may be running. After the upgrade completes and the lock is released, the bridge is in an undefined state relative to the DRM pipeline. There's no check for whether the bridge is currently attached/enabled. Other drivers with similar sysfs firmware interfaces (lt9611uxc) have the same issue, so this may be accepted practice, but it's worth noting.

**`lt7911exc_dsi_host_transfer` semantics:**

13. The transfer function doesn't actually forward DSI messages — it waits for video input and enables DSI output:
```c
	ret = regmap_read_poll_timeout(lt7911exc->regmap, 0xe084, val, (val & 0x01), 5000, 50000);
	...
	ret = regmap_write(lt7911exc->regmap, 0xe0b0, 0x01);
	...
	return msg->tx_len;
```
Every DSI transfer from the panel driver will re-poll the video-ready register (up to 50ms) and re-write the output enable register. If a panel driver sends multiple DCS init commands, this could add significant latency. Is the intent that only a single transfer ever happens (i.e., the panel driver sends exactly one DCS command to trigger output)? If so, this should be documented. If not, the enable should happen once and subsequent transfers should be no-ops.

**Missing `atomic_enable` callback:**

14. The bridge provides `atomic_pre_enable` and `atomic_disable`/`atomic_post_disable` but no `atomic_enable`. The enable logic appears to be implicitly handled through the DSI host transfer mechanism, but this coupling between bridge lifecycle and DSI host transfer is unusual and could use a comment explaining the design.

**Minor: `#include <linux/platform_device.h>`:**

15. This is an I2C driver, not a platform driver. The `platform_device.h` include appears unused.

**Minor: `np` check:**

16. In probe:
```c
	if (!np)
		return -ENODEV;
```
The driver has `depends on OF` in Kconfig and the `of_match_table` is set, so `of_node` will always be non-NULL when probe is called via DT matching. This check is unnecessary.

**Summary:** The driver structure is sound and follows existing patterns (especially the tc358768 model for DSI-host bridges). The main items to address are: the sysfs version format bug (#4), unchecked regmap return values (#7), the u64/type-width cleanup (#5), and the struct indentation (#3). The rest are minor improvements and style points.

---
Generated by Claude Code Patch Reviewer