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

在 2026/5/9 5:13, [email protected] 写道:
> 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.
    it will be fixed in next version.> - [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 <[email protected]>
> 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?
> 
it will be fixed in next version.

> [ ... ]
>> +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.
> 
it will be fixed in next version.> [ ... ]
>> +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.
> 
it will be fixed in next version.

> [ ... ]
>> +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.
it will be fixed in next version.
>