Re: [PATCH v3 2/2] drm/bridge: This patch add new DRM bridge driver for LT9611C(EX/UXD) chip

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

Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月21日周二 18:48写道：
>
> On Tue, Apr 21, 2026 at 03:37:52PM +0800, 杨孙运 wrote:
> > Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月20日周一 20:18写道：
> > >
> > > On Mon, Apr 20, 2026 at 02:16:44PM +0800, syyang@lontium.com wrote:
> > > > From: Sunyun Yang <syyang@lontium.com>
> > > >
> > > > LT9611C(EX/UXD) is a high performance Single/Dual-Port MIPI to
> > > > HDMI 1.4/2.0 converter:
> > > >
> > > > -Single/Dual-port MIPI DSI Receiver
> > > >  1. Compliantwith D-PHY1.2&DSI-2 1.0
> > > >  2. 1/2configurable ports
> > > >  3. 1 clock lane and 1/2/3/4 configurable data lanes per port
> > > >  4. 80Mbps~2.5Gbps per data lane
> > > >  5. Support RGB666, loosely RGB666, RGB888, RGB565,16-bit YCbCr4:2:2
> > > >
> > > > -HDMI 1.4/2.0 Transmitter
> > > >  1.Data rate up to 6Gbps
> > > >  2.Support HDCP1.4/2.3
> > > >  3.Support CEC,HDR10
> > > >  4.Support lane swap
> > > >
> > > > -audio
> > > >  1.sample rates of 32~192 KHz and sample sizes
> > > >    of 16~24 bits
> > > >  2.SPDIF interface supports PCM, Dolbydigital, DTS digital audio
> > > >    at up to 192KHz frame rate
> > > >
> > > > -Miscellaneous
> > > >  1.CSC:RGB<->YUV444<->YUV422
> > > >
> > > > 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-lt9611c.c | 1365 ++++++++++++++++++++++
> > > >  3 files changed, 1384 insertions(+)
> > > >  create mode 100755 drivers/gpu/drm/bridge/lontium-lt9611c.c
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > > index c3209b0f4678..32b85a2a65d9 100644
> > > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > > @@ -177,6 +177,24 @@ config DRM_LONTIUM_LT9611
> > > >         HDMI signals
> > > >         Please say Y if you have such hardware.
> > > >
> > > > +config DRM_LONTIUM_LT9611C
> > > > +     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_LONTIUM_LT9611UXC
> > > >       tristate "Lontium LT9611UXC DSI/HDMI bridge"
> > > >       select SND_SOC_HDMI_CODEC if SND_SOC
> > > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > > index beab5b695a6e..92688be9692f 100644
> > > > --- a/drivers/gpu/drm/bridge/Makefile
> > > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > > @@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
> > > > +obj-$(CONFIG_DRM_LONTIUM_LT9611C) += lontium-lt9611c.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
> > > >  obj-$(CONFIG_DRM_LONTIUM_LT8713SX) += lontium-lt8713sx.o
> > > >  obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> > > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > > > new file mode 100755
> > > > index 000000000000..a6d11d0bddf5
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > > > @@ -0,0 +1,1365 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2026 Lontium Semiconductor, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/crc8.h>
> > > > +#include <linux/firmware.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/media-bus-format.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/of_graph.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +#include <drm/drm_atomic_helper.h>
> > > > +#include <drm/drm_bridge.h>
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/drm_drv.h>
> > > > +#include <drm/drm_edid.h>
> > > > +#include <drm/drm_mipi_dsi.h>
> > > > +#include <drm/drm_modes.h>
> > > > +#include <drm/drm_of.h>
> > > > +#include <drm/drm_print.h>
> > > > +#include <drm/drm_probe_helper.h>
> > > > +
> > > > +#include <drm/display/drm_hdmi_audio_helper.h>
> > > > +#include <drm/display/drm_hdmi_state_helper.h>
> > > > +#include <sound/hdmi-codec.h>
> > > > +
> > > > +#define FW_SIZE (64 * 1024)
> > > > +#define LT_PAGE_SIZE 256
> > > > +#define FW_FILE  "LT9611C.bin"
> > >
> > > lt9611c_fw.bin
> > >
> > will use lt9611c_fw.bin in the next version
> >
> > > > +#define LT9611C_CRC_POLYNOMIAL 0x31
> > > > +#define LT9611C_PAGE_CONTROL 0xff
> > > > +
> > > > +struct lt9611c {
> > > > +     struct device *dev;
> > > > +     struct i2c_client *client;
> > > > +     struct drm_bridge bridge;
> > > > +     struct drm_bridge *next_bridge;
> > >
> > > Use drm_bridge::next_bridge instead.
> > >
> > it will be fixed  in the next version.
> >
> > > > +     struct regmap *regmap;
> > > > +     /* Protects all accesses to registers by stopping the on-chip MCU */
> > > > +     struct mutex ocm_lock;
> > > > +     struct work_struct work;
> > > > +     struct device_node *dsi0_node;
> > > > +     struct device_node *dsi1_node;
> > > > +     struct mipi_dsi_device *dsi0;
> > > > +     struct mipi_dsi_device *dsi1;
> > > > +     struct gpio_desc *reset_gpio;
> > > > +     struct regulator_bulk_data supplies[2];
> > > > +     u32 chip_type;
> > >
> > > Define a enum. Having if (chip_type == 2) doesn't help readability.
> > >
> > it will be fixed  in the next version.
> >
> > > > +     const struct firmware *fw;
> > >
> > > Please drop it from the global struct. It is not necessary once the
> > > bridge is up and running.
> > >
> >  Remove from struct.
> >  I need use fw  to calculate CRC8 and update the firmware.
> >  Can I use a global variable for 'fw'?
>
> Of course not. You need the struct firmware and CRC only when reflashing
> the firmware. Pass them within those functions as arguments.
>
 it will be fixed  in the next version.

> >
> > > > +     int fw_version;
> > > > +     u8 fw_crc;
> > > > +     bool hdmi_connected;
> > >
> > > You've lost the imortant comment here.
> > >
> > comment about hdmi_connected ?
>
> Yes.
>
it will be fixed in next version
> >
> > > > +};
> > > > +
> > > > +DECLARE_CRC8_TABLE(lt9611c_crc8_table);
> > > > +
> > > > +static const struct regmap_range_cfg lt9611c_ranges[] = {
> > > > +     {
> > > > +             .name = "register_range",
> > > > +             .range_min =  0,
> > > > +             .range_max = 0xffff,
> > > > +             .selector_reg = LT9611C_PAGE_CONTROL,
> > > > +             .selector_mask = 0xff,
> > > > +             .selector_shift = 0,
> > > > +             .window_start = 0,
> > > > +             .window_len = 0x100,
> > > > +     },
> > > > +};
> > > > +
> > > > +static const struct regmap_config lt9611c_regmap_config = {
> > > > +     .reg_bits = 8,
> > > > +     .val_bits = 8,
> > > > +     .max_register = 0xffff,
> > > > +     .ranges = lt9611c_ranges,
> > > > +     .num_ranges = ARRAY_SIZE(lt9611c_ranges),
> > > > +};
> > > > +
> > > > +static int lt9611c_read_write_flow(struct lt9611c *lt9611c, u8 *params,
> > > > +                                unsigned int param_count, u8 *return_buffer,
> > > > +                                unsigned int return_count)
> > > > +{
> > > > +     int count, i;
> > > > +     unsigned int temp;
> > > > +
> > > > +     regmap_write(lt9611c->regmap, 0xe0de, 0x01);
> > > > +
> > > > +     count = 0;
> > > > +     do {
> > > > +             regmap_read(lt9611c->regmap, 0xe0ae, &temp);
> > > > +             usleep_range(1000, 2000);
> > > > +             count++;
> > > > +     } while (count < 100 && temp != 0x01);
> > >
> > > read_poll_timeout()
> > >
> > will use  read_poll_timeout() in the next version.
>
> Or a more suitable definition from <linux/iopoll.h>. Please check those
> and find the most suitable.
>
I think regmap_read_poll_timeout is more suitable here.

> >
> >  > > +
> > > > +     if (temp != 0x01)
> > > > +             return -1;
> > >
> > > -ETIMEDOUT
> > >
> > return value of -1 means the chip did not receive the operation to
> > write 0x01 to 0xE0DE.
> > Below:
> >      if (temp != 0x02)
> >             return -2;
> > means the chip did not receive the operation to write 0x02 to 0xE0DE.
> >
> > -1 and -2 help us determine where in the code the error occurred.
> > If both return -ETIMEDOUT, it would be less friendly for debugging
> > when we encounter issues.
>
> Inside the kernel it is expected to use drm_dbg instead of returning a
> case-specific return codes.
>
it will be fixed in next version


> > Please confirm whether I should change all of them to return -ETIMEDOUT.
>
> Yes
>
> > > > +
> > > > +static int lt9611c_hdmi_clear_audio_infoframe(struct drm_bridge *bridge)
> > > > +{
> > > > +     return 0;
> > >
> > > Hmm? What if we need to clear the infoframe?
> > >
> > don't need to clear the infoframe.
> >
> > can remove.
>
> No. We need to be able to stop sending the infoframe. Please implement
> those.
>
it will be fixed in next version.

> >
> > > > +
> > > > +static int lt9611c_hdmi_audio_prepare(struct drm_bridge *bridge,
> > > > +                                   struct drm_connector *connector,
> > > > +                                   struct hdmi_codec_daifmt *fmt,
> > > > +                                   struct hdmi_codec_params *hparms)
> > > > +{
> > > > +     struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > > > +     u8 audio_cmd[6] = {0x57, 0x48, 0x36, 0x3a};
> > > > +     u8 data[5];
> > > > +     int ret;
> > > > +
> > > > +     /* Validate sample rate and width (LT9611C auto-detects but we still check) */
> > >
> > > What for? You don't trust ASoC / ALSA core that the rates would match
> > > HDMI_RATES?
> >
> > Chip has limitations on sample_rate, sample_width, and fmt, and cannot
> > support all formats.
> > The validation here is to ensure that only the formats supported by
> > the chip are used.
>
> Again, for sample rates, is it different from HDMI_RATES? If not, it is
> useless protective coding.
>

I checked the definition of HDMI_RATES and ended up doing a redundant
validation.

#define HDMI_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\
SNDRV_PCM_RATE_192000)

However, if HDMI_RATES is extended in the future to include sample
rate not supported by LT9611C, this driver would require corresponding
updates.
If this is not expected to happen, I can remove the redundant check.

> > >
> > > > +     switch (hparms->sample_rate) {
> > > > +     case 32000:
> > > > +     case 44100:
> > > > +     case 48000:
> > > > +     case 88200:
> > > > +     case 96000:
> > > > +     case 176400:
> > > > +     case 192000:
> > > > +             break;
> > > > +     default:
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     switch (hparms->sample_width) {
> > > > +     case 16:
> > > > +     case 18:
> > > > +     case 20:
> > > > +     case 24:
> > >
> > > and no support for 32?
> > >
> > no support for 32
>
> Then check for that rather than listing all the widths. If you check the
> hdmi-codec, you can't get 18-bit samples at all.
>
You are right, there is no 18-bit support.

same reason with sample rate.

> > > > +MODULE_LICENSE("GPL v2");
> > >
> > >
> > > I think, checkpatch.pl should have flagged this.
> > >
> > yes,  it is need use GPL?
>
> If it flagged the line, why didn't you fix it? What did checkpatch say?
>
Sorry, it is my mistake. i will fix.
checkpatch say:
WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db
("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#1450: FILE: drivers/gpu/drm/bridge/lontium-lt9611c.c:1364:


> > I see that many of the latest code still use GPL v2.
>
> --
> With best wishes
> Dmitry