On Sat, Sep 12, 2026 at 09:51:09PM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 6 potential issue(s) > to consider: > - [High] The driver fails to invoke the DRM HDMI state helper to flush > infoframes to the hardware during a modeset. > - [High] Missing polling for flash page program completion before writing the > next page. > - [High] Hardware reset causes silent desynchronization of the regmap page > selector cache, leading to corrupt I2C accesses. > - [High] Incorrect interrupt clear sequence inadvertently asserts clear > signals for other pending interrupts. > - [Medium] The bridge driver completely omits an `atomic_disable` (or > `disable`) callback, preventing the hardware from being powered down during > DPMS off. > - [Low] Data race when accessing `lt9611c->fw_version` concurrently. > -- > > commit 00cae86a0097b37f64f26f28bf0a922224a6461d > Author: Sunyun Yang <[email protected]> > > drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver > > This commit introduces a DRM bridge driver for the Lontium LT9611C(EX/UXD) > MIPI DSI to HDMI converter chips. The driver supports single or dual MIPI DSI > inputs, I2S/SPDIF audio, and handles HDMI infoframes alongside I2C-based > firmware flashing and power management. > > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c > > b/drivers/gpu/drm/bridge/lontium-lt9611c.c > > new file mode 100644 > > index 0000000000000..fbd6209287998 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c > [ ... ] > > +static int lt9611c_write_data(struct lt9611c *lt9611c, const struct > > firmware *fw, size_t addr) > > +{ > [ ... ] > > + for (unsigned int i = 0; i < LT_PAGE_SIZE; i++) { > > + size_t index = num * LT_PAGE_SIZE + i; > > + u8 value = (index < size) ? data[index] : 0xff; > > + int ret; > > + > > + ret = regmap_write(lt9611c->regmap, 0xe059, value); > > + if (ret < 0) { > > + dev_err(dev, "write error at page %u, index > > %u\n", num, i); > > + return ret; > > + } > > + } > > + > > + lt9611c_wren(lt9611c); > > + lt9611c_sram_to_flash(lt9611c, addr); > > + > > + addr += LT_PAGE_SIZE; > > + } > > + > > + lt9611c_wrdi(lt9611c); > > [Severity: High] > Does this sequence overwrite the SRAM buffer over I2C while the internal SPI > flash controller is still programming the previous page? It appears there is > no > delay or polling for the flash program command to complete before looping back > to write the next page's data to SRAM, which might silently corrupt the > flashed > firmware. Will be fixed in next version. > > [ ... ] > > +static int lt9611c_firmware_upgrade(struct lt9611c *lt9611c) > > +{ > [ ... ] > > + ret = lt9611c_write_data(lt9611c, fw, 0); > > + if (ret < 0) { > > + dev_err(dev, "failed to write firmware data\n"); > > + goto out_unlock; > > + } > > + > > + ret = lt9611c_write_crc(lt9611c, fw_crc, FW_SIZE - 1); > > + if (ret < 0) { > > + dev_err(dev, "failed to write firmware crc\n"); > > + goto out_unlock; > > + } > > + > > + lt9611c_reset(lt9611c); > > + ret = lt9611c_upgrade_result(lt9611c, fw_crc); > > [Severity: High] > Could the hard reset immediately following the CRC flash write command abort > the > final write while it is still in-flight? Polling for completion before > resetting > might be necessary here as well. Will be fixed in next version. > > [ ... ] > > +static void lt9611c_reset(struct lt9611c *lt9611c) > > +{ > > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); > > + usleep_range(10000, 12000); > > + > > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); > > + msleep(400); > > +} > > [Severity: High] > Will the physical reset clear the chip's internal page register back to 0 > without updating the regmap cache? Regmap's internal cache of the page > selector could become desynchronized, causing subsequent register accesses to > skip setting the page if it matches the cached value, but occurring on the > wrong hardware page. This would affect accesses in probe, firmware_upgrade, > and bridge_resume. The driver uses REGCACHE_NONE (no cache_type set in regmap_config). With this mode, _regmap_update_bits always performs a real I2C read of the selector register before writing it — there is no cached page state that can become stale. After GPIO reset the chip reverts the page register to 0x00, and regmap reads that back on the next access, correctly detecting the change and re-issuing the page selector write as needed. > > [ ... ] > > +static irqreturn_t lt9611c_irq_thread_handler(int irq, void *dev_id) > > +{ > [ ... ] > > + if (!(irq_status & BIT(0))) > > + return IRQ_NONE; > > + > > + /* Clear interrupt: hardware requires two writes with delay */ > > + regmap_write(lt9611c->regmap, 0xe0df, irq_status & BIT(0)); > > + usleep_range(10000, 12000); > > + regmap_write(lt9611c->regmap, 0xe0df, irq_status & (~BIT(0))); > > [Severity: High] > Is the second regmap_write unintentionally asserting clear signals for other > pending interrupts? If other interrupts are pending (for example, BIT(1) is > set > in irq_status), irq_status & (~BIT(0)) will write a 1 to those bits in the > clear register. This might permanently assert their clear signals without ever > deasserting them. Only one HPD interrupt is there. > > [ ... ] > > +static void lt9611c_bridge_atomic_enable(struct drm_bridge *bridge, > > + struct drm_atomic_commit *state) > > +{ > [ ... ] > > + mode = &crtc_state->adjusted_mode; > > + > > + lt9611c_video_setup(lt9611c, mode); > > +} > > [Severity: High] > Should this enable path invoke > drm_atomic_helper_connector_hdmi_update_infoframes > to flush the infoframes to the hardware? Without this call, it seems the > registered infoframe callbacks are never triggered, preventing AVI and other > infoframes from being transmitted during a modeset. Will be fixed in next version. > > [ ... ] > > +static const struct drm_bridge_funcs lt9611c_bridge_funcs = { > > + .attach = lt9611c_bridge_attach, > > + .detect = lt9611c_bridge_detect, > > + .edid_read = lt9611c_bridge_edid_read, > > + .atomic_enable = lt9611c_bridge_atomic_enable, > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > + .atomic_create_state = drm_atomic_helper_bridge_create_state, > > [Severity: Medium] > Is the omission of an atomic_disable (or disable) callback intentional? It > appears this leaves the bridge permanently active and internal PHYs powered > even when the display pipeline is blanked or during DPMS off. The chip does support power management via the existing suspend/resume callbacks which disable the regulators and assert reset. > > [ ... ] > > +static ssize_t firmware_store(struct device *dev, struct device_attribute > > *attr, > > + const char *buf, size_t len) > > +{ > [ ... ] > > + lt9611c_lock(lt9611c); > > + lt9611c->fw_version = lt9611c_read_version(lt9611c); > > + lt9611c_unlock(lt9611c); > [ ... ] > > + return len; > > +} > > + > > +static ssize_t firmware_show(struct device *dev, struct device_attribute > > *attr, char *buf) > > +{ > > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > > + > > + return sysfs_emit(buf, "0x%04x\n", lt9611c->fw_version); > > +} > > [Severity: Low] > Could this result in a data race on lt9611c->fw_version? The variable is > written > under lt9611c_lock in firmware_store(), but it is read here in firmware_show() > without any locking. Will be fixed in next version. > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=2
