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

[PATCH 0/2] 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>

---
Esben Haabendal (2):
      drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84
      drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

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

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

[PATCH 1/2] 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 | 36 ++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 42b451432bbb..32e9eadd7452 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,40 @@ 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 */
+			pr_warn("%s: DSI Channel A to LVDS Channel A\n", __func__);
+		} else if (port0 && !port1 && !port2 && port3) {
+			/* DSI Channel A to LVDS Channel B */
+			pr_warn("%s: DSI Channel A to LVDS Channel B\n", __func__);
+			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 1/2] 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] Leftover debug `pr_warn` statements unconditionally print warnings on standard, valid hardware configurations during device probe.
--

--- Patch [1]: [PATCH 1/2] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84 ---
commit f232cbf82d686f0a82f41ef8eedda5654e9deea0
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 42b451432bbb6..32e9eadd7452f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c

[ ... ]

> @@ -904,29 +907,40 @@ 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 */
> +			pr_warn("%s: DSI Channel A to LVDS Channel A\n", __func__);
> +		} else if (port0 && !port1 && !port2 && port3) {
> +			/* DSI Channel A to LVDS Channel B */
> +			pr_warn("%s: DSI Channel A to LVDS Channel B\n", __func__);

[Severity: Medium]
Are these pr_warn() statements leftover debugging code?

Since this patch introduces normal support for single-link modes, printing
unconditional warnings in sn65dsi83_parse_dt() on perfectly valid hardware 
configurations (single-link DSI to LVDS channel A or B) will pollute the 
dmesg log during device probe. This might cause false failures in CI systems 
that check for kernel warnings, and confuse users into thinking there is a 
hardware or driver issue.

> +			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/20260711-ti-sn65dsi83-fixes-v1-0-d85eb5342b98@geanix.com?part=1

[PATCH 2/2] 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/

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 32e9eadd7452..e87bed3a6640 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -532,7 +532,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;
@@ -682,26 +681,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);
@@ -712,11 +697,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);
+		return;
+	}
+
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);

-- 
2.55.0

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

[sashiko-bot]

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

New issues:
- [High] Missing SRCU read lock release in error path of sn65dsi83_atomic_enable().

Pre-existing issues:
- [High] Missing SRCU read lock release in error path of sn65dsi83_reset_work().
--

--- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking ---
commit 0c69248e520fff24e51d45bfab961991fbdb4267
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

This patch moves the PLL locking sequence from atomic_pre_enable to
atomic_enable. This ensures the DSI HS clock is running before attempting to
lock the PLL.

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 32e9eadd7452f..e87bed3a6640b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c

[ ... ]

> @@ -712,11 +697,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);
> +		return;

[Severity: High]
Does this error path leak the SRCU read lock acquired by drm_bridge_enter()?

If the PLL lock fails and regmap_read_poll_timeout() returns an error, the
function returns immediately without calling drm_bridge_exit(idx). 

This unreleased lock could cause subsequent calls to synchronize_srcu() to
hang during module unload or bridge unplug.

> +	}
> +
>  	/* Clear all errors that got asserted during initialization. */
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);

[Severity: High]
This is a pre-existing issue, but I noticed a similar pattern in another
function in this file. Does sn65dsi83_reset_work() also leak the SRCU read
lock on failure?

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
    if (!drm_bridge_enter(&ctx->bridge, &idx))
        return;
    ...
    ret = sn65dsi83_reset_pipe(ctx);
    if (ret) {
        dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
        return;
    }
    ...
    drm_bridge_exit(idx);
}

Like the atomic_enable path, if sn65dsi83_reset_pipe() fails, it appears to
return early without calling drm_bridge_exit(idx).

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

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

[Esben Haabendal]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Missing SRCU read lock release in error path of sn65dsi83_atomic_enable().
>
> Pre-existing issues:
> - [High] Missing SRCU read lock release in error path of sn65dsi83_reset_work().
> --
>
> --- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking ---
> commit 0c69248e520fff24e51d45bfab961991fbdb4267
> Author: Esben Haabendal <esben@geanix.com>
>
> drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking
>
> This patch moves the PLL locking sequence from atomic_pre_enable to
> atomic_enable. This ensures the DSI HS clock is running before attempting to
> lock the PLL.
>
> 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 32e9eadd7452f..e87bed3a6640b 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>
> [ ... ]
>
>> @@ -712,11 +697,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);
>> +		return;
>
> [Severity: High]
> Does this error path leak the SRCU read lock acquired by drm_bridge_enter()?
>
> If the PLL lock fails and regmap_read_poll_timeout() returns an error, the
> function returns immediately without calling drm_bridge_exit(idx).
>
> This unreleased lock could cause subsequent calls to synchronize_srcu() to
> hang during module unload or bridge unplug.

Yes, that does look like a bug. I will fix it for v2.

>> +	}
>> +
>>  	/* Clear all errors that got asserted during initialization. */
>>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>
> [Severity: High]
> This is a pre-existing issue, but I noticed a similar pattern in another
> function in this file. Does sn65dsi83_reset_work() also leak the SRCU read
> lock on failure?
>
> drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
>     if (!drm_bridge_enter(&ctx->bridge, &idx))
>         return;
>     ...
>     ret = sn65dsi83_reset_pipe(ctx);
>     if (ret) {
>         dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
>         return;
>     }
>     ...
>     drm_bridge_exit(idx);
> }
>
> Like the atomic_enable path, if sn65dsi83_reset_pipe() fails, it appears to
> return early without calling drm_bridge_exit(idx).

Yes.

But is it enough to call drm_bridge_exit(idx), or do we want to
enable_irq() also? We are calling disable_irq_nosync() before scheduling
reset_ork, so if don't enable it again in case of error, it will be
stuck disabled. But what is supposed to happen if/when
sn65dsi83_reset_pipe() fails?

If we enable_irq() again, are we just going to endure endless pain?

/Esben