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: Sat, 16 May 2026 09:43:28 +1000 Message-ID: In-Reply-To: <20260515080934.9870-3-syyang@lontium.com> References: <20260515080934.9870-1-syyang@lontium.com> <20260515080934.9870-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 **Kconfig issues:** 1. Missing space in help text: ```c chip.The LT7911EXC converts eDP input to MIPI ``` Should be `chip. The LT7911EXC...`. 2. Consider adding `select DRM_MIPI_DSI` =E2=80=94 the driver implements a = `mipi_dsi_host` and uses MIPI DSI APIs. **Struct formatting:** 3. Inconsistent closing brace indentation in `lt7911exc_dsi_output`: ```c struct lt7911exc_dsi_output { struct mipi_dsi_device *dev; struct drm_panel *panel; struct drm_bridge *bridge; }; ``` The `};` has an extra tab. Should be flush left. **`fw_version` sysfs display bug:** 4. `lt7911exc_read_version` returns a 24-bit value: ```c return (buf[0] << 16) | (buf[1] << 8) | buf[2]; ``` But `lt7911exc_firmware_show` formats it as: ```c return sysfs_emit(buf, "0x%04x\n", lt7911exc->fw_version); ``` `%04x` pads to 4 hex digits minimum but a 24-bit version (e.g., `0x010203`)= needs 6 hex digits. Use `"0x%06x\n"` to match the 3-byte version, or `"0x%= x\n"` if there's no fixed width convention. **Unnecessary `u64` types:** 5. Several functions use `u64` for addresses and sizes that are at most 64K= B: ```c static u32 cal_crc32_custom(const u8 *data, u64 length) static int lt7911exc_prog_init(struct lt7911exc *lt7911exc, u64 addr) static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct f= irmware *fw, u64 addr) ``` These should be `size_t` for lengths and `u32` for addresses. Firmware is c= apped at `FW_SIZE` (64KB), so `u64` is misleading. This also causes mixed-t= ype arithmetic in `lt7911exc_write_data`: ```c u64 size, offset; ... page =3D (size + LT_PAGE_SIZE - 1) / LT_PAGE_SIZE; ``` Here `page` is `int` but the RHS is `u64`, resulting in silent truncation. **`cal_crc32_custom` fragility:** 6. The function accesses `data[i+3]` without checking that `length` is a mu= ltiple of 4: ```c for (i =3D 0; i < length; i +=3D 4) { buf[0] =3D data[i + 3]; ``` Currently all callers pass `FW_SIZE - 4 =3D 65532` which is divisible by 4,= so there's no actual bug. But the function is fragile =E2=80=94 consider a= dding a `WARN_ON(length % 4)` or a comment documenting the requirement. **Unchecked `regmap_write` return values:** 7. Several `regmap_write` calls in `lt7911exc_write_crc` ignore return valu= es: ```c regmap_write(lt7911exc->regmap, 0xe05f, 0x01); regmap_write(lt7911exc->regmap, 0xe05a, (addr >> 16) & 0xff); regmap_write(lt7911exc->regmap, 0xe05b, (addr >> 8) & 0xff); regmap_write(lt7911exc->regmap, 0xe05c, addr & 0xff); ``` Same issue in `lt7911exc_write_data` for the short-page handling path: ```c if (page_len < LT_PAGE_SIZE) { regmap_write(lt7911exc->regmap, 0xe05f, 0x05); regmap_write(lt7911exc->regmap, 0xe05f, 0x01); ``` And in `lt7911exc_upgrade_result`: ```c regmap_write(lt7911exc->regmap, 0xe0ee, 0x01); regmap_write(lt7911exc->regmap, 0xe07b, 0x60); regmap_write(lt7911exc->regmap, 0xe07b, 0x40); ``` These should check return values or at minimum use `regmap_multi_reg_write`= as done in the erase function for consistency. **`lt7911exc_lock`/`lt7911exc_unlock` vs. internal OCM register writes:** 8. `lt7911exc_lock` writes `0xe0ee =3D 0x01` to stop the on-chip MCU, but `= lt7911exc_block_erase` and `lt7911exc_prog_init` also write `0xe0ee =3D 0x0= 1` via their `seq_write` arrays: ```c const struct reg_sequence seq_write[] =3D { REG_SEQ0(0xe0ee, 0x01), /* redundant when caller holds lock */ REG_SEQ0(0xe054, 0x01), ``` This is harmless but confusing =E2=80=94 the OCM lock is managed at two dif= ferent layers. Consider removing the redundant `0xe0ee` writes from the fir= mware functions since the caller always holds the lock, or document that th= ese functions must not be called without the lock held. **Redundant `dev_set_drvdata` / `i2c_set_clientdata`:** 9. In probe: ```c dev_set_drvdata(dev, lt7911exc); ... i2c_set_clientdata(client, lt7911exc); ``` `i2c_set_clientdata` calls `dev_set_drvdata(&client->dev, data)` internally= , so this is redundant. One call to `i2c_set_clientdata` is sufficient (and= is what `lt7911exc_remove` uses to retrieve it). **Comment style:** 10. Missing spaces after `/*` in comments: ```c /*3. calculate crc32 */ /*4. firmware upgrade */ /*5. check upgrade of result */ ``` Should be `/* 3.` etc. **`lt7911exc_prog_init` unnecessary early return:** 11. The function ends with: ```c ret =3D regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(se= q_write)); if (ret) return ret; return 0; ``` This is just `return regmap_multi_reg_write(...)`. **Firmware upgrade safety:** 12. `lt7911exc_firmware_store` can be triggered while the display pipeline = is active. The function takes `ocm_lock`, resets the chip, erases flash, an= d reprograms firmware =E2=80=94 all while a display may be running. After t= he upgrade completes and the lock is released, the bridge is in an undefine= d state relative to the DRM pipeline. There's no check for whether the brid= ge is currently attached/enabled. Other drivers with similar sysfs firmware= interfaces (lt9611uxc) have the same issue, so this may be accepted practi= ce, but it's worth noting. **`lt7911exc_dsi_host_transfer` semantics:** 13. The transfer function doesn't actually forward DSI messages =E2=80=94 i= t waits for video input and enables DSI output: ```c ret =3D regmap_read_poll_timeout(lt7911exc->regmap, 0xe084, val, (val & 0x= 01), 5000, 50000); ... ret =3D regmap_write(lt7911exc->regmap, 0xe0b0, 0x01); ... return msg->tx_len; ``` Every DSI transfer from the panel driver will re-poll the video-ready regis= ter (up to 50ms) and re-write the output enable register. If a panel driver= sends multiple DCS init commands, this could add significant latency. Is t= he intent that only a single transfer ever happens (i.e., the panel driver = sends exactly one DCS command to trigger output)? If so, this should be doc= umented. If not, the enable should happen once and subsequent transfers sho= uld be no-ops. **Missing `atomic_enable` callback:** 14. The bridge provides `atomic_pre_enable` and `atomic_disable`/`atomic_po= st_disable` but no `atomic_enable`. The enable logic appears to be implicit= ly handled through the DSI host transfer mechanism, but this coupling betwe= en bridge lifecycle and DSI host transfer is unusual and could use a commen= t explaining the design. **Minor: `#include `:** 15. This is an I2C driver, not a platform driver. The `platform_device.h` i= nclude appears unused. **Minor: `np` check:** 16. In probe: ```c if (!np) return -ENODEV; ``` The driver has `depends on OF` in Kconfig and the `of_match_table` is set, = so `of_node` will always be non-NULL when probe is called via DT matching. = This check is unnecessary. **Summary:** The driver structure is sound and follows existing patterns (e= specially the tc358768 model for DSI-host bridges). The main items to addre= ss are: the sysfs version format bug (#4), unchecked regmap return values (= #7), the u64/type-width cleanup (#5), and the struct indentation (#3). The = rest are minor improvements and style points. --- Generated by Claude Code Patch Reviewer