From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge Date: Thu, 04 Jun 2026 16:43:05 +1000 Message-ID: In-Reply-To: <20260529112928.873766-3-syyang@lontium.com> References: <20260529112928.873766-1-syyang@lontium.com> <20260529112928.873766-3-syyang@lontium.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Issues (High):** 1. **`.detach` callback manually NULLs `bridge->dev` =E2=80=94 this is wron= g and racy.** ```c static void lt7911exc_bridge_detach(struct drm_bridge *bridge) { struct lt7911exc *lt7911exc =3D bridge_to_lt7911exc(bridge); guard(mutex)(<7911exc->ocm_lock); bridge->dev =3D NULL; } ``` The DRM core in `drm_bridge_detach()` (drm_bridge.c:632) already sets `b= ridge->dev =3D 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 =E2=80=94 creating an inconsistent= locking protocol. The firmware upgrade worker reads `bridge->dev` under `o= cm_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(<7911exc->ocm_lock); lt7911exc_hw_mcu_halt(lt7911exc); mutex_unlock(<7911exc->ocm_lock); ret =3D lt7911exc_block_erase(lt7911exc); ``` After `lt7911exc_reset()`, the `ocm_lock` is taken only around `lt7911ex= c_hw_mcu_halt()`, but then `lt7911exc_block_erase()`, `lt7911exc_write_data= ()`, and `lt7911exc_write_crc()` all run **without holding `ocm_lock`**. Th= ese functions perform I2C register writes that could race with `lt7911exc_a= tomic_pre_enable()` / `lt7911exc_atomic_post_disable()`. The `lt7911exc->up= grade` flag guards the bridge callbacks, but `lt7911exc_block_erase()` itse= lf calls `regmap_write(..., 0xee, 0x01)` (MCU halt) redundantly without the= lock. If the intention is that the `upgrade` flag prevents bridge callback= s from touching registers, this should be documented more clearly, and `blo= ck_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 =3D devm_regulator_get_enable(lt7911exc->dev, "vcc"); ... usleep_range(5000, 10000); ret =3D 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 remova= l. However, the driver also asserts reset in `lt7911exc_remove()` via `gpio= d_set_value_cansleep(lt7911exc->reset_gpio, 1)`. Since the GPIO was obtaine= d via `devm_gpiod_get()`, it will be released during devm teardown =E2=80= =94 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_brid= ge_remove()` in `lt7911exc_remove()` and on `mipi_dsi_host_register()` fail= ure. Since the driver already uses `devm_drm_bridge_alloc()`, it would be m= ore consistent to use `devm_drm_bridge_add()` =E2=80=94 but this may be int= entional to control ordering relative to `mipi_dsi_host_unregister()`. Eith= er way, the current manual approach works correctly. 5. **`lt7911exc_write_data()` error handling: short-write path ignores `reg= map_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 da= ta 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 re= gmap 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 e= rrno on failure. The version is stored as `int fw_version`. This works sinc= e 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 v= ersion. 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 `defau= lt`/`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 i= nside the `case`. 11. **`#include ` is unnecessary** =E2=80=94 this = is an I2C driver, not a platform driver. 12. **`dev_set_drvdata()` and `i2c_set_clientdata()` both called in probe**= =E2=80=94 these are equivalent for I2C devices (`i2c_set_clientdata` wraps= `dev_set_drvdata`). Only one is needed; `i2c_set_clientdata()` is the conv= entional one for I2C drivers. --- Generated by Claude Code Patch Reviewer