Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Regulators enabled in `probe()` are never disabled on the success path, causing a permanent power leak.
- [High] Reset GPIO is driven high after cutting off power, violating power sequencing and risking hardware backpowering.
- [High] Synchronous `request_firmware` in `probe` can cause permanent probe failure if the driver is built-in.
- [Medium] Missing `depends on I2C` and `select REGMAP_I2C` in Kconfig.
- [Medium] The return value of `devm_drm_bridge_add()` is ignored in `probe()`.
- [Low] Missing header includes for core kernel APIs.
--

commit d9cebea8e10096b0200ef8f220724550dd554963
Author: Sunyun Yang <syyang@lontium.com>
drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

This commit adds support for the Lontium LT7911EXC eDP to MIPI DSI
bridge. It implements I2C-based setup, display configuration, and
provides a firmware update mechanism.

> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c3209b0f46786..fe45a9052aa0d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -132,6 +132,19 @@ config DRM_ITE_IT6505
>  	help
>  	  ITE IT6505 DisplayPort bridge chip driver.
>  
> +config DRM_LONTIUM_LT7911EXC
> +	tristate "Lontium eDP/MIPI DSI bridge"
> +	depends on OF

Should this config also depend on I2C and select REGMAP_I2C?

The driver explicitly registers an I2C driver and utilizes
devm_regmap_init_i2c(). Building without I2C enabled might cause
compilation or linker errors.

[ ... ]
> diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> new file mode 100644
> index 0000000000000..d50fc979ebd4b
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> @@ -0,0 +1,493 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2026 Lontium Semiconductor, Inc.
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

This isn't a bug, but it looks like some core headers are missing.

The driver uses kzalloc() and kfree(), msleep() and usleep_range(), as well
as regulator_bulk_enable(). Could linux/slab.h, linux/delay.h, and
linux/regulator/consumer.h be included directly to avoid relying on
transitive includes?

[ ... ]
> +static int lt7911exc_firmware_upgrade(struct lt7911exc *lt7911exc)
> +{
> +	struct device *dev = lt7911exc->dev;
> +	const struct firmware *fw;
> +	u8 *buffer;
> +	size_t total_size = FW_SIZE - 4;
> +	u32 crc32;
> +	int ret;
> +
> +	/*1. load firmware*/
> +	ret = request_firmware(&fw, FW_FILE, dev);

Can this synchronous request_firmware() call cause a permanent probe failure
if the driver is built-in?

When built into the kernel, probe executes before the root filesystem is
mounted, which would cause this to fail with -ENOENT. Because the error
is propagated without returning -EPROBE_DEFER, the subsystem might not
retry the probe later.

[ ... ]
> +static void lt7911exc_atomic_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
> +{
> +	struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> +	int ret;
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(lt7911exc->supplies), lt7911exc->supplies);
> +	if (ret)
> +		return;
> +
> +	gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1);
> +}

Does driving the reset GPIO high after cutting off the regulator power
violate power sequencing?

Driving a logic high signal into an unpowered IC can sometimes backpower
the device through its internal ESD protection diodes.

[ ... ]
> +static int lt7911exc_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct lt7911exc *lt7911exc;
> +	bool fw_updated = false;
> +	int ret;
[ ... ]
> +	ret = regulator_bulk_enable(ARRAY_SIZE(lt7911exc->supplies), lt7911exc->supplies);
> +	if (ret)
> +		return ret;
> +
> +	lt7911exc_reset(lt7911exc);
> +	lt7911exc_lock(lt7911exc);
[ ... ]
> +	lt7911exc_unlock(lt7911exc);
> +
> +	lt7911exc->bridge.of_node = dev->of_node;
> +	devm_drm_bridge_add(dev, &lt7911exc->bridge);
> +
> +	return 0;

Is there a resource leak for the regulators here?

The regulator_bulk_enable() call at the beginning of probe increments the
use count to power the device, but it appears the success path returns 0
without calling regulator_bulk_disable().

If lt7911exc_atomic_post_disable() later decrements the count, it seems
the count might still remain elevated by 1, leaving the hardware permanently
powered on.

Additionally, should the return value of devm_drm_bridge_add() be checked?

If it fails, the bridge is internally removed, but returning 0 here would
lead the driver to successfully bind to the I2C device without exposing
a functional DRM bridge.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508134702.4713-1-syyang@lontium.com?part=2