From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH 2/2] drm/bridge: Add LT7911EXC edp to mipi bridge driver Date: Thu, 23 Apr 2026 10:26:24 +1000 Message-ID: In-Reply-To: References: <20260420023354.1192642-1-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 **Commit message:** The description has formatting/spacing issues ("Complia= ntwithD-PHY1.2" etc. with missing spaces). Looks like copy-pasted datasheet= text without cleanup. #### Critical Bugs 1. **Wrong Kconfig symbol.** The Kconfig adds `DRM_LONTIUM_LT9611C` (a DSI/= HDMI bridge) but the Makefile references `DRM_LONTIUM_LT7911EXC`. The Kconf= ig entry describes the wrong chip entirely =E2=80=94 it mentions "DSI to HD= MI" and selects `SND_SOC_HDMI_CODEC`, `DRM_DISPLAY_HDMI_STATE_HELPER`, etc.= which are irrelevant for an eDP-to-MIPI bridge. This is clearly a copy-pas= te from a different chip's Kconfig that was never updated. ``` +config DRM_LONTIUM_LT9611C + tristate "Lontium LT9611C DSI/HDMI bridge" ``` Should be: ``` config DRM_LONTIUM_LT7911EXC tristate "Lontium LT7911EXC eDP/MIPI bridge" ``` And the selected symbols should match what the driver actually needs (no HD= MI helpers, no SND_SOC_HDMI_CODEC, etc.). The driver needs at minimum `REGM= AP_I2C`, `FW_LOADER`, and `DRM_PANEL_BRIDGE`. 2. **Buffer overflow in `lt7911exc_read_version`.** The buffer is declared = as `u8 buf[2]` but `regmap_bulk_read` is called with count 3: ```c u8 buf[2]; ... ret =3D regmap_bulk_read(lt7911exc->regmap, 0xe081, buf, 3); ... return (buf[0] << 16) | (buf[1] << 8) | buf[2]; ``` This writes one byte past the buffer and accesses `buf[2]` which is out of = bounds. The buffer must be `u8 buf[3]`. 3. **`lt7911exc_write_data` never advances the flash address correctly.** T= he function calls `lt7911exc_prog_init(lt7911exc, addr)` inside the loop, b= ut `addr` is the *parameter* which is `0` when called from `lt7911exc_firmw= are_upgrade`. The local `addr +=3D LT_PAGE_SIZE` at the end of the loop adv= ances a local copy, but `addr` is a parameter passed by value =E2=80=94 wai= t, actually `addr` *is* the local since it's a parameter, so `addr +=3D LT_= PAGE_SIZE` does advance it correctly through the loop. That part is OK on r= e-inspection. #### Missing Headers The driver uses many APIs without including the proper headers: - `struct drm_bridge`, `drm_bridge_add`, `drm_bridge_attach`, etc. =E2=80= =94 needs `#include ` - `struct regulator_bulk_data`, `regulator_enable/disable` =E2=80=94 needs = `#include ` - `module_i2c_driver`, `MODULE_*` =E2=80=94 needs `#include ` - `mutex_init`, `mutex_lock` =E2=80=94 needs `#include ` - `msleep`, `usleep_range` =E2=80=94 needs `#include ` - `kzalloc`, `kfree` =E2=80=94 needs `#include ` The driver only includes ``, ``, ``, ``, ``, and ``= . It compiles by accident through transitive includes, which is fragile. #### Design / Architecture Issues 4. **Firmware upgrade during probe is problematic.** The driver does synchr= onous firmware loading (`request_firmware`) and flash programming during `p= robe()`. This blocks the boot path and is generally discouraged. Use `reque= st_firmware_nowait` or a sysfs trigger, or at minimum document why synchron= ous loading is required. The firmware upgrade also erases and reprograms th= e chip's flash every time the firmware version reads as 0, with no user con= trol. 5. **`lt7911exc_block_erase` ignores errors.** `regmap_multi_reg_write` can= fail, but the return value is silently discarded. Then the code sleeps 200= ms and assumes success. Same issue in `lt7911exc_prog_init`. 6. **`cal_crc32_custom` has no bounds check.** If `length` is not a multipl= e of 4, the function reads past the end of `data` since it accesses `data[i= +3]` etc. The caller happens to pass `FW_SIZE - 4` which is a multiple of 4= , but the function itself is fragile. Also, using `u64` for the length/inde= x of a 64KB buffer is unnecessary =E2=80=94 `size_t` would be appropriate. 7. **`lt7911exc_regulator_enable/disable` should use `regulator_bulk_enable= /disable`.** The manual sequencing with individual `regulator_enable` calls= and the manual cleanup in the error path is exactly what the bulk APIs han= dle. 8. **Inconsistent i2c_device_id format.** The table has a space in the wron= g place: ```c {"lontium, lt7911exc", 0}, ``` This should be `"lontium,lt7911exc"` (no space) to match the compatible str= ing in the DT binding, or better yet just drop the `i2c_device_id` table en= tirely since DT matching via `of_match_table` is sufficient and the i2c_dev= ice_id is not used for DT-based probing. 9. **`lt7911exc_attach` uses `lt7911exc->bridge.encoder` instead of the `en= coder` parameter.** While these should be the same value at the point `atta= ch` is called, it's cleaner and more correct to use the parameter directly: ```c return drm_bridge_attach(lt7911exc->bridge.encoder, lt7911exc->panel_bridge, <7911exc->bridge, flags); ``` Should be: ```c return drm_bridge_attach(encoder, lt7911exc->panel_bridge, <7911exc->bridge, flags); ``` 10. **`lt7911exc_disable` is a no-op with just a sleep.** If the only purpo= se is a delay, this should be documented. A bare `msleep(20)` with a vague = comment "Delay after panel is disabled" is not informative. Consider whethe= r this delay actually belongs here or if the hardware requires it (and if s= o, document the requirement). 11. **`lt7911exc_upgrade_result` reads CRC from address 0x22** =E2=80=94 th= is is a raw page-relative register address, not a full 16-bit address. Give= n the regmap range configuration, this will read from page 0 register 0x22.= If this is intentional it should have a comment; if not, it's a bug (the o= ther register accesses use full 16-bit addresses like `0xe081`). 12. **`goto retry` pattern.** Using goto for retry logic in probe is unusua= l and harder to follow. A simple loop with a retry count would be clearer. 13. **`enabled` flag is racy.** The `enabled` bool is checked/set in `pre_e= nable`/`post_disable` without any locking. While DRM serializes bridge call= s in practice, the field pattern is fragile. 14. **Memory leak on firmware upgrade failure paths.** In `lt7911exc_firmwa= re_upgrade`, if `lt7911exc_write_data` fails, the function returns without = calling `release_firmware`. The caller `lt7911exc_probe` checks `lt7911exc-= >fw` in the error path, so this is partially handled, but the cleanup logic= is split across two functions making it easy to miss. 15. **`lt7911exc_post_disable` sets reset GPIO to 0 (deasserted) after disa= bling regulators.** Setting GPIO state after cutting power to the chip is p= ointless and potentially harmful depending on the board design (leaking cur= rent through GPIO into unpowered chip). #### Minor Style Issues - Indentation error in `cal_crc32_custom`: the closing brace of the for loo= p uses a tab+tab indent instead of one tab. - The `bridge_to_lt7911exc` inline function has unusual line-breaking with = excessive indentation. - Mixed use of `dev_err`/`dev_dbg` =E2=80=94 some error paths silently retu= rn without logging. --- Generated by Claude Code Patch Reviewer