public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v1 0/1] gpu/drm: bridge: tc358768: Add delay after PLL setup
@ 2026-05-20  9:26 Svyatoslav Ryhel
  2026-05-20  9:26 ` [PATCH v1 1/1] " Svyatoslav Ryhel
  2026-05-25 12:05 ` Claude Code Review Bot
  0 siblings, 2 replies; 4+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-20  9:26 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Svyatoslav Ryhel
  Cc: dri-devel, linux-kernel

After tc358768_setup_pll() enables PLL_CKEN and the lock indicator comes
up, the DSI register sequence runs near immediately. On TF700T this
results in the bridge claiming PLL lock but producing no DSI video output:
the panel powers up, the backlight comes on, but the framebuffer is never
scanned out.

Insert a small settling delay between PLL setup and the rest of the
attach.

Svyatoslav Ryhel (1):
  gpu/drm: bridge: tc358768: Add delay after PLL setup

 drivers/gpu/drm/bridge/tc358768.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.51.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v1 1/1] gpu/drm: bridge: tc358768: Add delay after PLL setup
  2026-05-20  9:26 [PATCH v1 0/1] gpu/drm: bridge: tc358768: Add delay after PLL setup Svyatoslav Ryhel
@ 2026-05-20  9:26 ` Svyatoslav Ryhel
  2026-05-25 12:05   ` Claude review: " Claude Code Review Bot
  2026-05-25 12:05 ` Claude Code Review Bot
  1 sibling, 1 reply; 4+ messages in thread
From: Svyatoslav Ryhel @ 2026-05-20  9:26 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Svyatoslav Ryhel
  Cc: dri-devel, linux-kernel

After tc358768_setup_pll() enables PLL_CKEN and the lock indicator comes
up, the DSI register sequence runs near immediately. On TF700T this
results in the bridge claiming PLL lock but producing no DSI video output:
the panel powers up, the backlight comes on, but the framebuffer is never
scanned out.

Insert a small settling delay between PLL setup and the rest of the
attach.

Suggested-by: Michele Brocco <brocco@perceptos.de>
Tested-by: Michele Brocco <brocco@perceptos.de> # TF700T
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 0f2820b37302..17abb24a7181 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -824,6 +824,9 @@ static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 		return;
 	}
 
+	/* Allow the PLL to settle */
+	msleep(20);
+
 	drm_display_mode_to_videomode(mode, &vm);
 
 	dsiclk = priv->dsiclk;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Claude review: gpu/drm: bridge: tc358768: Add delay after PLL setup
  2026-05-20  9:26 [PATCH v1 0/1] gpu/drm: bridge: tc358768: Add delay after PLL setup Svyatoslav Ryhel
  2026-05-20  9:26 ` [PATCH v1 1/1] " Svyatoslav Ryhel
@ 2026-05-25 12:05 ` Claude Code Review Bot
  1 sibling, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:05 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: gpu/drm: bridge: tc358768: Add delay after PLL setup
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 2
Reviewed: 2026-05-25T22:05:01.975192

---

This is a single-patch series adding a 20ms `msleep()` delay after PLL setup in the TC358768 DSI bridge driver. The fix addresses a real hardware issue on the TF700T (ASUS Transformer Pad Infinity) where the PLL reports lock but the clock hasn't fully stabilized, resulting in a blank screen.

The change is small, low-risk, and placed at the correct location. The existing `tc358768_setup_pll()` already has a 1-2ms `usleep_range()` for PLL lock, then enables `PLL_CKEN`. The new delay gives the clock output time to settle after `PLL_CKEN` is asserted — a distinct and valid concern from the lock wait itself. The patch has a Tested-by tag on the affected hardware.

**Verdict**: Looks reasonable. One minor concern worth raising.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: gpu/drm: bridge: tc358768: Add delay after PLL setup
  2026-05-20  9:26 ` [PATCH v1 1/1] " Svyatoslav Ryhel
@ 2026-05-25 12:05   ` Claude Code Review Bot
  0 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:05 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The fix itself is correct in placement.** Looking at the PLL setup sequence:

```c
/* wait for lock */
usleep_range(1000, 2000);

/* FRS[11:10] LBWS[9:8] CKEN[4] PLL_CKEN[4] RESETB[1] EN[0] */
tc358768_write(priv, TC358768_PLLCTL1,
               (frs << 10) | (0x2 << 8) | BIT(4) | BIT(1) | BIT(0));
```

The existing 1-2ms delay waits for the PLL to lock before enabling `PLL_CKEN` (BIT(4)). The new 20ms `msleep()` is placed *after* `tc358768_setup_pll()` returns — i.e., after `PLL_CKEN` is enabled — giving the clock output time to propagate and stabilize before the DSI register configuration begins. This is a reasonable hardware settling requirement.

**Minor concerns:**

1. **Delay magnitude**: 20ms is a fairly generous delay. The commit message says "small settling delay" but doesn't reference a datasheet timing spec. It would be good to know whether 20ms is derived from the TC358768 datasheet or found empirically. If empirical, a comment noting the origin (e.g., "empirically determined on TF700T") would help future maintainers understand whether this can be tightened. That said, 20ms in a `pre_enable` path is unlikely to cause user-visible latency issues — modeset paths are inherently slow.

2. **`msleep` vs `usleep_range`**: The kernel convention is to use `usleep_range()` for delays under ~20ms. At exactly 20ms, `msleep()` is acceptable per `Documentation/timers/timers-howto.rst`, but `usleep_range(20000, 25000)` would be slightly more idiomatic and give the scheduler more flexibility. This is a stylistic nit, not a blocking issue.

3. **No datasheet reference**: The TC358768 datasheet likely has a timing spec for the PLL settling time after `PLL_CKEN` assertion. Referencing it (or noting the absence of one) would strengthen the commit message.

**No correctness issues.** The patch is safe to apply — it adds latency only in the modeset path, affects only the TC358768 bridge, and has hardware testing confirmation.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-05-25 12:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-20  9:26 [PATCH v1 0/1] gpu/drm: bridge: tc358768: Add delay after PLL setup Svyatoslav Ryhel
2026-05-20  9:26 ` [PATCH v1 1/1] " Svyatoslav Ryhel
2026-05-25 12:05   ` Claude review: " Claude Code Review Bot
2026-05-25 12:05 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox