Re: [PATCH v17 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge
From: Sunyun Yang
Date: Tue Jul 28 2026 - 23:38:19 EST
Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx> 于2026年7月28日周二 17:40写道：
>
> On Wed, Jul 15, 2026 at 09:54:38AM +0800, syyang@xxxxxxxxxxx wrote:
> > From: Sunyun Yang <syyang@xxxxxxxxxxx>
> >
> > Add support for the Lontium LT7911EXC bridge chip, which converts
> > eDP input to MIPI DSI output using an internal firmware-controlled
> > pipeline.
> >
> > The driver provides:
> > - DRM bridge integration for eDP-to-DSI routing
> > - MIPI DSI host interface for downstream panel attachment
> > - Firmware upgrade mechanism over I2C (erase/program/verify)
> > - GPIO-based reset and regulator management
> >
> > Display timing and MIPI DCS packet generation are handled by the chip
> > firmware and are not configured by the driver.
> >
> > Signed-off-by: Sunyun Yang <syyang@xxxxxxxxxxx>
> > ---
> > +static void lt7911exc_reset(struct lt7911exc *lt7911exc)
> > +{
> > + /* Assert reset pin: logical 1 -> physical state low (Reset Active) */
>
> Redundant comment.
>
we will remove this comment in next version.
> > + gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1);
> > + usleep_range(5000, 6000);
> > +
> > + /* Deassert reset pin: logical 0 -> physical state high (Run state) */
>
> This one too.
>
we will remove this comment in next version.
> > + gpiod_set_value_cansleep(lt7911exc->reset_gpio, 0);
> > + msleep(400);
> > +}
> > +
>
> [...]
>
> > +static void lt7911exc_firmware_upgrade_work(struct work_struct *work)
> > +{
> > + struct lt7911exc *lt7911exc = container_of(work, struct lt7911exc, work);
> > + struct device *dev = lt7911exc->dev;
> > + const struct firmware *fw;
> > + u8 *buffer;
> > + size_t total_size = FW_SIZE - 4;
> > + u32 crc32, version;
> > + int ret;
> > +
> > + mutex_lock(<7911exc->upgrade_lock);
> > +
> > + scoped_guard(mutex, <7911exc->ocm_lock) {
> > + if (lt7911exc->removed)
> > + goto out_unlock_upgrade;
> > + }
> > +
> > + ret = request_firmware(&fw, FW_FILE, dev);
> > + if (ret) {
> > + dev_err(dev, "failed to load '%s'\n", FW_FILE);
> > + goto out_unlock_upgrade;
> > + }
> > +
> > + if (fw->size > total_size) {
> > + dev_err(dev, "firmware too large (%zu > %zu)\n", fw->size, total_size);
> > + goto out_release_fw;
> > + }
> > +
> > + buffer = kvmalloc(total_size, GFP_KERNEL);
> > + if (!buffer)
> > + goto out_release_fw;
> > +
> > + memset(buffer, 0xff, total_size);
> > + memcpy(buffer, fw->data, fw->size);
>
> memcpy(buffer, fw->data, fw->size);
> memset(buffer + fw->size, 0xff, total_size - fw->size);
>
we will fix.
>
> > + crc32 = cal_crc32_custom(buffer, total_size);
> > + kvfree(buffer);
> > +
> > + lt7911exc_reset(lt7911exc);
> > +
> > + scoped_guard(mutex, <7911exc->ocm_lock) {
> > + ret = lt7911exc_inside_mcu_stop(lt7911exc);
>
> Move guard() into the function.
>
we will fix.
> > + }
> > + if (ret)
> > + goto out_release_fw;
> > +
> > + ret = lt7911exc_block_erase(lt7911exc);
> > + if (ret) {
> > + dev_err(dev, "failed to block erase.\n");
> > + goto out_mcu_run;
> > + }
> > +
> > + ret = lt7911exc_write_data(lt7911exc, fw, 0);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to write firmware data\n");
> > + goto out_mcu_run;
> > + }
> > +
> > + ret = lt7911exc_write_crc(lt7911exc, crc32, FW_SIZE - 4);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to write firmware crc\n");
> > + goto out_mcu_run;
> > + }
> > +
> > + lt7911exc_reset(lt7911exc);
> > +
> > + ret = lt7911exc_upgrade_result(lt7911exc, crc32);
> > + if (ret)
> > + dev_err(dev, "firmware verification failed\n");
> > +
> > + /*
> > + * Always restore MCU to running state: flash is already erased,
> > + * leaving MCU stopped would render the chip completely unusable.
> > + */
> > + scoped_guard(mutex, <7911exc->ocm_lock) {
> > + ret = lt7911exc_inside_mcu_run(lt7911exc);
>
> Move guard() into the run function.
>
we will fix.
> > + if (!ret) {
> > + ret = lt7911exc_read_version(lt7911exc, &version);
>
> Why is ihis being executed under the OCM lock?
>
fw_version = version need OCM lock, and we will fix ocm lock code.
> > + if (!ret)
> > + lt7911exc->fw_version = version;
> > + }
> > + }
> > +
> > + if (ret)
> > + dev_err(dev, "failed to read version after upgrade\n");
> > +
> > + goto out_release_fw;
> > +
> > +out_mcu_run:
> > + scoped_guard(mutex, <7911exc->ocm_lock)
> > + lt7911exc_inside_mcu_run(lt7911exc);
> > +
> > +out_release_fw:
> > + release_firmware(fw);
>
> release_firmware() could have happened long ago, after copying the
> buffer.
>
we will fix
> > +
> > +out_unlock_upgrade:
> > + scoped_guard(mutex, <7911exc->ocm_lock) {
> > + if (!lt7911exc->removed)
> > + lt7911exc->upgrade = false;
> > + }
> > +
> > + /* Notify DRM to re-trigger modeset after firmware upgrade */
> > + if (!lt7911exc->removed && lt7911exc->bridge.dev)
> > + drm_kms_helper_hotplug_event(lt7911exc->bridge.dev);
>
> drm_bridge_hpd_notify(), outside of the lock.
>
we will fix
> > +
> > + mutex_unlock(<7911exc->upgrade_lock);
> > +}
> > +
> > +static void lt7911exc_atomic_pre_enable(struct drm_bridge *bridge, struct drm_atomic_commit *state)
> > +{
> > + struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> > + struct device *dev = lt7911exc->dev;
> > + int ret;
> > +
> > + guard(mutex)(<7911exc->ocm_lock);
> > +
> > + if (!lt7911exc->upgrade) {
>
> Why? Can't you just use a lock to prevent DRM vs firmware upgrade races.
>
The upgrade process takes about 10 seconds. Using only the OCM lock,
the AI bot believes it would block the DRM modeset path and cause
timeouts.
The upgrade flag provides a non-blocking check: DRM operations return
immediately during upgrade rather than waiting.
Our design intent is that firmware upgrades are only performed during
debugging — once the firmware is working properly, upgrades won't
happen in normal operation.
I believe the AI bot's concern can be ignored. I would like to use a
single OCM lock to reduce driver complexity. Dmitry, do you think
using a single OCM lock is acceptable?
> > + ret = regmap_write(lt7911exc->regmap, 0xe0b0, 0x01);
>
> What will enable the output if bridge was upgrading during
> atomic_pre_enable?
The output is not enabled during upgrade. atomic_pre_enable returns
early when upgrade=true. After upgrade completes,
upgrade=false and calls drm_bridge_hpd_notify() to trigger a new
modeset, which re-enters atomic_pre_enable and writes 0xe0b0 to enable
the output.
>
> > + if (ret)
> > + dev_err(dev, "failed to enable mipi output stream\n");
> > + }
> > +}
> > +
> > +static void lt7911exc_atomic_post_disable(struct drm_bridge *bridge,
> > + struct drm_atomic_commit *state)
> > +{
> > + struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> > + struct device *dev = lt7911exc->dev;
> > + int ret;
> > +
> > + guard(mutex)(<7911exc->ocm_lock);
> > +
> > + if (!lt7911exc->upgrade) {
> > + ret = regmap_write(lt7911exc->regmap, 0xe0b0, 0x00);
> > + if (ret)
> > + dev_err(dev, "failed to disable mipi output stream\n");
> > + }
> > +}
> > +
> > +static int lt7911exc_bridge_attach(struct drm_bridge *bridge,
> > + struct drm_encoder *encoder,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> > +
> > + if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> > + dev_err(lt7911exc->dev, "needs atomic updates support\n");
> > + return -ENOTSUPP;
> > + }
>
> I'd say, it's redundant.
>
we will fix.
> > +
> > + return drm_bridge_attach(encoder, lt7911exc->output_bridge, bridge, flags);
> > +}
> > +
> > +static const struct drm_bridge_funcs lt7911exc_bridge_funcs = {
> > + .attach = lt7911exc_bridge_attach,
> > + .atomic_pre_enable = lt7911exc_atomic_pre_enable,
> > + .atomic_post_disable = lt7911exc_atomic_post_disable,
> > + .atomic_reset = drm_atomic_helper_bridge_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > +};
> > +
> > +static int lt7911exc_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *dev)
> > +{
> > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> > + struct drm_bridge *bridge;
> > +
> > + /* currently do not support connecting several DSI devices to the same host */
> > + if (lt7911exc->output_bridge)
> > + return -EBUSY;
> > +
> > + if (dev->lanes > 4) {
> > + dev_err(lt7911exc->dev, "unsupported number of data lanes(%u)\n", dev->lanes);
> > + return -EINVAL;
> > + }
> > +
> > + bridge = devm_drm_of_get_bridge(lt7911exc->dev, host->dev->of_node, 1, 0);
> > + if (IS_ERR(bridge)) {
> > + dev_err(lt7911exc->dev, "failed to add DSI device\n");
> > + return PTR_ERR(bridge);
> > + }
> > +
> > + lt7911exc->output_bridge = bridge;
>
> Follow what other drivers are doing. Please store the next bridge in
> drm_bridge::next_bridge. It saves you from manual handling of its
> refcounts.
>
we will fix
> > +
> > + /*
> > + * Force panel-first enable order: the DSI output stream (0xe0b0)
> > + * must not be enabled before the panel is powered on.
> > + * This override is safe because LT7911EXC's transfer() is a stub —
> > + * panel DCS commands are silently discarded and panel init is done
>
> Is it a hardware limitation or something which is not yet implemented in
> the driver?
>
This is a hardware limitation. The LT7911EXC contains an internal MCU
that runs its own firmware, which is responsible for all panel
initialization sequences. The DSI link is owned and managed by the
chip's internal firmware. Linux-side DCS commands cannot be forwarded
to the actual DSI bus — the transfer() callback accepts the message
and reports success solely to prevent the panel driver from aborting
its init sequence. Actual panel initialization is performed entirely
by the chip's internal firmware after the chip comes out of reset.
> > + * by the chip's internal firmware. The panel driver's
> > + * prepare_prev_first preference is therefore irrelevant here.
> > + */
>
> Do you have an actual issue with one of the panels? The overall logic
> should be to let the panels decide whether it needs to setup anything
> before the DSI output stream comes up. If there are any issues, they
> should be fixed on the DSI host side.
>
Thank you for the suggestion. We will move the DSI output stream
enable from atomic_pre_enable to atomic_enable. Since atomic_enable is
called after all pre_enable callbacks complete (regardless of
pre_enable_prev_first), the panel is guaranteed to be ready before the
DSI output stream is enabled. The pre_enable_prev_first override will
be removed, and the panel's ordering preference will be fully
respected.
> > + lt7911exc->output_bridge->pre_enable_prev_first = false;
> > +
> > + drm_bridge_add(<7911exc->bridge);
> > +
> > + return 0;
> > +}
> > +
> > +static int lt7911exc_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *dev)
> > +{
> > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> > +
> > + drm_bridge_remove(<7911exc->bridge);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * LT7911EXC's internal MCU owns the DSI link and handles all panel
> > + * initialization. The host transfer() is a no-op sink: accept the
>
> How does it know panel init commands? What about the panels which use
> DCS commands to setup brightness?
>
The panel initialization commands need to be embedded into the
firmware code. When register 0xe0b0 is written with 0x01, the firmware
outputs the MIPI DSI initialization commands. — (hardware limitation)
The brightness information is incorporated into the firmware code
according to the defined convention. The chip provides registers to
control brightness using the agreed-upon brightness values.
> > + * message and report it as sent so the panel driver does not abort
> > + * its init sequence. Actual DSI transmission is done by chip firmware.
> > + */
> > +static ssize_t lt7911exc_dsi_host_transfer(struct mipi_dsi_host *host,
> > + const struct mipi_dsi_msg *msg)
> > +{
> > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> > +
> > + if (msg->rx_len) {
> > + dev_warn(lt7911exc->dev, "MIPI rx is not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + switch (msg->type) {
> > + case MIPI_DSI_DCS_SHORT_WRITE:
> > + case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> > + case MIPI_DSI_DCS_LONG_WRITE:
> > + case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> > + case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> > + case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> > + case MIPI_DSI_GENERIC_LONG_WRITE:
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + guard(mutex)(<7911exc->ocm_lock);
> > +
> > + if (lt7911exc->upgrade)
> > + return -EBUSY;
> > +
> > + return msg->tx_len;
> > +}
> > +
> > +static const struct mipi_dsi_host_ops lt7911exc_dsi_host_ops = {
> > + .attach = lt7911exc_dsi_host_attach,
> > + .detach = lt7911exc_dsi_host_detach,
> > + .transfer = lt7911exc_dsi_host_transfer,
> > +};
> > +
>
> --
> With best wishes
> Dmitry