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

Dmitry Baryshkov
2026-04-21T10:48:46Z

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.

>
> > > + int fw_version;
> > > + u8 fw_crc;
> > > + bool hdmi_connected;
> >
> > You've lost the imortant comment here.
> >
> comment about hdmi_connected ?

Yes.

>
> > > +};
> > > +
> > > +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.

>
> > > +
> > > + 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.

> 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.

>
> > > +
> > > +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.

> >
> > > + 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.

> > > +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?

> I see that many of the latest code still use GPL v2.

--
With best wishes
Dmitry