Re: [PATCH 2/2] drm/bridge: Add LT7911EXC edp to mipi bridge driver

[杨孙运 <yangsunyun1993@gmail.com>]

Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月21日周二 19:15写道：
>
> On Tue, Apr 21, 2026 at 11:13:30AM +0800, 杨孙运 wrote:
> > Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月20日周一 11:57写道：
> > >
> > > On Mon, 20 Apr 2026 at 05:34, <syyang@lontium.com> wrote:
> > > >
> > > > From: Sunyun Yang <syyang@lontium.com>
> > > >
> > > > LT7911EXC is a high performance  eDP1.4 to MIPI chip for
> > >
> > > MIPI what?
> > >
> > MIPI DSI,
> > It will be modified in the next version.
> >
> > > > VR/Display application.
> > > >
> > > > -eDP1.4Receiver
> > > >  1.Support SSC
> > > >  2.Support 1/2/4 lanes
> > > >  3.Support up to 4K@60HzRGB/YCbCr4:4:48bpc
> > > >  4.Support lane swap and PN swap
> > > >
> > > > -MIPI Transmitter
> > > >  1.CompliantwithD-PHY1.2&DSI1.1&CSI-22.0；1 clock lane,
> > > >    and1/2/3/4 configurable data lanes:2.5Gbpsperdatalane
> > > >  2.CompliantwithC-PHY1.0&DSI-21.0&CSI-22.0;
> > > >    1/2/3 configurable data trio；2.5Gsps perdatatrio
> > > >  3.Support1/2configurable ports
> > > >  4.DSISupport16/20/24-bit YCbCr4:2:2,16/18/24/30-bit RGB
> > > >
> > > > Signed-off-by: Sunyun Yang <syyang@lontium.com>
> > > > ---
> > > >  drivers/gpu/drm/bridge/Kconfig             |  18 +
> > > >  drivers/gpu/drm/bridge/Makefile            |   1 +
> > > >  drivers/gpu/drm/bridge/lontium-lt7911exc.c | 571 +++++++++++++++++++++
> > > >  3 files changed, 590 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/bridge/lontium-lt7911exc.c
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > > index c3209b0f4678..bae8cdaea666 100644
> > > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > > @@ -202,6 +202,24 @@ config DRM_LONTIUM_LT8713SX
> > > >           to 3 configurable Type-C/DP1.4/HDMI2.0 outputs
> > > >           Please say Y if you have such hardware.
> > > >
> > > > +config DRM_LONTIUM_LT9611C
> > >
> > > I thought the patch is for LT7911EXC
> > >
> > Yes, it is LT7911EXC, It will be modified in the next version.
> >
> > > > +       tristate "Lontium LT9611C DSI/HDMI bridge"
> > > > +       select SND_SOC_HDMI_CODEC if SND_SOC
> > > > +       depends on OF
> > > > +       select CRC8
> > > > +       select FW_LOADER
> > > > +       select DRM_PANEL_BRIDGE
> > > > +       select DRM_KMS_HELPER
> > > > +       select DRM_MIPI_DSI
> > > > +       select DRM_DISPLAY_HELPER
> > > > +       select DRM_DISPLAY_HDMI_STATE_HELPER
> > > > +       select REGMAP_I2C
> > > > +       help
> > > > +         Driver for Lontium DSI to HDMI bridge
> > > > +         chip driver that converts dual DSI and I2S to
> > > > +         HDMI signals
> > > > +         Please say Y if you have such hardware.
> > > > +
> > > >  config DRM_ITE_IT66121
> > > >         tristate "ITE IT66121 HDMI bridge"
> > > >         depends on OF
> > > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > > index beab5b695a6e..54b293d1663e 100644
> > > > --- a/drivers/gpu/drm/bridge/Makefile
> > > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > > @@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT8713SX) += lontium-lt8713sx.o
> > > > +obj-$(CONFIG_DRM_LONTIUM_LT7911EXC) += lontium-lt7911exc.o
> > >
> > > Keep the list sorted, please.
> > >
> > Where can I see the sorting rules? Please guide me.
> > it is:
> > obj-$(CONFIG_DRM_LONTIUM_LT7911EXC) += lontium-lt7911exc.o
> > obj-$(CONFIG_DRM_LONTIUM_LT8713SX) += lontium-lt8713sx.o
>
> zoom out, check the overall contents, submit a patch fixing the error.
>
It will be fixed in the next version

> >
> > > >  obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> > > >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > > >  obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
> > > > diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> > > > new file mode 100644
> > > > index 000000000000..d1c1d9e073ef
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> > > > @@ -0,0 +1,571 @@
> > > > +// 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/regmap.h>
> > >
> > > I think you use more than that.
> > >
> > It will be modified in the next version.
> >
> > > > +#include <drm/drm_of.h>
> > > > +
> > > > +#define FW_SIZE (64 * 1024)
> > > > +#define LT_PAGE_SIZE 32
> > > > +#define FW_FILE  "LT7911EXC.bin"
> > > > +#define LT7911EXC_PAGE_CONTROL 0xff
> > > > +
> > > > +struct lt7911exc {
> > > > +       struct device *dev;
> > > > +       struct i2c_client *client;
> > > > +       struct drm_bridge bridge;
> > > > +       struct drm_bridge *panel_bridge;
> > >
> > > Use next_bridge from struct drm_bridge instead.
> > >
> > It will be modified in the next version.
> >
> > > > +       struct regmap *regmap;
> > > > +       /* Protects all accesses to registers by stopping the on-chip MCU */
> > > > +       struct mutex ocm_lock;
> > > > +       struct regulator_bulk_data supplies[2];
> > > > +
> > > > +       struct gpio_desc *reset_gpio;
> > > > +       const struct firmware *fw;
> > >
> > > Do you need to store it during the runtime? If not, please remove from
> > > the data struct.
> > >
> > Don't need  store during the runtime.
> > Can I use the global variable 'fw'?
>
> Of course not.
>
It will use local variable in the next version

> > Because I need use 'fw' to calculate the CRC32 and burn the firmware.
>
> Sure, but what does it have to do with the field in struct lt7911exc?
>
You are correct, and I will fix according to your suggestions.

> >
> > > > +       int fw_version;
> > > > +       u32 fw_crc;
> > > > +
> > > > +       bool enabled;
> > >
> > > What for?
> > >
> > (bool enabled;) is used as a flag in the code to reduce the frequency
> > of power supply switching.
>
> How does it help to reduce the rate?
>
The enabled flag is used to track the hardware power state. In
lt7911exc_pre_enable(), we check if (lt7911exc->enabled) return; to
avoid redundant regulator enabling and reset pulses when the DRM
framework calls pre_enable multiple times.

Similarly, lt7911exc_post_disable() uses it to prevent multiple
power-down sequences. This reduces unnecessary power supply switching
and potential side effects.

However, if you consider this optimization unnecessary, I can remove
the flag and the related checks in the next version. Please let me
know your preference.

> > Of course, it can also be removed in the next version.
> >
> > > > +};
> > > > +
> > > > +static const struct regmap_range_cfg lt7911exc_ranges[] = {
> > > > +       {
> > > > +               .name = "register_range",
> > > > +               .range_min =  0,
> > > > +               .range_max = 0xffff,
> > >
> > > Is it an actual range?
> > >
> > 0xe8ff is actual  range.
>
> Then why?
>
The actual valid register address ends at 0xe8ff. I mistakenly set
range_max to 0xffff.
I will fix it to 0xe8ff in the next version. Thank you for catching this.

> >
> > > > +               .selector_reg = LT7911EXC_PAGE_CONTROL,
> > > > +               .selector_mask = 0xff,
> > > > +               .selector_shift = 0,
> > > > +               .window_start = 0,
> > > > +               .window_len = 0x100,
> > > > +       },
> > > > +};
> > > > +
> > > > +static const struct regmap_config lt7911exc_regmap_config = {
> > > > +       .reg_bits = 8,
> > > > +       .val_bits = 8,
> > > > +       .max_register = 0xffff,
> > > > +       .ranges = lt7911exc_ranges,
> > > > +       .num_ranges = ARRAY_SIZE(lt7911exc_ranges),
> > > > +};
> > > > +
> > > > +static u32 cal_crc32_custom(const u8 *data, u64 length)
> > > > +{
> > > > +       u32 crc = 0xffffffff;
> > > > +       u8 buf[4];
> > > > +       u64 i;
> > > > +
> > > > +       for (i = 0; i < length; i += 4) {
> > > > +               buf[0] = data[i + 3];
> > > > +               buf[1] = data[i + 2];
> > > > +               buf[2] = data[i + 1];
> > > > +               buf[3] = data[i + 0];
> > > > +               crc = crc32_be(crc, buf, 4);
> > >
> > > How is it different from crc32_le()?
> > >
> > The implementation differs from crc32_le() in both byte ordering and
> > processing granularity.
> > This function performs a 32-bit word-wise byte swap (little-endian to
> > big-endian) before feeding data into crc32_be(), while crc32_le()
> > processes the input stream directly in little-endian order without
> > transformation.
> > Therefore, the result is not equivalent to crc32_le(), and is required
> > to match the firmware's expected big-endian word-based CRC format.
>
> ack.
>
> >
> >
> > > > +MODULE_LICENSE("GPL v2");
> > Should GPL be used here? I got an error during testing: GPL v2;
>
> Which error?
>
Sorry, is  warning:
WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db
("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#649: FILE: drivers/gpu/drm/bridge/lontium-lt7911exc.c:571:
+MODULE_LICENSE("GPL v2");

 It will be fixed in the next version.

> --
> With best wishes
> Dmitry