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 LT9611C(EX/UXD) MIPI DSI to HDMI driver Date: Thu, 07 May 2026 13:46:47 +1000 Message-ID: In-Reply-To: <20260506095100.78998-3-syyang@lontium.com> References: <20260506095100.78998-1-syyang@lontium.com> <20260506095100.78998-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 Issues **1. Missing atomic state helper callbacks (will crash or fail to enable)** The driver sets `DRM_BRIDGE_OP_HDMI` and implements `atomic_enable`, but do= es not implement the required atomic state management callbacks. The existi= ng `lontium-lt9611.c` driver correctly provides these: ```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 these, the HDMI state helper infrastructure will not function corre= ctly. These must be added to `lt9611c_bridge_funcs`. **2. Double-free of IRQ: `devm_request_threaded_irq` + manual `free_irq`** The IRQ is allocated with `devm_request_threaded_irq` at line ~1453, but `f= ree_irq` is called manually in both the `err_remove_bridge` error path and = in `lt9611c_remove`: ```c err_remove_bridge: free_irq(client->irq, lt9611c); /* manual free of devm resource */ ``` ```c static void lt9611c_remove(struct i2c_client *client) { free_irq(client->irq, lt9611c); /* double-free: devm will also free */ ``` Either use `devm_request_threaded_irq` and let devm handle teardown, or use= `request_threaded_irq` with manual cleanup. Don't mix them. **3. Double-removal of bridge: `devm_drm_bridge_add` + manual `drm_bridge_r= emove`** Same pattern: `devm_drm_bridge_add` is called at line ~1476, but `drm_bridg= e_remove` is called in `err_remove_bridge`. `devm_drm_bridge_add` registers= a devm action that calls `drm_bridge_remove` automatically. The manual cal= l causes a double-removal. **4. of_node reference leak from `lt9611c_parse_dt` on failure** When `lt9611c_parse_dt` fails, the probe function returns directly: ```c ret =3D lt9611c_parse_dt(dev, lt9611c); if (ret) return dev_err_probe(dev, ret, "failed to parse device tree\n"); ``` But `lt9611c_parse_dt` may have already acquired references via `of_graph_g= et_remote_node` for `dsi0_node` (and possibly `dsi1_node`) before failing a= t `drm_of_find_panel_or_bridge`. Those references are never released becaus= e the code returns instead of jumping to `err_of_put`. The early returns be= fore the goto-based error cleanup section (for `lt9611c_gpio_init` and `lt9= 611c_regulator_init`) have the same problem if `lt9611c_parse_dt` succeeded. **5. `lt9611c_upgrade_result` returns bare `-1`** ```c if (crc_result !=3D fw_crc) { ... return -1; } ``` Should return a proper errno like `-EIO`. #### Moderate Issues **6. Missing `atomic_disable` callback** The driver has `atomic_enable` but no `atomic_disable`. Compare with the ex= isting lt9611 driver which implements both. Even if the on-chip MCU handles= everything, the framework expects symmetry. At minimum, consider if any st= ate needs to be cleaned up on disable (e.g., clearing infoframes). **7. `SET_SYSTEM_SLEEP_PM_OPS` is deprecated** ```c static const struct dev_pm_ops lt9611c_bridge_pm_ops =3D { SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend, lt9611c_bridge_resume) }; ``` Use the modern pattern (as `lontium-lt8912b.c` does): ```c static DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops, lt9611c_bridge_suspend, lt9611c_bridge_resume); ``` And reference it via `pm_sleep_ptr(<9611c_bridge_pm_ops)` in the driver s= truct. **8. `i2c_device_id` table missing `const`** ```c static struct i2c_device_id lt9611c_id[] =3D { ``` Should be `static const struct i2c_device_id`. **9. `lt9611c_block_erase` ignores erase failures** The busy-wait loop breaks after 50 iterations but doesn't return an error: ```c if (i > 50) break; ``` If flash erase fails, the driver proceeds to write firmware on top of unera= sed flash, silently producing corrupted firmware. The function should retur= n `int` and propagate the timeout. **10. `lt9611c_write_data` doesn't poll flash write completion** After `lt9611c_sram_to_flash`, there's no check that the page write complet= ed before proceeding to the next page. Compare with the erase path which at= least polls `read_flash_reg_status`. **11. Firmware sysfs attribute ignores input** ```c static ssize_t lt9611c_firmware_store(struct device *dev, struct device_att= ribute *attr, const char *buf, size_t len) { ... ret =3D lt9611c_firmware_upgrade(lt9611c); ``` The `buf` parameter is completely ignored. Any write triggers a full firmwa= re flash. Consider at minimum requiring a specific value (e.g., "1") to tri= gger the upgrade to prevent accidental writes. **12. Inconsistent use of `sizeof` vs `ARRAY_SIZE` for u8 arrays** In `lt9611c_hdmi_audio_prepare`: ```c ret =3D lt9611c_read_write_flow(lt9611c, audio_cmd, sizeof(audio_cmd), data, sizeof(data)); ``` Elsewhere the driver uses `ARRAY_SIZE`. For `u8[]` these are equivalent, bu= t using `ARRAY_SIZE` consistently avoids bugs if the type ever changes. #### Minor Issues **13. `lt9611c_regulator_init` has unnecessary temporary variable** ```c ret =3D devm_regulator_bulk_get(dev, 2, lt9611c->supplies); return ret; ``` Can just be `return devm_regulator_bulk_get(...)`. **14. Stale numbered comments in `lt9611c_firmware_upgrade`** ```c /*1. load firmware*/ ... /*2. check size*/ ... /*3. calculate crc8 */ ... /*5. check upgrade of result*/ ``` Comment 4 is missing (was presumably the write+erase step). Either fix the = numbering or remove these comments entirely. **15. `bridge_to_lt9611c_const` with `/*read only*/` comment** ```c /*read only*/ static const struct lt9611c *bridge_to_lt9611c_const(const struct drm_bridg= e *bridge) ``` The comment adds nothing =E2=80=94 the `const` qualifier already conveys th= e intent. Remove the comment. **16. Regmap error checking in `lt9611c_read_write_flow`** The `regmap_write` calls for the initial protocol handshake (writing to 0xe= 0de, writing parameters to 0xe0b0+i) don't check return values. A failed I2= C transaction here would go unnoticed. --- Generated by Claude Code Patch Reviewer