* [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver
@ 2026-05-15 21:44 Kaustabh Chakraborty
2026-05-15 21:44 ` [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM Kaustabh Chakraborty
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Kaustabh Chakraborty @ 2026-05-15 21:44 UTC (permalink / raw)
To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Kaustabh Chakraborty, stable
Since v6.17, there is a regression for the Exynos 7870 DSIM driver. The
display occasionally had random aberration when the panel was turned on.
The first patch addresses that.
The second patch replaces an implicit loop for waiting for PLL
stabilization with an interrupt-based solution, which should be more
reliable. This solution was suggested by Inki Dae in a discussion of an
earlier patch series sent by me. For further details, refer to its
commit description.
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
Changes in v2:
- drop now-not-required [v1 1/3] (Marek Szyprowski)
- Link to v1: https://lore.kernel.org/r/20260124-exynos-dsim-fixes-v1-0-122d047a23d1@disroot.org
---
Kaustabh Chakraborty (2):
drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM
drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
drivers/gpu/drm/bridge/samsung-dsim.c | 48 +++++++++++++++++++++++++----------
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 35 insertions(+), 14 deletions(-)
---
base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
change-id: 20260124-exynos-dsim-fixes-5383d6a6f073
Best regards,
--
Kaustabh Chakraborty <kauschluss@disroot.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM
2026-05-15 21:44 [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver Kaustabh Chakraborty
@ 2026-05-15 21:44 ` Kaustabh Chakraborty
2026-05-15 22:42 ` Claude review: " Claude Code Review Bot
2026-05-15 21:44 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
2026-05-15 22:42 ` Claude review: Fixes and enhancements for Exynos (7870) DSIM bridge driver Claude Code Review Bot
2 siblings, 1 reply; 7+ messages in thread
From: Kaustabh Chakraborty @ 2026-05-15 21:44 UTC (permalink / raw)
To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Kaustabh Chakraborty, stable
Commit a36c533ad3e1 ("drm/bridge: samsung-dsim: Always flush display
FIFO on vsync pulse") intends to enable FIFO flushing at v-sync pulse by
not setting the active-low MFLUSH_VS bit.
However, in Exynos 7870 DSIM, the MFLUSH_VS bit is active-high. There is
no publicly available documentation to the best of my knowledge, but
downstream kernel code [1] supports this claim. Enable the bit for
Exynos 7870.
Cc: stable@vger.kernel.org # v6.17 and later
Link: https://github.com/samsungexynos7870/android_kernel_samsung_exynos7870/blob/a3762bb1761ae/drivers/video/fbdev/exynos/decon_7870/dsim_reg_7870.c#L699 [1]
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
drivers/gpu/drm/bridge/samsung-dsim.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 1d85e706c74b9..70f8946ad3b24 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1089,6 +1089,13 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
reg |= DSIM_HBP_DISABLE_MODE;
if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
reg |= DSIM_HSA_DISABLE_MODE;
+
+ /*
+ * For some hardware types, DSIM_MFLUSH_VS bit needs to be
+ * enabled explicitly.
+ */
+ if (dsi->plat_data->hw_type == DSIM_TYPE_EXYNOS7870)
+ reg |= DSIM_MFLUSH_VS;
}
if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Claude review: drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM
2026-05-15 21:44 ` [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM Kaustabh Chakraborty
@ 2026-05-15 22:42 ` Claude Code Review Bot
0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-15 22:42 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Verdict: Looks good.**
This patch fixes a regression introduced in v6.17 by commit `a36c533ad3e1`, which assumed MFLUSH_VS is active-low across all variants. On Exynos 7870 the bit is active-high, so the bit must be explicitly set.
The fix is minimal and correctly scoped — it adds a hw_type check inside the existing video-mode configuration block:
```c
if (dsi->plat_data->hw_type == DSIM_TYPE_EXYNOS7870)
reg |= DSIM_MFLUSH_VS;
```
The downstream kernel reference is provided and the patch carries both Tested-by and stable Cc tags, which is appropriate for a regression fix. The comment is adequate. No issues found.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
2026-05-15 21:44 [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver Kaustabh Chakraborty
2026-05-15 21:44 ` [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM Kaustabh Chakraborty
@ 2026-05-15 21:44 ` Kaustabh Chakraborty
2026-05-15 22:42 ` Claude review: " Claude Code Review Bot
2026-05-15 22:42 ` Claude review: Fixes and enhancements for Exynos (7870) DSIM bridge driver Claude Code Review Bot
2 siblings, 1 reply; 7+ messages in thread
From: Kaustabh Chakraborty @ 2026-05-15 21:44 UTC (permalink / raw)
To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Kaustabh Chakraborty
Stabilizing PLL needs to be waited for. This is done using a loop,
checking the PLL_STABLE bit in the status register.
DSIM fires an interrupt when the PLL is stabilized. Rely on this
functionality for stabilization wait, getting rid of the implicit loop.
This has been tested on a Galaxy J6 (Exynos 7870). Unfortunately, since
testing on all supported devices is less feasible, introduce a stop-gap
measure where the timeout has a gracious lower bound of 100
microseconds. This will (hopefully) prevent regressions due to timeout
on other devices.
Suggested-by: Inki Dae <inki.dae@samsung.com>
Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
drivers/gpu/drm/bridge/samsung-dsim.c | 41 +++++++++++++++++++++++------------
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 70f8946ad3b24..0ca6c6484c9a6 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -17,6 +17,7 @@
#include <linux/export.h>
#include <linux/irq.h>
#include <linux/media-bus-format.h>
+#include <linux/minmax.h>
#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
@@ -788,7 +789,7 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
{
const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
unsigned long fin, fout;
- int timeout;
+ unsigned int timeout;
u8 p, s;
u16 m;
u32 reg;
@@ -849,19 +850,26 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
if (dsi->swap_dn_dp_data)
reg |= DSIM_PLL_DPDNSWAP_DAT;
+ /*
+ * The PLL_TIMER value is the product of the timeout delay and the APB
+ * bus clock rate. Calcutate the timeout delay on-the-fly here.
+ * It is assumed that the bus clock is the first clock in the provided
+ * bulk clock data.
+ */
+ timeout = 100;
+ fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
+ if (fin)
+ timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
+ timeout);
+
+ reinit_completion(&dsi->pll_stabilized);
samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
- timeout = 3000;
- do {
- if (timeout-- == 0) {
- dev_err(dsi->dev, "PLL failed to stabilize\n");
- return 0;
- }
- if (driver_data->has_legacy_status_reg)
- reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
- else
- reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
- } while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
+ if (wait_for_completion_timeout(&dsi->pll_stabilized,
+ usecs_to_jiffies(timeout))) {
+ dev_err(dsi->dev, "PLL failed to stabilize\n");
+ return 0;
+ }
dsi->hs_clock = fout;
@@ -1596,8 +1604,12 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
- if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
- DSIM_INT_PLL_STABLE)))
+ if (status & DSIM_INT_PLL_STABLE) {
+ complete(&dsi->pll_stabilized);
+ return IRQ_HANDLED;
+ }
+
+ if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
return IRQ_HANDLED;
if (samsung_dsim_transfer_finish(dsi))
@@ -2146,6 +2158,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
return PTR_ERR(dsi);
init_completion(&dsi->completed);
+ init_completion(&dsi->pll_stabilized);
spin_lock_init(&dsi->transfer_lock);
INIT_LIST_HEAD(&dsi->transfer_list);
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index 03005e474704b..e3433da21ad08 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -123,6 +123,7 @@ struct samsung_dsim {
int state;
struct drm_property *brightness;
struct completion completed;
+ struct completion pll_stabilized;
spinlock_t transfer_lock; /* protects transfer_list */
struct list_head transfer_list;
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Claude review: drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
2026-05-15 21:44 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
@ 2026-05-15 22:42 ` Claude Code Review Bot
0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-15 22:42 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Verdict: Has critical bugs — needs respin.**
#### Bug 1 (Critical): Inverted `wait_for_completion_timeout` return value
```c
if (wait_for_completion_timeout(&dsi->pll_stabilized,
usecs_to_jiffies(timeout))) {
dev_err(dsi->dev, "PLL failed to stabilize\n");
return 0;
}
```
`wait_for_completion_timeout()` returns **0 on timeout** and **positive (remaining jiffies) on success** (`kernel/sched/completion.c:166`). This condition is inverted — it prints the error and returns failure when the PLL *successfully* stabilizes, and silently continues when it *times out*.
The correct check should be:
```c
if (!wait_for_completion_timeout(&dsi->pll_stabilized,
usecs_to_jiffies(timeout))) {
```
#### Bug 2 (Critical): Timeout rounds to zero jiffies
The computed timeout is in microseconds. For Exynos 7870, `PLL_TIMER = 80000` and assuming a ~100 MHz bus clock, `timeout = max(80000/100, 100) = 800` microseconds. For other variants the values are even smaller (e.g., `PLL_TIMER = 500` → ~100 us floor).
`usecs_to_jiffies(800)` returns **0** at any standard `HZ` value (100, 250, 1000), because one jiffy is always >= 1 ms. With a zero timeout, `wait_for_completion_timeout` returns immediately without waiting, making this a no-op.
Combined with Bug 1, the code "works" by accident: the PLL hasn't stabilized yet (returns 0 → treated as success), and the PLL presumably finishes stabilizing shortly after. But **no actual wait occurs**, which defeats the entire purpose of the patch.
The timeout should use a more appropriate scale. Consider `fsleep()` + manual completion check, or use `msecs_to_jiffies()` with a millisecond-scale timeout, or add 1 jiffy to ensure a non-zero wait:
```c
usecs_to_jiffies(timeout) + 1
```
or convert the timeout to a reasonable millisecond value with a sane minimum (e.g., 1ms).
#### Issue 3 (Moderate): PLL_STABLE interrupt may be masked
After `DSIM_INT_SW_RST_RELEASE`, the IRQ handler sets the interrupt mask to:
```c
unsigned long mask = ~(DSIM_INT_RX_DONE |
DSIM_INT_SFR_FIFO_EMPTY |
DSIM_INT_SFR_HDR_FIFO_EMPTY |
DSIM_INT_RX_ECC_ERR |
DSIM_INT_SW_RST_RELEASE);
```
`DSIM_INT_PLL_STABLE` (`BIT(31)`) is **not** in this unmasked set, so it remains masked (bit set = masked). If `samsung_dsim_set_pll` is called after this mask is applied, the PLL_STABLE interrupt will never reach the CPU, and the completion will never fire.
The mask should include `DSIM_INT_PLL_STABLE` in the unmasked set, or the mask should be explicitly updated before calling `samsung_dsim_set_pll`.
#### Issue 4 (Minor): Typo in comment
```c
* bus clock rate. Calcutate the timeout delay on-the-fly here.
```
"Calcutate" → "Calculate"
#### Issue 5 (Minor): Assumption about clk_data[0]
```c
fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
```
The comment says "It is assumed that the bus clock is the first clock in the provided bulk clock data." This is true for `exynos7870_clk_bulk_data` (where `[0]` is `"bus"`), but other variants have different first clocks (e.g., `exynos3_clk_bulk_data[0]` is `"bus_clk"`). The assumption should be validated or the clock should be looked up by name rather than index.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
* Claude review: Fixes and enhancements for Exynos (7870) DSIM bridge driver
2026-05-15 21:44 [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver Kaustabh Chakraborty
2026-05-15 21:44 ` [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM Kaustabh Chakraborty
2026-05-15 21:44 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
@ 2026-05-15 22:42 ` Claude Code Review Bot
2 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-05-15 22:42 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: Fixes and enhancements for Exynos (7870) DSIM bridge driver
Author: Kaustabh Chakraborty <kauschluss@disroot.org>
Patches: 3
Reviewed: 2026-05-16T08:42:28.672947
---
This is a 2-patch series for the Samsung DSIM bridge driver targeting Exynos 7870. Patch 1 is a straightforward, well-justified fix for a regression. Patch 2 replaces a busy-poll PLL stabilization loop with an interrupt-based completion mechanism, but **contains a critical bug** (inverted `wait_for_completion_timeout` return value check) and a **likely-broken timeout** (sub-jiffy microsecond values rounding to 0). Despite carrying Tested-by tags, the patch appears to "work" only by accident — two bugs cancel each other out, but no actual PLL stabilization wait is performed.
**Recommendation:** Patch 1 is ready to merge. Patch 2 needs a respin to fix the inverted completion check and the inadequate timeout granularity.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver
@ 2026-03-22 17:02 Kaustabh Chakraborty
2026-03-22 17:02 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
0 siblings, 1 reply; 7+ messages in thread
From: Kaustabh Chakraborty @ 2026-03-22 17:02 UTC (permalink / raw)
To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Kaustabh Chakraborty, stable
Since v6.17, there is a regression for the Exynos 7870 DSIM driver. The
display occasionally had random aberration when the panel was turned on.
The first patch addresses that.
The second patch replaces an implicit loop for waiting for PLL
stabilization with an interrupt-based solution, which should be more
reliable. This solution was suggested by Inki Dae in a discussion of an
earlier patch series sent by me. For further details, refer to its
commit description.
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
Changes in v2:
- drop now-not-required [v1 1/3] (Marek Szyprowski)
- Link to v1: https://lore.kernel.org/r/20260124-exynos-dsim-fixes-v1-0-122d047a23d1@disroot.org
---
Kaustabh Chakraborty (2):
drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM
drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
drivers/gpu/drm/bridge/samsung-dsim.c | 48 +++++++++++++++++++++++++----------
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 35 insertions(+), 14 deletions(-)
---
base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
change-id: 20260124-exynos-dsim-fixes-5383d6a6f073
Best regards,
--
Kaustabh Chakraborty <kauschluss@disroot.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
2026-03-22 17:02 [PATCH RESEND v2 0/2] " Kaustabh Chakraborty
@ 2026-03-22 17:02 ` Kaustabh Chakraborty
2026-03-24 22:14 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 7+ messages in thread
From: Kaustabh Chakraborty @ 2026-03-22 17:02 UTC (permalink / raw)
To: Inki Dae, Jagan Teki, Marek Szyprowski, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Kaustabh Chakraborty
Stabilizing PLL needs to be waited for. This is done using a loop,
checking the PLL_STABLE bit in the status register.
DSIM fires an interrupt when the PLL is stabilized. Rely on this
functionality for stabilization wait, getting rid of the implicit loop.
This has been tested on a Galaxy J6 (Exynos 7870). Unfortunately, since
testing on all supported devices is less feasible, introduce a stop-gap
measure where the timeout has a gracious lower bound of 100
microseconds. This will (hopefully) prevent regressions due to timeout
on other devices.
Suggested-by: Inki Dae <inki.dae@samsung.com>
Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
drivers/gpu/drm/bridge/samsung-dsim.c | 41 +++++++++++++++++++++++------------
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 70f8946ad3b24..0ca6c6484c9a6 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -17,6 +17,7 @@
#include <linux/export.h>
#include <linux/irq.h>
#include <linux/media-bus-format.h>
+#include <linux/minmax.h>
#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
@@ -788,7 +789,7 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
{
const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
unsigned long fin, fout;
- int timeout;
+ unsigned int timeout;
u8 p, s;
u16 m;
u32 reg;
@@ -849,19 +850,26 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
if (dsi->swap_dn_dp_data)
reg |= DSIM_PLL_DPDNSWAP_DAT;
+ /*
+ * The PLL_TIMER value is the product of the timeout delay and the APB
+ * bus clock rate. Calcutate the timeout delay on-the-fly here.
+ * It is assumed that the bus clock is the first clock in the provided
+ * bulk clock data.
+ */
+ timeout = 100;
+ fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
+ if (fin)
+ timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
+ timeout);
+
+ reinit_completion(&dsi->pll_stabilized);
samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
- timeout = 3000;
- do {
- if (timeout-- == 0) {
- dev_err(dsi->dev, "PLL failed to stabilize\n");
- return 0;
- }
- if (driver_data->has_legacy_status_reg)
- reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
- else
- reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
- } while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
+ if (wait_for_completion_timeout(&dsi->pll_stabilized,
+ usecs_to_jiffies(timeout))) {
+ dev_err(dsi->dev, "PLL failed to stabilize\n");
+ return 0;
+ }
dsi->hs_clock = fout;
@@ -1596,8 +1604,12 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
- if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
- DSIM_INT_PLL_STABLE)))
+ if (status & DSIM_INT_PLL_STABLE) {
+ complete(&dsi->pll_stabilized);
+ return IRQ_HANDLED;
+ }
+
+ if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
return IRQ_HANDLED;
if (samsung_dsim_transfer_finish(dsi))
@@ -2146,6 +2158,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
return PTR_ERR(dsi);
init_completion(&dsi->completed);
+ init_completion(&dsi->pll_stabilized);
spin_lock_init(&dsi->transfer_lock);
INIT_LIST_HEAD(&dsi->transfer_list);
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index 03005e474704b..e3433da21ad08 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -123,6 +123,7 @@ struct samsung_dsim {
int state;
struct drm_property *brightness;
struct completion completed;
+ struct completion pll_stabilized;
spinlock_t transfer_lock; /* protects transfer_list */
struct list_head transfer_list;
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Claude review: drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
2026-03-22 17:02 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
@ 2026-03-24 22:14 ` Claude Code Review Bot
0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-24 22:14 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Verdict: Has bugs that need fixing.**
**Bug 1 (Critical): Inverted `wait_for_completion_timeout` check**
At lines 868-871 of the patched file:
```c
if (wait_for_completion_timeout(&dsi->pll_stabilized,
usecs_to_jiffies(timeout))) {
dev_err(dsi->dev, "PLL failed to stabilize\n");
return 0;
}
```
`wait_for_completion_timeout()` returns **0 on timeout** and **>0 on success**. The condition is inverted — the error path runs on success, and the success path runs on timeout. This should be:
```c
if (!wait_for_completion_timeout(...)) {
```
**Bug 2 (Critical): PLL_STABLE interrupt is masked**
Looking at the interrupt handler at line 1596-1602, after `DSIM_INT_SW_RST_RELEASE`, the interrupt mask is set to block everything except `DSIM_INT_RX_DONE`, `DSIM_INT_SFR_FIFO_EMPTY`, `DSIM_INT_SFR_HDR_FIFO_EMPTY`, `DSIM_INT_RX_ECC_ERR`, and `DSIM_INT_SW_RST_RELEASE`. Notably, `DSIM_INT_PLL_STABLE` is **not** in this unmasked set — it remains masked. This means the PLL_STABLE interrupt will never be delivered after reset, and `wait_for_completion_timeout` will always time out.
The mask at line 1597 needs to also include `DSIM_INT_PLL_STABLE`:
```c
unsigned long mask = ~(DSIM_INT_RX_DONE |
DSIM_INT_SFR_FIFO_EMPTY |
DSIM_INT_SFR_HDR_FIFO_EMPTY |
DSIM_INT_RX_ECC_ERR |
DSIM_INT_SW_RST_RELEASE |
DSIM_INT_PLL_STABLE);
```
(Note: since bug 1 inverts the sense, on current hardware these two bugs may accidentally cancel out — the masked interrupt causes a timeout, which the inverted check treats as success. But that's fragile and wrong.)
**Minor: Typo in comment**
Line 855: `Calcutate` should be `Calculate`.
**Minor: Assumption about clk_data[0] being bus clock**
The comment says "It is assumed that the bus clock is the first clock in the provided bulk clock data." This holds for all current variants (exynos3 has `bus_clk`, exynos4 has `bus_clk`, exynos5433 has `bus_clk`, exynos7870 has `bus`), but for the IMX variants the first clock is also `bus_clk`. Still, this is fragile — if a future driver data entry reorders clocks, this silently breaks. Consider whether a named lookup would be more robust, though this is a minor concern.
**Minor: `usecs_to_jiffies` with small values**
With a typical HZ=250, `usecs_to_jiffies(100)` returns 1 jiffy (4ms), which is already much larger than 100µs. The "gracious lower bound of 100 microseconds" described in the commit message is somewhat misleading since the actual wait granularity is jiffies. This isn't a bug but worth being aware of.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-05-15 22:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-15 21:44 [PATCH RESEND v2 0/2] Fixes and enhancements for Exynos (7870) DSIM bridge driver Kaustabh Chakraborty
2026-05-15 21:44 ` [PATCH RESEND v2 1/2] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM Kaustabh Chakraborty
2026-05-15 22:42 ` Claude review: " Claude Code Review Bot
2026-05-15 21:44 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
2026-05-15 22:42 ` Claude review: " Claude Code Review Bot
2026-05-15 22:42 ` Claude review: Fixes and enhancements for Exynos (7870) DSIM bridge driver Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-22 17:02 [PATCH RESEND v2 0/2] " Kaustabh Chakraborty
2026-03-22 17:02 ` [PATCH RESEND v2 2/2] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability Kaustabh Chakraborty
2026-03-24 22:14 ` Claude review: " 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