RE: [PATCH v3 2/2] drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver

[<Hermes.Wu@ite.com.tw>]

Hi

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 
>Sent: Saturday, March 14, 2026 10:09 AM
>To: Hermes Wu (吳佳宏) <Hermes.Wu@ite.com.tw>
>Cc: Andrzej Hajda <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Jonas Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Pet Weng (翁玉芬) <Pet.Weng@ite.com.tw>; Kenneth Hung (洪家倫) <Kenneth.Hung@ite.com.tw>; dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v3 2/2] drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver
>
>On Fri, Mar 13, 2026 at 02:16:01PM +0800, Hermes Wu via B4 Relay wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> Add support for the ITE IT6162 MIPI DSI to HDMI 2.0 bridge chip.
>> The IT6162 is an I2C-controlled bridge that supports the following
>> configurations:
>> 
>>   - Single MIPI DSI input: up to 4K @ 30Hz
>>   - Dual MIPI DSI input (combined): up to 4K @ 60Hz
>> 
>> The driver implements the DRM bridge and connector frameworks, 
>> including mode setting, EDID retrieval, and HPD support.
>> 
>> Also add a MAINTAINERS entry for the newly introduced ITE IT6162 MIPI 
>> DSI to HDMI bridge driver, covering the driver source file and the 
>> device tree binding document.
>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>> Changes in v3:
>>   * Fix OFFSET_VERSION_H register offset from 0x03 to 0x05
>>   * Add MIPI_PORT_EN_MASK macro combining MIPI_PORT1_EN_MASK and MIPI_PORT0_EN_MASK
>>   * Rename HDCP enums: NO_HDCP -> HDCP_DISABLE, NO_HDCP_STATE -> HDCP_STATE_IDLE,
>>     AUTH_DONE -> HDCP_STATE_AUTH_DONE, AUTH_FAIL -> HDCP_STATE_AUTH_FAIL
>>   * Rename it6162_infoblock_complete() to it6162_wait_command_complete()
>>   * Rename it6162_infoblock_host_set() to it6162_infoblock_trigger()
>>   * Remove it6162_infoblock_mipi_config() and it6162_infoblock_write_msg()
>>     wrappers, inline into it6162_reset_init() and
>>     it6162_mipi_set_video_timing() respectively
>>   * Remove it6162_set_default_config() wrapper, inline into
>>     it6162_reset_init()
>>   * Fix typo: hdcp_encyption -> hdcp_encryption
>>   * Fix typo: it6162_hdcp_read_infomation -> it6162_hdcp_read_information
>>   * Remove dev_err_probe() usage outside of probe path
>>   * Remove verbose success-path dev_info/dev_dbg logging throughout
>>   * Replace __func__ usage in error messages with descriptive strings
>>   * Fix double error printing in probe for it6162_init_pdata failure path
>>   * Fix uninitialized variable warning: initialize cp_status to
>>     DRM_MODE_CONTENT_PROTECTION_DESIRED at declaration; move
>>     drm_hdcp_update_content_protection() inside the state-change block
>>   * Fix audio sample width mapping: case 20 now maps to WORD_LENGTH_20BIT,
>>     case 24 to WORD_LENGTH_24BIT
>>   * Remove stray drm_dbg("it6162_bridge_atomic_disable") call
>>   * Remove drm_dbg() calls from it6162_display_mode_to_settings()
>>   * Drop unused struct it6162 * parameter from it6162_avi_to_video_setting()
>>     and it6162_display_mode_to_settings()
>>   * Fold it6162_set_default_config() body directly into it6162_reset_init(),
>>     removing the wrapper
>>   * Fix it6162_infoblock_request_data(): split command complete polling and
>>     buffer status check into two steps; use wait_event_timeout() for
>>     data_buf_sts since it is updated asynchronously by the interrupt
>>     handler; add wait_queue_head_t data_buf_wait to struct it6162 and
>>     wake_up() in interrupt handler
>> ---
>>  MAINTAINERS                         |    7 +
>>  drivers/gpu/drm/bridge/Kconfig      |   17 +
>>  drivers/gpu/drm/bridge/Makefile     |    1 +
>>  drivers/gpu/drm/bridge/ite-it6162.c | 1631 
>> +++++++++++++++++++++++++++++++++++
>>  4 files changed, 1656 insertions(+)
>> 
>> +
>> +static int it6162_bridge_hdmi_audio_prepare(struct drm_bridge *bridge,
>> +					    struct drm_connector *connector,
>> +					    struct hdmi_codec_daifmt *fmt,
>> +					    struct hdmi_codec_params *params) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +	struct it6162_audio config;
>> +
>> +	it6162_audio_update_hw_params(it6162, &config, fmt, params);
>> +	it6162_enable_audio(it6162, &config);
>
>You write the intermediate structure, use it and then forget it immediately. Can it be removed completely?

yes. it will remove in next patch

>> +	return drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
>> +								       &params->cea);
>> +}
>> +
>> +static int it6162_bridge_hdmi_audio_startup(struct drm_bridge *bridge,
>> +					    struct drm_connector *connector) {
>> +	return 0;
>> +}
>
>This is not necessary and can be dropped.
>
>> +
>> +static void it6162_bridge_hdmi_audio_shutdown(struct drm_bridge *bridge,
>> +					      struct drm_connector *connector) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>
>drm_atomic_helper_connector_hdmi_clear_audio_infoframe() here.
>
>> +
>> +	it6162_disable_audio(it6162);
>> +}
>> +
>> +static enum drm_mode_status
>> +it6162_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
>> +				 const struct drm_display_mode *mode,
>> +				 unsigned long long tmds_rate)
>> +{
>> +	/*IT6162 hdmi supports HDMI2.0 600Mhz*/
>> +	if (tmds_rate > 600000000)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	return MODE_OK;
>> +}
>> +
>> +static inline int
>> +it6162_write_infoframe(struct it6162 *it6162, const u8 *buffer, 
>> +size_t len) {
>> +	if (len > DATA_BUFFER_DEPTH)
>> +		return -EINVAL;
>> +
>> +	regmap_bulk_write(it6162->regmap, OFFSET_DATA_BUFFER, buffer, len);
>> +	regmap_write(it6162->regmap, OFFSET_DATA_TYPE_IDX, len);
>> +	it6162_infoblock_trigger(it6162, HOST_SET_CEA_INFOFRAME);
>
>Does the hardware automatically identify, which infoframe is it?
>
FW will check infoframe type in Header.

>> +	return 0;
>> +}
>> +
>> +static inline int it6162_clear_infoframe(struct it6162 *it6162, u8 
>> +type) {
>> +	regmap_write(it6162->regmap, OFFSET_DATA_TYPE_IDX, 3);
>> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER, type);
>> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER + 1, 0x00);
>> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER + 2, 0x00);
>> +	it6162_infoblock_trigger(it6162, HOST_SET_CEA_INFOFRAME);
>> +	return 0;
>> +}
>> +
>> +static int
>> +it6162_bridge_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
>> +				       const u8 *buffer, size_t len) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_write_infoframe(it6162, buffer, len); }
>> +
>> +static int
>> +it6162_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridge) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_clear_infoframe(it6162, HDMI_INFOFRAME_TYPE_AVI); }
>> +
>> +static int
>> +it6162_bridge_hdmi_write_audio_infoframe(struct drm_bridge *bridge,
>> +					 const u8 *buffer, size_t len)
>> +{
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_write_infoframe(it6162, buffer, len); }
>> +
>> +static int
>> +it6162_bridge_hdmi_clear_audio_infoframe(struct drm_bridge *bridge) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_clear_infoframe(it6162, HDMI_INFOFRAME_TYPE_AUDIO); }
>> +
>> +static int
>> +it6162_bridge_hdmi_write_spd_infoframe(struct drm_bridge *bridge,
>> +				       const u8 *buffer, size_t len) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_write_infoframe(it6162, buffer, len); }
>> +
>> +static int
>> +it6162_bridge_hdmi_clear_spd_infoframe(struct drm_bridge *bridge) {
>> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
>> +
>> +	return it6162_clear_infoframe(it6162, HDMI_INFOFRAME_TYPE_SPD); }
>> +
>> +static const struct drm_bridge_funcs it6162_bridge_funcs = {
>> +	.attach = it6162_bridge_attach,
>> +	.mode_valid = it6162_bridge_mode_valid,
>> +	.detect = it6162_bridge_detect,
>> +
>> +	.atomic_enable = it6162_bridge_atomic_enable,
>> +	.atomic_disable = it6162_bridge_atomic_disable,
>> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>> +	.atomic_check = it6162_bridge_atomic_check,
>> +
>> +	.edid_read = it6162_bridge_read_edid,
>> +
>> +	.hdmi_clear_avi_infoframe = it6162_bridge_hdmi_clear_avi_infoframe,
>> +	.hdmi_write_avi_infoframe = it6162_bridge_hdmi_write_avi_infoframe,
>> +	.hdmi_clear_spd_infoframe = it6162_bridge_hdmi_clear_spd_infoframe,
>> +	.hdmi_write_spd_infoframe = it6162_bridge_hdmi_write_spd_infoframe,
>> +	.hdmi_clear_audio_infoframe = it6162_bridge_hdmi_clear_audio_infoframe,
>> +	.hdmi_write_audio_infoframe = 
>> +it6162_bridge_hdmi_write_audio_infoframe,
>
>No HDMI Infoframes support? That's really sad.

Is HDMI Infoframe means HDMI-VSIF and HDMI-HF-VSIF?
>
>> +
>> +	.hdmi_tmds_char_rate_valid = it6162_hdmi_tmds_char_rate_valid,
>> +	.hdmi_audio_prepare = it6162_bridge_hdmi_audio_prepare,
>> +	.hdmi_audio_startup = it6162_bridge_hdmi_audio_startup,
>> +	.hdmi_audio_shutdown = it6162_bridge_hdmi_audio_shutdown,
>> +};
>> +
>> +static int it6162_probe(struct i2c_client *client) {
>> +	struct device *dev = &client->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct it6162 *it6162;
>> +	int ret;
>> +
>> +	it6162 = devm_drm_bridge_alloc(dev, struct it6162, bridge,
>> +				       &it6162_bridge_funcs);
>> +	if (IS_ERR(it6162))
>> +		return PTR_ERR(it6162);
>> +
>> +	it6162->dev = dev;
>> +
>> +	ret = it6162_of_get_dsi_host(it6162);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = it6162_i2c_regmap_init(client, it6162);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	ret = it6162_init_pdata(it6162);
>> +	if (ret)
>> +		return ret;
>> +
>> +	it6162_config_default(it6162);
>> +	it6162_parse_dt(it6162);
>> +
>> +	if (it6162_detect_devices(it6162) < 0)
>> +		return -ENODEV;
>> +
>> +	if (!client->irq) {
>> +		dev_err(dev, "Failed to get INTP IRQ");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +					it6162_int_threaded_handler,
>> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT |
>> +					IRQF_NO_AUTOEN,
>> +					"it6162-intp", it6162);
>> +	if (ret)
>> +		return ret;
>> +
>> +	INIT_DELAYED_WORK(&it6162->hdcp_work, it6162_hdcp_work);
>> +	init_waitqueue_head(&it6162->data_buf_wait);
>> +
>> +	mutex_init(&it6162->lock);
>> +
>> +	it6162->bridge.of_node = np;
>> +	it6162->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>> +			     DRM_BRIDGE_OP_MODES;
>> +
>> +	it6162->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>> +
>> +	it6162->bridge.vendor = "ITE";
>> +	it6162->bridge.product = "IT6162";
>
>You missed to specify DRM_BRIDGE_OP_HDMI, so all HDMI callbacks will be ignored.
>
>> +
>> +	if (it6162_of_get_audio(it6162)) {
>> +		it6162->bridge.ops |= DRM_BRIDGE_OP_HDMI_AUDIO;
>> +		it6162->bridge.hdmi_audio_dev = dev;
>> +		it6162->bridge.hdmi_audio_max_i2s_playback_channels = 8;
>> +		it6162->bridge.hdmi_audio_dai_port = 2;
>> +	}
>> +
>> +	devm_drm_bridge_add(dev, &it6162->bridge);
>> +
>> +	return it6162_attach_dsi(it6162);
>> +}
>> +
>> +static void it6162_remove(struct i2c_client *client) {
>> +	struct it6162 *it6162 = i2c_get_clientdata(client);
>> +
>> +	disable_irq(client->irq);
>> +	cancel_delayed_work_sync(&it6162->hdcp_work);
>> +	mutex_destroy(&it6162->lock);
>> +}
>> +
>> +static const struct it6162_chip_info it6162_chip_info = {
>> +	.chip_id = 0x616200,
>> +	.version = 0x006500,
>> +};
>> +
>> +static const struct of_device_id it6162_dt_ids[] = {
>> +	{ .compatible = "ite,it6162", .data = &it6162_chip_info},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, it6162_dt_ids);
>> +
>> +static const struct i2c_device_id it6162_i2c_ids[] = {
>> +	{ "it6162", 0 },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, it6162_i2c_ids);
>> +
>> +static struct i2c_driver it6162_driver = {
>> +	.driver = {
>> +		.name = "it6162",
>> +		.of_match_table = it6162_dt_ids,
>> +	},
>> +	.probe = it6162_probe,
>> +	.remove = it6162_remove,
>> +	.id_table = it6162_i2c_ids,
>> +};
>> +module_i2c_driver(it6162_driver);
>> +
>> +MODULE_AUTHOR("Pet Weng <pet.weng@ite.com.tw>"); 
>> +MODULE_AUTHOR("Hermes Wu <Hermes.Wu@ite.com.tw>");
>> +MODULE_DESCRIPTION("it6162 MIPI to HDMI driver"); 
>> +MODULE_LICENSE("GPL");
>> 
>> --
>> 2.34.1
>> 
>> 
>
>--
>With best wishes
>Dmitry
>

BR,
Hermes