From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge
Date: Sat, 16 May 2026 14:16:45 +1000
Message-ID: <review-patch2-20260512064013.40066-3-syyang@lontium.com>
In-Reply-To: <20260512064013.40066-3-syyang@lontium.com>
References: <20260512064013.40066-1-syyang@lontium.com> <20260512064013.40066-3-syyang@lontium.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Critical:**

1. **Missing atomic state management callbacks.** The driver uses `atomic_pre_enable`, `atomic_disable`, and `atomic_post_disable` but does not provide `atomic_duplicate_state`, `atomic_destroy_state`, or `atomic_reset`. The kernel documentation in `drm_bridge.c` explicitly states:

   > Bridge drivers may implement the legacy version of those operations, or the atomic version (prefixed with atomic_), in which case they shall also implement the atomic state bookkeeping operations (atomic_duplicate_state, atomic_destroy_state and reset).

   Without `atomic_reset`, `drm_bridge_is_atomic()` returns false and `drm_atomic_private_obj_init()` is never called during `drm_bridge_attach()`. Every other bridge driver in the tree using atomic callbacks provides these. The fix is to add the three helper functions, exactly as the sibling `lontium-lt9211.c` does:

   ```c
   .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,
   ```

**Medium:**

2. **`lt7911exc_attach` uses `lt7911exc->bridge.encoder` instead of the `encoder` parameter.** While functionally equivalent (since `drm_bridge_attach` sets `bridge->encoder` before calling the callback), it's non-idiomatic and fragile:

   ```c
   return drm_bridge_attach(lt7911exc->bridge.encoder, lt7911exc->bridge.next_bridge,
                            &lt7911exc->bridge, flags);
   ```

   Should use `encoder` directly:
   ```c
   return drm_bridge_attach(encoder, lt7911exc->bridge.next_bridge,
                            &lt7911exc->bridge, flags);
   ```

3. **`lt7911exc_firmware_store` does not validate input.** The sysfs store callback ignores the buffer content entirely -- any write triggers a firmware upgrade. It should check for a specific trigger value (e.g., `"1\n"`) to prevent accidental upgrades.

4. **`lt7911exc_firmware_store` does not check `lt7911exc_read_version()` return value.** After firmware upgrade, the version is read but the error code is silently stored in `fw_version`:

   ```c
   lt7911exc->fw_version = lt7911exc_read_version(lt7911exc);
   ```

   If `lt7911exc_read_version` returns a negative error, subsequent reads from the `show` function would display a garbage version.

5. **`cal_crc32_custom` is unsafe for lengths not a multiple of 4.** The loop accesses `data[i+3]` without bounds checking. While the current caller always passes a length divisible by 4, the function itself is fragile. Either add a length check/assertion or document the requirement.

6. **Many register write return values are silently ignored.** `lt7911exc_block_erase`, `lt7911exc_prog_init`, `lt7911exc_lock`, `lt7911exc_unlock`, and `lt7911exc_write_crc` all ignore `regmap_write` / `regmap_multi_reg_write` return values. At minimum, `lt7911exc_block_erase` and `lt7911exc_prog_init` should propagate errors since they're part of the firmware upgrade path.

**Minor:**

7. **Kconfig help text formatting.** Missing space after period:

   ```
   chip.The LT7911EXC converts eDP input to MIPI
   ```

   Should be `chip. The LT7911EXC`.

8. **Unused include.** `#include <linux/platform_device.h>` is included but the driver is an I2C driver and never uses any platform_device APIs.

9. **Sysfs attribute naming.** `lt7911exc_firmware` embeds the driver name in the attribute name, which is unusual. Convention is a simple descriptive name like `firmware_version` (for the show) and `firmware_update` (for the store), or split into two attributes.

10. **`fw_version` format specifier.** `sysfs_emit(buf, "0x%04x\n", lt7911exc->fw_version)` uses `%04x` (4 hex digits, 16 bits), but the version is constructed from 3 bytes (24 bits). Should be `%06x` to consistently zero-pad the full version.

11. **`u64` used where `u32` suffices.** The `addr` parameter in `lt7911exc_prog_init`, `lt7911exc_write_data`, and `lt7911exc_write_crc` is `u64` but only 24 bits are ever used (3 bytes extracted). `u32` would be more appropriate.

12. **`lt7911exc_atomic_disable` is a bare delay.** The callback contains only `msleep(20)` with no hardware interaction. If this delay is truly needed, a comment explaining *why* (e.g., panel timing requirement) would help reviewers. If it's not needed, it should be removed.

---
Generated by Claude Code Patch Reviewer