Re: [PATCH v18 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 5 potential issue(s) to consider:
- [Medium] drm/bridge: lt9611c: negative errno stored in fw_version corrupts firmware_show()
- [Medium] drm/bridge: lt9611c: unlocked reset and power control in PM callbacks
- [Low] drm/bridge: lt9611c: unused function warnings when CONFIG_PM_SLEEP=n
- [Low] drm/bridge: lt9611c: missing MODULE_DEVICE_TABLE() for lt9611c_id
- [Low] drm/bridge: lt9611c: unlocked reset of hdmi_connected after bridge attach
--

commit 990744b0d700fbd77a4343ac0bdea653a817a6ce
Author: Sunyun Yang <syyang@lontium.com>

drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

This commit adds a DRM bridge driver for the Lontium LT9611C, LT9611EX, and
LT9611UXD MIPI DSI to HDMI bridge chips. It supports EDID reading, HPD
detection, HDMI audio over I2S, dual-DSI mode, and firmware upgrades via
sysfs.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> new file mode 100644
> index 0000000000000..8fc86c0732122
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> @@ -0,0 +1,1361 @@

[ ... ]

> +static ssize_t firmware_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t len)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!lt9611c)
> +		return -ENODEV;
> +
> +	dev_warn(dev, "starting firmware upgrade — display will be disrupted\n");
> +
> +	ret = lt9611c_firmware_upgrade(lt9611c);
> +	if (ret < 0) {
> +		dev_err(dev, "upgrade failure\n");
> +		return ret;
> +	}
> +
> +	lt9611c_lock(lt9611c);
> +	WRITE_ONCE(lt9611c->fw_version, lt9611c_read_version(lt9611c));
> +	lt9611c_unlock(lt9611c);
> +
> +	if (READ_ONCE(lt9611c->fw_version) < 0)
> +		dev_warn(dev, "upgrade succeeded but failed to read new fw version\n");
> +	else
> +		dev_info(dev, "firmware upgrade succeeded, version: 0x%04x\n",
> +			 READ_ONCE(lt9611c->fw_version));
> +
> +	return len;
> +}
> +
> +static ssize_t firmware_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +
> +	if (!lt9611c)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "0x%04x\n", READ_ONCE(lt9611c->fw_version));
> +}

[Severity: Medium]
In drivers/gpu/drm/bridge/lontium-lt9611c.c, what happens in firmware_show()
if reading the version fails after a firmware upgrade in firmware_store()?

When regmap_write() or regmap_bulk_read() fails inside lt9611c_read_version(),
it returns a negative errno that firmware_store() writes directly into
lt9611c->fw_version via WRITE_ONCE().

Subsequent reads of the sysfs firmware attribute will then format that
negative errno with "0x%04x" in firmware_show(), emitting a sign-extended
32-bit hex error code (such as 0xfffffffb) to userspace instead of a 16-bit
firmware version or an error return.

Should firmware_store() only update lt9611c->fw_version when
lt9611c_read_version() succeeds, or should firmware_show() check for a
negative lt9611c->fw_version before formatting it?

[ ... ]

> +static int lt9611c_probe(struct i2c_client *client)
> +{

[ ... ]

> +	drm_bridge_add(&lt9611c->bridge);
> +
> +	/* Attach primary DSI */
> +	dsi = lt9611c_attach_dsi(lt9611c, dsi0_node);
> +	if (IS_ERR(dsi)) {
> +		ret = PTR_ERR(dsi);
> +		goto err_remove_bridge;
> +	}
> +
> +	/* Attach secondary DSI, if specified */
> +	if (dsi1_node) {
> +		dsi = lt9611c_attach_dsi(lt9611c, dsi1_node);
> +		if (IS_ERR(dsi)) {
> +			ret = PTR_ERR(dsi);
> +			goto err_remove_bridge;
> +		}
> +	}
> +
> +	lt9611c->hdmi_connected = false;
> +	enable_irq(client->irq);

[Severity: Low]
In lt9611c_probe() in drivers/gpu/drm/bridge/lontium-lt9611c.c, can setting
lt9611c->hdmi_connected = false here race with lt9611c_bridge_detect()?

Once drm_bridge_add() and lt9611c_attach_dsi() have been called, the DSI host
can bind the DRM device and run initial connector detection, which invokes
lt9611c_bridge_detect() and updates lt9611c->hdmi_connected under
lt9611c->mcu_lock:

	guard(mutex)(&lt9611c->mcu_lock);

	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
	if (ret)
		dev_err(dev, "failed to read HPD status (err=%d)\n", ret);
	else
		connected = (hpd_status == 0x02);

	lt9611c->hdmi_connected = connected;

Writing lt9611c->hdmi_connected = false in lt9611c_probe() without holding
lt9611c->mcu_lock after the bridge is attached can clobber the detected state
(and lt9611c is already zero-initialized by devm_drm_bridge_alloc()). Could
this assignment be dropped or moved before drm_bridge_add()?

[ ... ]

> +static int lt9611c_bridge_suspend(struct device *dev)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	disable_irq(lt9611c->client->irq);
> +	cancel_work_sync(&lt9611c->work);
> +
> +	gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);
> +	if (ret) {
> +		dev_err(lt9611c->dev, "regulator bulk disable failed.\n");
> +		gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> +		msleep(400);
> +		enable_irq(lt9611c->client->irq);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lt9611c_bridge_resume(struct device *dev)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);
> +	if (ret) {
> +		dev_err(lt9611c->dev, "regulator bulk enable failed.\n");
> +		return ret;
> +	}
> +	lt9611c_reset(lt9611c);
> +	enable_irq(lt9611c->client->irq);
> +
> +	return ret;
> +}

[Severity: Medium]
In drivers/gpu/drm/bridge/lontium-lt9611c.c, should lt9611c_bridge_suspend()
and lt9611c_bridge_resume() hold lt9611c->mcu_lock while asserting reset_gpio,
toggling the regulators, and running lt9611c_reset()?

During system suspend and resume, disable_irq() and cancel_work_sync() only
stop the HPD interrupt and lt9611c->work. Because fw_devlink relaxes
bidirectional OF-graph links and there is no PM device link ordering this I2C
client against the DRM master device or audio codec, bridge callbacks such as
lt9611c_bridge_detect(), lt9611c_bridge_edid_read(),
lt9611c_bridge_atomic_enable(), or lt9611c_hdmi_audio_prepare() can run
concurrently.

Those callbacks serialize hardware access with lt9611c->mcu_lock, whereas
lt9611c_bridge_suspend() and lt9611c_bridge_resume() assert reset_gpio,
disable/enable the vcc and vdd supplies, and run the 400ms reset sequence
without acquiring lt9611c->mcu_lock, which could reset or power down the chip
mid-transaction in lt9611c_read_write_flow().

> +
> +static const struct dev_pm_ops lt9611c_bridge_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend,
> +				lt9611c_bridge_resume)
> +};
> +
> +static const struct i2c_device_id lt9611c_id[] = {
> +	{ .name = "lt9611c",   .driver_data = (kernel_ulong_t)&lt9611c_chip_data[CHIP_LT9611C]   },
> +	{ .name = "lt9611ex",  .driver_data = (kernel_ulong_t)&lt9611c_chip_data[CHIP_LT9611EX]  },
> +	{ .name = "lt9611uxd", .driver_data = (kernel_ulong_t)&lt9611c_chip_data[CHIP_LT9611UXD] },
> +	{ /* sentinel */ }
> +};

[Severity: Low]
In drivers/gpu/drm/bridge/lontium-lt9611c.c, is MODULE_DEVICE_TABLE(i2c,
lt9611c_id) missing after lt9611c_id?

Without MODULE_DEVICE_TABLE(i2c, lt9611c_id), building the driver as a module
(CONFIG_DRM_LONTIUM_LT9611C=m) omits the i2c:lt9611c, i2c:lt9611ex, and
i2c:lt9611uxd modalias entries from modules.alias, preventing automatic module
loading when devices are instantiated via I2C device IDs.

> +
> +static const struct of_device_id lt9611c_match_table[] = {
> +	{ .compatible = "lontium,lt9611c",   .data = &lt9611c_chip_data[CHIP_LT9611C]   },
> +	{ .compatible = "lontium,lt9611ex",  .data = &lt9611c_chip_data[CHIP_LT9611EX]  },
> +	{ .compatible = "lontium,lt9611uxd", .data = &lt9611c_chip_data[CHIP_LT9611UXD] },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, lt9611c_match_table);
> +
> +static struct i2c_driver lt9611c_driver = {
> +	.driver = {
> +		.name = "lt9611c",
> +		.of_match_table = lt9611c_match_table,
> +		.pm = &lt9611c_bridge_pm_ops,
> +		.dev_groups = lt9611c_attr_groups,
> +	},

[Severity: Low]
In drivers/gpu/drm/bridge/lontium-lt9611c.c, will building with
CONFIG_PM_SLEEP=n trigger unused-function warnings for
lt9611c_bridge_suspend() and lt9611c_bridge_resume()?

SET_SYSTEM_SLEEP_PM_OPS() expands to nothing when CONFIG_PM_SLEEP is disabled,
leaving both static functions unreferenced without __maybe_unused annotations.

Could this use DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops,
lt9611c_bridge_suspend, lt9611c_bridge_resume) (or SYSTEM_SLEEP_PM_OPS())
along with pm_sleep_ptr(&lt9611c_bridge_pm_ops) on .pm so the compiler can
dead-code eliminate the callbacks and dev_pm_ops structure when
CONFIG_PM_SLEEP=n?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-lt9611c-v7-v18-0-4978b94896f1@oss.qualcomm.com?part=2