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

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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