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: Mon, 25 May 2026 22:53:58 +1000 Message-ID: In-Reply-To: <20260519135816.26996-3-syyang@lontium.com> References: <20260519135816.26996-1-syyang@lontium.com> <20260519135816.26996-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 (by severity):** #### 1. Firmware data / CRC mismatch (bug) The CRC is computed on a zero-padded copy of the firmware: ```c buffer =3D kmalloc(total_size, GFP_KERNEL); ... memset(buffer, 0xff, total_size); memcpy(buffer, fw->data, fw->size); crc32 =3D cal_crc32_custom(buffer, total_size); kfree(buffer); ``` But `lt7911exc_write_data` writes from the original `fw`, not from `buffer`: ```c ret =3D lt7911exc_write_data(lt7911exc, fw, 0); ``` Inside `lt7911exc_write_data`, only `fw->size` bytes are written. The flash= region from `fw->size` to `total_size` will contain whatever was there aft= er the erase (typically 0xff from a flash erase, which matches the `memset`= padding). So this _might_ work in practice if the erase sets bytes to 0xff= , but it's fragile =E2=80=94 the correctness depends on an implicit assumpt= ion about the erase pattern. Either write the padded `buffer` instead of `f= w->data`, or compute the CRC only over `fw->size` bytes. As written, this i= s a latent correctness bug. #### 2. `regmap_raw_write` vs `regmap_noinc_write` for firmware FIFO writes ```c ret =3D regmap_raw_write(lt7911exc->regmap, 0xe05d, &data[offset], page_len= ); ``` Register `0xe05d` appears to be a FIFO data port (all page bytes go to the = same address). The sister driver `lontium-lt9611uxc.c` uses `regmap_noinc_w= rite` for the same pattern. `regmap_raw_write` will auto-increment the regi= ster address, which is incorrect for a FIFO. This should be `regmap_noinc_w= rite`. The same issue applies to `lt7911exc_write_crc`: ```c ret =3D regmap_raw_write(lt7911exc->regmap, 0xe05d, crc, 4); ``` #### 3. `lt7911exc_prog_init` uses `u64` for a 24-bit address ```c static int lt7911exc_prog_init(struct lt7911exc *lt7911exc, u64 addr) ``` The address is masked to 24 bits (`addr >> 16 & 0xff`, etc.), so `u32` is s= ufficient and more appropriate. Similarly, `lt7911exc_write_data` and `lt79= 11exc_write_crc` use `u64 addr` =E2=80=94 all should be `u32`. #### 4. `lt7911exc_write_data` takes `const struct firmware *` but only nee= ds `data`/`size` The function signature couples it to the firmware API, yet after the CRC co= mputation the padded buffer is freed and lost. If the intent is to always w= rite the padded image (see issue 1), the function should take the buffer di= rectly. #### 5. Redundant `dev_set_drvdata` and `i2c_set_clientdata` In `lt7911exc_probe`: ```c dev_set_drvdata(dev, lt7911exc); ... i2c_set_clientdata(client, lt7911exc); ``` For I2C devices, `i2c_set_clientdata(client, data)` calls `dev_set_drvdata(= &client->dev, data)`, so these are redundant. Only `i2c_set_clientdata` is = needed (it covers both the sysfs `dev_get_drvdata` path and the `i2c_get_cl= ientdata` path in `remove`). The reference driver `lontium-lt8713sx.c` uses= only `i2c_set_clientdata`. #### 6. Version read without MCU halt departs from established pattern ```c static int lt7911exc_read_version(struct lt7911exc *lt7911exc) { ... /* no need to halt MCU for this register access */ ret =3D regmap_bulk_read(lt7911exc->regmap, 0xe081, buf, ARRAY_SIZE(buf)); ``` The sister driver `lontium-lt9611uxc.c` always halts the MCU before reading= the version register. The comment claims this is safe but provides no hard= ware rationale. If the register truly is readable without MCU halt, a brief= explanation of why (e.g., "read-only status register not updated by MCU") = would be valuable. #### 7. `lt7911exc_dsi_host_transfer` whitelist does nothing The transfer function accepts specific DSI write message types via a switch= /case whitelist, but then unconditionally returns `msg->tx_len` without act= ually sending any data: ```c switch (msg->type) { case MIPI_DSI_DCS_SHORT_WRITE: ... break; default: return -EOPNOTSUPP; } ... return msg->tx_len; ``` The comment says the internal firmware handles DSI commands, so this is ess= entially a no-op stub. The whitelist filtering adds code without adding val= ue =E2=80=94 if nothing is actually transmitted, all types could be silentl= y accepted (or all rejected). The current middle ground is misleading. A si= ngle-line comment like `/* firmware handles all panel init; silently accept= writes */` before a direct `return msg->tx_len` would be clearer. #### 8. `lt7911exc_prog_init` redundant `return 0` ```c ret =3D regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(seq= _write)); if (ret) return ret; return 0; ``` This can be simplified to `return regmap_multi_reg_write(...)`. #### 9. Kconfig help text formatting ``` chip.The LT7911EXC converts eDP input to MIPI ``` Missing space after period: should be `chip. The LT7911EXC`. #### 10. Comment style Several C++-style comments remain: ```c //enable mipi stream //disable mipi stream //hardware requires delay ``` Kernel coding style requires `/* */` comments. These should be `/* enable m= ipi stream */` etc. #### 11. `lt7911exc_write_data` unchecked return values for partial page ha= ndling ```c if (page_len < LT_PAGE_SIZE) { regmap_write(lt7911exc->regmap, 0xe05f, 0x05); regmap_write(lt7911exc->regmap, 0xe05f, 0x01); ... } regmap_write(lt7911exc->regmap, 0xe05f, 0x00); ``` These `regmap_write` calls don't check return values, whereas other similar= calls in the function do. Consistency would be good, even if errors here a= re unlikely. #### 12. `lt7911exc_write_crc` unchecked return values Same pattern =E2=80=94 several `regmap_write` calls are not checked: ```c regmap_write(lt7911exc->regmap, 0xe05f, 0x01); regmap_write(lt7911exc->regmap, 0xe05a, (addr >> 16) & 0xff); ... ``` #### 13. `struct device_node *np` usage ```c struct device_node *np =3D dev->of_node; ... if (!np) return -ENODEV; ``` The `np` variable is only used for the NULL check and one assignment (`lt79= 11exc->bridge.of_node =3D np`). The OF-matching already guarantees `of_node= ` is non-NULL when probe runs, so the check is unnecessary. If kept, `dev->= of_node` could be used directly without the local variable. #### 14. `lt7911exc_block_erase` redundantly halts MCU ```c static int lt7911exc_block_erase(struct lt7911exc *lt7911exc) { ... const struct reg_sequence seq_write[] =3D { REG_SEQ0(0xe0ee, 0x01), /* MCU halt */ ``` This function writes 0x01 to the MCU halt register (0xe0ee), but the caller= (`lt7911exc_firmware_upgrade_work`) already calls `lt7911exc_hw_mcu_halt()= ` before `lt7911exc_block_erase()`. This is harmless (idempotent write) but= suggests the register sequence was copied verbatim from a vendor reference= without cleanup. --- Generated by Claude Code Patch Reviewer