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: Tue, 05 May 2026 10:47:37 +1000 Message-ID: In-Reply-To: <20260430094612.3408174-3-syyang@lontium.com> References: <20260430094612.3408174-1-syyang@lontium.com> <20260430094612.3408174-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 This patch has significant issues: **Critical: Missing atomic state helpers** The `lt7911exc_bridge_funcs` provides `atomic_pre_enable`, `atomic_disable`= , and `atomic_post_disable` but does **not** provide the mandatory `atomic_= duplicate_state`, `atomic_destroy_state`, and `atomic_reset` callbacks. Per= `include/drm/drm_bridge.h`, these are "mandatory if the bridge implements = any of the atomic hooks." Compare with the sibling driver `lontium-lt9611.c= ` which correctly provides all three using the standard helpers: ```c static const struct drm_bridge_funcs lt7911exc_bridge_funcs =3D { .attach =3D lt7911exc_attach, .atomic_pre_enable =3D lt7911exc_atomic_pre_enable, .atomic_disable =3D lt7911exc_atomic_disable, .atomic_post_disable =3D lt7911exc_atomic_post_disable, + .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, }; ``` This will need `#include `. **Critical: No MIPI DSI host registration** The driver claims to output MIPI DSI but never calls `of_find_mipi_dsi_host= _by_node()`, `devm_mipi_dsi_device_register_full()`, or `devm_mipi_dsi_atta= ch()`. Without registering as a DSI device with the downstream DSI host, th= e MIPI output will not function. Every other Lontium bridge driver with DSI= output implements this =E2=80=94 see `lt9611uxc_attach_dsi()` in `lontium-= lt9611uxc.c:248-279` for the pattern. This is fundamental missing functiona= lity. **Critical: No mode detection or mode validation** The driver has no `mode_valid`, `mode_fixup`, `edid_read`, `detect`, or `at= omic_get_input_bus_fmts` callbacks. This means the bridge cannot: - Report what modes the connected eDP source provides - Validate or constrain the display mode - Report the input bus format Without these, the display pipeline has no way to negotiate a valid mode th= rough this bridge. **Kconfig issues** ```c select DRM_PANEL select DRM_KMS_HELPER ``` `DRM_PANEL` is selected but never used anywhere in the driver =E2=80=94 the= re is no panel reference or panel bridge. `DRM_KMS_HELPER` is questionable;= the driver doesn't directly use KMS helpers. On the other hand, `depends o= n I2C` is missing (the driver is an I2C driver), and `select DRM_MIPI_DSI` = should be added since the driver includes `` (and shoul= d be using DSI APIs). **Missing `#include `** The driver uses `msleep()` and `usleep_range()` but does not include ``. It may compile by accident via transitive includes, but the in= clude should be explicit. **Regulators left enabled after probe success** ```c ret =3D regulator_bulk_enable(ARRAY_SIZE(lt7911exc->supplies), lt7911exc->s= upplies); // ... firmware check ... lt7911exc_unlock(lt7911exc); lt7911exc->bridge.of_node =3D dev->of_node; devm_drm_bridge_add(dev, <7911exc->bridge); return 0; // regulators still enabled! ``` On the success path, regulators are enabled for the firmware version check = but never disabled. They remain on until `atomic_post_disable` is eventuall= y called. Regulators should be disabled after the probe-time version check = and only re-enabled in `atomic_pre_enable`. **Firmware upgrade runs synchronously in probe** `lt7911exc_firmware_upgrade()` erases flash and writes 64KB of firmware wit= h 32-byte pages, each requiring I2C transactions. This is slow and blocks t= he boot process. Consider deferring this to a workqueue or using `request_f= irmware_nowait()`. **Error handling gaps in register write sequences** Multiple functions ignore `regmap_write` / `regmap_multi_reg_write` return = values: - `lt7911exc_block_erase()` =E2=80=94 ignores `regmap_multi_reg_write` retu= rn - `lt7911exc_lock()` =E2=80=94 ignores `regmap_write` return - `lt7911exc_unlock()` =E2=80=94 ignores `regmap_write` return =20 - `lt7911exc_upgrade_result()` =E2=80=94 ignores several `regmap_write` ret= urns - `lt7911exc_write_crc()` =E2=80=94 ignores most `regmap_write` returns If the I2C bus has errors, the firmware flash will silently corrupt. These = should check and propagate errors. **`kzalloc` + `memset` redundancy** ```c buffer =3D kzalloc(total_size, GFP_KERNEL); if (!buffer) { ... } memset(buffer, 0xff, total_size); ``` `kzalloc` zeroes the buffer, then `memset` immediately overwrites with `0xf= f`. Use `kmalloc` instead to avoid the pointless zeroing. **Firmware version format string mismatch** ```c dev_err(dev, "fw version 0x%04x, update failed\n", lt7911exc->fw_version); ``` `fw_version` is constructed from 3 bytes (`(buf[0] << 16) | (buf[1] << 8) |= buf[2]`), making it a 24-bit value. The format `0x%04x` only prints 4 hex = digits (16 bits), truncating the high byte. Should be `0x%06x`. **`u64` types used unnecessarily** `lt7911exc_prog_init()` takes `u64 addr` and `lt7911exc_write_data()` uses = `u64 size, offset`, but the maximum address is `FW_SIZE` (65536) and the ma= ximum register is `0xe8ff`. These should all be `u32` to match the actual d= ata widths and avoid implying 64-bit addressing. **`cal_crc32_custom` is unsafe for non-multiple-of-4 lengths** ```c for (i =3D 0; i < length; i +=3D 4) { buf[0] =3D data[i + 3]; // reads data[length-1+3] on last iteration ``` If `length` is not a multiple of 4, this reads past the end of the buffer. = The caller currently pads to `FW_SIZE - 4 =3D 65532` which is divisible by = 4, so this works in practice, but the function should either validate or do= cument the alignment requirement. **Comment style violations** ```c /*1. load firmware*/ /*2. check size*/ ``` Kernel style requires spaces: `/* 1. load firmware */`. Also the `//hardwar= e requires delay` should use C-style `/* ... */` comments per kernel conven= tion (though `//` is technically allowed now, consistency matters). **`lt7911exc_attach` uses `bridge->encoder` instead of parameter** ```c static int lt7911exc_attach(struct drm_bridge *bridge, struct drm_encoder *encoder, enum drm_bridge_attach_flags flags) { ... return drm_bridge_attach(lt7911exc->bridge.encoder, ...); } ``` The `encoder` parameter should be used directly rather than reading `lt7911= exc->bridge.encoder`. Compare with `lontium-lt9611uxc.c:287` which correctl= y uses the `encoder` parameter. While `bridge->encoder` may already be set = by the framework, using the parameter is the correct pattern. **`atomic_disable` has only a sleep** ```c static void lt7911exc_atomic_disable(struct drm_bridge *bridge, struct drm_= atomic_state *state) { /* Delay after panel is disabled */ msleep(20); } ``` This does nothing except sleep. If the bridge has nothing to do at disable = time, remove this callback. If the delay is truly needed for panel sequenci= ng, this should be documented with a reference to the datasheet timing requ= irement, and it's questionable whether `atomic_disable` is the right place = (vs. `atomic_post_disable`). --- Generated by Claude Code Patch Reviewer