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

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

[PATCH v5 0/3] drm/bridge: ite-it66121: Switch to HDMI helpers and fix DVI output

[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. It also implements the
needed callbacks to send HDMI Vendor Specific and Audio Infoframes.
    
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 v5 of the series, which addresses issues pointed out by
Maxime Ripard.
    
The patches were tested on both DVI and an HDMI monitors.

To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Phong LE <ple@baylibre.com>
To: Sen Wang <sen@ti.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

Changes in v5:
- Implement Audio InfoFrame packet support.
- Implement HDMI VSIF via Null Packet registers (Maxime Ripard).
- Use tmds_rate instead of mode->clock (Maxime Ripard).
- Drop ctx->connector, pass connector as argument (Maxime Ripard).
- Add WARN_ON for NULL connector and state checks (Maxime Ripard).
- Use ternary operator for TX mode in regmap_write (Maxime Ripard).
    
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 .atomic_enable (Maxime Ripard).
    
Changes in v2:
- Don't store the sink type in a bridge state (Maxime Ripard).

---
Javier Martinez Canillas (3):
      drm/bridge: ite-it66121: Switch to the HDMI connector helpers
      drm/bridge: ite-it66121: Move .mode_set logic 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 | 284 +++++++++++++++++++++++++----------
 2 files changed, 207 insertions(+), 79 deletions(-)
---
base-commit: 213c92ac9717e4951f052a499f91c89302889813
change-id: 20260523-it66121-fix-dvi-mode-v5-ed42429761ab

Best regards,
-- 
Javier Martinez Canillas <javierm@redhat.com>

[PATCH v5 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.

Also, add callbacks to implement HDMI Vendor Specific Infoframe and Audio
InfoFrame support. The driver was not sending these before, but they are
required when using the HDMI helpers.

These were implemented following the IT66121 Programming Guide.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/bridge/Kconfig       |   2 +
 drivers/gpu/drm/bridge/ite-it66121.c | 211 ++++++++++++++++++++++++++---------
 2 files changed, 162 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 19e188fe6e3b..f41f51f300c9 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>
@@ -161,6 +163,14 @@
 #define IT66121_PKT_GEN_CTRL_ON			BIT(0)
 #define IT66121_PKT_GEN_CTRL_RPT		BIT(1)
 
+#define IT66121_PKT_NULL_CTRL_REG		0xC9
+#define IT66121_PKT_NULL_CTRL_ON		BIT(0)
+#define IT66121_PKT_NULL_CTRL_RPT		BIT(1)
+
+/* Null packet data registers (used for HDMI Vendor Specific InfoFrame) */
+#define IT66121_PKT_NULL_HB(n)			(0x138 + (n))
+#define IT66121_PKT_NULL_PB(n)			(0x13B + (n))
+
 #define IT66121_AVIINFO_DB1_REG			0x158
 #define IT66121_AVIINFO_DB2_REG			0x159
 #define IT66121_AVIINFO_DB3_REG			0x15A
@@ -180,6 +190,13 @@
 #define IT66121_AVI_INFO_PKT_ON			BIT(0)
 #define IT66121_AVI_INFO_PKT_RPT		BIT(1)
 
+#define IT66121_AUD_INFO_PKT_REG		0xCE
+#define IT66121_AUD_INFO_PKT_ON			BIT(0)
+#define IT66121_AUD_INFO_PKT_RPT		BIT(1)
+
+#define IT66121_AUD_INFO_DB1_REG		0x168
+#define IT66121_AUD_INFO_CSUM_REG		0x16D
+
 #define IT66121_HDMI_MODE_REG			0xC0
 #define IT66121_HDMI_MODE_HDMI			BIT(0)
 
@@ -304,7 +321,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 {
 		u8 ch_enable;
 		u8 fs;
@@ -726,6 +742,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 (WARN_ON(!ctx->connector))
+		return;
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
 
 	it66121_set_mute(ctx, false);
 }
@@ -763,40 +783,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;
@@ -824,24 +814,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)
 {
@@ -872,6 +844,128 @@ 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 long max_rate;
+
+	max_rate = (ctx->bus_width == 12) ? 74250000ULL : 148500000ULL;
+
+	if (tmds_rate > max_rate)
+		return MODE_CLOCK_HIGH;
+
+	if (tmds_rate < HDMI_TMDS_CHAR_RATE_MIN_HZ)
+		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);
+
+	/* Clear both IT66121_AVI_INFO_PKT_ON and IT66121_AVI_INFO_PKT_RPT */
+	return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 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_clear_hdmi_infoframe(struct drm_bridge *bridge)
+{
+	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+
+	/* Clear both IT66121_PKT_NULL_CTRL_ON and IT66121_PKT_NULL_CTRL_RPT */
+	return regmap_write(ctx->regmap, IT66121_PKT_NULL_CTRL_REG, 0);
+}
+
+static int it66121_bridge_hdmi_write_hdmi_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 HDMI Vendor Specific Infoframe packet */
+	ret = regmap_bulk_write(ctx->regmap, IT66121_PKT_NULL_HB(0), buffer, len);
+	if (ret)
+		goto unlock;
+
+	/* Enable HDMI Vendor Specific Infoframe */
+	ret = regmap_write(ctx->regmap, IT66121_PKT_NULL_CTRL_REG,
+			   IT66121_PKT_NULL_CTRL_ON | IT66121_PKT_NULL_CTRL_RPT);
+
+unlock:
+	mutex_unlock(&ctx->lock);
+	return ret;
+}
+
+static int it66121_bridge_hdmi_clear_audio_infoframe(struct drm_bridge *bridge)
+{
+	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+
+	/* Clear both IT66121_AUD_INFO_PKT_ON and IT66121_AUD_INFO_PKT_RPT */
+	return regmap_write(ctx->regmap, IT66121_AUD_INFO_PKT_REG, 0);
+}
+
+static int it66121_bridge_hdmi_write_audio_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 Audio infoframe packet */
+	ret = regmap_bulk_write(ctx->regmap, IT66121_AUD_INFO_DB1_REG,
+				&buffer[HDMI_INFOFRAME_HEADER_SIZE],
+				min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5));
+	if (ret)
+		goto unlock;
+
+	ret = regmap_write(ctx->regmap, IT66121_AUD_INFO_CSUM_REG, buffer[3]);
+	if (ret)
+		goto unlock;
+
+	/* Enable Audio infoframe */
+	ret = regmap_write(ctx->regmap, IT66121_AUD_INFO_PKT_REG,
+			   IT66121_AUD_INFO_PKT_ON | IT66121_AUD_INFO_PKT_RPT);
+
+unlock:
+	mutex_unlock(&ctx->lock);
+	return ret;
+}
+
 static const struct drm_edid *it66121_bridge_edid_read(struct drm_bridge *bridge,
 						       struct drm_connector *connector)
 {
@@ -1359,6 +1453,10 @@ static int it66121_hdmi_audio_prepare(struct drm_bridge *bridge,
 out:
 	mutex_unlock(&ctx->lock);
 
+	if (!ret)
+		ret = drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
+									      &params->cea);
+
 	return ret;
 }
 
@@ -1384,6 +1482,8 @@ static void it66121_hdmi_audio_shutdown(struct drm_bridge *bridge,
 	int ret;
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
 
+	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
+
 	mutex_lock(&ctx->lock);
 	ret = it661221_audio_output_enable(ctx, false);
 	if (ret)
@@ -1433,11 +1533,17 @@ 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,
+	.hdmi_clear_audio_infoframe = it66121_bridge_hdmi_clear_audio_infoframe,
+	.hdmi_write_audio_infoframe = it66121_bridge_hdmi_write_audio_infoframe,
 	.hdmi_audio_startup = it66121_hdmi_audio_startup,
 	.hdmi_audio_prepare = it66121_hdmi_audio_prepare,
 	.hdmi_audio_shutdown = it66121_hdmi_audio_shutdown,
@@ -1538,7 +1644,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 v5 2/3] drm/bridge: ite-it66121: Move .mode_set logic 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.

Also, drop the struct it66121_ctx.connector field because the connector
can be accessed through the atomic state and there is no need to store
it anymore.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 100 ++++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index f41f51f300c9..9725e5db5db3 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -315,7 +315,6 @@ struct it66121_chip_info {
 struct it66121_ctx {
 	struct regmap *regmap;
 	struct drm_bridge bridge;
-	struct drm_connector *connector;
 	struct device *dev;
 	struct gpio_desc *gpio_reset;
 	struct i2c_client *client;
@@ -668,6 +667,58 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 	return 0;
 }
 
+static void it66121_set_mode(struct it66121_ctx *ctx,
+			     struct drm_connector *connector,
+			     struct drm_atomic_commit *state)
+{
+	const struct drm_connector_state *conn_state;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_display_mode *mode;
+	struct drm_crtc *crtc;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc = conn_state->crtc;
+	if (WARN_ON(!crtc))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
+	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;
@@ -740,13 +791,15 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 				  struct drm_atomic_commit *state)
 {
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct drm_connector *connector;
 
-	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
-	if (WARN_ON(!ctx->connector))
+	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	if (WARN_ON(!connector))
 		return;
 
-	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
 
+	it66121_set_mode(ctx, connector, state);
 	it66121_set_mute(ctx, false);
 }
 
@@ -756,8 +809,6 @@ static void it66121_bridge_disable(struct drm_bridge *bridge,
 	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
 
 	it66121_set_mute(ctx, true);
-
-	ctx->connector = NULL;
 }
 
 static int it66121_bridge_check(struct drm_bridge *bridge,
@@ -778,42 +829,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)
 {
@@ -1532,7 +1547,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 v5 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 the
connector's display information sink type to identify the correct mode
to configure the bridge.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 9725e5db5db3..a469e8d9d2d8 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -199,6 +199,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)
@@ -692,8 +693,10 @@ static void it66121_set_mode(struct it66121_ctx *ctx,
 
 	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,
+			 connector->display_info.is_hdmi ?
+			 IT66121_HDMI_MODE_HDMI : IT66121_HDMI_MODE_DVI))
 		goto unlock;
 
 	if ((ctx->id == ID_IT66121 || ctx->id == ID_IT66122) &&

-- 
2.54.0

Claude review: drm/bridge: ite-it66121: Switch to HDMI helpers and fix DVI output

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ite-it66121: Switch to HDMI helpers and fix DVI output
Author: Javier Martinez Canillas <javierm@redhat.com>
Patches: 4
Reviewed: 2026-05-25T17:50:13.282912

---

This is a well-structured 3-patch series that converts the ite-it66121 bridge driver to use the DRM HDMI connector helpers and fixes DVI output on BeaglePlay. The series is logically ordered: (1) switch to HDMI helpers, (2) move mode_set to atomic_enable, (3) fix DVI mode selection.

**One build-breaking issue:** Patch 1 uses `HDMI_TMDS_CHAR_RATE_MIN_HZ` which does not exist anywhere in the kernel tree. This will fail to compile.

Otherwise, the series is clean and follows the patterns established by other HDMI bridge drivers (e.g., ite-it6263). The HDMI helper conversion, VSIF implementation via null packet registers, and audio infoframe support all look correct. The DVI fix in patch 3 is straightforward and correct.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Build error — undefined symbol:**

The `it66121_bridge_hdmi_tmds_char_rate_valid()` function uses `HDMI_TMDS_CHAR_RATE_MIN_HZ`:
```c
if (tmds_rate < HDMI_TMDS_CHAR_RATE_MIN_HZ)
    return MODE_CLOCK_LOW;
```
This macro is not defined anywhere in the kernel tree. The original code used `mode->clock < 25000` (25 MHz in kHz). This needs to be either defined locally (e.g., `#define HDMI_TMDS_CHAR_RATE_MIN_HZ 25000000ULL`) or replaced with a literal `25000000ULL`. The sister driver `ite-it6263.c` defines its own `MAX_HDMI_TMDS_CHAR_RATE_HZ` locally, suggesting local definition is the expected pattern.

**VSIF write — no length bounds check:**

The VSIF write function writes the full buffer directly to registers starting at `IT66121_PKT_NULL_HB(0)` (0x138):
```c
ret = regmap_bulk_write(ctx->regmap, IT66121_PKT_NULL_HB(0), buffer, len);
```
There is no validation that `len` fits within the available null packet register space. The AVI infoframe write is careful to write exactly `HDMI_AVI_INFOFRAME_SIZE` bytes, and the audio infoframe uses `min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5)` to clamp. The VSIF write should have similar protection, or at least a comment documenting the maximum supported size. In practice the framework sends well-formed packets, so this is a minor robustness concern rather than a functional bug.

**Locking asymmetry between clear and write callbacks:**

The `write_*` callbacks all acquire `ctx->lock`, but the `clear_*` callbacks do not:
```c
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);
}
```
Since these are all called from the serialized atomic commit path, this is safe in practice. However, the inconsistency is worth noting — if `ctx->lock` protects register access, the clears should arguably also take it for defensive consistency.

**Rate conversion correctness:**

The max rate conversion from kHz to Hz is correct:
- Old: `max_clock = (ctx->bus_width == 12) ? 74250 : 148500` (kHz, compared against `mode->clock`)
- New: `max_rate = (ctx->bus_width == 12) ? 74250000ULL : 148500000ULL` (Hz, compared against `tmds_rate`)

**Audio infoframe data clamped to 5 bytes:**
```c
min_t(size_t, len - HDMI_INFOFRAME_HEADER_SIZE, 5)
```
This is correct for the IT66121 register layout where `IT66121_AUD_INFO_DB1_REG` (0x168) through 0x16C provides 5 data registers, with the checksum at 0x16D.

**Bridge configuration in probe looks correct:**
```c
ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
                  DRM_BRIDGE_OP_HDMI;
ctx->bridge.vendor = "ITE";
ctx->bridge.product = "IT66121";
```
Adding `DRM_BRIDGE_OP_HDMI` and the vendor/product strings are required for the HDMI helper framework.

**Calling `drm_atomic_helper_connector_hdmi_update_audio_infoframe` outside the lock in audio_prepare:**
```c
out:
    mutex_unlock(&ctx->lock);

    if (!ret)
        ret = drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
                                                                      &params->cea);
    return ret;
```
This is correct — the helper will call back into `hdmi_write_audio_infoframe` which acquires the lock itself, so calling it outside the outer lock avoids deadlock.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Clean refactor.** The mode_set logic is correctly extracted into `it66121_set_mode()` and called from `atomic_enable`. The `ctx->connector` field is removed in favor of a local variable, which is the right approach.

**Mode lookup through atomic state is correct:**
```c
conn_state = drm_atomic_get_new_connector_state(state, connector);
crtc = conn_state->crtc;
crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
mode = &crtc_state->adjusted_mode;
```
Each step correctly chains through the state to get the adjusted mode. The `WARN_ON` checks for NULL at each stage are defensive and appropriate.

**Enable ordering:**
```c
drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
it66121_set_mode(ctx, connector, state);
it66121_set_mute(ctx, false);
```
Infoframes are written before the TX mode is configured. This works because the infoframe data is just stored in hardware registers — actual transmission starts when the TX mode is enabled. However, the more natural order would be to configure the TX mode first, then enable infoframes. If there's a reason the IT66121 requires this order, a comment would help.

**The `connector = NULL` cleanup in `bridge_disable` is correctly removed**, since `connector` is now a local variable.

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

**This is the actual bug fix and it's correct.**

```c
if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG,
                 connector->display_info.is_hdmi ?
                 IT66121_HDMI_MODE_HDMI : IT66121_HDMI_MODE_DVI))
```

Using `connector->display_info.is_hdmi` to select between HDMI mode (BIT(0) = 1) and DVI mode (0) is the standard kernel pattern. The `IT66121_HDMI_MODE_DVI` define as `0` is slightly unconventional (defining a zero constant) but makes the intent clear and is fine.

**The connector is available at this point** because `it66121_set_mode()` (introduced in patch 2) receives it as a parameter from `atomic_enable`, where it's obtained from `drm_atomic_get_new_connector_for_encoder()`. The `display_info.is_hdmi` field is populated during EDID parsing and is reliable at enable time.

**No other mode-dependent logic needs updating.** The HDMI helpers handle infoframe suppression for DVI sinks internally, so no additional changes are needed in the infoframe callbacks.

---

**Summary:** Fix the `HDMI_TMDS_CHAR_RATE_MIN_HZ` build error in patch 1 (define it locally as `25000000ULL` or use the literal). Everything else looks good — the HDMI helper conversion is thorough and correct, and the DVI fix is clean and minimal.

---
Generated by Claude Code Patch Reviewer