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 16:53:54 +1000 Message-ID: In-Reply-To: <20260525010545.9470-3-syyang@lontium.com> References: <20260525010545.9470-1-syyang@lontium.com> <20260525010545.9470-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 **Critical / Functional Issues:** 1. **`lt7911exc_write_data` does not advance the address for `prog_init`** = =E2=80=94 the `addr` parameter passed into `lt7911exc_prog_init` is always = `0` (the initial value from the caller). Only the local `addr` at the botto= m of the loop is incremented. But `lt7911exc_prog_init` receives the functi= on parameter `addr`, not a local tracking variable: ```c static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct f= irmware *fw, u64 addr) { ... for (num =3D 0; num < page; num++) { offset =3D num * LT_PAGE_SIZE; ... ret =3D lt7911exc_prog_init(lt7911exc, addr); ... addr +=3D LT_PAGE_SIZE; } ``` Wait =E2=80=94 re-reading more carefully, `addr` *is* the function paramete= r and it is modified in the loop (`addr +=3D LT_PAGE_SIZE`). So this is act= ually correct since C passes by value and `addr` is used as a local accumul= ator. My mistake =E2=80=94 this is fine. 2. **Goto jumps skip `out_unlock` label on early exits** =E2=80=94 In `lt79= 11exc_firmware_upgrade_work`, the `goto out_release_fw` path (firmware too = large) and `goto out_clear_status` (request_firmware failure) both skip the= `out_unlock` label. This is correct since `upgrade_lock` is only acquired = after those checks. However, the `buffer` allocation failure path (`goto ou= t_release_fw`) also skips `out_clear_status` =E2=80=94 actually no, the lab= els are ordered so `out_release_fw` falls through to `out_clear_status`. Le= t me re-examine: ```c out_unlock: ... mutex_unlock(<7911exc->upgrade_lock); if (lt7911exc->bridge.dev) drm_kms_helper_hotplug_event(lt7911exc->bridge.dev); out_release_fw: release_firmware(fw); out_clear_status: mutex_lock(<7911exc->ocm_lock); if (!lt7911exc->removed) lt7911exc->upgrade =3D false; mutex_unlock(<7911exc->ocm_lock); ``` This is correct =E2=80=94 the labels cascade. Good. 3. **`kvfree(buffer)` is called before the firmware data is fully consumed*= * =E2=80=94 The buffer is freed immediately after computing the CRC, but th= en `lt7911exc_write_data(lt7911exc, fw, 0)` uses `fw->data` directly, not t= he padded buffer. This means the CRC is computed over the padded data (firm= ware + 0xff padding to `FW_SIZE - 4`), but only the raw firmware bytes are = written to flash. The hardware CRC verification in `lt7911exc_upgrade_resul= t` reads back what was written. **If the flash does not already contain 0xf= f in the padded region, the CRC will mismatch.** This is probably fine beca= use `lt7911exc_block_erase` erases the entire block first (erased flash is = typically 0xff), but the logic is fragile and implicit. Worth a comment at = minimum. **Medium Issues:** 4. **Unchecked `regmap_write` return values in multiple places** =E2=80=94 = Several functions call `regmap_write` without checking the return value: - `lt7911exc_write_crc`: First four `regmap_write` calls and last three= are unchecked. - `lt7911exc_upgrade_result`: `regmap_write` calls to 0xe0ee, 0xe07b ar= e unchecked. - `lt7911exc_write_data`: The `regmap_write` calls for the short-page h= andling and the final `0xe05f` write are unchecked. While regmap-over-I2C errors are rare, silently continuing after a fail= ed register write during firmware programming could corrupt the flash. 5. **`lt7911exc_write_data` uses `u64` for sizes/addresses but firmware is = max 64KB** =E2=80=94 The `addr` parameter and `size`/`offset` locals are al= l `u64`, but `FW_SIZE` is `64 * 1024`. Using `u32` or even `unsigned int` w= ould be more appropriate and avoids confusion. Similarly, `lt7911exc_prog_i= nit` takes `u64 addr`. The register writes only use the low 24 bits anyway = (`(addr >> 16) & 0xff` etc.), so a `u64` is misleading. 6. **`lt7911exc_read_version` returns a negative error code or a positive v= ersion packed into `int`** =E2=80=94 The version is `(buf[0] << 16) | (buf[= 1] << 8) | buf[2]`, which could set bit 23 if `buf[0] >=3D 0x80`, producing= a negative value indistinguishable from an error code. This would cause a = spurious probe failure. Consider using `unsigned int` for the version, or m= asking to ensure positivity. 7. **`dev_set_drvdata` and `i2c_set_clientdata` are both called in probe** = =E2=80=94 For an I2C driver, `dev_set_drvdata(&client->dev, ...)` and `i2c_= set_clientdata(client, ...)` are the same thing. One of them is redundant. = The remove path uses `i2c_get_clientdata`, so keep that one and drop `dev_s= et_drvdata`. (Actually, the sysfs show/store callbacks use `dev_get_drvdata= ` on the I2C client's dev, so they are equivalent =E2=80=94 but having both= is confusing.) 8. **Kconfig help text formatting** =E2=80=94 Missing space after period: ``` chip.The LT7911EXC converts eDP input to MIPI ``` Should be `chip. The LT7911EXC...`. **Minor / Style Issues:** 9. **Comment style** =E2=80=94 A couple of `//` (C++) style comments remain: ```c //enable mipi stream //disable mipi stream //hardware requires delay ``` The kernel coding style requires `/* ... */` comments. These should be conv= erted. 10. **`lt7911exc_prog_init` has a redundant return pattern:** ```c ret =3D regmap_multi_reg_write(...); if (ret) return ret; return 0; ``` This can be simply `return regmap_multi_reg_write(...)`. 11. **`struct device_node *np` usage in probe** =E2=80=94 `np` is only used= for the NULL check and assigning `bridge.of_node`. Since this is an OF-dep= endent driver (`depends on OF`), the `if (!np) return -ENODEV` check is red= undant. And `bridge.of_node =3D np` could be `bridge.of_node =3D dev->of_no= de` directly, eliminating the variable. 12. **The `lt7911exc_dsi_host_transfer` accepted message types have no inde= ntation in the switch body:** ```c case MIPI_DSI_DCS_SHORT_WRITE: case MIPI_DSI_DCS_SHORT_WRITE_PARAM: ... break; default: return -EOPNOTSUPP; ``` The `break` and the `default` body should be indented one level under their= case labels per kernel coding style. 13. **The `cal_crc32_custom` function name** =E2=80=94 "cal" is an unusual = abbreviation. Consider `lt7911exc_calc_crc32` to be consistent with driver = namespace and clearer naming. 14. **`platform_device.h` is included but not used** =E2=80=94 This is an I= 2C driver, not a platform driver. The include should be removed. **Summary:** The driver is close but needs attention to unchecked regmap wr= ites in the firmware programming path, the `u64`/`u32` type confusion, the = version-sign issue, and the coding style issues (C++ comments, switch inden= tation, Kconfig typo). The overall locking design is workable but complex f= or a v12 submission =E2=80=94 it would benefit from clearer documentation o= f which lock protects what invariant. --- Generated by Claude Code Patch Reviewer