From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH v3 2/2] drm/bridge: This patch add new DRM bridge driver for LT9611C(EX/UXD) chip Date: Thu, 23 Apr 2026 10:19:49 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Subject line**: Same issue =E2=80=94 should be `drm/bridge: lontium-lt961= 1c: Add DSI-to-HDMI bridge driver`. **File permissions**: The new file is created with `100755` (executable). C= source files should be `100644`. ``` create mode 100755 drivers/gpu/drm/bridge/lontium-lt9611c.c ``` #### Critical: Missing DRM_BRIDGE_OP_HDMI flag The driver implements `hdmi_write_avi_infoframe`, `hdmi_clear_avi_infoframe= `, `hdmi_write_hdmi_infoframe`, `hdmi_clear_hdmi_infoframe`, `hdmi_write_sp= d_infoframe`, `hdmi_clear_spd_infoframe` but only sets: ```c lt9611c->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_HDMI_AUDIO; ``` The `DRM_BRIDGE_OP_HDMI` flag is mandatory for AVI and HDMI infoframe callb= acks, and `DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME` is needed for SPD infoframes. = Without these flags, the framework will never invoke these callbacks. Compa= re with the lt9611 driver which correctly sets: ```c lt9611->bridge.ops =3D DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES | DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_HDMI_AUDIO | DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME; ``` Additionally, when `DRM_BRIDGE_OP_HDMI` is set, the bridge must also set: - `bridge.vendor` (e.g. `"Lontium"`) - `bridge.product` (e.g. `"LT9611C"`) - `bridge.supported_formats` - `bridge.max_bpc` None of these are set. #### Critical: Buffer overflow in infoframe write functions All four `hdmi_write_*_infoframe` functions copy `len` bytes into a fixed 1= 6-byte array: ```c static int lt9611c_hdmi_write_avi_infoframe(struct drm_bridge *bridge, const u8 *buffer, size_t len) { ... u8 avi_infoframe_cmd[16] =3D {0x57, 0x48, 0x35, 0x3a, 0x01}; ... for (i =3D 0; i < len; i++) avi_infoframe_cmd[i + 5] =3D buffer[i]; ``` With 5 bytes already used for the command prefix, only 11 bytes remain. An = AVI infoframe is typically 17 bytes (4-byte header + 13-byte body), and HDM= I Vendor Specific infoframes can be larger. This is a stack buffer overflow= . The buffer needs to be either dynamically sized or properly bounds-checke= d. The same pattern repeats in `lt9611c_hdmi_write_audio_infoframe`, `lt961= 1c_hdmi_write_spd_infoframe`, and `lt9611c_hdmi_write_hdmi_infoframe`. The `lt9611c_read_write_flow` function also limits params to `0xdd - 0xb0` = (45) bytes, so a larger buffer (e.g., 48 bytes) could be used with a `len` = check capped to 43 bytes. #### Improper error codes ```c static int lt9611c_read_write_flow(...) { ... if (temp !=3D 0x01) return -1; ... if (temp !=3D 0x02) return -2; ``` These should return proper errno values like `-ETIMEDOUT` or `-EIO`. The ca= llers check `ret < 0` which works, but `-1` is `-EPERM` and `-2` is `-ENOEN= T`, which are misleading. Similarly: ```c static int lt9611c_upgrade_result(...) { ... if (crc_result !=3D lt9611c->fw_crc) { ... return -1; } ``` Should be `-EIO` or similar. #### Missing atomic state helpers The bridge funcs do not include `atomic_duplicate_state` and `atomic_reset_= state` callbacks. The lt9611 driver (and most modern bridge drivers) set: ```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, ``` These are needed when using `DRM_BRIDGE_OP_HDMI`. #### Missing `atomic_disable` callback The driver implements `atomic_enable` but not `atomic_disable`. When the di= splay is turned off, nothing is sent to the chip to stop video output. The = lt9611 driver properly implements this. #### Missing `DRM_BRIDGE_OP_MODES` The driver does not implement `mode_valid` or set `DRM_BRIDGE_OP_MODES`. Wh= ile `hdmi_tmds_char_rate_valid` provides some rate limiting, a `mode_valid`= callback could reject modes the chip cannot handle more cleanly. #### Deprecated PM macro ```c static const struct dev_pm_ops lt9611c_bridge_pm_ops =3D { SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend, lt9611c_bridge_resume) }; ``` `SET_SYSTEM_SLEEP_PM_OPS` is deprecated. Use `DEFINE_SIMPLE_DEV_PM_OPS` and= `pm_sleep_ptr()`: ```c static DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops, lt9611c_bridge_suspend, lt9611c_bridge_resume); ... .pm =3D pm_sleep_ptr(<9611c_bridge_pm_ops), ``` #### i2c_device_id table missing `const` ```c static struct i2c_device_id lt9611c_id[] =3D { ``` Should be `static const struct i2c_device_id`. #### Regulator disable order is wrong ```c static int lt9611c_regulator_disable(struct lt9611c *lt9611c) { ret =3D regulator_disable(lt9611c->supplies[0].consumer); /* vcc */ ... ret =3D regulator_disable(lt9611c->supplies[1].consumer); /* vdd */ ``` The enable order is vcc (3.3V) then vdd (1.8V). The disable order should be= the reverse: vdd first, then vcc. Also, both `lt9611c_regulator_enable` an= d `lt9611c_regulator_disable` exist but `regulator_bulk_disable()` is used = in the error path and remove function. Pick one approach and be consistent.= Consider using `regulator_bulk_enable`/`regulator_bulk_disable` throughout= if sequencing doesn't matter, or use the manual approach consistently if i= t does. #### Firmware upgrade at probe time with EPROBE_DEFER on missing firmware ```c static int lt9611c_prepare_firmware_data(struct lt9611c *lt9611c) { ret =3D request_firmware(<9611c->fw, FW_FILE, dev); if (ret) { ... return -EPROBE_DEFER; } ``` Returning `-EPROBE_DEFER` when firmware loading fails will cause the kernel= to retry probe indefinitely. If the firmware file doesn't exist, this will= never succeed. This should return the actual error from `request_firmware(= )`, or use `firmware_request_nowarn()` with a limited retry strategy. #### Sysfs firmware upgrade interface ```c static DEVICE_ATTR_RW(lt9611c_firmware); ``` The sysfs attribute name will be `lt9611c_firmware`, but sysfs attribute na= ming conventions suggest just `firmware` or `firmware_version`. More import= antly, the store function ignores the input buffer content entirely =E2=80= =94 any write triggers a firmware upgrade: ```c static ssize_t lt9611c_firmware_store(struct device *dev, ...) { ... ret =3D lt9611c_firmware_upgrade(lt9611c); ``` This is fragile and unusual. A firmware upgrade triggered by any sysfs writ= e (including accidental ones) is risky. Consider at minimum validating the = input (e.g., expecting "1" or "update"). #### `lt9611c_firmware_store` calls `lt9611c_reset` after unlock ```c out: lt9611c_unlock(lt9611c); lt9611c_reset(lt9611c); ``` `lt9611c_reset()` does register writes but the MCU was just unlocked (re-en= abled). There may be a race between the MCU resuming execution and the rese= t sequence. This also calls reset unconditionally even on failure, and call= s it twice on success (once inside the `if (ret < 0)` goto path isn't taken= , the reset before `lt9611c_upgrade_result` already happened). #### `bridge_to_lt9611c_const` is unnecessary boilerplate ```c /*read only*/ static const struct lt9611c *bridge_to_lt9611c_const(const struct drm_bridg= e *bridge) { return container_of(bridge, const struct lt9611c, bridge); } ``` This is only used in one place (`lt9611c_hdmi_tmds_char_rate_valid`). The `= const` correctness approach is fine but the `/*read only*/` comment is unne= cessary, and the function name doesn't match kernel conventions. Consider j= ust casting in the one callsite or dropping the `const` qualification. #### IRQ handler returns `IRQ_HANDLED` for spurious interrupts ```c if (!(irq_status & BIT(0))) { mutex_unlock(<9611c->ocm_lock); return IRQ_HANDLED; } ``` If the interrupt status bit is not set, the interrupt was not from this dev= ice and should return `IRQ_NONE` to allow shared interrupt handling to work= properly. #### Residual comments Several comments remain that should have been removed per the v3 changelog: ```c REG_SEQ0(0xe103, 0x3f), //fifo rst ... /*clear interrupt*/ ... //hardware need delay ``` These should either be removed or converted to proper `/* */` style. The `/= /` comment style is discouraged in kernel code. #### `video_timing_set_cmd` inconsistent hex case ```c video_timing_set_cmd[17] =3D vactive & 0xFF; ``` All other masks use lowercase `0xff`. This one uses `0xFF`. The v3 changelo= g says "Lowercase all hex values" but this was missed. #### `MODULE_LICENSE` mismatch ```c MODULE_LICENSE("GPL v2"); ``` The SPDX header says `GPL-2.0` which maps to `"GPL v2"`. However, the moder= n convention is to use `"GPL"` (which means "GPL v2 or later"). If the inte= nt is GPL-2.0-only, the SPDX should be `GPL-2.0-only` and `MODULE_LICENSE("= GPL")` is still correct for GPL-2.0-only. The `"GPL v2"` string is deprecat= ed =E2=80=94 use `"GPL"`. --- Generated by Claude Code Patch Reviewer