[PATCH v4 0/3] drm/bridge: ite-it66121: Fix display output for DVI monitors

[PATCH v4 0/3] drm/bridge: ite-it66121: Fix display output for DVI monitors

[Javier Martinez Canillas]

Display output does not work when connecting an AM625 BeaglePlay board to
a DVI monitor, because the DRM it66121 bridge driver assumes that the sink
type is always HDMI. This patch series fixes the issue.

Patch #1 reworks the driver to use the HDMI helpers instead of open coding
the AVI infoframes buffer management.

Patch #2 moves the .mode_set logic to the .atomic_enable handler.

Patch #3 finally fixes the mentioned issue by using the display information
to determine whether HDMI or DVI mode should be set.

This is a v4 of the series, that addresses issues pointed out by Maxime.

The patches were tested on both DVI and an HDMI monitors.

Changes in v4:
- Convert the driver to use the HDMI helpers (Maxime Ripard).
- Move .mode_set logic to .atomic_enable (Maxime Ripard).

Changes in v3:
- Move the HDMI/DVI mode set to the .atomic_enable handler (Maxime Ripard).

Changes in v2:
- Don't store the sink type in a per-commit bridge state (Maxime Ripard).

Javier Martinez Canillas (3):
  drm/bridge: ite-it66121: Switch to the HDMI connector helpers
  drm/bridge: ite-it66121: Move logic .mode_set setup to .atomic_enable
  drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

 drivers/gpu/drm/bridge/Kconfig       |   2 +
 drivers/gpu/drm/bridge/ite-it66121.c | 198 ++++++++++++++++-----------
 2 files changed, 121 insertions(+), 79 deletions(-)

-- 
2.54.0

base-commit: 4c26e162947f91aa78ba57dd4fddd38fc80e7d60
branch: it66121-fix-dvi-mode-v4

[PATCH v4 1/3] drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Javier Martinez Canillas]

Instead of open coding the HDMI AVI Infoframes buffer management,
use the helpers provided by the HDMI connector framework.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v4:
- New patch for v4

 drivers/gpu/drm/bridge/Kconfig       |   2 +
 drivers/gpu/drm/bridge/ite-it66121.c | 132 ++++++++++++++++-----------
 2 files changed, 83 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index f81b566c82a1..4a57d49b4c6d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -205,6 +205,8 @@ config DRM_LONTIUM_LT8713SX
 config DRM_ITE_IT66121
 	tristate "ITE IT66121 HDMI bridge"
 	depends on OF
+	select DRM_DISPLAY_HDMI_STATE_HELPER
+	select DRM_DISPLAY_HELPER
 	select DRM_KMS_HELPER
 	select REGMAP_I2C
 	help
diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 19a027d75b61..947b7a0f0a45 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -20,6 +20,8 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/regulator/consumer.h>
 
+#include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
@@ -304,7 +306,6 @@ struct it66121_ctx {
 	struct i2c_client *client;
 	u32 bus_width;
 	struct mutex lock; /* Protects fields below and device registers */
-	struct hdmi_avi_infoframe hdmi_avi_infoframe;
 	struct {
 		struct platform_device *pdev;
 		u8 ch_enable;
@@ -727,6 +728,10 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
 
 	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	if (!ctx->connector)
+		return;
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
 
 	it66121_set_mute(ctx, false);
 }
@@ -764,40 +769,10 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 			     const struct drm_display_mode *mode,
 			     const struct drm_display_mode *adjusted_mode)
 {
-	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
-	int ret;
 
 	mutex_lock(&ctx->lock);
 
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe, ctx->connector,
-						       adjusted_mode);
-	if (ret) {
-		DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
-		goto unlock;
-	}
-
-	ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf, sizeof(buf));
-	if (ret < 0) {
-		DRM_ERROR("Failed to pack infoframe: %d\n", ret);
-		goto unlock;
-	}
-
-	/* Write new AVI infoframe packet */
-	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
-				&buf[HDMI_INFOFRAME_HEADER_SIZE],
-				HDMI_AVI_INFOFRAME_SIZE);
-	if (ret)
-		goto unlock;
-
-	if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
-		goto unlock;
-
-	/* Enable AVI infoframe */
-	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
-			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
-		goto unlock;
-
 	/* Set TX mode to HDMI */
 	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
 		goto unlock;
@@ -825,24 +800,6 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 	mutex_unlock(&ctx->lock);
 }
 
-static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
-						      const struct drm_display_info *info,
-						      const struct drm_display_mode *mode)
-{
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
-	unsigned long max_clock;
-
-	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
-
-	if (mode->clock > max_clock)
-		return MODE_CLOCK_HIGH;
-
-	if (mode->clock < 25000)
-		return MODE_CLOCK_LOW;
-
-	return MODE_OK;
-}
-
 static enum drm_connector_status
 it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
 {
@@ -873,6 +830,72 @@ static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
 		dev_err(ctx->dev, "failed to disable HPD IRQ\n");
 }
 
+static enum drm_mode_status
+it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+					 const struct drm_display_mode *mode,
+					 unsigned long long tmds_rate)
+{
+	const struct it66121_ctx *ctx =
+		container_of(bridge, const struct it66121_ctx, bridge);
+	unsigned long max_clock;
+
+	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
+
+	if (mode->clock > max_clock)
+		return MODE_CLOCK_HIGH;
+
+	if (mode->clock < 25000)
+		return MODE_CLOCK_LOW;
+
+	return MODE_OK;
+}
+
+static int it66121_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridge)
+{
+	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+
+	return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0);
+}
+
+static int it66121_bridge_hdmi_clear_hdmi_infoframe(struct drm_bridge *bridge)
+{
+	return 0;
+}
+
+static int it66121_bridge_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
+						   const u8 *buffer, size_t len)
+{
+	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	int ret;
+
+	mutex_lock(&ctx->lock);
+
+	/* Write new AVI infoframe packet */
+	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
+				&buffer[HDMI_INFOFRAME_HEADER_SIZE],
+				HDMI_AVI_INFOFRAME_SIZE);
+	if (ret)
+		goto unlock;
+
+	ret = regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buffer[3]);
+	if (ret)
+		goto unlock;
+
+	/* Enable AVI infoframe */
+	ret = regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
+			   IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT);
+
+unlock:
+	mutex_unlock(&ctx->lock);
+	return ret;
+}
+
+static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
+						    const u8 *buffer, size_t len)
+{
+	return 0;
+}
+
 static const struct drm_edid *it66121_bridge_edid_read(struct drm_bridge *bridge,
 						       struct drm_connector *connector)
 {
@@ -913,11 +936,15 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
 	.atomic_disable = it66121_bridge_disable,
 	.atomic_check = it66121_bridge_check,
 	.mode_set = it66121_bridge_mode_set,
-	.mode_valid = it66121_bridge_mode_valid,
 	.detect = it66121_bridge_detect,
 	.edid_read = it66121_bridge_edid_read,
 	.hpd_enable = it66121_bridge_hpd_enable,
 	.hpd_disable = it66121_bridge_hpd_disable,
+	.hdmi_tmds_char_rate_valid = it66121_bridge_hdmi_tmds_char_rate_valid,
+	.hdmi_clear_avi_infoframe = it66121_bridge_hdmi_clear_avi_infoframe,
+	.hdmi_write_avi_infoframe = it66121_bridge_hdmi_write_avi_infoframe,
+	.hdmi_clear_hdmi_infoframe = it66121_bridge_hdmi_clear_hdmi_infoframe,
+	.hdmi_write_hdmi_infoframe = it66121_bridge_hdmi_write_hdmi_infoframe,
 };
 
 static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
@@ -1588,7 +1615,10 @@ static int it66121_probe(struct i2c_client *client)
 
 	ctx->bridge.of_node = dev->of_node;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
-	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
+			  DRM_BRIDGE_OP_HDMI;
+	ctx->bridge.vendor = "ITE";
+	ctx->bridge.product = "IT66121";
 	if (client->irq > 0) {
 		ctx->bridge.ops |= DRM_BRIDGE_OP_HPD;
 
-- 
2.54.0

[PATCH v4 2/3] drm/bridge: ite-it66121: Move logic .mode_set setup to .atomic_enable

[Javier Martinez Canillas]

Move the existing .mode_set logic to the .atomic_enable callback. The
former is deprecated and drivers are supposed to use the latter instead.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v4:
- New patch for v4

 drivers/gpu/drm/bridge/ite-it66121.c | 79 +++++++++++++++-------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 947b7a0f0a45..c8795247cfa8 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -654,6 +654,47 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 	return 0;
 }
 
+static void it66121_set_mode(struct it66121_ctx *ctx,
+			     struct drm_atomic_commit *state)
+{
+	struct drm_connector *connector = ctx->connector;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_display_mode *mode;
+	struct drm_crtc *crtc;
+
+	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	mode = &crtc_state->adjusted_mode;
+
+	mutex_lock(&ctx->lock);
+
+	/* Set TX mode to HDMI */
+	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
+		goto unlock;
+
+	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
+	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+			      IT66121_CLK_BANK_PWROFF_TXCLK,
+			      IT66121_CLK_BANK_PWROFF_TXCLK)) {
+		goto unlock;
+	}
+
+	if (it66121_configure_input(ctx))
+		goto unlock;
+
+	if (it66121_configure_afe(ctx, mode))
+		goto unlock;
+
+	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
+	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
+			      IT66121_CLK_BANK_PWROFF_TXCLK, 0)) {
+		goto unlock;
+	}
+
+unlock:
+	mutex_unlock(&ctx->lock);
+}
+
 static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
 {
 	int ret;
@@ -733,6 +774,7 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 
 	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
 
+	it66121_set_mode(ctx, state);
 	it66121_set_mute(ctx, false);
 }
 
@@ -764,42 +806,6 @@ static int it66121_bridge_check(struct drm_bridge *bridge,
 	return 0;
 }
 
-static
-void it66121_bridge_mode_set(struct drm_bridge *bridge,
-			     const struct drm_display_mode *mode,
-			     const struct drm_display_mode *adjusted_mode)
-{
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
-
-	mutex_lock(&ctx->lock);
-
-	/* Set TX mode to HDMI */
-	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
-		goto unlock;
-
-	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
-	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
-			      IT66121_CLK_BANK_PWROFF_TXCLK,
-			      IT66121_CLK_BANK_PWROFF_TXCLK)) {
-		goto unlock;
-	}
-
-	if (it66121_configure_input(ctx))
-		goto unlock;
-
-	if (it66121_configure_afe(ctx, adjusted_mode))
-		goto unlock;
-
-	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
-	    regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
-			      IT66121_CLK_BANK_PWROFF_TXCLK, 0)) {
-		goto unlock;
-	}
-
-unlock:
-	mutex_unlock(&ctx->lock);
-}
-
 static enum drm_connector_status
 it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
 {
@@ -935,7 +941,6 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
 	.atomic_enable = it66121_bridge_enable,
 	.atomic_disable = it66121_bridge_disable,
 	.atomic_check = it66121_bridge_check,
-	.mode_set = it66121_bridge_mode_set,
 	.detect = it66121_bridge_detect,
 	.edid_read = it66121_bridge_edid_read,
 	.hpd_enable = it66121_bridge_hpd_enable,
-- 
2.54.0

[PATCH v4 3/3] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

[Javier Martinez Canillas]

The driver unconditionally sets the transmission mode to HDMI, which leads
to display output not working with DVI monitors. Check connector's display
information sink type to identify the correct mode to configure the bridge.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v4:
- Convert the driver to use the HDMI helpers (Maxime Ripard).
- Move .mode_set logic to .atomic_enable (Maxime Ripard).

Changes in v3:
- Move the HDMI/DVI mode set to the .atomic_enable handler (Maxime Ripard).

Changes in v2:
- Don't store the sink type in a per-commit bridge state (Maxime Ripard).

 drivers/gpu/drm/bridge/ite-it66121.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index c8795247cfa8..95ee98a4a8df 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -184,6 +184,7 @@
 
 #define IT66121_HDMI_MODE_REG			0xC0
 #define IT66121_HDMI_MODE_HDMI			BIT(0)
+#define IT66121_HDMI_MODE_DVI			0
 
 #define IT66121_SYS_STATUS_REG			0x0E
 #define IT66121_SYS_STATUS_ACTIVE_IRQ		BIT(7)
@@ -658,6 +659,7 @@ static void it66121_set_mode(struct it66121_ctx *ctx,
 			     struct drm_atomic_commit *state)
 {
 	struct drm_connector *connector = ctx->connector;
+	unsigned int tx_mode = IT66121_HDMI_MODE_HDMI;
 	const struct drm_crtc_state *crtc_state;
 	const struct drm_display_mode *mode;
 	struct drm_crtc *crtc;
@@ -666,10 +668,13 @@ static void it66121_set_mode(struct it66121_ctx *ctx,
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	mode = &crtc_state->adjusted_mode;
 
+	if (!connector->display_info.is_hdmi)
+		tx_mode = IT66121_HDMI_MODE_DVI;
+
 	mutex_lock(&ctx->lock);
 
-	/* Set TX mode to HDMI */
-	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
+	/* Set TX mode to HDMI or DVI */
+	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, tx_mode))
 		goto unlock;
 
 	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&
-- 
2.54.0

Re: [PATCH v4 1/3] drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6695 bytes --]

On Fri, May 15, 2026 at 11:02:09AM +0200, Javier Martinez Canillas wrote:
> Instead of open coding the HDMI AVI Infoframes buffer management,
> use the helpers provided by the HDMI connector framework.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v4:
> - New patch for v4
> 
>  drivers/gpu/drm/bridge/Kconfig       |   2 +
>  drivers/gpu/drm/bridge/ite-it66121.c | 132 ++++++++++++++++-----------
>  2 files changed, 83 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index f81b566c82a1..4a57d49b4c6d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -205,6 +205,8 @@ config DRM_LONTIUM_LT8713SX
>  config DRM_ITE_IT66121
>  	tristate "ITE IT66121 HDMI bridge"
>  	depends on OF
> +	select DRM_DISPLAY_HDMI_STATE_HELPER
> +	select DRM_DISPLAY_HELPER
>  	select DRM_KMS_HELPER
>  	select REGMAP_I2C
>  	help
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 19a027d75b61..947b7a0f0a45 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -20,6 +20,8 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <drm/display/drm_hdmi_helper.h>
> +#include <drm/display/drm_hdmi_state_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_edid.h>
> @@ -304,7 +306,6 @@ struct it66121_ctx {
>  	struct i2c_client *client;
>  	u32 bus_width;
>  	struct mutex lock; /* Protects fields below and device registers */
> -	struct hdmi_avi_infoframe hdmi_avi_infoframe;
>  	struct {
>  		struct platform_device *pdev;
>  		u8 ch_enable;
> @@ -727,6 +728,10 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
>  
>  	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> +	if (!ctx->connector)
> +		return;
> +
> +	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
>  
>  	it66121_set_mute(ctx, false);
>  }
> @@ -764,40 +769,10 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>  			     const struct drm_display_mode *mode,
>  			     const struct drm_display_mode *adjusted_mode)
>  {
> -	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> -	int ret;
>  
>  	mutex_lock(&ctx->lock);
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe, ctx->connector,
> -						       adjusted_mode);
> -	if (ret) {
> -		DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
> -		goto unlock;
> -	}
> -
> -	ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf, sizeof(buf));
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to pack infoframe: %d\n", ret);
> -		goto unlock;
> -	}
> -
> -	/* Write new AVI infoframe packet */
> -	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
> -				&buf[HDMI_INFOFRAME_HEADER_SIZE],
> -				HDMI_AVI_INFOFRAME_SIZE);
> -	if (ret)
> -		goto unlock;
> -
> -	if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
> -		goto unlock;
> -
> -	/* Enable AVI infoframe */
> -	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> -			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
> -		goto unlock;
> -
>  	/* Set TX mode to HDMI */
>  	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
>  		goto unlock;
> @@ -825,24 +800,6 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>  	mutex_unlock(&ctx->lock);
>  }
>  
> -static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
> -						      const struct drm_display_info *info,
> -						      const struct drm_display_mode *mode)
> -{
> -	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> -	unsigned long max_clock;
> -
> -	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
> -
> -	if (mode->clock > max_clock)
> -		return MODE_CLOCK_HIGH;
> -
> -	if (mode->clock < 25000)
> -		return MODE_CLOCK_LOW;
> -
> -	return MODE_OK;
> -}
> -
>  static enum drm_connector_status
>  it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>  {
> @@ -873,6 +830,72 @@ static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
>  		dev_err(ctx->dev, "failed to disable HPD IRQ\n");
>  }
>  
> +static enum drm_mode_status
> +it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> +					 const struct drm_display_mode *mode,
> +					 unsigned long long tmds_rate)
> +{
> +	const struct it66121_ctx *ctx =
> +		container_of(bridge, const struct it66121_ctx, bridge);
> +	unsigned long max_clock;
> +
> +	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
> +
> +	if (mode->clock > max_clock)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (mode->clock < 25000)
> +		return MODE_CLOCK_LOW;
> +
> +	return MODE_OK;
> +}
> +
> +static int it66121_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridge)
> +{
> +	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +
> +	return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0);
> +}
> +
> +static int it66121_bridge_hdmi_clear_hdmi_infoframe(struct drm_bridge *bridge)
> +{
> +	return 0;
> +}
> +
> +static int it66121_bridge_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
> +						   const u8 *buffer, size_t len)
> +{
> +	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +	int ret;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	/* Write new AVI infoframe packet */
> +	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
> +				&buffer[HDMI_INFOFRAME_HEADER_SIZE],
> +				HDMI_AVI_INFOFRAME_SIZE);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buffer[3]);
> +	if (ret)
> +		goto unlock;
> +
> +	/* Enable AVI infoframe */
> +	ret = regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> +			   IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT);
> +
> +unlock:
> +	mutex_unlock(&ctx->lock);
> +	return ret;
> +}
> +
> +static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
> +						    const u8 *buffer, size_t len)
> +{
> +	return 0;
> +}
> +

When the code says it's mandatory, it means "it needs to be implemented" ;)

Looks good otherwise
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/3] drm/bridge: ite-it66121: Move logic .mode_set setup to .atomic_enable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On Fri, May 15, 2026 at 11:02:10AM +0200, Javier Martinez Canillas wrote:
> Move the existing .mode_set logic to the .atomic_enable callback. The
> former is deprecated and drivers are supposed to use the latter instead.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v4:
> - New patch for v4
> 
>  drivers/gpu/drm/bridge/ite-it66121.c | 79 +++++++++++++++-------------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 947b7a0f0a45..c8795247cfa8 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -654,6 +654,47 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>  	return 0;
>  }
>  
> +static void it66121_set_mode(struct it66121_ctx *ctx,
> +			     struct drm_atomic_commit *state)
> +{
> +	struct drm_connector *connector = ctx->connector;

We should drop this entirely now. It's available in atomic_enable, you should pass it as an
argument.

> +	const struct drm_crtc_state *crtc_state;
> +	const struct drm_display_mode *mode;
> +	struct drm_crtc *crtc;
> +
> +	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);

drm_atomic_get_new_connector_state and drm_atomic_get_new_crtc_state can
return a NULL pointer. We should check, warn and return if it's the
case.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 3/3] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2264 bytes --]

On Fri, May 15, 2026 at 11:02:11AM +0200, Javier Martinez Canillas wrote:
> The driver unconditionally sets the transmission mode to HDMI, which leads
> to display output not working with DVI monitors. Check connector's display
> information sink type to identify the correct mode to configure the bridge.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v4:
> - Convert the driver to use the HDMI helpers (Maxime Ripard).
> - Move .mode_set logic to .atomic_enable (Maxime Ripard).
> 
> Changes in v3:
> - Move the HDMI/DVI mode set to the .atomic_enable handler (Maxime Ripard).
> 
> Changes in v2:
> - Don't store the sink type in a per-commit bridge state (Maxime Ripard).
> 
>  drivers/gpu/drm/bridge/ite-it66121.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index c8795247cfa8..95ee98a4a8df 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -184,6 +184,7 @@
>  
>  #define IT66121_HDMI_MODE_REG			0xC0
>  #define IT66121_HDMI_MODE_HDMI			BIT(0)
> +#define IT66121_HDMI_MODE_DVI			0
>  
>  #define IT66121_SYS_STATUS_REG			0x0E
>  #define IT66121_SYS_STATUS_ACTIVE_IRQ		BIT(7)
> @@ -658,6 +659,7 @@ static void it66121_set_mode(struct it66121_ctx *ctx,
>  			     struct drm_atomic_commit *state)
>  {
>  	struct drm_connector *connector = ctx->connector;
> +	unsigned int tx_mode = IT66121_HDMI_MODE_HDMI;
>  	const struct drm_crtc_state *crtc_state;
>  	const struct drm_display_mode *mode;
>  	struct drm_crtc *crtc;
> @@ -666,10 +668,13 @@ static void it66121_set_mode(struct it66121_ctx *ctx,
>  	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  	mode = &crtc_state->adjusted_mode;
>  
> +	if (!connector->display_info.is_hdmi)
> +		tx_mode = IT66121_HDMI_MODE_DVI;
> +
>  	mutex_lock(&ctx->lock);
>  
> -	/* Set TX mode to HDMI */
> -	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
> +	/* Set TX mode to HDMI or DVI */
> +	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, tx_mode))

A ternary operator would probably be better here?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 1/3] drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

> On Fri, May 15, 2026 at 11:02:09AM +0200, Javier Martinez Canillas wrote:

[...]

>> +static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
>> +						    const u8 *buffer, size_t len)
>> +{
>> +	return 0;
>> +}
>> +
>
> When the code says it's mandatory, it means "it needs to be implemented" ;)
>

Fair. The driver currently is only implenting AVI infoframes, but I agree
that needs to implement these too. I'll tackle that in the next iteration.

> Looks good otherwise

Thanks again for all your feedback and patience reviewing this series.

> Maxime

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 1/3] drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Fri, May 15, 2026 at 11:02:09AM +0200, Javier Martinez Canillas wrote:
> +static enum drm_mode_status
> +it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> +					 const struct drm_display_mode *mode,
> +					 unsigned long long tmds_rate)
> +{
> +	const struct it66121_ctx *ctx =
> +		container_of(bridge, const struct it66121_ctx, bridge);
> +	unsigned long max_clock;
> +
> +	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
> +
> +	if (mode->clock > max_clock)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (mode->clock < 25000)
> +		return MODE_CLOCK_LOW;
> +
> +	return MODE_OK;
> +}

You must not use mode->clock here, use the tdms_rate passed as an argument.

>  static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
> @@ -1588,7 +1615,10 @@ static int it66121_probe(struct i2c_client *client)
>  
>  	ctx->bridge.of_node = dev->of_node;
>  	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> -	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
> +	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> +			  DRM_BRIDGE_OP_HDMI;
> +	ctx->bridge.vendor = "ITE";
> +	ctx->bridge.product = "IT66121";
>  	if (client->irq > 0) {
>  		ctx->bridge.ops |= DRM_BRIDGE_OP_HPD;

It's not really for today, but ideally we should move to the audio
helpers at some point too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Claude review: drm/bridge: ite-it66121: Fix display output for DVI monitors

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ite-it66121: Fix display output for DVI monitors
Author: Javier Martinez Canillas <javierm@redhat.com>
Patches: 9
Reviewed: 2026-05-16T09:32:44.392832

---

This is a well-motivated 3-patch series that fixes a real bug: DVI monitors don't work with the IT66121 bridge because the driver unconditionally sets HDMI mode. The approach is sound — convert to the HDMI connector helpers first (patch 1), move `.mode_set` to `.atomic_enable` (patch 2), then add DVI/HDMI mode selection (patch 3). The series follows the pattern established by the sibling `ite-it6263` driver.

There are a couple of issues worth addressing, one functional concern and a few minor observations.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Claude Code Review Bot]

Patch Review

**Good:**
- Correctly adds `DRM_BRIDGE_OP_HDMI` to bridge ops and provides the required `vendor`/`product` fields.
- The infoframe write/clear callbacks properly replicate the existing register programming.
- Adding `DRM_DISPLAY_HDMI_STATE_HELPER` and `DRM_DISPLAY_HELPER` to Kconfig is correct.
- The NULL check added for `ctx->connector` in `it66121_bridge_enable` is a good defensive addition.

**Issue — `hdmi_tmds_char_rate_valid` ignores `tmds_rate`:**

The new callback signature receives a `tmds_rate` parameter but the implementation only validates `mode->clock`:

```c
static enum drm_mode_status
it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
					 const struct drm_display_mode *mode,
					 unsigned long long tmds_rate)
{
	...
	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;

	if (mode->clock > max_clock)
		return MODE_CLOCK_HIGH;

	if (mode->clock < 25000)
		return MODE_CLOCK_LOW;

	return MODE_OK;
}
```

The purpose of this callback is specifically to validate the TMDS character rate, not the pixel clock. The `tmds_rate` value may differ from the pixel clock (e.g., deep color modes multiply the rate). Looking at the reference `it6263` driver, it validates *both* `mode->clock` and `tmds_rate`. The `max_clock` values here (74250/148500 kHz) are pixel clock limits, while `tmds_rate` is in Hz. Consider converting the max clock limits to a TMDS rate check against `tmds_rate` as well, or at minimum validating `tmds_rate` against the maximum TMDS rate the hardware supports. As-is, this could allow modes that exceed the hardware's TMDS capability when deep color is in use. Not a regression since the old `mode_valid` had the same limitation, but this is the right time to fix it.

**Minor — no-op HDMI infoframe callbacks:**

```c
static int it66121_bridge_hdmi_clear_hdmi_infoframe(struct drm_bridge *bridge)
{
	return 0;
}

static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
						    const u8 *buffer, size_t len)
{
	return 0;
}
```

These are stubs that silently succeed. If the hardware genuinely can't send Vendor Specific InfoFrames, this is fine, but a brief comment explaining why would help future readers understand this is intentional rather than a TODO.

**Minor — locking in `it66121_bridge_hdmi_write_avi_infoframe`:**

The write callback takes `ctx->lock` internally. This is fine for now, but worth noting that `drm_atomic_helper_connector_hdmi_update_infoframes` (called from `atomic_enable`) will call both clear and write — the clear callback does *not* take the lock. This asymmetry is harmless since the clear only does a single `regmap_write` (which has its own lock), but it's worth being aware of.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ite-it66121: Move logic .mode_set setup to .atomic_enable

[Claude Code Review Bot]

Patch Review

**Good:**
- Correctly moves from the deprecated `.mode_set` to `.atomic_enable`.
- The new `it66121_set_mode` properly extracts the mode from the atomic state through the connector → CRTC → adjusted_mode chain.

**Issue — ordering of `it66121_set_mode` vs `drm_atomic_helper_connector_hdmi_update_infoframes`:**

In the resulting `atomic_enable`:

```c
	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);

	it66121_set_mode(ctx, state);
	it66121_set_mute(ctx, false);
```

The infoframes are written *before* the hardware is configured (TX mode, clock bank, AFE, etc.). In the original code, the infoframe write happened in `mode_set` which runs before `atomic_enable`, so the ordering was the same. But logically, writing infoframes before the TX mode is set seems backwards — the hardware should probably be configured first, then infoframes sent. Looking at the register programming sequence (power off TX clock → configure input → configure AFE → power on TX clock → set HDMI mode), infoframes should come *after* the TX path is set up. Consider swapping the order so `it66121_set_mode` runs before the infoframe update. However, if this matches the tested behavior on real hardware, it may be intentional.

**Observation — no error propagation from `it66121_set_mode`:**

The function silently swallows errors via `goto unlock`. This isn't new (the old `mode_set` did the same), but now that it's called from `atomic_enable`, it might be worth at least a `dev_err()` on failure so issues are visible in dmesg.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

[Claude Code Review Bot]

Patch Review

**Good:**
- The fix is minimal and correct. Checking `connector->display_info.is_hdmi` is the right way to determine the sink type.
- The `IT66121_HDMI_MODE_DVI` define (value 0) is a nice readability improvement even though it's just clearing the bit.

**Observation:** This is the actual bug-fix patch. When DVI mode is selected, patch 1's infoframe helpers will still be called from `drm_atomic_helper_connector_hdmi_update_infoframes`, but the HDMI connector framework should handle this correctly by not sending infoframes when `is_hdmi` is false (since the connector state's `hdmi.is_limited_range` / infoframe generation should be gated on HDMI mode). Worth verifying that the framework indeed skips AVI infoframe generation for DVI sinks — if not, the driver would be writing infoframes to hardware that's in DVI mode, which would be harmless but wasteful.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

[Claude Code Review Bot]

Patch Review

**Correctness: The core logic is correct**

The approach of checking `connector->display_info.is_hdmi` and conditionally setting the mode/AVI infoframes is the right fix:

```c
if (connector->display_info.is_hdmi) {
	mode = IT66121_HDMI_MODE_HDMI;
	avi_pkt = IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT;
}
```

For DVI sinks, `mode` stays `IT66121_HDMI_MODE_DVI` (0) and `avi_pkt` stays 0, which correctly disables HDMI mode and AVI infoframes.

**Issue: `connector->display_info.is_hdmi` accessed outside mutex**

```c
struct drm_connector *connector = ctx->connector;

if (connector->display_info.is_hdmi) {
	...
}

mutex_lock(&ctx->lock);
```

The `display_info.is_hdmi` check happens before `ctx->lock` is taken. While `display_info` is typically stable once the EDID has been read (it's set during `drm_edid_connector_update()` which happens at detect time), and `ctx->connector` was just set in the calling function, this is not a serious concern in practice — the lock here is protecting the regmap writes, not the connector state. But it's worth noting the code reads `ctx->connector` without the lock.

**Issue: Duplicate writes in `.mode_set` still not addressed**

As mentioned in patch 1, the `.mode_set` callback still unconditionally writes `IT66121_HDMI_MODE_HDMI` and enables AVI infoframes. So the sequence during a mode change is:
1. `.mode_set` writes HDMI mode + AVI infoframes ON (always)
2. `.atomic_enable` writes DVI mode + AVI infoframes OFF (for DVI sinks)

While the end state is correct, the DVI monitor momentarily receives HDMI-mode signaling between steps 1 and 2. This could cause transient issues depending on the monitor. Removing the writes from `.mode_set` would eliminate this.

**Good: `IT66121_HDMI_MODE_DVI` define**

```c
#define IT66121_HDMI_MODE_DVI			0
```

Adding a named constant for the DVI mode value (0) instead of using a bare 0 is good practice and makes the code self-documenting.

**Observation: AVI infoframe content still written in `.mode_set` for DVI**

Even after this series, the `.mode_set` callback still writes AVI infoframe *content* to the `IT66121_AVIINFO_DB1_REG` registers and the checksum register for DVI sinks. While the AVI infoframe *transmission* is disabled (the PKT register is cleared in `.atomic_enable`), writing infoframe content to hardware for a DVI connection is wasted work. This is not a bug — just unnecessary register writes. A cleaner approach would be to skip the AVI infoframe preparation in `.mode_set` for DVI sinks, but that would require `connector` to be available in `.mode_set`, which is the whole problem this series is solving. So this is acceptable as-is; the infoframe content simply goes unused.

**Summary**: The series fixes a real bug correctly but needs the duplicate register writes removed from `.mode_set` in patch 1 (or a clear explanation in the commit message for why they're intentionally kept). The enable ordering (unmute before TX mode set) is also worth reconsidering.

---
Generated by Claude Code Patch Reviewer