Re: [PATCH v14 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

[sashiko-bot@kernel.org]

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 <syyang@lontium.com>

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.

[ ... ]
> +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.

[ ... ]
> +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.

[ ... ]
> +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.

[ ... ]
> +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.

[ ... ]
> +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.

[ ... ]
> +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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-lt9611c-v7-v14-0-534b43a07662@oss.qualcomm.com?part=2