* [PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking
2026-07-15 13:10 [PATCH v2 0/3] drm/bridge: ti-sn65dsi83: Various fixes Esben Haabendal
@ 2026-07-15 13:10 ` Esben Haabendal
2026-07-15 13:10 ` [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() Esben Haabendal
2026-07-15 13:10 ` [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84 Esben Haabendal
2 siblings, 0 replies; 9+ messages in thread
From: Esben Haabendal @ 2026-07-15 13:10 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,
Linus Walleij, Frieder Schrempf, Marek Vasut
Cc: Esben Haabendal, dri-devel, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
2026-07-15 13:10 [PATCH v2 0/3] drm/bridge: ti-sn65dsi83: Various fixes Esben Haabendal
2026-07-15 13:10 ` [PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking Esben Haabendal
@ 2026-07-15 13:10 ` Esben Haabendal
2026-07-16 15:22 ` Luca Ceresoli
2026-07-15 13:10 ` [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84 Esben Haabendal
2 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-07-15 13:10 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,
Linus Walleij, Frieder Schrempf, Marek Vasut
Cc: Esben Haabendal, dri-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
2026-07-15 13:10 ` [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() Esben Haabendal
@ 2026-07-16 15:22 ` Luca Ceresoli
2026-07-16 17:09 ` Herve Codina
0 siblings, 1 reply; 9+ messages in thread
From: Luca Ceresoli @ 2026-07-16 15:22 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,
Linus Walleij, Frieder Schrempf, Marek Vasut
Cc: Esben Haabendal, dri-devel, linux-kernel, Herve Codina
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
2026-07-16 15:22 ` Luca Ceresoli
@ 2026-07-16 17:09 ` Herve Codina
0 siblings, 0 replies; 9+ messages in thread
From: Herve Codina @ 2026-07-16 17:09 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Linus Walleij,
Frieder Schrempf, Marek Vasut, Esben Haabendal, dri-devel,
linux-kernel
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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84
2026-07-15 13:10 [PATCH v2 0/3] drm/bridge: ti-sn65dsi83: Various fixes Esben Haabendal
2026-07-15 13:10 ` [PATCH v2 1/3] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking Esben Haabendal
2026-07-15 13:10 ` [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() Esben Haabendal
@ 2026-07-15 13:10 ` Esben Haabendal
2026-07-16 15:22 ` Luca Ceresoli
2 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-07-15 13:10 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,
Linus Walleij, Frieder Schrempf, Marek Vasut
Cc: Esben Haabendal, dri-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84
2026-07-15 13:10 ` [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84 Esben Haabendal
@ 2026-07-16 15:22 ` Luca Ceresoli
2026-07-16 16:03 ` Esben Haabendal
0 siblings, 1 reply; 9+ messages in thread
From: Luca Ceresoli @ 2026-07-16 15:22 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Linus Walleij, Frieder Schrempf, Marek Vasut, dri-devel,
linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84
2026-07-16 15:22 ` Luca Ceresoli
@ 2026-07-16 16:03 ` Esben Haabendal
2026-07-16 17:03 ` Luca Ceresoli
0 siblings, 1 reply; 9+ messages in thread
From: Esben Haabendal @ 2026-07-16 16:03 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Linus Walleij,
Frieder Schrempf, Marek Vasut, dri-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84
2026-07-16 16:03 ` Esben Haabendal
@ 2026-07-16 17:03 ` Luca Ceresoli
0 siblings, 0 replies; 9+ messages in thread
From: Luca Ceresoli @ 2026-07-16 17:03 UTC (permalink / raw)
To: Esben Haabendal, Luca Ceresoli
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Linus Walleij,
Frieder Schrempf, Marek Vasut, dri-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread