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 Lontiun LT7911EXC eDP to MIPI DSI bridge Date: Tue, 05 May 2026 11:58:23 +1000 Message-ID: In-Reply-To: <20260429040541.3404116-3-syyang@lontium.com> References: <20260429040541.3404116-1-syyang@lontium.com> <20260429040541.3404116-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 #### Major Issues **1. Missing atomic state management helpers.** The bridge uses `atomic_pre= _enable`, `atomic_disable`, and `atomic_post_disable` callbacks, but does n= ot provide the required state helpers: ```c static const struct drm_bridge_funcs lt7911exc_bridge_funcs =3D { .attach =3D lt7911exc_attach, .atomic_pre_enable =3D lt7911exc_atomic_pre_enable, .atomic_disable =3D lt7911exc_atomic_disable, .atomic_post_disable =3D lt7911exc_atomic_post_disable, }; ``` Every other bridge driver using atomic callbacks provides these (see `lonti= um-lt9611.c`): ```c .atomic_duplicate_state =3D drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state =3D drm_atomic_helper_bridge_destroy_state, .atomic_reset =3D drm_atomic_helper_bridge_reset, ``` Without them, the atomic state machine cannot function correctly. **2. No MIPI DSI device registration.** The bridge outputs MIPI DSI but nev= er registers as a DSI device. Other Lontium bridge drivers with DSI output = (e.g., lt9611, lt8912b) call `of_find_mipi_dsi_host_by_node()`, `devm_mipi_= dsi_device_register_full()`, and `devm_mipi_dsi_attach()`. Without this, th= e DSI link to the downstream panel cannot be established by the framework. **3. No `atomic_enable` or mode configuration.** The bridge has `atomic_pre= _enable` (power-on + reset) but no `atomic_enable`. There is no code to con= figure the bridge for a particular display mode =E2=80=94 no lane count, fo= rmat, timing, or link training. The bridge currently just powers up and hop= es the hardware auto-configures, which seems unlikely for an eDP-to-DSI con= verter. **4. Regulators left enabled after probe.** In `lt7911exc_probe`: ```c ret =3D regulator_bulk_enable(ARRAY_SIZE(lt7911exc->supplies), lt7911exc->s= upplies); // ... version check, possible fw upgrade ... lt7911exc_unlock(lt7911exc); lt7911exc->bridge.of_node =3D dev->of_node; devm_drm_bridge_add(dev, <7911exc->bridge); return 0; ``` The success path never disables the regulators. They stay enabled until the= first `atomic_post_disable`. The regulators should be disabled after the v= ersion/firmware check completes successfully, since `atomic_pre_enable` wil= l re-enable them when the display is actually used. #### Medium Issues **5. `lt7911exc_attach` uses `bridge.encoder` instead of the `encoder` para= meter:** ```c static int lt7911exc_attach(struct drm_bridge *bridge, struct drm_encoder *encoder, enum drm_bridge_attach_flags flags) { struct lt7911exc *lt7911exc =3D bridge_to_lt7911exc(bridge); return drm_bridge_attach(lt7911exc->bridge.encoder, lt7911exc->bridge.next= _bridge, <7911exc->bridge, flags); } ``` Other Lontium drivers use the `encoder` parameter directly: ```c return drm_bridge_attach(encoder, lt9611->next_bridge, bridge, flags); ``` Use `encoder` from the parameter and `bridge` directly rather than going th= rough the container. **6. `lt7911exc_block_erase` ignores `regmap_multi_reg_write` return value:= ** ```c regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(seq_write)); msleep(200); ``` A failed flash erase will silently proceed to write firmware data, corrupti= ng the device. The return value must be checked, and the function should re= turn `int`, not `void`. **7. `lt7911exc_prog_init` also ignores its regmap return value.** Same iss= ue =E2=80=94 a failed write during flash programming should abort the opera= tion. **8. `kzalloc` immediately overwritten with `memset(buffer, 0xff, ...)`:** ```c buffer =3D kzalloc(total_size, GFP_KERNEL); ... memset(buffer, 0xff, total_size); ``` Use `kmalloc` instead of `kzalloc` since the zeroed memory is immediately o= verwritten with 0xff. **9. `atomic_disable` only contains a sleep:** ```c static void lt7911exc_atomic_disable(struct drm_bridge *bridge, struct drm_= atomic_state *state) { msleep(20); } ``` If the delay is required for sequencing between panel disable and regulator= shutdown, it would be better placed in `atomic_post_disable` before the re= gulator disable, with a comment explaining why. A callback that only sleeps= with no bridge-specific action is suspect. #### Minor Issues **10. Missing headers.** The driver uses `msleep`/`usleep_range` but does n= ot include ``, and uses regulator APIs but does not include = ``. These likely compile via transitive include= s, but explicit includes are preferred practice. **11. `cal_crc32_custom` does not validate length alignment.** The function= assumes `length` is a multiple of 4 and reads `data[i+3]` without bounds c= hecking. While callers currently pass aligned lengths, the function should = either assert/document this requirement or handle the tail bytes: ```c static u32 cal_crc32_custom(const u8 *data, u64 length) ``` **12. `lt7911exc_read_version` redundantly writes OCM register.** It writes= `0xe0ee =3D 0x01`, but it's always called with the OCM lock already held (= which already wrote the same value in `lt7911exc_lock`). **13. Kconfig selects `DRM_PANEL` unnecessarily.** The driver does not use = any panel API (`drm_panel_*`). Remove this select. Also verify whether `DRM= _KMS_HELPER` is actually needed =E2=80=94 the driver doesn't appear to use = any KMS helper functions directly. **14. `#include ` is unnecessary.** This is an I2C= driver, not a platform driver. **15. `fw_version` stored but never exposed.** The firmware version is read= into `lt7911exc->fw_version` but never made available via sysfs, debugfs, = or dev_info. Consider at least logging it at probe time so users/developers= can verify the firmware level. --- Generated by Claude Code Patch Reviewer