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

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

Patch Review

**Issues (High):**

1. **`.detach` callback manually NULLs `bridge->dev` — this is wrong and racy.**

   ```c
   static void lt7911exc_bridge_detach(struct drm_bridge *bridge)
   {
       struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);

       guard(mutex)(&lt7911exc->ocm_lock);
       bridge->dev = NULL;
   }
   ```

   The DRM core in `drm_bridge_detach()` (drm_bridge.c:632) already sets `bridge->dev = NULL` **after** calling the `.detach` callback. This driver's detach is doing the core's job redundantly, and worse, it does it under `ocm_lock` while the core does it without — creating an inconsistent locking protocol. The firmware upgrade worker reads `bridge->dev` under `ocm_lock`, but the core's subsequent NULL write is not under that lock. This callback should either be removed entirely or should only contain hardware-specific teardown. The `bridge->dev` check in the firmware upgrade worker needs rethinking if it genuinely needs synchronization with detach.

2. **Firmware upgrade path: locking gap after `lt7911exc_reset()`.**

   In `lt7911exc_firmware_upgrade_work()`:
   ```c
   lt7911exc_reset(lt7911exc);

   mutex_lock(&lt7911exc->ocm_lock);
   lt7911exc_hw_mcu_halt(lt7911exc);
   mutex_unlock(&lt7911exc->ocm_lock);

   ret = lt7911exc_block_erase(lt7911exc);
   ```

   After `lt7911exc_reset()`, the `ocm_lock` is taken only around `lt7911exc_hw_mcu_halt()`, but then `lt7911exc_block_erase()`, `lt7911exc_write_data()`, and `lt7911exc_write_crc()` all run **without holding `ocm_lock`**. These functions perform I2C register writes that could race with `lt7911exc_atomic_pre_enable()` / `lt7911exc_atomic_post_disable()`. The `lt7911exc->upgrade` flag guards the bridge callbacks, but `lt7911exc_block_erase()` itself calls `regmap_write(..., 0xee, 0x01)` (MCU halt) redundantly without the lock. If the intention is that the `upgrade` flag prevents bridge callbacks from touching registers, this should be documented more clearly, and `block_erase` should not redundantly re-halt the MCU.

3. **`lt7911exc_regulator_enable()` uses `devm_regulator_get_enable()` with ordering dependency but no proper sequencing guarantee.**

   ```c
   static int lt7911exc_regulator_enable(struct lt7911exc *lt7911exc)
   {
       int ret;

       ret = devm_regulator_get_enable(lt7911exc->dev, "vcc");
       ...
       usleep_range(5000, 10000);

       ret = devm_regulator_get_enable(lt7911exc->dev, "vdd");
   ```

   `devm_regulator_get_enable()` is a "get and enable, devm-managed disable" helper. These regulators will be disabled in reverse devm order on removal. However, the driver also asserts reset in `lt7911exc_remove()` via `gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1)`. Since the GPIO was obtained via `devm_gpiod_get()`, it will be released during devm teardown — but the regulators may be disabled *before* the reset is fully asserted, depending on devm ordering. This is fragile but likely not a real bug in practice since the reset GPIO assertion happens explicitly in `remove()`.

**Issues (Medium):**

4. **`drm_bridge_add()` is used instead of `devm_drm_bridge_add()`; manual cleanup on error and in `remove()`.**

   The probe function calls `drm_bridge_add()` and manually calls `drm_bridge_remove()` in `lt7911exc_remove()` and on `mipi_dsi_host_register()` failure. Since the driver already uses `devm_drm_bridge_alloc()`, it would be more consistent to use `devm_drm_bridge_add()` — but this may be intentional to control ordering relative to `mipi_dsi_host_unregister()`. Either way, the current manual approach works correctly.

5. **`lt7911exc_write_data()` error handling: short-write path ignores `regmap_write()` return values.**

   ```c
   if (page_len < LT_PAGE_SIZE) {
       regmap_write(lt7911exc->regmap, 0x5f, 0x05);
       regmap_write(lt7911exc->regmap, 0x5f, 0x01);
       //hardware requires delay
       usleep_range(1000, 2000);
   }

   regmap_write(lt7911exc->regmap, 0x5f, 0x00);
   ```

   Several `regmap_write()` calls have their return values silently dropped. If any of these fail, the firmware write will silently produce corrupt data in flash. These should check return values.

6. **`regmap_config` has no `max_raw_write` set.**

   The driver calls `regmap_raw_write()` with up to 32 bytes (`LT_PAGE_SIZE`). While 32 bytes plus register address will fit within most I2C adapters' transfer limits, not setting `max_raw_write` in the regmap config means regmap won't perform any splitting. This is likely fine in practice but could be more robust.

7. **`lt7911exc_read_version()` return value overloading.**

   ```c
   static int lt7911exc_read_version(struct lt7911exc *lt7911exc)
   {
       ...
       return (buf[0] << 16) | (buf[1] << 8) | buf[2];
   }
   ```

   The function returns a positive version value on success or a negative errno on failure. The version is stored as `int fw_version`. This works since the version fits in 24 bits, but the `sysfs_emit(buf, "0x%04x\n", version)` format only shows 4 hex digits (16 bits), truncating the actual 24-bit version. Should be `"0x%06x\n"`.

**Issues (Low / Style):**

8. **Kconfig help text formatting:**

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

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

9. **Comment style:** Several `//` C++ style comments should be `/* */` per kernel coding style:

   ```c
   //enable mipi stream
   //disable mipi stream
   //write method for less than LT_PAGE_SIZE bytes.
   //hardware requires delay
   ```

10. **`lt7911exc_dsi_host_transfer()` indentation:** The `break` and `default`/`return` inside the switch are indented at the same level as the `case` labels:

    ```c
    case MIPI_DSI_DCS_LONG_WRITE:
    ...
    case MIPI_DSI_GENERIC_LONG_WRITE:
    break;
    default:
    return -EOPNOTSUPP;
    ```

    The `break`, `default`, and its `return` should be indented one level inside the `case`.

11. **`#include <linux/platform_device.h>` is unnecessary** — this is an I2C driver, not a platform driver.

12. **`dev_set_drvdata()` and `i2c_set_clientdata()` both called in probe** — these are equivalent for I2C devices (`i2c_set_clientdata` wraps `dev_set_drvdata`). Only one is needed; `i2c_set_clientdata()` is the conventional one for I2C drivers.

---
Generated by Claude Code Patch Reviewer