[PATCH v2 0/3] drm/bridge: ti-sn65dsi83: Various fixes

[PATCH v2 0/3] drm/bridge: ti-sn65dsi83: Various fixes

[Esben Haabendal]

This small series adds support for using SN65DSI84 in single-link mode with
output to LVDS channel B, and provides a fix for a PLL locking issue.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v2:
- Fix error handling in sn65dsi83_atomic_enable() to ensure
  drm_brige_exit() is always called on exit.
- Change logging level from warn to dbg for two messages in
  sn65dsi83_parse_dt().
- Removed trailing dot in comment lines.
- Added patch with fix so that DRM bridge critical section is exited on
  error in sn65dsi83_reset_work().
- Added Fixes and Cc: stable tags to the premature PLL patch.
- Re-ordered patch so fixes comes first.
- Link to v1: https://patch.msgid.link/20260711-ti-sn65dsi83-fixes-v1-0-d85eb5342b98@geanix.com

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: Linus Walleij <linusw@kernel.org>
To: Frieder Schrempf <frieder.schrempf@kontron.de>
To: Marek Vasut <marex@denx.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

---
Esben Haabendal (3):
      drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking
      drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
      drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 68 ++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 28 deletions(-)
---
base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
change-id: 20260710-ti-sn65dsi83-fixes-1d08e0ac67b3

Best regards,
--  
Esben Haabendal <esben@geanix.com>

[PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

[Esben Haabendal]

Locking PLL requires the DSI HS clock to be running, which it might not be
in probe(), but should be in atomic_enable().

This resolves issues like this:

sn65dsi83 1-002c: failed to lock PLL, ret=-110
sn65dsi83 1-002c: Unexpected link status 0x01
sn65dsi83 1-002c: Unexpected link status 0x01
sn65dsi83 1-002c: reset the pipe

as seen with nwl-dsi bridge and others.

This is the same issue as addressed in the patch by Gary Bisson [1],
but changing the ti-sn65dsi83 driver instead, so we don't have to change
all other drivers that could potentially be used with this chip.

[1] https://lore.kernel.org/all/20260120-mtkdsi-v1-1-b0f4094f3ac3@gmail.com/

Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 42b451432bbb..b4b220eee790 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -530,7 +530,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	bool test_pattern = sn65dsi83_test_pattern;
 	bool lvds_format_24bpp;
 	bool lvds_format_jeida;
-	unsigned int pval;
 	__le16 le16val;
 	u16 val;
 	int ret;
@@ -680,26 +679,12 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN,
 		     test_pattern ? REG_VID_CHA_TEST_PATTERN_EN : 0);
 
-	/* Enable PLL */
-	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
-	usleep_range(3000, 4000);
-	ret = regmap_read_poll_timeout(ctx->regmap, REG_RC_LVDS_PLL, pval,
-				       pval & REG_RC_LVDS_PLL_PLL_EN_STAT,
-				       1000, 100000);
-	if (ret) {
-		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
-		/* On failure, disable PLL again and exit. */
-		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
-		goto err_add_action;
-	}
-
 	/* Trigger reset after CSR register update. */
 	regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
 
 	/* Wait for 10ms after soft reset as specified in datasheet */
 	usleep_range(10000, 12000);
 
-err_add_action:
 	devm_add_action(ctx->dev, sn65dsi83_release_resources, ctx);
 err_exit:
 	drm_bridge_exit(idx);
@@ -710,11 +695,24 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	unsigned int pval;
-	int idx;
+	int idx, ret;
 
 	if (!drm_bridge_enter(bridge, &idx))
 		return;
 
+	/* Enable PLL */
+	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
+	usleep_range(3000, 4000);
+	ret = regmap_read_poll_timeout(ctx->regmap, REG_RC_LVDS_PLL, pval,
+				       pval & REG_RC_LVDS_PLL_PLL_EN_STAT,
+				       1000, 100000);
+	if (ret) {
+		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
+		/* On failure, disable PLL again and exit. */
+		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
+		goto err_exit;
+	}
+
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
@@ -734,6 +732,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 		sn65dsi83_monitor_start(ctx);
 	}
 
+err_exit:
 	drm_bridge_exit(idx);
 }
 

-- 
2.55.0

[PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[Esben Haabendal]

We obviously should not leave the DRM bridge critical section activated
when exiting on error.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index b4b220eee790..7e73035d7798 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
 	ret = sn65dsi83_reset_pipe(ctx);
 	if (ret) {
 		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
-		return;
+		goto err_exit;
 	}
 	if (ctx->irq)
 		enable_irq(ctx->irq);
 
+err_exit:
 	drm_bridge_exit(idx);
 }
 

-- 
2.55.0

[PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[Esben Haabendal]

This adds support for using SN65DSI84 in single-link mode with output to
LVDS Channel B.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 7e73035d7798..02ae2b8179b0 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -11,6 +11,8 @@
  *   = 1x Single-link DSI ~ 2x Single-link or 1x Dual-link LVDS
  *   - Supported
  *   - Dual-link LVDS mode tested
+ *   - Single-link to LVDS Channel A tested
+ *   - Single-link to LVDS Channel B tested
  *   - 2x Single-link LVDS mode unsupported
  *     (should be easy to add by someone who has the HW)
  * - SN65DSI85
@@ -162,7 +164,7 @@ struct sn65dsi83 {
 	struct gpio_desc		*enable_gpio;
 	struct regulator		*vcc;
 	bool				lvds_dual_link;
-	bool				lvds_dual_link_even_odd_swap;
+	bool				lvds_channel_swap;
 	int				lvds_vod_swing_conf[2];
 	int				lvds_term_conf[2];
 	int				irq;
@@ -642,7 +644,7 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
 			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
 	regmap_write(ctx->regmap, REG_LVDS_LANE,
-		     (ctx->lvds_dual_link_even_odd_swap ?
+		     (ctx->lvds_channel_swap ?
 		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
 		     (ctx->lvds_term_conf[CHANNEL_A] ?
 			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
@@ -893,6 +895,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 {
 	struct drm_bridge *panel_bridge;
 	struct device *dev = ctx->dev;
+	u32 panel_port = 2;
 	int ret;
 
 	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
@@ -904,29 +907,38 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 		return ret;
 
 	ctx->lvds_dual_link = false;
-	ctx->lvds_dual_link_even_odd_swap = false;
+	ctx->lvds_channel_swap = false;
 	if (model != MODEL_SN65DSI83) {
-		struct device_node *port2, *port3;
+		struct device_node *port0, *port1, *port2, *port3;
 		int dual_link;
 
+		port0 = of_graph_get_port_by_id(dev->of_node, 0);
+		port1 = of_graph_get_port_by_id(dev->of_node, 1);
 		port2 = of_graph_get_port_by_id(dev->of_node, 2);
 		port3 = of_graph_get_port_by_id(dev->of_node, 3);
 		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
-		of_node_put(port2);
-		of_node_put(port3);
 
 		if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
-			ctx->lvds_dual_link = true;
 			/* Odd pixels to LVDS Channel A, even pixels to B */
-			ctx->lvds_dual_link_even_odd_swap = false;
-		} else if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
 			ctx->lvds_dual_link = true;
+		} else if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
 			/* Even pixels to LVDS Channel A, odd pixels to B */
-			ctx->lvds_dual_link_even_odd_swap = true;
+			ctx->lvds_dual_link = true;
+			ctx->lvds_channel_swap = true;
+		} else if (port0 && !port1 && port2 && !port3) {
+			/* DSI Channel A to LVDS Channel A */
+		} else if (port0 && !port1 && !port2 && port3) {
+			/* DSI Channel A to LVDS Channel B */
+			ctx->lvds_channel_swap = true;
+			panel_port = 3;
 		}
+		of_node_put(port0);
+		of_node_put(port1);
+		of_node_put(port2);
+		of_node_put(port3);
 	}
 
-	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
+	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, panel_port, 0);
 	if (IS_ERR(panel_bridge))
 		return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n");
 

-- 
2.55.0

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The logic to detect single-link LVDS Channel B requires `port@2` to be absent from the device tree, but the DT binding strictly requires `port@2` to be present.
--

--- Patch [3]: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84 ---
commit e7655ab4d66cbec701adf0102160008944fa2a88
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

This adds support for using SN65DSI84 in single-link mode with output to
LVDS Channel B.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 7e73035d77986..02ae2b8179b07 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
[ ... ]
> @@ -904,29 +907,38 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>  		return ret;
>  
>  	ctx->lvds_dual_link = false;
> -	ctx->lvds_dual_link_even_odd_swap = false;
> +	ctx->lvds_channel_swap = false;
>  	if (model != MODEL_SN65DSI83) {
> -		struct device_node *port2, *port3;
> +		struct device_node *port0, *port1, *port2, *port3;
>  		int dual_link;
>  
> +		port0 = of_graph_get_port_by_id(dev->of_node, 0);
> +		port1 = of_graph_get_port_by_id(dev->of_node, 1);
>  		port2 = of_graph_get_port_by_id(dev->of_node, 2);
>  		port3 = of_graph_get_port_by_id(dev->of_node, 3);
>  		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
>  
>  		if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
>  			/* Odd pixels to LVDS Channel A, even pixels to B */
>  			ctx->lvds_dual_link = true;
>  		} else if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
>  			/* Even pixels to LVDS Channel A, odd pixels to B */
>  			ctx->lvds_dual_link = true;
> +			ctx->lvds_channel_swap = true;
> +		} else if (port0 && !port1 && port2 && !port3) {
> +			/* DSI Channel A to LVDS Channel A */
> +		} else if (port0 && !port1 && !port2 && port3) {
                                              ^^^^^^

[Severity: Medium]
Will this !port2 condition prevent the single-link LVDS Channel B feature
from working with a standard-compliant device tree?

If a user provides a compliant device tree by including an empty or
unconnected port@2 to satisfy the DT schema required list for this binding,
of_graph_get_port_by_id() will return a valid node.

The driver would then fail this check, fall through leaving panel_port = 2,
and subsequently fail to probe when it cannot find a panel connected to the
empty port@2.

> +			/* DSI Channel A to LVDS Channel B */
> +			ctx->lvds_channel_swap = true;
> +			panel_port = 3;
>  		}
> +		of_node_put(port0);
> +		of_node_put(port1);
> +		of_node_put(port2);
> +		of_node_put(port3);
>  	}
>  
> -	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
> +	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, panel_port, 0);
>  	if (IS_ERR(panel_bridge))
>  		return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-ti-sn65dsi83-fixes-v2-0-ebc4c3fe29b6@geanix.com?part=3

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The error handling branch patched by this commit is dead code because sn65dsi83_reset_pipe() unconditionally returns 0.
- [High] Missing cancellation of reset_work on driver removal leads to a potential Use-After-Free.
--

--- Patch [2]: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() ---
commit da59dc684f40189c9b3344ad029c137b580d644a
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

This commit updates the error handling path in sn65dsi83_reset_work() to
ensure the DRM bridge critical section is cleanly exited when resetting the
pipe fails.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index b4b220eee7909..7e73035d77986 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  	ret = sn65dsi83_reset_pipe(ctx);
>  	if (ret) {
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> +		goto err_exit;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but is this error handling block
actually dead code?

Looking at sn65dsi83_reset_pipe(), it appears to unconditionally return 0,
even if drm_bridge_helper_reset_crtc() returns an error like -EINVAL or
-ENOMEM:

static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
{
	...
retry:
	err = drm_bridge_helper_reset_crtc(&sn65dsi83->bridge, &ctx);
	if (err == -EDEADLK) {
		drm_modeset_backoff(&ctx);
		goto retry;
	}

	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);

	return 0;
}

Since ret is always 0, this error branch will never execute. If an actual
hardware error persists (e.g., PLL unlock), the driver will unconditionally
proceed to enable_irq(ctx->irq) below. 

Could this result in an immediate subsequent interrupt and cause an interrupt
storm that locks up the CPU?

>  	if (ctx->irq)
>  		enable_irq(ctx->irq);
>  
> +err_exit:
>  	drm_bridge_exit(idx);
>  }

[Severity: High]
This is also a pre-existing issue, but does this workqueue function have a
use-after-free risk during driver removal?

When the driver is unbound (e.g., via sysfs), sn65dsi83_remove() unregisters
the bridge but does not appear to call cancel_work_sync(&ctx->reset_work).

If an interrupt fires right before cleanup and schedules this work,
devm_drm_bridge_alloc cleanup will free the ctx structure. The workqueue
could then execute sn65dsi83_reset_work() asynchronously, causing a UAF
when dereferencing ctx->bridge.

Should sn65dsi83_remove() be updated to cancel the pending reset work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-ti-sn65dsi83-fixes-v2-0-ebc4c3fe29b6@geanix.com?part=2

Re: [PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The reset_work work item is never cancelled during device removal, leading to a Use-After-Free if executed after devres cleanup.
--

--- Patch [1]: [PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking ---
commit 6d16fdef43813e12caf14d60e8f68b785f57a520
Author: Esben Haabendal <esben@geanix.com>
Subject: drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

This commit moves the PLL locking sequence from atomic_pre_enable() to
atomic_enable(), ensuring the DSI HS clock is running before the PLL is
locked. This resolves PLL lock failures seen with certain configurations.

Link: https://lore.kernel.org/all/20260120-mtkdsi-v1-1-b0f4094f3ac3@gmail.com/

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 42b451432bbb6..b4b220eee7909 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c

[ ... ]

> @@ -734,6 +732,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>  		sn65dsi83_monitor_start(ctx);
>  	}
>  
> +err_exit:
>  	drm_bridge_exit(idx);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing
issue in this driver regarding device removal.

Could the reset_work item execute after the device has been unbound?

Looking at sn65dsi83_remove(), it unplugs the bridge but doesn't cancel
the pending work:

static void sn65dsi83_remove(struct i2c_client *client)
{
	struct sn65dsi83 *ctx = i2c_get_clientdata(client);

	drm_bridge_unplug(&ctx->bridge);
}

Because ctx is allocated via devres, the memory is freed immediately after
removal completes. If sn65dsi83_reset_work() is scheduled (e.g., from
hardware errors) and runs after this cleanup, it dereferences the freed
pointer:

static void sn65dsi83_reset_work(struct work_struct *ws)
{
	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
	...
	if (!drm_bridge_enter(&ctx->bridge, &idx))
		return;

Does this race condition lead to a use-after-free if the device is removed
while reset_work is pending?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-ti-sn65dsi83-fixes-v2-0-ebc4c3fe29b6@geanix.com?part=1

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[Luca Ceresoli]

On Wed, 15 Jul 2026 15:10:32 +0200, Esben Haabendal <esben@geanix.com> wrote:

Hi Esben,

+Cc Hervé

>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index b4b220eee790..7e73035d7798 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  	ret = sn65dsi83_reset_pipe(ctx);
>  	if (ret) {
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> +		goto err_exit;
>  	}

This looked like a nasty bug! But it is not, out of pure luck. As sashiko
noticed:

 | sashiko-bot@kernel.org <sashiko-bot@kernel.org>:
 |
 | [Severity: High]
 | This isn't a bug introduced by this patch, but is this error handling block
 | actually dead code?
 |
 | Looking at sn65dsi83_reset_pipe(), it appears to unconditionally return 0,
 | even if drm_bridge_helper_reset_crtc() returns an error like -EINVAL or
 | -ENOMEM:

So perhaps we should just remove the dead code, and also make
sn65dsi83_reset_pipe() return void.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[Luca Ceresoli]

On Wed, 15 Jul 2026 15:10:33 +0200, Esben Haabendal <esben@geanix.com> wrote:

Hi Esben,

>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 7e73035d7798..02ae2b8179b0 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -893,6 +895,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>  {
>  	struct drm_bridge *panel_bridge;
>  	struct device *dev = ctx->dev;
> +	u32 panel_port = 2;

Thanks for having applied the changes I had suggested.

I'm afraid I realized right now the "panel_port" name is not ideal because
there could be another bridge at the output port and not directly a
panel. Should you have a reason to send a v3 I'd rename to output_port (and
keep my R-by if that's the only change). Otherwise I can do it while
applying the patch.


Luca

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[Esben Haabendal]

On Thursday, 16 July 2026 at 17:22, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> On Wed, 15 Jul 2026 15:10:33 +0200, Esben Haabendal <esben@geanix.com> wrote:
> 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 7e73035d7798..02ae2b8179b0 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -893,6 +895,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
> >  {
> >  	struct drm_bridge *panel_bridge;
> >  	struct device *dev = ctx->dev;
> > +	u32 panel_port = 2;
> 
> Thanks for having applied the changes I had suggested.
> 
> I'm afraid I realized right now the "panel_port" name is not ideal because
> there could be another bridge at the output port and not directly a
> panel. Should you have a reason to send a v3 I'd rename to output_port (and
> keep my R-by if that's the only change). Otherwise I can do it while
> applying the patch.

What about the panel_bridge member of struct sn65dsi83? We use
panel_port/output_port as argument to devm_drm_of_get_bridge() and assign the
bridge we get from that to ctx->panel_bridge. So are you sure that it makes
sense to rename panel_port to output_port? Or do you want to rename the
panel_bridge member also?

/Esben

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[Luca Ceresoli]

Hi,

On Thu Jul 16, 2026 at 6:03 PM CEST, Esben Haabendal wrote:
> On Thursday, 16 July 2026 at 17:22, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> On Wed, 15 Jul 2026 15:10:33 +0200, Esben Haabendal <esben@geanix.com> wrote:
>>
>> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> > index 7e73035d7798..02ae2b8179b0 100644
>> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> > @@ -893,6 +895,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>> >  {
>> >  	struct drm_bridge *panel_bridge;
>> >  	struct device *dev = ctx->dev;
>> > +	u32 panel_port = 2;
>>
>> Thanks for having applied the changes I had suggested.
>>
>> I'm afraid I realized right now the "panel_port" name is not ideal because
>> there could be another bridge at the output port and not directly a
>> panel. Should you have a reason to send a v3 I'd rename to output_port (and
>> keep my R-by if that's the only change). Otherwise I can do it while
>> applying the patch.
>
> What about the panel_bridge member of struct sn65dsi83? We use
> panel_port/output_port as argument to devm_drm_of_get_bridge() and assign the
> bridge we get from that to ctx->panel_bridge. So are you sure that it makes
> sense to rename panel_port to output_port? Or do you want to rename the
> panel_bridge member also?

Ah, I understand. Well, the panel_bridge name is confusing, perhaps doe to
historical legacy. Perhaps the name was given when the next thing after the
sn65dsi8x could only be a panel. Now if you look at the
devm_drm_of_get_bridge() code it is one of:

  1. a panel_bridge created on the fly to wrap a panel
     (a panel is connected at the sn65dsi8x output)
  2. a pre-existing drm_bridge that does not wrap a panel
     (a LVDS-to-something-else-bridge is connected at the sn65dsi8x output)

In the latter case the panel_bridge does not make sense. But it works
because int he end it is a struct drm_bridge *, not a panel-specific thing.

I am about to change all that and eventually have:

 - a panel_bridge created for every drm_panel
 - all interactions with the next element done accessing the next_bridge,
   no more accessing a drm_panel directly
 - drm_of_find_panel_or_bridge() disappear, because there will be no need
   to access the "next panel"
 - devm_drm_of_get_bridge() just return the next bridge, not bothering
   about panels

I hope this clarifies.

That said, I wouldn't bother renaming existing variables, but for new
variables being added I'd try to use the best possible name. Here you are
storing the number of the output port, so I suggested output_port.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[Herve Codina]

Hi Luca, Esben,

On Thu, 16 Jul 2026 17:22:24 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> On Wed, 15 Jul 2026 15:10:32 +0200, Esben Haabendal <esben@geanix.com> wrote:
> 
> Hi Esben,
> 
> +Cc Hervé
> 
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index b4b220eee790..7e73035d7798 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
> >  	ret = sn65dsi83_reset_pipe(ctx);
> >  	if (ret) {
> >  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> > -		return;
> > +		goto err_exit;
> >  	}  
> 
> This looked like a nasty bug! But it is not, out of pure luck. As sashiko
> noticed:
> 
>  | sashiko-bot@kernel.org <sashiko-bot@kernel.org>:
>  |
>  | [Severity: High]
>  | This isn't a bug introduced by this patch, but is this error handling block
>  | actually dead code?
>  |
>  | Looking at sn65dsi83_reset_pipe(), it appears to unconditionally return 0,
>  | even if drm_bridge_helper_reset_crtc() returns an error like -EINVAL or
>  | -ENOMEM:
> 
> So perhaps we should just remove the dead code, and also make
> sn65dsi83_reset_pipe() return void.
> 

I would keep an error code from sn65dsi83_reset_pipe() but change its code
from
---- 8< ----
retry:
	err = drm_bridge_helper_reset_crtc(&sn65dsi83->bridge, &ctx);
	if (err == -EDEADLK) {
		drm_modeset_backoff(&ctx);
		goto retry;
	}

	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);

	return 0;
---- 8< ----

to 

---- 8< ----
retry:
	err = drm_bridge_helper_reset_crtc(&sn65dsi83->bridge, &ctx);
	if (err == -EDEADLK) {
		drm_modeset_backoff(&ctx);
		goto retry;
	}

	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);

	return err;
---- 8< ----

Indeed, if drm_bridge_helper_reset_crtc() fails, we can consider that
sn65dsi83_reset_pipe() fails.

With that done, Esben's modification is still relevant (i.e. goto err_exit
on failure).

Best regards,
Hervé