Re: [PATCH v8 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

On Wed, Jul 29, 2026 at 04:51:28PM +0300, Dmitry Baryshkov wrote:
> On Tue, Jul 28, 2026 at 04:39:19PM +0530, [email protected] wrote:
> > From: Sunyun Yang <[email protected]>
> > 
> > LT9611C(EX/UXD) is an I2C-controlled chip that Receiver signal/dual port
> > mipi dsi and output hdmi, differences in hardware features:
> > - LT9611C: supports 1-port mipi dsi to hdmi 1.4
> > - LT9611EX: supports 2-port mipi dsi to hdmi 1.4
> > - LT9611UXD: supports 2-port mipi dsi to hdmi 1.4/2.0
> > 
> > Signed-off-by: Sunyun Yang <[email protected]>
> > Co-developed-by: Mohit Dsor <[email protected]>
> > Signed-off-by: Mohit Dsor <[email protected]>
> > ---
> >  .../ABI/testing/sysfs-driver-lontium-lt9611c       |   11 +
> >  drivers/gpu/drm/bridge/Kconfig                     |   17 +
> >  drivers/gpu/drm/bridge/Makefile                    |    1 +
> >  drivers/gpu/drm/bridge/lontium-lt9611c.c           | 1344 ++++++++++++++++++++
> >  4 files changed, 1373 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-lontium-lt9611c b/Documentation/ABI/testing/sysfs-driver-lontium-lt9611c
> > new file mode 100644
> > index 000000000000..440e102a8bd8
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-lontium-lt9611c
> > @@ -0,0 +1,11 @@
> > +What:		/sys/bus/i2c/drivers/lt9611c/.../lt9611c_firmware
> 
> Let's unify this a bit. Rename it to be just 'firmware' and define it as
> a generic attribute for all Lontium bridges. I will follow up with a
> change to LT9611UXC driver.
Sure, I will try to do it in v9.
> 
> > +Date:		July 2026
> > +KernelVersion:	7.3
> > +Contact:	Mohit Dsor <[email protected]>
> > +Description:
> > +		Read: Returns the current firmware version loaded on the
> > +		LT9611C(EX/UXD) chip in hex format (e.g. "0x0105").
> > +
> > +		Write: Triggers a firmware upgrade of the LT9611C(EX/UXD)
> > +		chip using the firmware file Lontium/lt9611c_fw.bin loaded
> > +		via the firmware loader.
> 
> 
> 
> > +
> > +struct lt9611c {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	struct drm_bridge bridge;
> > +	struct regmap *regmap;
> > +	/* Protects all accesses to registers by stopping the on-chip MCU */
> 
> The mutex doesn't stop the MCU.
Will remove the comment and rename the mutex.
> 
> > +	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];
> > +	int fw_version;
> > +	/* Chip variant: C/EX/UXD */
> > +	enum lt9611_chip_type chip_type;
> > +	 /* HDMI cable connection status */
> > +	bool hdmi_connected;
> > +};
> > +
> > +static int lt9611c_read_write_flow(struct lt9611c *lt9611c,
> > +				   const struct lt9611c_cmd *cmd,
> > +				   struct lt9611c_rsp *rsp)
> > +{
> > +	int ret;
> > +	unsigned int i;
> > +	unsigned int temp;
> > +	unsigned int max_params = 0xe0dd - 0xe0b0 + 1;
> > +
> > +	regmap_write(lt9611c->regmap, 0xe0de, 0x01);
> > +
> > +	ret = regmap_read_poll_timeout(lt9611c->regmap, 0xe0ae, temp,
> > +				       temp == 0x01, 1000, 200 * 1000);
> > +	if (ret)
> > +		return -ETIMEDOUT;
> > +
> > +	regmap_write(lt9611c->regmap, 0xe0b0 + 0, cmd->hdr.func);
> > +	regmap_write(lt9611c->regmap, 0xe0b0 + 1, cmd->hdr.type);
> > +	regmap_write(lt9611c->regmap, 0xe0b0 + 2, cmd->hdr.seq);
> > +	regmap_write(lt9611c->regmap, 0xe0b0 + 3, cmd->hdr.sep);
> 
> Can this use a bulk write?
Will fix it in v9.
> 
> > +
> > +	for (i = 0; cmd->data && i < cmd->data_len &&
> > +	     (LT9611C_CMD_HDR_SIZE + i) < max_params; i++)
> > +		regmap_write(lt9611c->regmap,
> > +			     0xe0b0 + LT9611C_CMD_HDR_SIZE + i, cmd->data[i]);
> > +
> > +	regmap_write(lt9611c->regmap, 0xe0de, 0x02);
> > +
> > +	ret = regmap_read_poll_timeout(lt9611c->regmap, 0xe0ae, temp,
> > +				       temp == 0x02, 1000, 200 * 1000);
> > +	if (ret)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = regmap_bulk_read(lt9611c->regmap, 0xe085,
> > +			       &rsp->hdr, LT9611C_CMD_HDR_SIZE);
> > +	if (ret)
> > +		return ret;
> > +
> > +
> > +	if (rsp->data && rsp->data_len)
> > +		ret = regmap_bulk_read(lt9611c->regmap,
> > +				       0xe085 + LT9611C_CMD_HDR_SIZE,
> > +				       rsp->data, rsp->data_len);
> > +
> > +	return ret;
> > +}
> > +
> > +static void lt9611c_config_parameters(struct lt9611c *lt9611c)
> > +{
> > +	const struct reg_sequence seq_write_paras[] = {
> > +		REG_SEQ0(0xe0ee, 0x01),
> > +		REG_SEQ0(0xe103, 0x3f), /*fifo rst*/
> > +		REG_SEQ0(0xe103, 0xff),
> > +		REG_SEQ0(0xe05e, 0xc1),
> > +		REG_SEQ0(0xe058, 0x00),
> > +		REG_SEQ0(0xe059, 0x50),
> > +		REG_SEQ0(0xe05a, 0x10),
> > +		REG_SEQ0(0xe05a, 0x00),
> > +		REG_SEQ0(0xe058, 0x21),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write_paras, ARRAY_SIZE(seq_write_paras));
> > +}
> > +
> > +static void lt9611c_wren(struct lt9611c *lt9611c)
> > +{
> > +	regmap_write(lt9611c->regmap, 0xe05a, 0x04);
> > +	regmap_write(lt9611c->regmap, 0xe05a, 0x00);
> > +}
> > +
> > +static void lt9611c_wrdi(struct lt9611c *lt9611c)
> > +{
> > +	regmap_write(lt9611c->regmap, 0xe05a, 0x08);
> > +	regmap_write(lt9611c->regmap, 0xe05a, 0x00);
> > +}
> > +
> > +static void lt9611c_erase_op(struct lt9611c *lt9611c, u32 addr)
> > +{
> > +	const struct reg_sequence seq_write[] = {
> > +		REG_SEQ0(0xe0ee, 0x01),
> > +		REG_SEQ0(0xe05a, 0x04),
> > +		REG_SEQ0(0xe05a, 0x00),
> > +		REG_SEQ0(0xe05b, (addr >> 16) & 0xff),
> > +		REG_SEQ0(0xe05c, (addr >> 8) & 0xff),
> > +		REG_SEQ0(0xe05d, addr & 0xff),
> > +		REG_SEQ0(0xe05a, 0x01),
> > +		REG_SEQ0(0xe05a, 0x00),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write, ARRAY_SIZE(seq_write));
> > +}
> > +
> > +static void read_flash_reg_status(struct lt9611c *lt9611c, unsigned int *status)
> > +{
> > +	const struct reg_sequence seq_write[] = {
> > +		REG_SEQ0(0xe103, 0x3f),
> > +		REG_SEQ0(0xe103, 0xff),
> > +		REG_SEQ0(0xe05e, 0x40),
> > +		REG_SEQ0(0xe056, 0x05),
> > +		REG_SEQ0(0xe055, 0x25),
> > +		REG_SEQ0(0xe055, 0x01),
> > +		REG_SEQ0(0xe058, 0x21),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write, ARRAY_SIZE(seq_write));
> > +
> > +	regmap_read(lt9611c->regmap, 0xe05f, status);
> > +}
> > +
> > +static void lt9611c_crc_to_sram(struct lt9611c *lt9611c)
> > +{
> > +	const struct reg_sequence seq_write[] = {
> > +		REG_SEQ0(0xe051, 0x00),
> > +		REG_SEQ0(0xe055, 0xc0),
> > +		REG_SEQ0(0xe055, 0x80),
> > +		REG_SEQ0(0xe05e, 0xc0),
> > +		REG_SEQ0(0xe058, 0x21),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write, ARRAY_SIZE(seq_write));
> > +}
> > +
> > +static void lt9611c_data_to_sram(struct lt9611c *lt9611c)
> > +{
> > +	const struct reg_sequence seq_write[] = {
> > +		REG_SEQ0(0xe051, 0xff),
> > +		REG_SEQ0(0xe055, 0x80),
> > +		REG_SEQ0(0xe05e, 0xc0),
> > +		REG_SEQ0(0xe058, 0x21),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write, ARRAY_SIZE(seq_write));
> > +}
> > +
> > +static void lt9611c_sram_to_flash(struct lt9611c *lt9611c, size_t addr)
> > +{
> > +	const struct reg_sequence seq_write[] = {
> > +		REG_SEQ0(0xe05b, (addr >> 16) & 0xff),
> > +		REG_SEQ0(0xe05c, (addr >> 8) & 0xff),
> > +		REG_SEQ0(0xe05d, addr & 0xff),
> > +		REG_SEQ0(0xe05a, 0x30),
> > +		REG_SEQ0(0xe05a, 0x00),
> > +	};
> > +
> > +	regmap_multi_reg_write(lt9611c->regmap, seq_write, ARRAY_SIZE(seq_write));
> > +}
> > +
> > +static void lt9611c_block_erase(struct lt9611c *lt9611c)
> > +{
> > +	int i;
> > +	unsigned int block_num;
> > +	unsigned int flash_status = 0;
> > +	u32 flash_addr = 0;
> > +
> > +	for (block_num = 0; block_num < 2; block_num++) {
> > +		flash_addr = (block_num * 0x008000);
> 
> Drop extra brackets.
Sure, will fix it in v9.
> 
> > +		lt9611c_erase_op(lt9611c, flash_addr);
> > +		msleep(100);
> > +		i = 0;
> > +		while (1) {
> > +			read_flash_reg_status(lt9611c, &flash_status);
> > +			if ((flash_status & 0x01) == 0)
> > +				break;
> > +
> > +			if (i > 50)
> > +				break;
> > +
> > +			i++;
> > +			msleep(50);
> 
> Use existing polling macros.
Will change it in v9.
> 
> > +		}
> > +	}
> > +}
> > +
> > +static int lt9611c_write_data(struct lt9611c *lt9611c, const struct firmware *fw, size_t addr)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +	unsigned int page = 0, num = 0, i = 0;
> > +	size_t size, index;
> > +	const u8 *data;
> > +	u8 value;
> > +
> > +	data = fw->data;
> > +	size = fw->size;
> > +	page = (size + LT_PAGE_SIZE - 1) / LT_PAGE_SIZE;
> 
> DIV_ROUND_UP
Will fix it in v9.
> 
> > +	if (page * LT_PAGE_SIZE > FW_SIZE) {
> > +		dev_err(dev, "firmware size out of range\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(dev, "%u pages, total size %zu byte\n", page, size);
> > +
> > +	for (num = 0; num < page; num++) {
> > +		lt9611c_data_to_sram(lt9611c);
> > +
> > +		for (i = 0; i < LT_PAGE_SIZE; i++) {
> > +			index = num * LT_PAGE_SIZE + i;
> > +			value = (index < size) ? data[index] : 0xff;
> > +
> > +			ret = regmap_write(lt9611c->regmap, 0xe059, value);
> 
> Again, is it possible to use bulk functions instead of writing it byte
> per byte?
The I2C controller has a transfer size limit and writing 256 bytes in a single transaction was causing bus arbitration loss.
> 
> > +			if (ret < 0) {
> > +				dev_err(dev, "write error at page %u, index %u\n", num, i);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		lt9611c_wren(lt9611c);
> > +		lt9611c_sram_to_flash(lt9611c, addr);
> > +
> > +		addr += LT_PAGE_SIZE;
> > +	}
> > +
> > +	lt9611c_wrdi(lt9611c);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lt9611c_write_crc(struct lt9611c *lt9611c, u8 fw_crc, size_t addr)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +
> > +	lt9611c_crc_to_sram(lt9611c);
> > +	ret = regmap_write(lt9611c->regmap, 0xe059, fw_crc);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to write crc\n");
> > +		return ret;
> > +	}
> > +
> > +	lt9611c_wren(lt9611c);
> > +	lt9611c_sram_to_flash(lt9611c, addr);
> > +	lt9611c_wrdi(lt9611c);
> > +
> > +	dev_dbg(dev, "crc 0x%02x written to flash at addr 0x%zx\n", fw_crc, addr);
> > +
> > +	return 0;
> > +}
> > +
> > +static void lt9611c_reset(struct lt9611c *lt9611c)
> > +{
> > +	gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > +	usleep_range(10000, 12000);
> > +
> > +	gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > +	msleep(400);
> > +}
> > +
> > +static int lt9611c_upgrade_result(struct lt9611c *lt9611c, u8 fw_crc)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	unsigned int crc_result;
> > +
> > +	regmap_write(lt9611c->regmap, 0xe0ee, 0x01);
> > +	regmap_read(lt9611c->regmap, 0xe021, &crc_result);
> > +
> > +	if (crc_result != fw_crc) {
> > +		dev_err(dev, "lt9611c fw upgrade failed, expected crc=0x%02x, read crc=0x%02x\n",
> > +			fw_crc, crc_result);
> > +		return -1;
> > +	}
> > +
> > +	dev_dbg(dev, "lt9611c firmware upgrade success, crc=0x%02x\n", crc_result);
> > +	return 0;
> > +}
> > +
> > +static int lt9611c_firmware_upgrade(struct lt9611c *lt9611c)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	const struct firmware *fw;
> > +	u8 *buffer;
> > +	size_t total_size = FW_SIZE - 1;
> > +	u8 fw_crc;
> > +	int ret;
> > +
> > +	/* 1. load firmware */
> > +	ret = request_firmware(&fw, FW_FILE, dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to load '%s'\n", FW_FILE);
> > +
> > +	/* 2. check size */
> > +	if (fw->size > total_size) {
> > +		dev_err(dev, "firmware too large (%zu > %zu)\n", fw->size, total_size);
> > +		ret = -EINVAL;
> > +		goto out_release_fw;
> > +	}
> > +	dev_dbg(dev, "firmware size: %zu bytes\n", fw->size);
> > +
> > +	/* 3. calculate crc8 */
> > +	buffer = kzalloc(total_size, GFP_KERNEL);
> > +	if (!buffer) {
> > +		ret = -ENOMEM;
> > +		goto out_release_fw;
> > +	}
> > +
> > +	memcpy(buffer, fw->data, fw->size);
> > +	memset(buffer + fw->size, 0xff, total_size - fw->size);
> > +
> > +	fw_crc = crc8(lt9611c_crc8_table, buffer, total_size, 0);
> > +	kfree(buffer);
> > +
> > +	dev_info(dev, "starting firmware upgrade, size: %zu bytes, crc: 0x%02x\n",
> > +		 fw->size, fw_crc);
> > +
> > +	/* 4. firmware upgrade */
> > +	lt9611c_config_parameters(lt9611c);
> > +	lt9611c_block_erase(lt9611c);
> > +
> > +	ret = lt9611c_write_data(lt9611c, fw, 0);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to write firmware data\n");
> > +		goto out_release_fw;
> > +	}
> > +
> > +	ret = lt9611c_write_crc(lt9611c, fw_crc, FW_SIZE - 1);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to write firmware crc\n");
> > +		goto out_release_fw;
> > +	}
> > +
> > +	/* 5. check upgrade of result */
> > +	lt9611c_reset(lt9611c);
> > +	ret = lt9611c_upgrade_result(lt9611c, fw_crc);
> > +
> > +out_release_fw:
> > +	release_firmware(fw);
> > +	return ret;
> > +}
> > +
> > +static struct lt9611c *bridge_to_lt9611c(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct lt9611c, bridge);
> > +}
> > +
> > +static const struct lt9611c *bridge_to_lt9611c_const(const struct drm_bridge *bridge)
> > +{
> > +	return container_of_const(bridge, struct lt9611c, bridge);
> > +}
> > +
> > +static void lt9611c_lock(struct lt9611c *lt9611c)
> > +{
> > +	mutex_lock(&lt9611c->ocm_lock);
> > +	regmap_write(lt9611c->regmap, 0xe0ee, 0x01);
> > +}
> > +
> > +static void lt9611c_unlock(struct lt9611c *lt9611c)
> > +{
> > +	regmap_write(lt9611c->regmap, 0xe0ee, 0x00);
> > +	mutex_unlock(&lt9611c->ocm_lock);
> > +}
> > +
> > +static irqreturn_t lt9611c_irq_thread_handler(int irq, void *dev_id)
> > +{
> > +	struct lt9611c *lt9611c = dev_id;
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +	unsigned int irq_status;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = regmap_read(lt9611c->regmap, 0xe084, &irq_status);
> > +	if (ret) {
> > +		dev_err(dev, "failed to read irq status: %d\n", ret);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	if (!(irq_status & BIT(0)))
> > +		return IRQ_HANDLED;
> > +
> > +	/*Clear interrupt: hardware requires two writes with delay*/
> > +	regmap_write(lt9611c->regmap, 0xe0df, irq_status & BIT(0));
> > +	usleep_range(10000, 12000);
> > +	regmap_write(lt9611c->regmap, 0xe0df, irq_status & (~BIT(0)));
> > +
> > +	schedule_work(&lt9611c->work);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void lt9611c_hpd_work(struct work_struct *work)
> > +{
> > +	struct lt9611c *lt9611c = container_of(work, struct lt9611c, work);
> > +	struct device *dev = lt9611c->dev;
> > +	static const u8 hpd_data[] = { 0x00 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_READ, LT9611C_TYPE_HDMI, 0x31, LT9611C_CMD_SEP },
> > +		.data = hpd_data,
> > +		.data_len = 1,
> > +	};
> > +	u8 hpd_status;
> > +	struct lt9611c_rsp rsp = { .data = &hpd_status, .data_len = 1 };
> > +	bool connected;
> > +	int ret;
> > +
> > +	/* Added delay as need time to reflect hpd after interrupt*/
> > +	msleep(200);
> > +
> > +	mutex_lock(&lt9611c->ocm_lock);
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +	if (ret) {
> > +		dev_err(dev, "failed to read HPD status\n");
> > +	} else {
> > +		lt9611c->hdmi_connected = (hpd_status == 0x02);
> > +	}
> > +	connected = lt9611c->hdmi_connected;
> > +	mutex_unlock(&lt9611c->ocm_lock);
> > +
> > +	drm_bridge_hpd_notify(&lt9611c->bridge,
> > +			      connected ? connector_status_connected :
> > +			      connector_status_disconnected);
> > +}
> > +
> > +static int lt9611c_regulator_init(struct lt9611c *lt9611c)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +
> > +	lt9611c->supplies[0].supply = "vcc";
> > +	lt9611c->supplies[1].supply = "vdd";
> > +
> > +	ret = devm_regulator_bulk_get(dev, 2, lt9611c->supplies);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct mipi_dsi_device *lt9611c_attach_dsi(struct lt9611c *lt9611c,
> > +						  struct device_node *dsi_node)
> > +{
> > +	const struct mipi_dsi_device_info info = { "lt9611c", 0, NULL };
> > +	struct mipi_dsi_device *dsi;
> > +	struct mipi_dsi_host *host;
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +
> > +	host = of_find_mipi_dsi_host_by_node(dsi_node);
> > +	if (!host)
> > +		return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n"));
> > +
> > +	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> > +	if (IS_ERR(dsi))
> > +		return ERR_PTR(dev_err_probe(dev, PTR_ERR(dsi), "failed to create dsi device\n"));
> > +
> > +	dsi->lanes = 4;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > +			 MIPI_DSI_MODE_VIDEO_HSE;
> > +
> > +	ret = devm_mipi_dsi_attach(dev, dsi);
> > +	if (ret < 0)
> > +		return ERR_PTR(dev_err_probe(dev, ret, "failed to attach dsi to host\n"));
> > +
> > +	return dsi;
> > +}
> > +
> > +static int lt9611c_bridge_attach(struct drm_bridge *bridge,
> > +				 struct drm_encoder *encoder,
> > +				 enum drm_bridge_attach_flags flags)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +
> > +	return drm_bridge_attach(encoder, lt9611c->bridge.next_bridge, bridge, flags);
> > +}
> > +
> > +static enum drm_mode_status
> > +lt9611c_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> > +				  const struct drm_display_mode *mode,
> > +				  unsigned long long tmds_rate)
> > +{
> > +	const struct lt9611c *lt9611c = bridge_to_lt9611c_const(bridge);
> > +
> > +	if (lt9611c->chip_type == CHIP_LT9611UXD) {
> > +		if (tmds_rate > 600000000)
> 
> Extract max TMDS rate to match data. Compare to the value from the match
> data here.
Will fix it in v9.
> 
> > +			return MODE_CLOCK_HIGH;
> > +
> > +	} else {
> > +		if (tmds_rate > 340000000)
> > +			return MODE_CLOCK_HIGH;
> > +	}
> > +
> > +	if (tmds_rate < 25000000)
> > +		return MODE_CLOCK_LOW;
> > +
> > +	return MODE_OK;
> > +}
> > +
> > +static void lt9611c_video_setup(struct lt9611c *lt9611c,
> > +				const struct drm_display_mode *mode)
> > +{
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +	u32 h_total, hactive, hsync_len, hfront_porch, hback_porch;
> > +	u32 v_total, vactive, vsync_len, vfront_porch, vback_porch;
> > +	u8 timing_data[22];
> > +	struct lt9611c_rsp rsp = {};
> > +	u8 framerate;
> > +	u8 vic = 0x00;
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_MIPI, 0x33, LT9611C_CMD_SEP },
> > +		.data = timing_data,
> > +		.data_len = ARRAY_SIZE(timing_data),
> > +	};
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +	h_total = mode->htotal;
> > +	hactive = mode->hdisplay;
> > +	hsync_len = mode->hsync_end - mode->hsync_start;
> > +	hfront_porch = mode->hsync_start - mode->hdisplay;
> > +	hback_porch = mode->htotal - mode->hsync_end;
> > +
> > +	v_total = mode->vtotal;
> > +	vactive = mode->vdisplay;
> > +	vsync_len = mode->vsync_end - mode->vsync_start;
> > +	vfront_porch = mode->vsync_start - mode->vdisplay;
> > +	vback_porch = mode->vtotal - mode->vsync_end;
> > +	framerate = drm_mode_vrefresh(mode);
> > +	vic = drm_match_cea_mode(mode);
> > +
> > +	dev_dbg(dev, "hactive=%d, vactive=%d\n", hactive, vactive);
> > +	dev_dbg(dev, "framerate=%d\n", framerate);
> > +	dev_dbg(dev, "vic = 0x%02x\n", vic);
> > +
> > +	timing_data[0] = (h_total >> 8) & 0xff;
> > +	timing_data[1] = h_total & 0xff;
> 
> <linux/unaligned.h> ?
Will change it in v9.
> 
> > +	timing_data[2] = (hactive >> 8) & 0xff;
> > +	timing_data[3] = hactive & 0xff;
> > +	timing_data[4] = (hfront_porch >> 8) & 0xff;
> > +	timing_data[5] = hfront_porch & 0xff;
> > +	timing_data[6] = (hsync_len >> 8) & 0xff;
> > +	timing_data[7] = hsync_len & 0xff;
> > +	timing_data[8] = (hback_porch >> 8) & 0xff;
> > +	timing_data[9] = hback_porch & 0xff;
> > +	timing_data[10] = (v_total >> 8) & 0xff;
> > +	timing_data[11] = v_total & 0xff;
> > +	timing_data[12] = (vactive >> 8) & 0xff;
> > +	timing_data[13] = vactive & 0xFF;
> > +	timing_data[14] = (vfront_porch >> 8) & 0xff;
> > +	timing_data[15] = vfront_porch & 0xff;
> > +	timing_data[16] = (vsync_len >> 8) & 0xff;
> > +	timing_data[17] = vsync_len & 0xff;
> > +	timing_data[18] = (vback_porch >> 8) & 0xff;
> > +	timing_data[19] = vback_porch & 0xff;
> > +	timing_data[20] = framerate;
> > +	timing_data[21] = vic;
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +	if (ret)
> > +		dev_err(dev, "video set failed\n");
> > +}
> > +
> > +static void lt9611c_bridge_atomic_enable(struct drm_bridge *bridge,
> > +					 struct drm_atomic_commit *state)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *conn_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_display_mode *mode;
> > +
> > +	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> > +	if (WARN_ON(!connector))
> > +		return;
> > +
> > +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> > +	if (WARN_ON(!conn_state))
> > +		return;
> > +
> > +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> > +	if (WARN_ON(!crtc_state))
> > +		return;
> > +
> > +	mode = &crtc_state->adjusted_mode;
> > +
> > +	lt9611c_video_setup(lt9611c, mode);
> > +}
> > +
> > +static enum drm_connector_status
> > +lt9611c_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	struct device *dev = lt9611c->dev;
> > +	int ret;
> > +	bool connected = false;
> > +	static const u8 hpd_data[] = { 0x00 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_READ, LT9611C_TYPE_HDMI, 0x31, LT9611C_CMD_SEP },
> > +		.data = hpd_data,
> > +		.data_len = 1,
> > +	};
> > +	u8 hpd_status;
> > +	struct lt9611c_rsp rsp = { .data = &hpd_status, .data_len = 1 };
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +	if (ret) {
> > +		dev_err(dev, "failed to read HPD status (err=%d)\n", ret);
> > +	} else {
> > +		connected = (hpd_status == 0x02);
> > +	}
> > +
> > +	lt9611c->hdmi_connected = connected;
> > +
> > +	return connected ? connector_status_connected :
> > +				connector_status_disconnected;
> > +}
> > +
> > +static int lt9611c_get_edid_block(void *data, u8 *buf,
> > +				  unsigned int block, size_t len)
> > +{
> > +	struct lt9611c *lt9611c = data;
> > +	struct device *dev = lt9611c->dev;
> > +	u8 edid_raw[LT9611C_CMD_Y0_SIZE + LT9611C_EDID_BUF_SIZE];
> > +	u8 y0;
> > +	int ret, i, offset = 0;
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_READ, LT9611C_TYPE_HDMI, 0x33, LT9611C_CMD_SEP },
> > +	};
> > +	struct lt9611c_rsp rsp = {
> > +		.data = edid_raw,
> > +		.data_len = LT9611C_CMD_Y0_SIZE + LT9611C_EDID_BUF_SIZE,
> > +	};
> > +
> > +	if (len != 128)
> > +		return -EINVAL;
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		y0 = block * 4 + i;
> > +		cmd.data = &y0;
> > +		cmd.data_len = 1;
> > +		ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to read EDID block %u packet %d\n",
> > +				block, i);
> > +			return ret;
> > +		}
> > +		memcpy(buf + offset, &edid_raw[LT9611C_CMD_Y0_SIZE], LT9611C_EDID_BUF_SIZE);
> > +		offset += LT9611C_EDID_BUF_SIZE;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_edid *lt9611c_bridge_edid_read(struct drm_bridge *bridge,
> > +						       struct drm_connector *connector)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +
> > +	return drm_edid_read_custom(connector, lt9611c_get_edid_block, lt9611c);
> > +}
> > +
> > +static int lt9611c_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
> > +					    const u8 *buffer, size_t len)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	u8 extra[1 + LT9611C_INFOFRAME_MAX_SIZE];
> > +	struct lt9611c_rsp rsp = {};
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x35, LT9611C_CMD_SEP },
> > +	};
> > +
> > +	if (WARN_ON(len > LT9611C_INFOFRAME_MAX_SIZE))
> > +		return -EINVAL;
> > +
> > +	extra[0] = 0x01; /* write avi */
> > +	memcpy(&extra[1], buffer, len);
> > +	cmd.data = extra;
> > +	cmd.data_len = 1 + len;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	return lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +}
> > +
> > +static int lt9611c_hdmi_clear_avi_infoframe(struct drm_bridge *bridge)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	static const u8 clear_data[] = { 0x01 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x42, LT9611C_CMD_SEP },
> > +				   .data = clear_data, .data_len = 1 };
> > +	struct lt9611c_rsp rsp = {};
> > +	int ret;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +
> > +	if (ret < 0) {
> > +		dev_err(lt9611c->dev, "clear avi infoframe failed!\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lt9611c_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
> > +					     const u8 *buffer, size_t len)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	u8 extra[1 + LT9611C_INFOFRAME_MAX_SIZE];
> > +	struct lt9611c_rsp rsp = {};
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x35, LT9611C_CMD_SEP },
> > +	};
> > +
> > +	if (WARN_ON(len > LT9611C_INFOFRAME_MAX_SIZE))
> > +		return -EINVAL;
> > +
> > +	extra[0] = 0x04; /* write vsif */
> > +	memcpy(&extra[1], buffer, len);
> > +	cmd.data = extra;
> > +	cmd.data_len = 1 + len;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	return lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> 
> All the infoframe functions look (almost) exactly the same. Is it worth
> extracting the common code?
Will change it in v9.
> 
> > +}
> > +
> > +static int lt9611c_hdmi_clear_hdmi_infoframe(struct drm_bridge *bridge)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	static const u8 clear_data[] = { 0x04 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x42, LT9611C_CMD_SEP },
> > +				   .data = clear_data, .data_len = 1 };
> > +	struct lt9611c_rsp rsp = {};
> > +	int ret;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +
> > +	if (ret < 0) {
> > +		dev_err(lt9611c->dev, "clear hdmi infoframe failed!\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lt9611c_hdmi_write_audio_infoframe(struct drm_bridge *bridge,
> > +					      const u8 *buffer, size_t len)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	u8 extra[1 + LT9611C_INFOFRAME_MAX_SIZE];
> > +	struct lt9611c_rsp rsp = {};
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x35, LT9611C_CMD_SEP },
> > +	};
> > +
> > +	extra[0] = 0x02; /* write audio */
> > +	memcpy(&extra[1], buffer, len);
> > +	cmd.data = extra;
> > +	cmd.data_len = 1 + len;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	return lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +}
> > +
> > +static int lt9611c_hdmi_clear_audio_infoframe(struct drm_bridge *bridge)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	static const u8 clear_data[] = { 0x02 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x42, LT9611C_CMD_SEP },
> > +				   .data = clear_data, .data_len = 1 };
> > +	struct lt9611c_rsp rsp = {};
> > +	int ret;
> > +
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +
> > +	if (ret < 0) {
> > +		dev_err(lt9611c->dev, "clear audio infoframe failed!\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +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_extra[2];
> > +	struct lt9611c_rsp rsp = {};
> > +	int ret;
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_WRITE, LT9611C_TYPE_HDMI, 0x36, LT9611C_CMD_SEP },
> > +				   .data = audio_extra,
> > +				   .data_len = ARRAY_SIZE(audio_extra) };
> > +
> > +	if (hparms->sample_width == 32)
> > +		return -EINVAL;
> > +
> > +	switch (fmt->fmt) {
> > +	case HDMI_I2S:
> > +		audio_extra[0] = 0x01;
> > +		break;
> > +	case HDMI_SPDIF:
> > +		audio_extra[0] = 0x02;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	audio_extra[1] = hparms->channels;
> > +	guard(mutex)(&lt9611c->ocm_lock);
> > +
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +	if (ret < 0) {
> > +		dev_err(lt9611c->dev, "set audio info failed!\n");
> > +		return ret;
> > +	}
> > +
> > +	return drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
> > +									&hparms->cea);
> > +}
> > +
> > +static void lt9611c_hdmi_audio_shutdown(struct drm_bridge *bridge,
> > +					struct drm_connector *connector)
> > +{
> > +	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
> > +}
> > +
> > +static void lt9611c_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > +	static const u8 hpd_data[] = { 0x00 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_READ, LT9611C_TYPE_HDMI, 0x31, LT9611C_CMD_SEP },
> > +		.data = hpd_data,
> > +		.data_len = 1,
> > +	};
> > +	u8 hpd_status;
> > +	struct lt9611c_rsp rsp = { .data = &hpd_status, .data_len = 1 };
> > +	int ret;
> > +
> > +	mutex_lock(&lt9611c->ocm_lock);
> 
> scoped_guard()
Will change it in v9.
> 
> > +	ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp);
> > +	if (!ret)
> > +		lt9611c->hdmi_connected = (hpd_status == 0x02);
> > +	mutex_unlock(&lt9611c->ocm_lock);
> 
> How does hpd_enable work? Will it report HPD events even if the bridge
> is suspended? WHere is hpd_disable() callback?
We have added hpd_enable as there was tlmm interrupt issues, now it has
been fixed. We are removing this api.
> 
> > +
> > +	schedule_work(&lt9611c->work);
> > +}
> > +
> > +static int lt9611c_hdmi_audio_startup(struct drm_bridge *bridge,
> > +				      struct drm_connector *connector)
> > +{
> > +	return 0;
> > +}
> 
> Misplaced and unnecessary.
Will remove it in v9.
> 
> > +
> > +static const struct drm_bridge_funcs lt9611c_bridge_funcs = {
> > +	.attach = lt9611c_bridge_attach,
> > +	.detect = lt9611c_bridge_detect,
> > +	.edid_read = lt9611c_bridge_edid_read,
> > +	.atomic_enable = lt9611c_bridge_atomic_enable,
> > +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > +	.atomic_create_state = drm_atomic_helper_bridge_create_state,
> > +	.hpd_enable = lt9611c_bridge_hpd_enable,
> > +
> > +	.hdmi_tmds_char_rate_valid = lt9611c_hdmi_tmds_char_rate_valid,
> > +	.hdmi_write_avi_infoframe = lt9611c_hdmi_write_avi_infoframe,
> > +	.hdmi_clear_avi_infoframe = lt9611c_hdmi_clear_avi_infoframe,
> > +	.hdmi_write_hdmi_infoframe = lt9611c_hdmi_write_hdmi_infoframe,
> > +	.hdmi_clear_hdmi_infoframe = lt9611c_hdmi_clear_hdmi_infoframe,
> > +	.hdmi_write_audio_infoframe = lt9611c_hdmi_write_audio_infoframe,
> > +	.hdmi_clear_audio_infoframe = lt9611c_hdmi_clear_audio_infoframe,
> > +
> > +	.hdmi_audio_startup = lt9611c_hdmi_audio_startup,
> > +	.hdmi_audio_prepare = lt9611c_hdmi_audio_prepare,
> > +	.hdmi_audio_shutdown = lt9611c_hdmi_audio_shutdown,
> > +};
> > +
> > +static int lt9611c_parse_dt(struct device *dev,
> > +			    struct lt9611c *lt9611c)
> > +{
> > +	lt9611c->dsi0_node = of_graph_get_remote_node(dev->of_node, 0, -1);
> > +	if (!lt9611c->dsi0_node)
> > +		return dev_err_probe(dev, -ENODEV, "failed to get remote node for primary dsi\n");
> > +
> > +	lt9611c->dsi1_node = of_graph_get_remote_node(dev->of_node, 1, -1);
> > +
> > +	if (lt9611c->dsi1_node && lt9611c->chip_type == CHIP_LT9611C) {
> > +		of_node_put(lt9611c->dsi1_node);
> > +		of_node_put(lt9611c->dsi0_node);
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "LT9611C does not support dual DSI\n");
> > +	}
> > +
> > +	lt9611c->bridge.next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 2, -1);
> > +	if (IS_ERR(lt9611c->bridge.next_bridge)) {
> > +		of_node_put(lt9611c->dsi1_node);
> > +		of_node_put(lt9611c->dsi0_node);
> > +		return PTR_ERR(lt9611c->bridge.next_bridge);
> 
> I'd rather see a normal goto-based error path.
Will fix it in v9.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> -- 
> With best wishes
> Dmitry