public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
@ 2026-03-12  4:37 Sudarshan Shetty
  2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Sudarshan Shetty @ 2026-03-12  4:37 UTC (permalink / raw)
  To: andrzej.hajda, neil.armstrong, rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	marex, valentin, philippe.schenker, dri-devel, linux-kernel,
	devicetree, Sudarshan Shetty

Hi all,

This patch series improves dual-link LVDS support in the SN65DSI83
DSI-to-LVDS bridge driver.

Currently the driver programs identical horizontal timing parameters
for both single-link and dual-link LVDS modes. According to TI
documentation, when operating in dual-link mode the horizontal timing
values must be divided by two before being programmed into the device.
Without this adjustment, some panels fail to light up or produce
corrupted output.

TI also provides recommended register settings for dual-link LVDS
operation. This series adds support for an optional DT property
ti,dual-link-video-mode that enables the required configuration
in the driver.

When the property is present, the driver applies the recommended
register settings and uses a simplified DSI video mode configuration
to ensure correct dual-link LVDS operation.

Summary:
 - Add DT binding for ti,dual-link-video-mode
 - Add driver support to enable dual-link LVDS configuration
 - Apply recommended register settings for dual-link operation
 - Adjust DSI mode flags when dual-link mode is enabled

Changes in v2:
 - Introduce ti,dual-link-video-mode DT property
 - Add DT binding documentation for the new property
 - Update driver to read the DT property and apply dual-link
   configuration conditionally
 - Adjust DSI mode flags when dual-link video mode is enabled
 - Update commit messages

Thanks,
Anusha

Sudarshan Shetty (2):
  dt-bindings: display: bridge: ti,sn65dsi83: Add dual-link video mode
    property
  drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode

 .../bindings/display/bridge/ti,sn65dsi83.yaml |  9 ++++
 drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 52 +++++++++++++++++--
 2 files changed, 57 insertions(+), 4 deletions(-)

-- 
2.34.1


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

* [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property
  2026-03-12  4:37 [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Sudarshan Shetty
@ 2026-03-12  4:37 ` Sudarshan Shetty
  2026-03-12 15:46   ` Luca Ceresoli
  2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
  2026-03-12  4:37 ` [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Sudarshan Shetty
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Sudarshan Shetty @ 2026-03-12  4:37 UTC (permalink / raw)
  To: andrzej.hajda, neil.armstrong, rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	marex, valentin, philippe.schenker, dri-devel, linux-kernel,
	devicetree, Sudarshan Shetty

Add a new optional device tree property `ti,dual-link-video-mode`
to indicate that the bridge should configure the device for
dual-link LVDS video mode.

In dual-link configurations, some panels require the horizontal
timing parameters to be adjusted before programming them into
the device. In such cases, the horizontal timing values must be
divided by two when operating in dual-link mode.

Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
---
 .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index e69b6343a8eb..b610739555a4 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -38,6 +38,15 @@ properties:
   interrupts:
     maxItems: 1
 
+  ti,dual-link-video-mode:
+    type: boolean
+    description: |
+      Enables configuration settings required for correct dual-link
+      LVDS operation. Some panels require the horizontal timing
+      parameters to be adjusted before being programmed into the
+      device. The horizontal timing values must be divided by
+      two when operating in dual-link mode.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.34.1


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

* [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode
  2026-03-12  4:37 [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Sudarshan Shetty
  2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
@ 2026-03-12  4:37 ` Sudarshan Shetty
  2026-03-12 15:47   ` Luca Ceresoli
  2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
  2026-03-12  5:05 ` [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Marek Vasut
  2026-03-13  4:32 ` Claude review: " Claude Code Review Bot
  3 siblings, 2 replies; 11+ messages in thread
From: Sudarshan Shetty @ 2026-03-12  4:37 UTC (permalink / raw)
  To: andrzej.hajda, neil.armstrong, rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	marex, valentin, philippe.schenker, dri-devel, linux-kernel,
	devicetree, Sudarshan Shetty

Some LVDS panels operating in dual-link mode require adjusted
horizontal timing parameters when programmed into the SN65DSI84
bridge. According to TI documentation, horizontal timing values
must be divided by two when operating in dual-link mode. Without
this adjustment, the panel may fail to display or produce corrupted
output.

Add support for an optional DT property "ti,dual-link-video-mode"
to enable configuration required for dual-link LVDS operation.
These settings ensure correct LVDS output for panels that require
this mode of operation.

Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 52 ++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index f6736b4457bb..9b7d35487bd8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -164,6 +164,7 @@ struct sn65dsi83 {
 	int				irq;
 	struct delayed_work		monitor_work;
 	struct work_struct		reset_work;
+	bool				dual_link_video_mode;
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -667,8 +668,43 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		     mode->hsync_start - mode->hdisplay);
 	regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
 		     mode->vsync_start - mode->vdisplay);
-	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
 
+	/*
+	 * In dual-link LVDS mode, the SN65DSI84 requires the horizontal
+	 * timing parameters to be adjusted before being programmed into
+	 * the device. According to TI documentation, the horizontal timing
+	 * values must be divided by two when operating in dual-link mode.
+	 * Without this adjustment, the connected panel may fail to light up
+	 * or display corrupted output.
+	 *
+	 * TI also provides recommended register settings for this mode,
+	 * which were derived using the TI DSI-Tuner tool. When the optional
+	 * DT property "ti,dual-link-video-mode" is present, apply these
+	 * configuration settings to ensure correct dual-link LVDS operation.
+	 */
+	if (ctx->dual_link_video_mode) {
+		regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05);
+		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
+		regmap_write(ctx->regmap, REG_DSI_CLK, 0x53);
+		regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f);
+		regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_BACK_PORCH, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_HORIZONTAL_FRONT_PORCH, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_FRONT_PORCH, 0x00);
+	}
+
+	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
 	/* Enable PLL */
 	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
 	usleep_range(3000, 4000);
@@ -965,9 +1001,15 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
 
 	dsi->lanes = dsi_lanes;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
-			  MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP |
-			  MIPI_DSI_MODE_VIDEO_NO_HSA | MIPI_DSI_MODE_NO_EOT_PACKET;
+	if (ctx->dual_link_video_mode)
+		dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
+	else
+		dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+				  MIPI_DSI_MODE_VIDEO_BURST |
+				  MIPI_DSI_MODE_VIDEO_NO_HFP |
+				  MIPI_DSI_MODE_VIDEO_NO_HBP |
+				  MIPI_DSI_MODE_VIDEO_NO_HSA |
+				  MIPI_DSI_MODE_NO_EOT_PACKET;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
@@ -1021,6 +1063,8 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ctx->dual_link_video_mode =
+		of_property_read_bool(dev->of_node, "ti,dual-link-video-mode");
 	ctx->regmap = devm_regmap_init_i2c(client, &sn65dsi83_regmap_config);
 	if (IS_ERR(ctx->regmap))
 		return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to get regmap\n");
-- 
2.34.1


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

* Re: [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
  2026-03-12  4:37 [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Sudarshan Shetty
  2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
  2026-03-12  4:37 ` [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Sudarshan Shetty
@ 2026-03-12  5:05 ` Marek Vasut
  2026-03-12 12:35   ` tessolveupstream
  2026-03-13  4:32 ` Claude review: " Claude Code Review Bot
  3 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2026-03-12  5:05 UTC (permalink / raw)
  To: Sudarshan Shetty, andrzej.hajda, neil.armstrong, rfoss,
	Luca Ceresoli
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	valentin, philippe.schenker, dri-devel, linux-kernel, devicetree

On 3/12/26 5:37 AM, Sudarshan Shetty wrote:
> Hi all,
> 
> This patch series improves dual-link LVDS support in the SN65DSI83
> DSI-to-LVDS bridge driver.
> 
> Currently the driver programs identical horizontal timing parameters
> for both single-link and dual-link LVDS modes. According to TI
> documentation, when operating in dual-link mode the horizontal timing
> values must be divided by two before being programmed into the device.
> Without this adjustment, some panels fail to light up or produce
> corrupted output.
> 
> TI also provides recommended register settings for dual-link LVDS
> operation. This series adds support for an optional DT property
> ti,dual-link-video-mode that enables the required configuration
> in the driver.
> 
> When the property is present, the driver applies the recommended
> register settings and uses a simplified DSI video mode configuration
> to ensure correct dual-link LVDS operation.
> 
> Summary:
>   - Add DT binding for ti,dual-link-video-mode
>   - Add driver support to enable dual-link LVDS configuration
>   - Apply recommended register settings for dual-link operation
>   - Adjust DSI mode flags when dual-link mode is enabled
> 
> Changes in v2:
>   - Introduce ti,dual-link-video-mode DT property
>   - Add DT binding documentation for the new property
>   - Update driver to read the DT property and apply dual-link
>     configuration conditionally
>   - Adjust DSI mode flags when dual-link video mode is enabled
>   - Update commit messages
> 
> Thanks,
> Anusha
> 
> Sudarshan Shetty (2):
>    dt-bindings: display: bridge: ti,sn65dsi83: Add dual-link video mode
>      property
>    drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode
> 
>   .../bindings/display/bridge/ti,sn65dsi83.yaml |  9 ++++
>   drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 52 +++++++++++++++++--
>   2 files changed, 57 insertions(+), 4 deletions(-)
+CC Luca

You might want to look at recently posted:

[PATCH 2/3] drm/bridge: ti-sn65dsi83: halve horizontal syncs for dual 
LVDS output

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

* Re: [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
  2026-03-12  5:05 ` [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Marek Vasut
@ 2026-03-12 12:35   ` tessolveupstream
  2026-03-12 15:49     ` Luca Ceresoli
  0 siblings, 1 reply; 11+ messages in thread
From: tessolveupstream @ 2026-03-12 12:35 UTC (permalink / raw)
  To: Marek Vasut, andrzej.hajda, neil.armstrong, rfoss, Luca Ceresoli
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	valentin, philippe.schenker, dri-devel, linux-kernel, devicetree



On 12-03-2026 10:35, Marek Vasut wrote:
> On 3/12/26 5:37 AM, Sudarshan Shetty wrote:
>> Hi all,
>>
>> This patch series improves dual-link LVDS support in the SN65DSI83
>> DSI-to-LVDS bridge driver.
>>
>> Currently the driver programs identical horizontal timing parameters
>> for both single-link and dual-link LVDS modes. According to TI
>> documentation, when operating in dual-link mode the horizontal timing
>> values must be divided by two before being programmed into the device.
>> Without this adjustment, some panels fail to light up or produce
>> corrupted output.
>>
>> TI also provides recommended register settings for dual-link LVDS
>> operation. This series adds support for an optional DT property
>> ti,dual-link-video-mode that enables the required configuration
>> in the driver.
>>
>> When the property is present, the driver applies the recommended
>> register settings and uses a simplified DSI video mode configuration
>> to ensure correct dual-link LVDS operation.
>>
>> Summary:
>>   - Add DT binding for ti,dual-link-video-mode
>>   - Add driver support to enable dual-link LVDS configuration
>>   - Apply recommended register settings for dual-link operation
>>   - Adjust DSI mode flags when dual-link mode is enabled
>>
>> Changes in v2:
>>   - Introduce ti,dual-link-video-mode DT property
>>   - Add DT binding documentation for the new property
>>   - Update driver to read the DT property and apply dual-link
>>     configuration conditionally
>>   - Adjust DSI mode flags when dual-link video mode is enabled
>>   - Update commit messages
>>
>> Thanks,
>> Anusha
>>
>> Sudarshan Shetty (2):
>>    dt-bindings: display: bridge: ti,sn65dsi83: Add dual-link video mode
>>      property
>>    drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode
>>
>>   .../bindings/display/bridge/ti,sn65dsi83.yaml |  9 ++++
>>   drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 52 +++++++++++++++++--
>>   2 files changed, 57 insertions(+), 4 deletions(-)
> +CC Luca
> 
> You might want to look at recently posted:
> 
> [PATCH 2/3] drm/bridge: ti-sn65dsi83: halve horizontal syncs for dual LVDS output

Thanks for pointing this out.
I tried applying the patch “[PATCH 2/3] drm/bridge: ti-sn65dsi83: halve horizontal syncs for dual LVDS output” on top of the current tree and 
removed the changes that I had previously added in the driver.
However, with this patch applied, I am currently seeing only the backlight turning on and no image on the LVDS panel.
For reference, the LVDS panel used on our platform is G133HAN01.1 and the 
DSI-to-dual-link LVDS bridge is SN65DSI84ZXHR.

During our earlier debugging, we went through several trial-and-error 
iterations and also received support from TI. According to TI, when 
operating in dual-link mode the horizontal timing parameters must be 
divided by two before being written to the device. Without this 
adjustment, the panel either does not light up or shows corrupted output.

TI also shared a set of recommended register settings for dual-link mode, 
which were derived using the TI DSI-Tuner tool. These settings helped us 
get the panel working on our hardware during testing.
For reference, the register configuration suggested by TI is as follows:

	regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05);
	regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
	regmap_write(ctx->regmap, REG_DSI_CLK, 0x53);
	regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f);
	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_VERTICAL_BACK_PORCH, 0x00);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_HORIZONTAL_FRONT_PORCH, 0x00);
	regmap_write(ctx->regmap,
		     REG_VID_CHA_VERTICAL_FRONT_PORCH, 0x00);

If it would help, we can test any proposed changes on our hardware. 
Please let me know if incorporating these register settings or additional adjustments would be the right direction for supporting dual-link LVDS 
in this driver.
Is the current patch expected to fully support dual-link LVDS, or are 
there additional changes planned for the SN65DSI84 driver?


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

* Re: [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property
  2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
@ 2026-03-12 15:46   ` Luca Ceresoli
  2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2026-03-12 15:46 UTC (permalink / raw)
  To: Sudarshan Shetty, andrzej.hajda, neil.armstrong, rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	marex, valentin, philippe.schenker, dri-devel, linux-kernel,
	devicetree, dri-devel

Hello Sudarshan,

On Thu Mar 12, 2026 at 5:37 AM CET, Sudarshan Shetty wrote:
> Add a new optional device tree property `ti,dual-link-video-mode`
> to indicate that the bridge should configure the device for
> dual-link LVDS video mode.
>
> In dual-link configurations, some panels require the horizontal
> timing parameters to be adjusted before programming them into
> the device. In such cases, the horizontal timing values must be
> divided by two when operating in dual-link mode.
>
> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>

This is not needed. Dual link mode is already implied by the presence of
port@2 and port@3.

Also, the driver implements that already, and handles even/odd pixel swap
as well:

	ctx->lvds_dual_link = false;
	ctx->lvds_dual_link_even_odd_swap = false;
	if (model != MODEL_SN65DSI83) {
		struct device_node *port2, *port3;
		int dual_link;

		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;
			/* Even pixels to LVDS Channel A, odd pixels to B */
			ctx->lvds_dual_link_even_odd_swap = true;
		}
	}

(https://elixir.bootlin.com/linux/v7.0-rc3/source/drivers/gpu/drm/bridge/ti-sn65dsi83.c#L895-L916)

Luca

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

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

* Re: [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode
  2026-03-12  4:37 ` [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Sudarshan Shetty
@ 2026-03-12 15:47   ` Luca Ceresoli
  2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2026-03-12 15:47 UTC (permalink / raw)
  To: Sudarshan Shetty, andrzej.hajda, neil.armstrong, rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	marex, valentin, philippe.schenker, dri-devel, linux-kernel,
	devicetree

Hello Sudarshan,

On Thu Mar 12, 2026 at 5:37 AM CET, Sudarshan Shetty wrote:
> Some LVDS panels operating in dual-link mode require adjusted
> horizontal timing parameters when programmed into the SN65DSI84
> bridge. According to TI documentation, horizontal timing values
> must be divided by two when operating in dual-link mode. Without
> this adjustment, the panel may fail to display or produce corrupted
> output.
>
> Add support for an optional DT property "ti,dual-link-video-mode"
> to enable configuration required for dual-link LVDS operation.
> These settings ensure correct LVDS output for panels that require
> this mode of operation.
>
> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 52 ++++++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index f6736b4457bb..9b7d35487bd8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -164,6 +164,7 @@ struct sn65dsi83 {
>  	int				irq;
>  	struct delayed_work		monitor_work;
>  	struct work_struct		reset_work;
> +	bool				dual_link_video_mode;

As said in the reply to patch 1, there is already 'bool lvds_dual_link'
carrying the same info.

>  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> @@ -667,8 +668,43 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  		     mode->hsync_start - mode->hdisplay);
>  	regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
>  		     mode->vsync_start - mode->vdisplay);
> -	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
>
> +	/*
> +	 * In dual-link LVDS mode, the SN65DSI84 requires the horizontal
> +	 * timing parameters to be adjusted before being programmed into
> +	 * the device. According to TI documentation, the horizontal timing
> +	 * values must be divided by two when operating in dual-link mode.
> +	 * Without this adjustment, the connected panel may fail to light up
> +	 * or display corrupted output.
> +	 *
> +	 * TI also provides recommended register settings for this mode,
> +	 * which were derived using the TI DSI-Tuner tool. When the optional
> +	 * DT property "ti,dual-link-video-mode" is present, apply these
> +	 * configuration settings to ensure correct dual-link LVDS operation.
> +	 */
> +	if (ctx->dual_link_video_mode) {
> +		regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05);
> +		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> +		regmap_write(ctx->regmap, REG_DSI_CLK, 0x53);
> +		regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f);
> +		regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_VERTICAL_BACK_PORCH, 0x00);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_HORIZONTAL_FRONT_PORCH, 0x00);
> +		regmap_write(ctx->regmap,
> +			     REG_VID_CHA_VERTICAL_FRONT_PORCH, 0x00);
> +	}

I guess these hard-coded values are sepcific to your panel. They must
instead be computed based on the timings in order to work for every panel.

> +
> +	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
>  	/* Enable PLL */
>  	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
>  	usleep_range(3000, 4000);
> @@ -965,9 +1001,15 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
>
>  	dsi->lanes = dsi_lanes;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> -			  MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP |
> -			  MIPI_DSI_MODE_VIDEO_NO_HSA | MIPI_DSI_MODE_NO_EOT_PACKET;
> +	if (ctx->dual_link_video_mode)
> +		dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +	else
> +		dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +				  MIPI_DSI_MODE_VIDEO_BURST |
> +				  MIPI_DSI_MODE_VIDEO_NO_HFP |
> +				  MIPI_DSI_MODE_VIDEO_NO_HBP |
> +				  MIPI_DSI_MODE_VIDEO_NO_HSA |
> +				  MIPI_DSI_MODE_NO_EOT_PACKET;

There is no explanation about this, can you elaborate on why?

I'm working on bringing up a dual-LVDS panel on a board with the SN65DSI84,
and the removing MIPI_DSI_MODE_VIDEO_BURST seems to help, but I still have
no idea why. Should you have any info, maybe from TI, it would be very
interesting.

Luca

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

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

* Re: [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
  2026-03-12 12:35   ` tessolveupstream
@ 2026-03-12 15:49     ` Luca Ceresoli
  0 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2026-03-12 15:49 UTC (permalink / raw)
  To: tessolveupstream, Marek Vasut, andrzej.hajda, neil.armstrong,
	rfoss
  Cc: Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
	mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt,
	valentin, philippe.schenker, dri-devel, linux-kernel, devicetree

Hello Sudarshan,

and thanks Marek for copying me, I hadn't noticed this series.

On Thu Mar 12, 2026 at 1:35 PM CET, tessolveupstream wrote:

[...]

>> +CC Luca
>>
>> You might want to look at recently posted:
>>
>> [PATCH 2/3] drm/bridge: ti-sn65dsi83: halve horizontal syncs for dual LVDS output
>
> Thanks for pointing this out.
> I tried applying the patch “[PATCH 2/3] drm/bridge: ti-sn65dsi83: halve horizontal syncs for dual LVDS output” on top of the current tree and
> removed the changes that I had previously added in the driver.
> However, with this patch applied, I am currently seeing only the backlight turning on and no image on the LVDS panel.
> For reference, the LVDS panel used on our platform is G133HAN01.1 and the
> DSI-to-dual-link LVDS bridge is SN65DSI84ZXHR.

Thanks for having tried.

Can you please test with both the fixes in the series applied + the test
pattern feature and report the results you get with and without test
pattern enabled?

The patches to apply are:

 - https://lore.kernel.org/all/20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-1-2e15f5a9a6a0@bootlin.com/
 - https://lore.kernel.org/all/20260226-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v1-2-2e15f5a9a6a0@bootlin.com/
 - https://lore.kernel.org/lkml/20260309-ti-sn65dsi83-dual-lvds-fixes-and-test-pattern-v2-1-e6aaa7e1d181@bootlin.com/

> During our earlier debugging, we went through several trial-and-error
> iterations and also received support from TI. According to TI, when
> operating in dual-link mode the horizontal timing parameters must be
> divided by two before being written to the device. Without this
> adjustment, the panel either does not light up or shows corrupted output.
>
> TI also shared a set of recommended register settings for dual-link mode,
> which were derived using the TI DSI-Tuner tool. These settings helped us
> get the panel working on our hardware during testing.
> For reference, the register configuration suggested by TI is as follows:
>
> 	regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05);
> 	regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> 	regmap_write(ctx->regmap, REG_DSI_CLK, 0x53);
> 	regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f);
> 	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_VERTICAL_BACK_PORCH, 0x00);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_HORIZONTAL_FRONT_PORCH, 0x00);
> 	regmap_write(ctx->regmap,
> 		     REG_VID_CHA_VERTICAL_FRONT_PORCH, 0x00);
>
> If it would help, we can test any proposed changes on our hardware.

The first thing I suggest doing on your side is testing with the 3 patches
mentioned above.

If you display works, good! Let us know (you can also add your Tested-by /
Reviewed-by tags to the test_pattern patch too if applicable).

If it doesn't work, compare the individual register values to find the
differences, try to figure out why the working setting works and how to
apply that change to the driver in away that keeps other boards
working. You're welcome to come back here to discuss it in case you can't
find out on your own.

Hope this helps,
Luca

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

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

* Claude review: drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
  2026-03-12  4:37 [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Sudarshan Shetty
                   ` (2 preceding siblings ...)
  2026-03-12  5:05 ` [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Marek Vasut
@ 2026-03-13  4:32 ` Claude Code Review Bot
  3 siblings, 0 replies; 11+ messages in thread
From: Claude Code Review Bot @ 2026-03-13  4:32 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support
Author: Sudarshan Shetty <tessolveupstream@gmail.com>
Patches: 8
Reviewed: 2026-03-13T14:32:11.443355

---

This series attempts to improve dual-link LVDS support for the TI SN65DSI84 bridge driver by adding a new DT property `ti,dual-link-video-mode` and hardcoding a set of register values when that property is present. The approach has **fundamental design problems**: rather than computing the correct register values from the display mode timing (divided by 2 for dual-link), the patch overwrites previously-programmed registers with magic hardcoded constants from the "TI DSI-Tuner tool." This makes the configuration panel-specific and non-generic, defeating the purpose of a general-purpose driver. The existing driver already detects dual-link via the `lvds_dual_link` flag from the OF graph ports — the cover letter's stated goal of "dividing horizontal timings by 2" would be better achieved by adjusting the existing timing calculations based on `ctx->lvds_dual_link`, not by adding a new boolean property that overwrites everything with fixed values.

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property
  2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
  2026-03-12 15:46   ` Luca Ceresoli
@ 2026-03-13  4:32   ` Claude Code Review Bot
  1 sibling, 0 replies; 11+ messages in thread
From: Claude Code Review Bot @ 2026-03-13  4:32 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Questionable need for the property.** The driver already determines dual-link mode from the OF graph port topology (`ctx->lvds_dual_link`). A separate boolean `ti,dual-link-video-mode` is redundant — if dual-link is already detected, the driver should automatically apply the correct register programming. If the intent is to distinguish between different dual-link sub-modes, the property name and description don't make that clear.

**DT binding description is vague:**
```yaml
+  ti,dual-link-video-mode:
+    type: boolean
+    description: |
+      Enables configuration settings required for correct dual-link
+      LVDS operation. Some panels require the horizontal timing
+      parameters to be adjusted before being programmed into the
+      device. The horizontal timing values must be divided by
+      two when operating in dual-link mode.
```
The description says "some panels require" this — but a boolean property doesn't encode which panels or under what conditions. If all dual-link panels need this, it should be automatic. If only some do, the binding needs to explain what distinguishes those panels.

**No `allOf`/`if-then` constraint.** The property should only be valid for `ti,sn65dsi84` (and possibly `ti,sn65dsi85`), not `ti,sn65dsi83` which only supports single-link. The binding should add a conditional to restrict where this property is allowed.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode
  2026-03-12  4:37 ` [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Sudarshan Shetty
  2026-03-12 15:47   ` Luca Ceresoli
@ 2026-03-13  4:32   ` Claude Code Review Bot
  1 sibling, 0 replies; 11+ messages in thread
From: Claude Code Review Bot @ 2026-03-13  4:32 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch has several serious issues:

**1. Hardcoded magic values overwrite mode-derived calculations.**

The entire block starting at the `if (ctx->dual_link_video_mode)` writes fixed constants to registers that were *already programmed* with values derived from the actual display mode timing just lines above:

```c
+	if (ctx->dual_link_video_mode) {
+		regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05);
+		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
+		regmap_write(ctx->regmap, REG_DSI_CLK, 0x53);
+		regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f);
+		regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
```

These hardcoded values (e.g., `REG_DSI_CLK = 0x53`, `REG_LVDS_FMT = 0x6f`) are specific to a particular panel resolution and pixel clock. They will not work for any other panel. This is essentially dumping a register configuration from a tuning tool into the driver as if it were generic.

**2. Vertical display size zeroed out.**

```c
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00);
```

This overwrites the correctly computed vertical display size (set earlier via `regmap_bulk_write` from `mode->vdisplay`) with zero. Setting the vertical display size to 0 is almost certainly wrong for any panel and contradicts the earlier programming.

**3. Horizontal timing values are also hardcoded, not "divided by 2".**

The cover letter and commit message claim horizontal timing should be divided by 2 for dual-link mode, but the code doesn't divide anything — it writes fixed constants:

```c
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10);
+		regmap_write(ctx->regmap,
+			     REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28);
```

The correct approach would be something like:
```c
hsync_width = (mode->hsync_end - mode->hsync_start) / (ctx->lvds_dual_link ? 2 : 1);
hbp = (mode->htotal - mode->hsync_end) / (ctx->lvds_dual_link ? 2 : 1);
```

**4. `REG_RC_LVDS_PLL` overwritten with `0x05` breaks the PLL clock range calculation.**

Earlier in `sn65dsi83_atomic_pre_enable()`, the driver computes the LVDS PLL register value using `sn65dsi83_get_lvds_range()` which selects the correct clock range for the display mode. Writing `0x05` overwrites this with a fixed clock range that only works for one specific pixel clock.

**5. `REG_RC_PLL_EN` written to `0x00` is redundant and confusing.**

The PLL is already disabled at the start of `sn65dsi83_atomic_pre_enable()` (line 588 in the existing code). Writing it again inside this block, only to then enable it a few lines later outside the block, is confusing but harmless.

**6. DSI mode flags change is too broad.**

```c
+	if (ctx->dual_link_video_mode)
+		dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
+	else
+		dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+				  MIPI_DSI_MODE_VIDEO_BURST | ...
```

Stripping `MIPI_DSI_MODE_VIDEO_BURST`, `MIPI_DSI_MODE_NO_EOT_PACKET`, etc. with no explanation of why dual-link mode requires non-burst DSI video mode. The commit message doesn't justify this change, and it may cause issues with DSI hosts that prefer burst mode.

**7. `of_property_read_bool()` should be `device_property_read_bool()`.**

Modern kernel style prefers `device_property_read_bool(dev, ...)` over `of_property_read_bool(dev->of_node, ...)` for better abstraction:

```c
+	ctx->dual_link_video_mode =
+		of_property_read_bool(dev->of_node, "ti,dual-link-video-mode");
```

**Summary:** The patch as written is a panel-specific hack, not a generic dual-link improvement. The correct fix would modify the existing timing calculations in `sn65dsi83_atomic_pre_enable()` to divide horizontal timing parameters by 2 when `ctx->lvds_dual_link` is already true, without requiring any new DT property or hardcoded register values.

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-03-13  4:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-12  4:37 [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Sudarshan Shetty
2026-03-12  4:37 ` [PATCH v2 1/2] dt-bindings: display: bridge: ti, sn65dsi83: Add dual-link video mode property Sudarshan Shetty
2026-03-12 15:46   ` Luca Ceresoli
2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
2026-03-12  4:37 ` [PATCH v2 2/2] drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Sudarshan Shetty
2026-03-12 15:47   ` Luca Ceresoli
2026-03-13  4:32   ` Claude review: " Claude Code Review Bot
2026-03-12  5:05 ` [PATCH v2 0/2] drm: bridge: ti-sn65dsi83: Improve dual-link LVDS support Marek Vasut
2026-03-12 12:35   ` tessolveupstream
2026-03-12 15:49     ` Luca Ceresoli
2026-03-13  4:32 ` 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