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 support for lontium lt8713sx bridge driver Date: Fri, 27 Feb 2026 14:52:52 +1000 Message-ID: In-Reply-To: <20260224-lt8713sx-bridge-driver-v4-2-b5603f5458d8@oss.qualcomm.com> References: <20260224-lt8713sx-bridge-driver-v4-0-b5603f5458d8@oss.qualcomm.com> <20260224-lt8713sx-bridge-driver-v4-2-b5603f5458d8@oss.qualcomm.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 **`bridge.funcs` assigned redundantly:** `devm_drm_bridge_alloc()` already sets `bridge->funcs =3D funcs` internally= . The manual assignment at line is unnecessary and misleading: ```c + lt8713sx->bridge.funcs =3D <8713sx_bridge_funcs; ``` Remove this line. **`enable_gpio` acquired but never used:** The driver requests an "enable" GPIO and stores it, but never reads or togg= les it after acquisition: ```c + lt8713sx->enable_gpio =3D devm_gpiod_get_optional(dev, "enable", GPIOD_OU= T_HIGH); ``` If the purpose is just to drive it high at probe time, that side effect rel= ies on the `GPIOD_OUT_HIGH` flag, which is subtle and undocumented. This sh= ould either be used explicitly (e.g., in enable/disable callbacks) or remov= ed. It's also not documented in the DT binding. **`bridge_attach` signature may be wrong:** On the current drm-next tree, the attach callback signature includes the en= coder parameter: ```c int (*attach)(struct drm_bridge *bridge, struct drm_encoder *encoder, enum drm_bridge_attach_flags flags); ``` The patch matches this, but the driver only passes through to `drm_bridge_a= ttach`. This is fine but should be verified against the exact tree it's tar= geting. **No bridge `ops` set =E2=80=94 driver does nothing as a bridge:** The bridge has no `ops` flags set (no `DRM_BRIDGE_OP_DETECT`, `DRM_BRIDGE_O= P_EDID`, etc.) and the only bridge function is `attach`. This means the bri= dge does nothing in the display pipeline =E2=80=94 it just chains to `next_= bridge`. This raises the question of whether this needs to be a `drm_bridge= ` driver at all, or whether a simpler i2c driver would suffice for firmware= loading. **Sysfs attribute naming and approach:** ```c +static DEVICE_ATTR_WO(lt8713sx_firmware); ``` This creates a sysfs attribute named `lt8713sx_firmware`. The naming conven= tion is unusual =E2=80=94 sysfs attributes typically use shorter generic na= mes (e.g., `firmware_update` or `update_fw`). More importantly, using a raw= sysfs write-triggered firmware flash is not the preferred approach. The ke= rnel has the firmware upload API (`firmware_upload`) in `include/linux/firm= ware.h` designed exactly for this use case, or firmware can be loaded autom= atically at probe time. The `store` function ignores the buffer content entirely: ```c +static ssize_t lt8713sx_firmware_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct lt8713sx *lt8713sx =3D dev_get_drvdata(dev); + int ret; + + ret =3D lt8713sx_firmware_update(lt8713sx); ``` Writing any value triggers a firmware flash. This is fragile =E2=80=94 ther= e's no validation, no idempotency check. **Integer type issues with `sz_12k`:** ```c + u64 sz_12k =3D 12 * SZ_1K; ``` This is `u64` for a value of 12288 =E2=80=94 completely unnecessary. Just u= se a `#define` or plain `unsigned int`. The subsequent division by `sz_12k`= then does a 64-bit divide which is expensive on 32-bit architectures (and = may need `do_div`): ```c + lt8713sx->bank_num =3D (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12= k; ``` This will break on 32-bit builds because it does a 64-bit divide. Use `DIV_= ROUND_UP()` with appropriate types. **`lt8713sx_write_data` always returns 0:** ```c +static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, = u64 filesize) +{ + ... + return 0; +} ``` The function never checks `regmap_write()` return values and always returns= success. If any register write fails during firmware flashing, the failure= is silently ignored. All the `regmap_write` calls in this driver are unche= cked =E2=80=94 this is a pervasive issue throughout the driver. **`lt8713sx_block_erase` has unbounded-ish busy wait:** ```c + while (1) { + flash_status =3D lt8713sx_read_flash_status(lt8713sx); + if ((flash_status & 0x01) =3D=3D 0) + break; + if (i > 50) + break; + i++; + msleep(50); + } ``` The loop breaks after 50 iterations but doesn't report an error if the flas= h never becomes ready. A block erase that fails silently will lead to corru= pted firmware. **Firmware size check is off-by-one:** ```c + if (lt8713sx->fw->size > SZ_256K - 1) { ``` This allows firmware of size exactly `SZ_256K - 1` (262143 bytes). If the i= ntent is to allow up to 256KB, use `>=3D SZ_256K` or `> SZ_256K`. The `- 1`= suggests confusion. **Main firmware copy is also off-by-one:** ```c + memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1); ``` This copies 65535 bytes instead of 65536. This means the last byte of the f= irst 64K is not copied from the firmware file =E2=80=94 it's left as 0xFF (= from the memset). The CRC is then computed over 65535 bytes and stored at o= ffset `SZ_64K - 1`. This seems intentional (the CRC replaces the last byte)= , but it means the firmware format requires the last byte of the first 64K = block to be reserved. This should be documented. **`bank_crc_value` array size is 17 but not bounds-checked:** ```c + u32 bank_crc_value[17]; + ... + lt8713sx->bank_num =3D (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12= k; + ... + for (int i =3D 0; i < lt8713sx->bank_num; i++) { + lt8713sx->bank_crc_value[i] =3D ... ``` The maximum number of 12K banks in a 192KB region (256K - 64K) is `(192*102= 4) / (12*1024) =3D 16`, so `bank_crc_value[17]` is sufficient. However, the= re is no explicit bounds check on `bank_num` before indexing into the array= . If the firmware size calculation yields a `bank_num > 17`, this is a buff= er overflow. **`i2c_device_id` should be `const`:** ```c +static struct i2c_device_id lt8713sx_id[] =3D { ``` Should be: ```c +static const struct i2c_device_id lt8713sx_id[] =3D { ``` **`i2c_device_id` entry should not include the vendor prefix:** ```c + { "lontium,lt8713sx", 0 }, ``` The `i2c_device_id` `.name` field should be just `"lt8713sx"`, not `"lontiu= m,lt8713sx"`. The compatible string in `of_device_id` uses the vendor prefi= x, but the I2C ID table does not. Also, the trailing `, 0` is unnecessary w= ith modern kernel conventions. **`crc8_populate_msb` called on a global table in probe:** ```c +DECLARE_CRC8_TABLE(lt8713sx_crc_table); +... + crc8_populate_msb(lt8713sx_crc_table, 0x31); ``` This is a module-level static array populated in `probe()`. If multiple dev= ices probe (unlikely but possible), there's a race on table initialization.= This should be done in `module_init` or with a `once` guard. Alternatively= , use a `CRC8_INIT_TABLE` or populate it at compile time. **`drm_bridge_remove` in remove but no unplug support:** ```c +static void lt8713sx_remove(struct i2c_client *client) +{ + struct lt8713sx *lt8713sx =3D i2c_get_clientdata(client); + drm_bridge_remove(<8713sx->bridge); +} ``` Modern bridge drivers using `devm_drm_bridge_alloc` should use `devm_drm_br= idge_add` instead of the manual `drm_bridge_add`/`drm_bridge_remove` pair, = which would eliminate the need for a `remove` callback entirely. **`lt8713sx_load_bank_fw_to_sram` takes `u64 addr` unnecessarily:** ```c +static void lt8713sx_load_bank_fw_to_sram(struct lt8713sx *lt8713sx, u64 a= ddr) ``` The address fits in 24 bits (max 0x040000). Using `u64` is misleading and w= asteful. Use `u32`. **No `DRM_BRIDGE_ATTACH_NO_CONNECTOR` handling:** The `attach` callback doesn't check for or pass through `DRM_BRIDGE_ATTACH_= NO_CONNECTOR` flags. Modern bridge drivers should support this flag. --- Generated by Claude Code Patch Reviewer