[PATCH v2 0/7] drm/bridge: tc358768: Long command support

[PATCH v2 0/7] drm/bridge: tc358768: Long command support

[Tomi Valkeinen]

This series makes some small improvements to the tc358768 driver, and
then adds support for long commands, commands that have more than 8
bytes of payload.

This has been tested on Toradex Verdin AM62 board, with a ST7703 based
DSI panel which requires initial configuration using commands that have
8+ bytes.

I tested the following combinations, by observing the panel visually and
the DSI lanes with an oscilloscope:

- HS command transfer, continuous clock
- HS command transfer, non-continuous clock
- LP command transfer, continuous clock
- LP command transfer, non-continuous clock

All except LP + continuous clock work fine. While observing the lanes
with normal oscilloscope is a very high level and vague view of what
exactly is going on the lanes, it still looks good to me: initial
commands are sent in LP, and then video data is being sent in HS, and
clock lane is continuous. However, the panel stays black, so something
is not quite right.

As it doesn't make sense to send commands in LP during initial
configuration, especially with longer commands, I will leave that
particular combination unresolved. I'd be interested to hear if it works
on some other panel.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v2:
- Add reviewed-by & tested-by tags
- Rebase on v7.0-rc2
- Link to v1: https://lore.kernel.org/r/20251021-tc358768-v1-0-d590dc6a1a0c@ideasonboard.com

---
Parth Pancholi (1):
      drm/bridge: tc358768: Set pre_enable_prev_first for reverse order

Tomi Valkeinen (6):
      drm/bridge: tc358768: Fix typo in TC358768_DSI_CONTROL_DIS_MODE
      drm/bridge: tc358768: Separate indirect register writes
      drm/bridge: tc358768: Support non-continuous clock
      drm/bridge: tc358768: Add LP mode command support
      drm/bridge: tc358768: Separate video format config
      drm/bridge: tc358768: Add support for long command tx via video buffer

 drivers/gpu/drm/bridge/tc358768.c | 202 +++++++++++++++++++++++++++++---------
 1 file changed, 157 insertions(+), 45 deletions(-)
---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20251021-tc358768-93eb23689b7a

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v2 1/7] drm/bridge: tc358768: Fix typo in TC358768_DSI_CONTROL_DIS_MODE

[Tomi Valkeinen]

It's "DSI_MODE", not "DIS_MODE".

Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index fbdc44e16229..c95d164bd3a0 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -115,7 +115,7 @@
 #define TC358768_DSI_HACT		0x062C
 
 /* TC358768_DSI_CONTROL (0x040C) register */
-#define TC358768_DSI_CONTROL_DIS_MODE	BIT(15)
+#define TC358768_DSI_CONTROL_DSI_MODE	BIT(15)
 #define TC358768_DSI_CONTROL_TXMD	BIT(7)
 #define TC358768_DSI_CONTROL_HSCKMD	BIT(5)
 #define TC358768_DSI_CONTROL_EOTDIS	BIT(0)
@@ -1082,7 +1082,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	tc358768_write(priv, TC358768_DSI_CONFW, val);
 
 	val = TC358768_DSI_CONFW_MODE_CLR | TC358768_DSI_CONFW_ADDR_DSI_CONTROL;
-	val |= TC358768_DSI_CONTROL_DIS_MODE; /* DSI mode */
+	val |= TC358768_DSI_CONTROL_DSI_MODE;
 	tc358768_write(priv, TC358768_DSI_CONFW, val);
 
 	ret = tc358768_clear_error(priv);

-- 
2.43.0

Claude review: drm/bridge: tc358768: Fix typo in TC358768_DSI_CONTROL_DIS_MODE

[Claude Code Review Bot]

Patch Review

Simple, correct rename from `TC358768_DSI_CONTROL_DIS_MODE` to `TC358768_DSI_CONTROL_DSI_MODE`. The comment `/* DSI mode */` that was next to the usage is also cleaned up since the macro name is now self-documenting. No issues.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/7] drm/bridge: tc358768: Set pre_enable_prev_first for reverse order

[Tomi Valkeinen]

From: Parth Pancholi <parth.pancholi@toradex.com>

Enable the pre_enable_prev_first flag on the tc358768 bridge to reverse
the pre-enable order, calling bridge pre_enable before panel prepare.
This ensures the bridge is ready before sending panel init commands in
the case of panels sending init commands in panel prepare function.

Signed-off-by: Parth Pancholi <parth.pancholi@toradex.com>
Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index c95d164bd3a0..dab9cdf5cb98 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -448,6 +448,8 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
 						    DRM_MODE_CONNECTOR_DSI);
 		if (IS_ERR(bridge))
 			return PTR_ERR(bridge);
+
+		bridge->pre_enable_prev_first = true;
 	}
 
 	priv->output.dev = dev;

-- 
2.43.0

Claude review: drm/bridge: tc358768: Set pre_enable_prev_first for reverse order

[Claude Code Review Bot]

Patch Review

Sets `bridge->pre_enable_prev_first = true` to ensure the bridge's `pre_enable` is called before the panel's `prepare`, which is necessary for panels that send init commands during prepare. This is a one-line functional change in the right place (`tc358768_dsi_host_attach`). Clean and correct.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 3/7] drm/bridge: tc358768: Separate indirect register writes

[Tomi Valkeinen]

Some registers can only be written indirectly, using DSI_CONFW register.
We don't have many uses for those registers (in fact, only DSI_CONTROL
is currently written), but the code to do those writes inline is a bit
confusing.

Add a new function, tc358768_confw_update_bits() which can be used to
write the bits indirectly. Only DSI_CONTROL is currently supported.

Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 52 +++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index dab9cdf5cb98..755ed6483b2e 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -123,7 +123,7 @@
 /* TC358768_DSI_CONFW (0x0500) register */
 #define TC358768_DSI_CONFW_MODE_SET	(5 << 29)
 #define TC358768_DSI_CONFW_MODE_CLR	(6 << 29)
-#define TC358768_DSI_CONFW_ADDR_DSI_CONTROL	(0x3 << 24)
+#define TC358768_DSI_CONFW_ADDR(x)	((x) << 24)
 
 /* TC358768_DSICMD_TX (0x0600) register */
 #define TC358768_DSI_CMDTX_DC_START	BIT(0)
@@ -232,6 +232,36 @@ static void tc358768_update_bits(struct tc358768_priv *priv, u32 reg, u32 mask,
 		tc358768_write(priv, reg, tmp);
 }
 
+static void tc358768_confw_update_bits(struct tc358768_priv *priv, u16 reg,
+				       u16 mask, u16 val)
+{
+	u8 confw_addr;
+	u32 confw_val;
+
+	switch (reg) {
+	case TC358768_DSI_CONTROL:
+		confw_addr = 0x3;
+		break;
+	default:
+		priv->error = -EINVAL;
+		return;
+	}
+
+	if (mask != val) {
+		confw_val = TC358768_DSI_CONFW_MODE_CLR |
+			TC358768_DSI_CONFW_ADDR(confw_addr) |
+			mask;
+		tc358768_write(priv, TC358768_DSI_CONFW, confw_val);
+	}
+
+	if (val & mask) {
+		confw_val = TC358768_DSI_CONFW_MODE_SET |
+			TC358768_DSI_CONFW_ADDR(confw_addr) |
+			(val & mask);
+		tc358768_write(priv, TC358768_DSI_CONFW, confw_val);
+	}
+}
+
 static void tc358768_dsicmd_tx(struct tc358768_priv *priv)
 {
 	u32 val;
@@ -693,7 +723,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 	struct mipi_dsi_device *dsi_dev = priv->output.dev;
 	unsigned long mode_flags = dsi_dev->mode_flags;
-	u32 val, val2, lptxcnt, hact, data_type;
+	u32 val, mask, val2, lptxcnt, hact, data_type;
 	s32 raw_val;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector_state *conn_state;
@@ -1065,13 +1095,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	tc358768_write(priv, TC358768_DSI_START, 0x1);
 
 	/* Configure DSI_Control register */
-	val = TC358768_DSI_CONFW_MODE_CLR | TC358768_DSI_CONFW_ADDR_DSI_CONTROL;
-	val |= TC358768_DSI_CONTROL_TXMD | TC358768_DSI_CONTROL_HSCKMD |
-	       0x3 << 1 | TC358768_DSI_CONTROL_EOTDIS;
-	tc358768_write(priv, TC358768_DSI_CONFW, val);
-
-	val = TC358768_DSI_CONFW_MODE_SET | TC358768_DSI_CONFW_ADDR_DSI_CONTROL;
-	val |= (dsi_dev->lanes - 1) << 1;
+	val = (dsi_dev->lanes - 1) << 1;
 
 	val |= TC358768_DSI_CONTROL_TXMD;
 
@@ -1081,11 +1105,13 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	if (dsi_dev->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
 		val |= TC358768_DSI_CONTROL_EOTDIS;
 
-	tc358768_write(priv, TC358768_DSI_CONFW, val);
+	mask = TC358768_DSI_CONTROL_TXMD | TC358768_DSI_CONTROL_HSCKMD |
+	       0x3 << 1 | TC358768_DSI_CONTROL_EOTDIS;
+
+	tc358768_confw_update_bits(priv, TC358768_DSI_CONTROL, mask, val);
 
-	val = TC358768_DSI_CONFW_MODE_CLR | TC358768_DSI_CONFW_ADDR_DSI_CONTROL;
-	val |= TC358768_DSI_CONTROL_DSI_MODE;
-	tc358768_write(priv, TC358768_DSI_CONFW, val);
+	tc358768_confw_update_bits(priv, TC358768_DSI_CONTROL,
+				   TC358768_DSI_CONTROL_DSI_MODE, 0);
 
 	ret = tc358768_clear_error(priv);
 	if (ret)

-- 
2.43.0

Claude review: drm/bridge: tc358768: Separate indirect register writes

[Claude Code Review Bot]

Patch Review

Introduces `tc358768_confw_update_bits()` to abstract the indirect DSI_CONFW register write pattern. The function properly handles set/clear operations.

One minor observation on operator precedence in the mask calculation at the call site:

```c
mask = TC358768_DSI_CONTROL_TXMD | TC358768_DSI_CONTROL_HSCKMD |
       0x3 << 1 | TC358768_DSI_CONTROL_EOTDIS;
```

The `<<` operator has higher precedence than `|`, so `0x3 << 1` evaluates to `0x6` before the OR, which is the intended behavior (lanes mask bits [2:1]). This expression was carried over from the original code, so it's correct but could benefit from parentheses for clarity: `(0x3 << 1)`. Very minor style nit.

The `tc358768_confw_update_bits` logic itself is sound: if `mask != val` it clears the full mask first, then if `val & mask` is non-zero it sets the desired bits. This is an efficient approach that avoids unnecessary CLR when setting all bits, and avoids unnecessary SET when clearing all bits.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 4/7] drm/bridge: tc358768: Support non-continuous clock

[Tomi Valkeinen]

The driver prints a warning if MIPI_DSI_CLOCK_NON_CONTINUOUS is set, and
falls back to continuous clock mode. This was added in commit
fbc5a90e82c1 ("drm/bridge: tc358768: Disable non-continuous clock mode").

However, there have been multiple changes to the driver since then, and
at least in my setup, non-continuous clock mode works: I can see an
image on the panel, and I can see the clock lanes being non-continuous
with an oscilloscope.

So, let's enable MIPI_DSI_CLOCK_NON_CONTINUOUS support.

Cc: Dmitry Osipenko <digetx@gmail.com>
Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 755ed6483b2e..a276fbc75dde 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -722,7 +722,6 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 	struct mipi_dsi_device *dsi_dev = priv->output.dev;
-	unsigned long mode_flags = dsi_dev->mode_flags;
 	u32 val, mask, val2, lptxcnt, hact, data_type;
 	s32 raw_val;
 	struct drm_crtc_state *crtc_state;
@@ -744,11 +743,6 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	u32 dsi_vsdly;
 	const u32 internal_dly = 40;
 
-	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
-		dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
-		mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
-	}
-
 	tc358768_hw_enable(priv);
 
 	ret = tc358768_sw_reset(priv);
@@ -1032,7 +1026,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	tc358768_write(priv, TC358768_HSTXVREGEN, val);
 
 	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
-		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
+		       (dsi_dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
 
 	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
 	val = tc358768_ps_to_ns((lptxcnt + 1) * hsbyteclk_ps * 4);
@@ -1099,7 +1093,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 
 	val |= TC358768_DSI_CONTROL_TXMD;
 
-	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
+	if (!(dsi_dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
 		val |= TC358768_DSI_CONTROL_HSCKMD;
 
 	if (dsi_dev->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)

-- 
2.43.0

Claude review: drm/bridge: tc358768: Support non-continuous clock

[Claude Code Review Bot]

Patch Review

Removes the `mode_flags` local variable and the `dev_warn_once` fallback that was disabling non-continuous clock. Now uses `dsi_dev->mode_flags` directly. The commit message explains the rationale well -- the original workaround was from an earlier state of the driver, and non-continuous mode now works. Clean change.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 5/7] drm/bridge: tc358768: Add LP mode command support

[Tomi Valkeinen]

Currently the driver ignores MIPI_DSI_MODE_LPM and always uses HS mode.
Add code to enable HS mode in pre_enable() only if MIPI_DSI_MODE_LPM is
not set, and always enable HS mode in enable() for video transmission.

Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index a276fbc75dde..a7a14c125ac4 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -1091,7 +1091,8 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	/* Configure DSI_Control register */
 	val = (dsi_dev->lanes - 1) << 1;
 
-	val |= TC358768_DSI_CONTROL_TXMD;
+	if (!(dsi_dev->mode_flags & MIPI_DSI_MODE_LPM))
+		val |= TC358768_DSI_CONTROL_TXMD;
 
 	if (!(dsi_dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
 		val |= TC358768_DSI_CONTROL_HSCKMD;
@@ -1123,6 +1124,11 @@ static void tc358768_bridge_atomic_enable(struct drm_bridge *bridge,
 		return;
 	}
 
+	/* Enable HS mode for video TX */
+	tc358768_confw_update_bits(priv, TC358768_DSI_CONTROL,
+				   TC358768_DSI_CONTROL_TXMD,
+				   TC358768_DSI_CONTROL_TXMD);
+
 	/* clear FrmStop and RstPtr */
 	tc358768_update_bits(priv, TC358768_PP_MISC, 0x3 << 14, 0);
 

-- 
2.43.0

Claude review: drm/bridge: tc358768: Add LP mode command support

[Claude Code Review Bot]

Patch Review

Adds LP mode support by conditionally setting `TC358768_DSI_CONTROL_TXMD` in `pre_enable()` only when `MIPI_DSI_MODE_LPM` is *not* set, then unconditionally enabling HS mode in `enable()` for video transmission. The logic is correct -- LP mode is used for command phase, HS mode is used for video phase.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 6/7] drm/bridge: tc358768: Separate video format config

[Tomi Valkeinen]

Sending long commands using the video buffer (to be implemented in
following patches) requires setting TC358768_DATAFMT and
TC358768_DSITX_DT registers for command transfer. The same registers
also need to be configured properly for video transfer.

The long commands will be sent between the bridge's pre_enable() and
enable(), and currently we configure the registers for video transfer in
pre_enable(). Thus, they would be overwritten by the long command
transfer code.

To prevent that from happening, set those registers for video transfer
in enable(), not in pre_enable().

Based on code from Parth Pancholi <parth.pancholi@toradex.com>

Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 51 ++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index a7a14c125ac4..e1ed4003b3c5 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -722,7 +722,7 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 	struct mipi_dsi_device *dsi_dev = priv->output.dev;
-	u32 val, mask, val2, lptxcnt, hact, data_type;
+	u32 val, mask, val2, lptxcnt, hact;
 	s32 raw_val;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector_state *conn_state;
@@ -768,30 +768,20 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	dsiclk = priv->dsiclk;
 	hsbyteclk = dsiclk / 4;
 
-	/* Data Format Control Register */
-	val = BIT(2) | BIT(1) | BIT(0); /* rdswap_en | dsitx_en | txdt_en */
 	switch (dsi_dev->format) {
 	case MIPI_DSI_FMT_RGB888:
-		val |= (0x3 << 4);
 		hact = vm.hactive * 3;
-		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
 		break;
 	case MIPI_DSI_FMT_RGB666:
-		val |= (0x4 << 4);
 		hact = vm.hactive * 3;
-		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB666_PACKED:
-		val |= (0x4 << 4) | BIT(3);
 		hact = vm.hactive * 18 / 8;
-		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB565:
-		val |= (0x5 << 4);
 		hact = vm.hactive * 2;
-		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
 		break;
 	default:
 		dev_err(dev, "Invalid data format (%u)\n",
@@ -947,9 +937,6 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	/* VSDly[9:0] */
 	tc358768_write(priv, TC358768_VSDLY, dsi_vsdly - internal_dly);
 
-	tc358768_write(priv, TC358768_DATAFMT, val);
-	tc358768_write(priv, TC358768_DSITX_DT, data_type);
-
 	/* Enable D-PHY (HiZ->LP11) */
 	tc358768_write(priv, TC358768_CLW_CNTRL, 0x0000);
 	/* Enable lanes */
@@ -1113,6 +1100,39 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 		dev_err(dev, "Bridge pre_enable failed: %d\n", ret);
 }
 
+static void tc358768_config_video_format(struct tc358768_priv *priv)
+{
+	struct mipi_dsi_device *dsi_dev = priv->output.dev;
+	u32 val, data_type;
+
+	/* Data Format Control Register */
+	val = BIT(2) | BIT(1) | BIT(0); /* rdswap_en | dsitx_en | txdt_en */
+	switch (dsi_dev->format) {
+	case MIPI_DSI_FMT_RGB888:
+		val |= (0x3 << 4);
+		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		val |= (0x4 << 4);
+		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
+		break;
+	case MIPI_DSI_FMT_RGB666_PACKED:
+		val |= (0x4 << 4) | BIT(3);
+		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
+		break;
+	case MIPI_DSI_FMT_RGB565:
+		val |= (0x5 << 4);
+		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
+		break;
+	default:
+		dev_err(priv->dev, "Invalid data format (%u)\n", dsi_dev->format);
+		return;
+	}
+
+	tc358768_write(priv, TC358768_DATAFMT, val);
+	tc358768_write(priv, TC358768_DSITX_DT, data_type);
+}
+
 static void tc358768_bridge_atomic_enable(struct drm_bridge *bridge,
 					  struct drm_atomic_state *state)
 {
@@ -1124,6 +1144,9 @@ static void tc358768_bridge_atomic_enable(struct drm_bridge *bridge,
 		return;
 	}
 
+	/* Configure video format registers */
+	tc358768_config_video_format(priv);
+
 	/* Enable HS mode for video TX */
 	tc358768_confw_update_bits(priv, TC358768_DSI_CONTROL,
 				   TC358768_DSI_CONTROL_TXMD,

-- 
2.43.0

Claude review: drm/bridge: tc358768: Separate video format config

[Claude Code Review Bot]

Patch Review

Moves `TC358768_DATAFMT` and `TC358768_DSITX_DT` register writes from `pre_enable()` to a new `tc358768_config_video_format()` function called from `enable()`. This is a prerequisite for patch 7, which needs to write these same registers with different values for long command transmission during the pre_enable/enable window.

The `default` case in the new function calls `return` without setting `priv->error`, unlike the original code path in `pre_enable()` which had a goto to handle the error. However, the function does call `dev_err()`, and the missing format would already have been caught during the `hact` computation in `pre_enable()` (which retains the `default` case with the `goto err` path). So the video format function's default case is effectively unreachable. Acceptable.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 7/7] drm/bridge: tc358768: Add support for long command tx via video buffer

[Tomi Valkeinen]

TC358768 has two ways to send DSI commands: 1) buffer the payload data
into registers (DSICMD_WDx), which supports up to 8 bytes of payload, 2)
buffer the payload data into the video buffer, which supports up to 1024
bytes of payload.

The driver currently supports method 1).

Add support for transmitting long DSI commands (more than 8 bytes, up to
1024 bytes) using the video buffer. This mode can only be used before
the actual video transmission is enabled, i.e. the initial configuration.

Original version from Parth Pancholi <parth.pancholi@toradex.com>

Tested-by: João Paulo Gonçalves <joao.goncalves@toradex.com> # Toradex Verdin AM62
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 79 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index e1ed4003b3c5..e0b5a4b5abbe 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -45,6 +45,9 @@
 
 /* Debug (16-bit addressable) */
 #define TC358768_VBUFCTRL		0x00E0
+#define TC358768_VBUFCTRL_VBUF_EN	BIT(15)
+#define TC358768_VBUFCTRL_TX_EN		BIT(14)
+#define TC358768_VBUFCTRL_MASK		BIT(13)
 #define TC358768_DBG_WIDTH		0x00E2
 #define TC358768_DBG_VBLANK		0x00E4
 #define TC358768_DBG_DATA		0x00E8
@@ -537,9 +540,21 @@ static ssize_t tc358768_dsi_host_transfer(struct mipi_dsi_host *host,
 		return -ENOTSUPP;
 	}
 
+	if (msg->tx_len > 1024) {
+		dev_warn(priv->dev, "Maximum 1024 byte MIPI tx is supported\n");
+		return -EINVAL;
+	}
+
 	if (msg->tx_len > 8) {
-		dev_warn(priv->dev, "Maximum 8 byte MIPI tx is supported\n");
-		return -ENOTSUPP;
+		u32 confctl;
+
+		tc358768_read(priv, TC358768_CONFCTL, &confctl);
+
+		if (confctl & BIT(6)) {
+			dev_warn(priv->dev,
+				 "Video is currently active. Unable to transmit long command\n");
+			return -EBUSY;
+		}
 	}
 
 	ret = mipi_dsi_create_packet(&packet, msg);
@@ -552,23 +567,66 @@ static ssize_t tc358768_dsi_host_transfer(struct mipi_dsi_host *host,
 		tc358768_write(priv, TC358768_DSICMD_WC, 0);
 		tc358768_write(priv, TC358768_DSICMD_WD0,
 			       (packet.header[2] << 8) | packet.header[1]);
-	} else {
-		int i;
-
+		tc358768_dsicmd_tx(priv);
+	} else if (packet.payload_length <= 8) {
 		tc358768_write(priv, TC358768_DSICMD_TYPE,
 			       (0x40 << 8) | (packet.header[0] & 0x3f));
 		tc358768_write(priv, TC358768_DSICMD_WC, packet.payload_length);
-		for (i = 0; i < packet.payload_length; i += 2) {
+
+		for (int i = 0; i < packet.payload_length; i += 2) {
 			u16 val = packet.payload[i];
 
 			if (i + 1 < packet.payload_length)
 				val |= packet.payload[i + 1] << 8;
-
 			tc358768_write(priv, TC358768_DSICMD_WD0 + i, val);
 		}
-	}
 
-	tc358768_dsicmd_tx(priv);
+		tc358768_dsicmd_tx(priv);
+	} else {
+		unsigned long tx_sleep_us;
+		size_t len;
+
+		/* For packets over 8 bytes we need to use the video buffer */
+		tc358768_write(priv, TC358768_DATAFMT, BIT(0));	/* txdt_en */
+		tc358768_write(priv, TC358768_DSITX_DT, packet.header[0] & 0x3f);
+		tc358768_write(priv, TC358768_CMDBYTE, packet.payload_length);
+		tc358768_write(priv, TC358768_VBUFCTRL, TC358768_VBUFCTRL_VBUF_EN);
+
+		/*
+		 * Write the payload in 2-byte chunks, and pad with zeroes to
+		 * align to 4 bytes.
+		 */
+		len = ALIGN(packet.payload_length, 4);
+
+		for (int i = 0; i < len; i += 2) {
+			u16 val = 0;
+
+			if (i < packet.payload_length)
+				val |= packet.payload[i];
+			if (i + 1 < packet.payload_length)
+				val |= packet.payload[i + 1] << 8;
+
+			tc358768_write(priv, TC358768_DBG_DATA, val);
+		}
+
+		/* Start transmission */
+		tc358768_write(priv, TC358768_VBUFCTRL,
+			       TC358768_VBUFCTRL_VBUF_EN |
+			       TC358768_VBUFCTRL_TX_EN |
+			       TC358768_VBUFCTRL_MASK);
+
+		/*
+		 * The TC358768 spec says to wait until the transmission has
+		 * been finished, estimating the sleep time based on the payload
+		 * and clock rates. We use a simple safe estimate of 2us per
+		 * byte (LP mode transmission).
+		 */
+		tx_sleep_us = packet.payload_length * 2;
+		usleep_range(tx_sleep_us, tx_sleep_us * 2);
+
+		tc358768_write(priv, TC358768_VBUFCTRL, TC358768_VBUFCTRL_MASK);
+		tc358768_write(priv, TC358768_VBUFCTRL, 0); /* Stop transmission */
+	}
 
 	ret = tc358768_clear_error(priv);
 	if (ret)
@@ -752,6 +810,9 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 		return;
 	}
 
+	/* Release RstPtr so that the video buffer can be used for DSI commands */
+	tc358768_update_bits(priv, TC358768_PP_MISC, BIT(14), 0);
+
 	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 	conn_state = drm_atomic_get_new_connector_state(state, connector);
 	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);

-- 
2.43.0

Claude review: drm/bridge: tc358768: Add support for long command tx via video buffer

[Claude Code Review Bot]

Patch Review

The main feature patch. Key observations:

1. **Guard against video-active state**: The check `if (confctl & BIT(6))` correctly tests `PP_en` to ensure long commands aren't sent while video is active. Good defensive check.

2. **Boundary condition for tx_len**: The check `msg->tx_len > 1024` correctly limits the maximum payload. Note however that `msg->tx_len` includes the header, but `packet.payload_length` is what actually gets written to the video buffer. The 1024-byte limit check against `msg->tx_len` is slightly conservative (by the header size) but this is safe and fine in practice.

3. **Sleep estimation**: The sleep calculation `packet.payload_length * 2` microseconds (2us per byte for LP mode) is described as a "safe estimate." For very small packets just over 8 bytes (e.g., 9 bytes), this gives ~18us which should be adequate. For maximum 1024-byte payloads, this gives ~2ms. The `usleep_range(tx_sleep_us, tx_sleep_us * 2)` pattern is proper kernel style.

4. **RstPtr release**: Line 814 releases RstPtr (BIT(14) of PP_MISC) early in `pre_enable()` so the video buffer can be used for DSI commands. In `enable()`, line 1217 clears both FrmStop and RstPtr (`0x3 << 14`), which is fine since RstPtr is already 0 at that point. The lifecycle is coherent.

5. **Missing poll for completion**: The comment says "The TC358768 spec says to wait until the transmission has been finished" but the implementation uses a time-based estimate rather than polling a status register. The cover letter acknowledges this approach. While polling would be more robust, the simple estimate with generous margins is pragmatic and has been tested working on hardware.

6. **DATAFMT register**: For long commands, `TC358768_DATAFMT` is set to `BIT(0)` (txdt_en only), which differs from the video mode setting that also includes `BIT(1) | BIT(2)` (dsitx_en, rdswap_en) and the format-specific bits. This is correct per the datasheet for command-mode buffer transmission.

Overall, a clean and well-organized series. No blocking issues found.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: tc358768: Long command support

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: tc358768: Long command support
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Patches: 8
Reviewed: 2026-03-12T07:14:49.415107

---

This is a well-structured 7-patch series by Tomi Valkeinen that incrementally improves the TC358768 DSI bridge driver, culminating in long command (>8 byte) support via the video buffer. The series is logically ordered: typo fix, bridge ordering fix, refactoring of indirect register writes, enabling non-continuous clock, LP mode command support, video format config separation, and finally the long command implementation.

The code quality is good, the patches are well-tested (Tested-by and Reviewed-by tags on all patches), and the commit messages are clear. The series has already been through one revision cycle. I have a few minor observations but no blocking issues.

---
Generated by Claude Code Patch Reviewer