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: Thu, 04 Jun 2026 16:51:15 +1000 Message-ID: In-Reply-To: <20260529094547.869919-3-syyang@lontium.com> References: <20260529094547.869919-1-syyang@lontium.com> <20260529094547.869919-3-syyang@lontium.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **HIGH: `bridge->dev = NULL` in `.detach` is incorrect** ```c static void lt7911exc_bridge_detach(struct drm_bridge *bridge) { struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge); guard(mutex)(<7911exc->ocm_lock); bridge->dev = NULL; } ``` No other bridge driver in the kernel sets `bridge->dev = NULL` in its `.detach` callback. This field is managed by the DRM core. The motivation appears to be preventing `drm_kms_helper_hotplug_event(lt7911exc->bridge.dev)` in the firmware upgrade worker from being called after detach, but the correct approach is to check the driver's own state (e.g., the `removed` flag or a dedicated `attached` flag) rather than corrupting framework state. **HIGH: Firmware upgrade error paths leave MCU halted** In `lt7911exc_firmware_upgrade_work`, after `lt7911exc_hw_mcu_halt()` is called, the error paths for `lt7911exc_block_erase`, `lt7911exc_write_data`, and `lt7911exc_write_crc` all jump to `out_release_fw` without restarting the MCU or resetting the chip: ```c lt7911exc_reset(lt7911exc); mutex_lock(<7911exc->ocm_lock); lt7911exc_hw_mcu_halt(lt7911exc); mutex_unlock(<7911exc->ocm_lock); ret = lt7911exc_block_erase(lt7911exc); if (ret) { dev_err(dev, "failed to block erase.\n"); goto out_release_fw; /* MCU left halted, device bricked until next probe */ } ``` After a failed erase or write, the device is left with MCU halted and potentially partially-erased flash. A `lt7911exc_reset()` or `lt7911exc_hw_mcu_run()` call should be added to error recovery paths. **MEDIUM: Redundant `dev_set_drvdata` and `i2c_set_clientdata`** Both are called in probe: ```c dev_set_drvdata(dev, lt7911exc); ... i2c_set_clientdata(client, lt7911exc); ``` `i2c_set_clientdata(client, data)` is equivalent to `dev_set_drvdata(&client->dev, data)`, so the `dev_set_drvdata` call is redundant. Only `i2c_set_clientdata` is needed (and `i2c_get_clientdata` is already used in `remove`). **MEDIUM: Unchecked `regmap_write` return values in `lt7911exc_write_data`** The short-write handling and page commit in the write loop don't check return values: ```c //write method for less than LT_PAGE_SIZE bytes. if (page_len < LT_PAGE_SIZE) { regmap_write(lt7911exc->regmap, 0x5f, 0x05); regmap_write(lt7911exc->regmap, 0x5f, 0x01); //hardware requires delay usleep_range(1000, 2000); } regmap_write(lt7911exc->regmap, 0x5f, 0x00); ``` All three `regmap_write` calls should have their return values checked, especially during a flash programming sequence where an I2C failure could corrupt firmware. **MEDIUM: Sysfs attribute naming convention** ```c static DEVICE_ATTR_RW(lt7911exc_firmware); ``` The sysfs attribute will be named `lt7911exc_firmware`. The kernel convention is to not prefix attributes with the driver name since they already live under the device's sysfs directory. Something like `firmware_upgrade` (with a matching `firmware_upgrade_store`/`firmware_upgrade_show`) would be more conventional. **MEDIUM: `sysfs_emit` format mismatch for firmware version** ```c return sysfs_emit(buf, "0x%04x\n", version); ``` But `lt7911exc_read_version` returns a 3-byte value `(buf[0] << 16) | (buf[1] << 8) | buf[2]`, which can be up to 0xFFFFFF (6 hex digits). The `%04x` format would display all digits but the `04` minimum width padding suggests the author expects 4 digits. Use `0x%06x` if the version is always 3 bytes. **LOW: Kconfig help text formatting** ``` chip.The LT7911EXC converts eDP input to MIPI ``` Missing space after the period: should be `chip. The LT7911EXC`. **LOW: C++ style comments** Several places use `//` comments which violate kernel coding style: ```c //enable mipi stream ... //disable mipi stream ... //write method for less than LT_PAGE_SIZE bytes. ... //hardware requires delay ``` These should all use `/* */` style comments per the kernel coding standard. **LOW: `u64` types for small address values** `lt7911exc_prog_init` and `lt7911exc_write_data` use `u64 addr` and `u64 size`/`u64 offset`, but the flash is only 64KB (`FW_SIZE = 64 * 1024`). `u32` is more than sufficient and avoids unnecessary widening on 32-bit platforms. **LOW: Missing explicit `#include `** The driver uses `INIT_WORK`, `schedule_work`, and `cancel_work_sync` but does not directly include ``. It works via transitive includes but should be explicit. **OBSERVATION: `lt7911exc_block_erase` redundantly halts MCU** `lt7911exc_block_erase` internally writes `0xee = 0x01` (MCU halt), but the caller `lt7911exc_firmware_upgrade_work` already calls `lt7911exc_hw_mcu_halt` before `lt7911exc_block_erase`. This isn't a bug but suggests the erase function's register sequence was written independently of the surrounding flow. Consider whether the redundant halt is intentional for robustness or an oversight. **OBSERVATION: `lt7911exc_upgrade_result` error message** ```c dev_err(dev, "Failed to switch to page 0xe0 in prog_init: %d\n", ret); ``` The function is `lt7911exc_upgrade_result`, not `prog_init`. This looks like a copy-paste artifact from `lt7911exc_prog_init`. --- Generated by Claude Code Patch Reviewer