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

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

Patch Review

This patch has significant issues:

**Critical: Missing atomic state helpers**

The `lt7911exc_bridge_funcs` provides `atomic_pre_enable`, `atomic_disable`, and `atomic_post_disable` but does **not** provide the mandatory `atomic_duplicate_state`, `atomic_destroy_state`, and `atomic_reset` callbacks. Per `include/drm/drm_bridge.h`, these are "mandatory if the bridge implements any of the atomic hooks." Compare with the sibling driver `lontium-lt9611.c` which correctly provides all three using the standard helpers:

```c
static const struct drm_bridge_funcs lt7911exc_bridge_funcs = {
	.attach = lt7911exc_attach,
	.atomic_pre_enable = lt7911exc_atomic_pre_enable,
	.atomic_disable = lt7911exc_atomic_disable,
	.atomic_post_disable = lt7911exc_atomic_post_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
};
```

This will need `#include <drm/drm_atomic_helper.h>`.

**Critical: No MIPI DSI host registration**

The driver claims to output MIPI DSI but never calls `of_find_mipi_dsi_host_by_node()`, `devm_mipi_dsi_device_register_full()`, or `devm_mipi_dsi_attach()`. Without registering as a DSI device with the downstream DSI host, the MIPI output will not function. Every other Lontium bridge driver with DSI output implements this — see `lt9611uxc_attach_dsi()` in `lontium-lt9611uxc.c:248-279` for the pattern. This is fundamental missing functionality.

**Critical: No mode detection or mode validation**

The driver has no `mode_valid`, `mode_fixup`, `edid_read`, `detect`, or `atomic_get_input_bus_fmts` callbacks. This means the bridge cannot:
- Report what modes the connected eDP source provides
- Validate or constrain the display mode
- Report the input bus format

Without these, the display pipeline has no way to negotiate a valid mode through this bridge.

**Kconfig issues**

```c
select DRM_PANEL
select DRM_KMS_HELPER
```

`DRM_PANEL` is selected but never used anywhere in the driver — there is no panel reference or panel bridge. `DRM_KMS_HELPER` is questionable; the driver doesn't directly use KMS helpers. On the other hand, `depends on I2C` is missing (the driver is an I2C driver), and `select DRM_MIPI_DSI` should be added since the driver includes `<drm/drm_mipi_dsi.h>` (and should be using DSI APIs).

**Missing `#include <linux/delay.h>`**

The driver uses `msleep()` and `usleep_range()` but does not include `<linux/delay.h>`. It may compile by accident via transitive includes, but the include should be explicit.

**Regulators left enabled after probe success**

```c
ret = regulator_bulk_enable(ARRAY_SIZE(lt7911exc->supplies), lt7911exc->supplies);
// ... firmware check ...
lt7911exc_unlock(lt7911exc);
lt7911exc->bridge.of_node = dev->of_node;
devm_drm_bridge_add(dev, &lt7911exc->bridge);
return 0;  // regulators still enabled!
```

On the success path, regulators are enabled for the firmware version check but never disabled. They remain on until `atomic_post_disable` is eventually called. Regulators should be disabled after the probe-time version check and only re-enabled in `atomic_pre_enable`.

**Firmware upgrade runs synchronously in probe**

`lt7911exc_firmware_upgrade()` erases flash and writes 64KB of firmware with 32-byte pages, each requiring I2C transactions. This is slow and blocks the boot process. Consider deferring this to a workqueue or using `request_firmware_nowait()`.

**Error handling gaps in register write sequences**

Multiple functions ignore `regmap_write` / `regmap_multi_reg_write` return values:

- `lt7911exc_block_erase()` — ignores `regmap_multi_reg_write` return
- `lt7911exc_lock()` — ignores `regmap_write` return
- `lt7911exc_unlock()` — ignores `regmap_write` return  
- `lt7911exc_upgrade_result()` — ignores several `regmap_write` returns
- `lt7911exc_write_crc()` — ignores most `regmap_write` returns

If the I2C bus has errors, the firmware flash will silently corrupt. These should check and propagate errors.

**`kzalloc` + `memset` redundancy**

```c
buffer = kzalloc(total_size, GFP_KERNEL);
if (!buffer) { ... }
memset(buffer, 0xff, total_size);
```

`kzalloc` zeroes the buffer, then `memset` immediately overwrites with `0xff`. Use `kmalloc` instead to avoid the pointless zeroing.

**Firmware version format string mismatch**

```c
dev_err(dev, "fw version 0x%04x, update failed\n", lt7911exc->fw_version);
```

`fw_version` is constructed from 3 bytes (`(buf[0] << 16) | (buf[1] << 8) | buf[2]`), making it a 24-bit value. The format `0x%04x` only prints 4 hex digits (16 bits), truncating the high byte. Should be `0x%06x`.

**`u64` types used unnecessarily**

`lt7911exc_prog_init()` takes `u64 addr` and `lt7911exc_write_data()` uses `u64 size, offset`, but the maximum address is `FW_SIZE` (65536) and the maximum register is `0xe8ff`. These should all be `u32` to match the actual data widths and avoid implying 64-bit addressing.

**`cal_crc32_custom` is unsafe for non-multiple-of-4 lengths**

```c
for (i = 0; i < length; i += 4) {
    buf[0] = data[i + 3];  // reads data[length-1+3] on last iteration
```

If `length` is not a multiple of 4, this reads past the end of the buffer. The caller currently pads to `FW_SIZE - 4 = 65532` which is divisible by 4, so this works in practice, but the function should either validate or document the alignment requirement.

**Comment style violations**

```c
/*1. load firmware*/
/*2. check size*/
```

Kernel style requires spaces: `/* 1. load firmware */`. Also the `//hardware requires delay` should use C-style `/* ... */` comments per kernel convention (though `//` is technically allowed now, consistency matters).

**`lt7911exc_attach` uses `bridge->encoder` instead of parameter**

```c
static int lt7911exc_attach(struct drm_bridge *bridge,
                            struct drm_encoder *encoder,
                            enum drm_bridge_attach_flags flags)
{
    ...
    return drm_bridge_attach(lt7911exc->bridge.encoder, ...);
}
```

The `encoder` parameter should be used directly rather than reading `lt7911exc->bridge.encoder`. Compare with `lontium-lt9611uxc.c:287` which correctly uses the `encoder` parameter. While `bridge->encoder` may already be set by the framework, using the parameter is the correct pattern.

**`atomic_disable` has only a sleep**

```c
static void lt7911exc_atomic_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
{
    /* Delay after panel is disabled */
    msleep(20);
}
```

This does nothing except sleep. If the bridge has nothing to do at disable time, remove this callback. If the delay is truly needed for panel sequencing, this should be documented with a reference to the datasheet timing requirement, and it's questionable whether `atomic_disable` is the right place (vs. `atomic_post_disable`).

---
Generated by Claude Code Patch Reviewer