Claude review: dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

Claude review: dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Claude Code Review Bot]

Patch Review

**Status: Minor questions.**

The patch adds `data-lanes` at two levels:

1. **Top-level `data-lanes` (deprecated)**:
```yaml
+  data-lanes:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    deprecated: true
+    minItems: 1
+    maxItems: 4
+    items:
+      maximum: 3
```

2. **Endpoint-level `data-lanes` in `port@1`**:
```yaml
+      port@1:
+        ...
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+                items:
+                  enum: [ 0, 1, 2, 3 ]
```

**Questions/Issues:**

- **Top-level deprecated `data-lanes`**: The cover letter doesn't mention backward compatibility with any existing DTS files that use a top-level `data-lanes`. Is there existing hardware using a non-standard top-level `data-lanes`? If not, adding a deprecated property that was never used seems unnecessary. If there are existing users, the driver patch (3/3) doesn't parse it from the top level at all, only from the endpoint. This should be clarified in the commit message.

- **`enum` vs `maximum`**: The top-level uses `maximum: 3` while the endpoint uses `enum: [0, 1, 2, 3]`. These are functionally equivalent for non-negative integers but the inconsistency is slightly odd. The endpoint style (`enum`) is more explicit and preferred.

- The description change from "Port node with one endpoint connected to a dp-connector node" to "...sink device node" is fine — it's more general and accurate for eDP panels that aren't connector nodes.

---

---
Generated by Claude Code Patch Reviewer

Claude review: dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Claude Code Review Bot]

Patch Review

**Status: Looks reasonable, minor comments**

The change from `$ref: /schemas/graph.yaml#/properties/port` to `$ref: /schemas/graph.yaml#/$defs/port-base` on port@1 is correct — `port-base` is needed to allow extending the port with additional endpoint properties, since `port` has `additionalProperties: false`.

The endpoint data-lanes constraint looks correct:
```yaml
endpoint:
  $ref: /schemas/media/video-interfaces.yaml#
  properties:
    data-lanes:
      minItems: 1
      maxItems: 4
      items:
        enum: [ 0, 1, 2, 3 ]
```

**Minor**: Consider adding `unevaluatedProperties: false` on the endpoint node to prevent unvalidated properties from slipping through. Without it, the schema permits any additional properties on the endpoint beyond what `video-interfaces.yaml` and this local schema define.

The cosmetic description reformatting (multi-line to single-line for port@0 and port@1) is fine but unrelated to the functional change — could be split out for clarity, though not worth blocking over.

---

---
Generated by Claude Code Patch Reviewer

Claude review: dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Claude Code Review Bot]

Patch Review

**Looks good overall.** The binding change correctly switches `port@1` from `$ref: /schemas/graph.yaml#/properties/port` to `$ref: /schemas/graph.yaml#/$defs/port-base` to allow extending the port definition, and adds the `data-lanes` property with appropriate constraints.

Minor observations:

1. **`data-lanes` enum values**: The enum `[ 0, 1, 2, 3 ]` correctly constrains individual lane values to valid physical lane indices. The `minItems: 1` / `maxItems: 4` bounds are correct since the driver code fills in unspecified lanes with the remaining unused ones.

2. **Example only shows 2 lanes**: The example in `rockchip,analogix-dp.yaml` adds `data-lanes = <0 1>;` which is fine to demonstrate partial lane specification, though it uses the default identity mapping. A more illustrative example might have shown a swap (e.g., `<1 0>`), but this is not a functional issue.

3. **`unevaluatedProperties: false` on both port and endpoint**: Good — this was requested in v3 review feedback and correctly prevents unrecognized properties.

No issues requiring changes.

---
Generated by Claude Code Patch Reviewer

[PATCH v5 0/4] Add eDP lane mapping support

[Damon Ding]

This series adds configurable eDP physical lane mapping support via
device tree data-lanes property.

Lane mapping is mainly used for below scenarios:
1. Correct PCB lane swap and differential line routing crossover
   without hardware changes;
2. Adapt mismatched lane pin definitions between SoC and eDP panel;
3. Support multiple panel hardware variants on the same board
   by configuring data-lanes in device tree only.

The series includes driver implementation and device tree binding
updates to support custom lane mapping configuration from endpoint
node, and keeps default linear lane order if no configuration is given.

Patch 1: Add endpoint data-lanes property to analogix-dp binding
Patch 2: Add DRM DP helper API to validate DP lane counts
Patch 3: Add validation for samsung,lane-count property as preparation
Patch 4: Implement lane mapping in analogix_dp driver

Damon Ding (4):
  dt-bindings: display: bridge: analogix-dp: Add data-lanes support for
    endpoint
  drm/dp: Add helper to validate DP lane counts
  drm/bridge: analogix_dp: Add validation for samsung,lane-count
    property
  drm/bridge: analogix_dp: Add support for optional data-lanes mapping

 .../bindings/display/bridge/analogix,dp.yaml  | 19 ++++--
 .../rockchip/rockchip,analogix-dp.yaml        |  1 +
 .../drm/bridge/analogix/analogix_dp_core.c    | 64 ++++++++++++++++++-
 .../drm/bridge/analogix/analogix_dp_core.h    |  4 +-
 .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 15 ++---
 .../gpu/drm/bridge/analogix/analogix_dp_reg.h |  4 ++
 include/drm/display/drm_dp_helper.h           |  6 ++
 7 files changed, 97 insertions(+), 16 deletions(-)

---

Changes in v2:
- Add lane mapping application scenarios in commit message.
- Remove redundant deprecated property 'data-lanes' for eDP node.
- Update port@1 $ref to /schemas/graph.yaml#/$defs/port-base.

Changes in v3:
- Squash [PATCH v2 2/3] into [PATCH v2 1/3].
- Add unevaluatedProperties: false to both the port@1 and endpoint
  nodes.

Changes in v4:
- Add validation for samsung,lane-count property as preparation.

Changes in v5:
- Add DRM DP helper API to validate DP lane counts.
- Apply DRM DP helper API to check the validity of samsung,lane-count
  property.
- Add Acked-by and Reviewed-by tags.

-- 
2.34.1

[PATCH v5 1/4] dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Damon Ding]

Add data-lanes property support to the port@1 endpoint for physical
lane mapping configuration.

Lane mapping is mainly used for below scenarios:
1. Correct PCB lane swap and differential line routing crossover
   without hardware changes;
2. Adapt mismatched lane pin definitions between SoC and eDP panel;
3. Support multiple panel hardware variants on the same board
   by configuring data-lanes in device tree only.

Additionally, add data-lanes setting in Rockchip eDP DT node example
to show actual lane mapping usage.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v2:
- Add lane mapping application scenarios in commit message.
- Remove redundant deprecated property 'data-lanes' for eDP node.
- Update port@1 $ref to /schemas/graph.yaml#/$defs/port-base.

Changes in v3:
- Squash data-lanes adjustment of Rockchip eDP DT example into this
  commit.
- Add unevaluatedProperties: false to both the port@1 and endpoint
  nodes.

Changes in v5
- Add Acked-by and Reviewed-by tags.
---
 .../bindings/display/bridge/analogix,dp.yaml  | 19 ++++++++++++++-----
 .../rockchip/rockchip,analogix-dp.yaml        |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
index 62f0521b0924..ecf206871cdd 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
@@ -42,13 +42,22 @@ properties:
     properties:
       port@0:
         $ref: /schemas/graph.yaml#/properties/port
-        description:
-          Input node to receive pixel data.
+        description: Input node to receive pixel data.
 
       port@1:
-        $ref: /schemas/graph.yaml#/properties/port
-        description:
-          Port node with one endpoint connected to a dp-connector node.
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Port node with one endpoint connected to sink device node.
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+                items:
+                  enum: [ 0, 1, 2, 3 ]
 
     required:
       - port@0
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
index 6548f157fd96..39dafe75a680 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
@@ -160,6 +160,7 @@ examples:
           reg = <1>;
 
           edp_out_panel: endpoint {
+            data-lanes = <0 1>;
             remote-endpoint = <&panel_in_edp>;
           };
         };
-- 
2.34.1

[PATCH v5 2/4] drm/dp: Add helper to validate DP lane counts

[Damon Ding]

Add a generic helper function drm_dp_lane_count_is_valid() to check
if a DisplayPort lane count is valid. According to the DP specification,
only 1, 2, or 4 lanes are supported.

This helper avoids duplicating DP lane count validation logic across
individual DisplayPort drivers.

Suggested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
---
 include/drm/display/drm_dp_helper.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 8c2d77a032f0..c904cb480d84 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -138,6 +138,12 @@ bool drm_dp_as_sdp_supported(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_C
 
 int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
 
+static inline bool
+drm_dp_lane_count_is_valid(int lane_count)
+{
+	return lane_count == 1 || lane_count == 2 || lane_count == 4;
+}
+
 static inline int
 drm_dp_max_link_rate(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
-- 
2.34.1

[PATCH v5 3/4] drm/bridge: analogix_dp: Add validation for samsung, lane-count property

[Damon Ding]

Add validity check for samsung,lane-count to ensure DT-provided
lane count values are specification-compliant.

Suggested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v5:
- Apply DRM DP helper API to check the validity of DP lane count.
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 5dc07ff84cd3..9300b0db8785 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1238,6 +1238,7 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 {
 	struct device_node *dp_node = dp->dev->of_node;
 	struct video_info *video_info = &dp->video_info;
+	int ret;
 
 	switch (dp->plat_data->dev_type) {
 	case RK3288_DP:
@@ -1261,8 +1262,11 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 		 */
 		of_property_read_u32(dp_node, "samsung,link-rate",
 				     &video_info->max_link_rate);
-		of_property_read_u32(dp_node, "samsung,lane-count",
-				     &video_info->max_lane_count);
+		ret = of_property_read_u32(dp_node, "samsung,lane-count",
+					   &video_info->max_lane_count);
+		if (ret || !drm_dp_lane_count_is_valid(video_info->max_lane_count))
+			return dev_err_probe(dp->dev, ret ? ret : -EINVAL,
+					     "failed to parse samsung,lane-count\n");
 		break;
 	}
 
-- 
2.34.1

[PATCH v5 4/4] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[Damon Ding]

Parse the optional 'data-lanes' device tree property to support
custom physical lane mapping configuration.

If no valid configuration is found, fall back to the default
lane map (0, 1, 2, 3) automatically and keep the driver running.

Lane mapping is mainly used for below scenarios:
1. Correct PCB lane swap and differential line routing crossover
   without hardware changes;
2. Adapt mismatched lane pin definitions between SoC and eDP panel;
3. Support multiple panel hardware variants on the same board
   by configuring data-lanes in device tree only.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v2:
- Add lane mapping application scenarios in commit message.

Changes in v5:
- Add Reviewed-by tag.
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 56 +++++++++++++++++++
 .../drm/bridge/analogix/analogix_dp_core.h    |  4 +-
 .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 15 +++--
 .../gpu/drm/bridge/analogix/analogix_dp_reg.h |  4 ++
 4 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 9300b0db8785..ea525aa3effd 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
 	.detect = analogix_dp_bridge_detect,
 };
 
+static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp)
+{
+	struct video_info *video_info = &dp->video_info;
+	struct device_node *endpoint;
+	u32 tmp[LANE_COUNT4];
+	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
+	bool used[LANE_COUNT4] = {false};
+	int num_lanes;
+	int ret, i;
+
+	memcpy(video_info->lane_map, map, sizeof(map));
+
+	num_lanes = drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1,
+						   video_info->max_lane_count);
+	if (num_lanes < 0)
+		return -EINVAL;
+
+	endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);
+	if (!endpoint)
+		return -EINVAL;
+
+	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);
+	of_node_put(endpoint);
+	if (ret)
+		return -EINVAL;
+
+	for (i = 0; i < num_lanes; i++) {
+		if (tmp[i] >= LANE_COUNT4) {
+			dev_dbg(dp->dev, "data-lanes[%d] = %u is out of range\n", i, tmp[i]);
+			return -EINVAL;
+		}
+
+		if (used[tmp[i]]) {
+			dev_dbg(dp->dev, "data-lanes[%d] = %u is duplicate\n", i, tmp[i]);
+			return -EINVAL;
+		}
+
+		used[tmp[i]] = true;
+		map[i] = tmp[i];
+	}
+
+	for (i = 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) {
+		if (!used[i])
+			map[num_lanes++] = i;
+	}
+
+	dev_dbg(dp->dev, "Using parsed lane map: <%u %u %u %u>\n", map[0], map[1], map[2], map[3]);
+
+	memcpy(video_info->lane_map, map, sizeof(map));
+
+	return 0;
+}
+
 static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 {
 	struct device_node *dp_node = dp->dev->of_node;
@@ -1270,6 +1323,9 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 		break;
 	}
 
+	if (analogix_dp_dt_parse_lanes_map(dp))
+		dev_dbg(dp->dev, "No valid data-lanes found, using default lane map\n");
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 94348c4e3623..a75d0fb8f980 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -137,6 +137,8 @@ struct video_info {
 
 	int max_link_rate;
 	enum link_lane_count_type max_lane_count;
+
+	u32 lane_map[LANE_COUNT4];
 };
 
 struct link_train {
@@ -175,7 +177,7 @@ struct analogix_dp_device {
 /* analogix_dp_reg.c */
 void analogix_dp_enable_video_mute(struct analogix_dp_device *dp, bool enable);
 void analogix_dp_stop_video(struct analogix_dp_device *dp);
-void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable);
+void analogix_dp_lane_mapping(struct analogix_dp_device *dp);
 void analogix_dp_init_analog_param(struct analogix_dp_device *dp);
 void analogix_dp_init_interrupt(struct analogix_dp_device *dp);
 void analogix_dp_reset(struct analogix_dp_device *dp);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index ea8401293a23..c1344a3f013a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
 }
 
-void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
+void analogix_dp_lane_mapping(struct analogix_dp_device *dp)
 {
+	u32 *lane_map = dp->video_info.lane_map;
 	u32 reg;
 
-	if (enable)
-		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
-		      LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
-	else
-		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
-		      LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
+	reg = lane_map[0] << LANE0_MAP_SHIFT;
+	reg |= lane_map[1] << LANE1_MAP_SHIFT;
+	reg |= lane_map[2] << LANE2_MAP_SHIFT;
+	reg |= lane_map[3] << LANE3_MAP_SHIFT;
 
 	writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
 }
@@ -140,7 +139,7 @@ void analogix_dp_reset(struct analogix_dp_device *dp)
 
 	usleep_range(20, 30);
 
-	analogix_dp_lane_swap(dp, 0);
+	analogix_dp_lane_mapping(dp);
 
 	writel(0x0, dp->reg_base + ANALOGIX_DP_SYS_CTL_1);
 	writel(0x40, dp->reg_base + ANALOGIX_DP_SYS_CTL_2);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
index 12735139046c..ac914e37089b 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
@@ -209,6 +209,10 @@
 #define LANE0_MAP_LOGIC_LANE_1			(0x1 << 0)
 #define LANE0_MAP_LOGIC_LANE_2			(0x2 << 0)
 #define LANE0_MAP_LOGIC_LANE_3			(0x3 << 0)
+#define LANE3_MAP_SHIFT				(6)
+#define LANE2_MAP_SHIFT				(4)
+#define LANE1_MAP_SHIFT				(2)
+#define LANE0_MAP_SHIFT				(0)
 
 /* ANALOGIX_DP_ANALOG_CTL_1 */
 #define TX_TERMINAL_CTRL_50_OHM			(0x1 << 4)
-- 
2.34.1

Re: [PATCH v5 4/4] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The logic-to-physical lane mapping is mathematically inverted when programming the hardware, breaking support for asymmetric lane swap configurations.
- [Medium] Inconsistent endpoint resolution when reading the data-lanes property.
--

commit b6abf6d746df1b04ca60b36cd37e77da204d915a
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Add support for optional data-lanes mapping
    
Parse the optional 'data-lanes' device tree property to support
custom physical lane mapping configuration. If no valid configuration
is found, fall back to the default lane map automatically.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index f23c2ae6e10d0..2dfe7de5d3101 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {

[ ... ]

> +static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp)
> +{
> +	struct video_info *video_info = &dp->video_info;
> +	struct device_node *endpoint;
> +	u32 tmp[LANE_COUNT4];
> +	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
> +	bool used[LANE_COUNT4] = {false};
> +	int num_lanes;
> +	int ret, i;
> +
> +	memcpy(video_info->lane_map, map, sizeof(map));
> +
> +	num_lanes = drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1,
> +						   video_info->max_lane_count);
> +	if (num_lanes < 0)
> +		return -EINVAL;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);

[Severity: Medium]
Will this result in reading from an inconsistent endpoint if port 1 has
multiple endpoints?

The call to drm_of_get_data_lanes_count_ep() above explicitly passes 0
for the endpoint reg, but this call uses -1. If there are multiple
endpoints, this could return the first one it finds rather than endpoint@0,
which might lead to reading the lane map from a different endpoint than
the one used for the count.

> +	if (!endpoint)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);

[ ... ]

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 38fd8d5014d29..45c7652645a13 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
>  	writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>  }
>  
> -void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
> +void analogix_dp_lane_mapping(struct analogix_dp_device *dp)
>  {
> +	u32 *lane_map = dp->video_info.lane_map;
>  	u32 reg;
>  
> -	if (enable)
> -		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
> -		      LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
> -	else
> -		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
> -		      LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> +	reg = lane_map[0] << LANE0_MAP_SHIFT;
> +	reg |= lane_map[1] << LANE1_MAP_SHIFT;
> +	reg |= lane_map[2] << LANE2_MAP_SHIFT;
> +	reg |= lane_map[3] << LANE3_MAP_SHIFT;

[Severity: High]
Does this logic mathematically invert the lane mapping for asymmetric
configurations?

The device tree data-lanes property specifies a mapping from logical to
physical lanes, so that lane_map[logical_lane] = physical_lane.
However, when programming the hardware, it expects the logical lane index
to be placed into the bit-field corresponding to the physical lane.

Here, the code shifts the physical lane number by the logical lane's
bit-shift, effectively programming the logical lane into the physical lane's
field. This happens to work by coincidence for symmetric mappings like
<3 2 1 0> or <0 1 2 3>, but it will corrupt asymmetric configurations
like <1 2 3 0>.

Should the logical lane index instead be shifted into the physical lane's
bit-field? For example, using reg |= i << (lane_map[i] * 2).

>  
>  	writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604085220.2862986-1-damon.ding@rock-chips.com?part=4

Re: [PATCH v5 1/4] dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `data-lanes` property is defined strictly for the `endpoint` node name, causing schema validation to fail or miss constraints if applied to valid unit-addressed nodes like `endpoint@0`.
--

commit c6792a12ea4c785878c85982c6a4537b6a1901d6
Author: Damon Ding <damon.ding@rock-chips.com>

dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint
    
This commit adds data-lanes property support to the port@1 endpoint in
the analogix-dp bridge binding to allow physical lane mapping configuration.
It also updates the Rockchip eDP DT node example to demonstrate its usage.

> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> index 62f0521b09245..ecf206871cdde 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> @@ -42,13 +42,22 @@ properties:
>      properties:
>        port@0:

[ ... ]

>        port@1:
> -        $ref: /schemas/graph.yaml#/properties/port
> -        description:
> -          Port node with one endpoint connected to a dp-connector node.
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: Port node with one endpoint connected to sink device node.
> +        properties:
> +          endpoint:
             ^^^^^^^^
[Severity: Medium]
Does restricting this to the exact node name 'endpoint' cause validation
issues for unit-addressed nodes?

Because JSON Schema strictly matches object keys, a node named endpoint@0
under port@1 will bypass this block, meaning the video-interfaces $ref
and data-lanes validation constraints are never applied.

Should this use patternProperties: "^endpoint(@[0-9a-f]+)?$": instead
of properties: endpoint: to ensure unit-addressed endpoint nodes don't
bypass these constraints?

> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +                items:
> +                  enum: [ 0, 1, 2, 3 ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604085220.2862986-1-damon.ding@rock-chips.com?part=1

Re: [PATCH v5 3/4] drm/bridge: analogix_dp: Add validation for samsung,lane-count property

[sashiko-bot]

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

New issues:
- [High] The patch makes the deprecated and optional `samsung,lane-count` property a strict requirement, causing an unconditional probe failure if it is omitted.

Pre-existing issues:
- [High] Missing handling for absent DT properties causes lane count and link rate to be incorrectly clamped to 0, breaking link training.
--

commit 65a66d32582ccfce55e0e84f53bc63077d25103c
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Add validation for samsung,lane-count property

Add validity check for samsung,lane-count to ensure DT-provided
lane count values are specification-compliant.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 8cf6b73bceac0..f23c2ae6e10d0 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
[ ... ]
> @@ -1260,8 +1261,11 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>  		 */
>  		of_property_read_u32(dp_node, "samsung,link-rate",
>  				     &video_info->max_link_rate);
> -		of_property_read_u32(dp_node, "samsung,lane-count",
> -				     &video_info->max_lane_count);
> +		ret = of_property_read_u32(dp_node, "samsung,lane-count",
> +					   &video_info->max_lane_count);
> +		if (ret || !drm_dp_lane_count_is_valid(video_info->max_lane_count))
> +			return dev_err_probe(dp->dev, ret ? ret : -EINVAL,
> +					     "failed to parse samsung,lane-count\n");

[Severity: High]
Does this make the optional and deprecated samsung,lane-count property a
strict requirement?

If samsung,lane-count is absent from the device tree, of_property_read_u32()
returns -EINVAL. This causes the condition to evaluate to true, aborting the
probe with an error.

According to the device tree bindings
(Documentation/devicetree/bindings/display/samsung/samsung,exynos5-dp.yaml),
this property is marked as deprecated and explicitly optional because the
lane count can be read from the monitor. Does this patch break compatibility
with device trees that rightfully omit this deprecated property?

[Severity: High]
This is a pre-existing issue, but there is an underlying problem in
analogix_dp_full_link_train() when these optional properties are omitted:

analogix_dp_full_link_train() {
    ...
	/* Setup TX lane count & rate */
	if (dp->link_train.lane_count > max_lanes)
		dp->link_train.lane_count = max_lanes;
	if (dp->link_train.link_rate > max_rate)
		dp->link_train.link_rate = max_rate;
    ...
}

If the deprecated samsung,lane-count or samsung,link-rate properties are
rightfully omitted from the device tree, their corresponding limits remain 0.

The driver then executes the above clamp logic, which incorrectly clamps the
valid DPCD values to 0. This completely breaks DP link training and results
in a non-functional display.

Should the driver ignore the limits if they are 0 (e.g., checking if
max_lanes is non-zero before clamping), allowing it to correctly fall back
to the monitor's DPCD capabilities? Fixing the probe failure above will
just expose this runtime breakage.

>  		break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604085220.2862986-1-damon.ding@rock-chips.com?part=3

Re: [PATCH v5 1/4] dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Rob Herring]

On Thu, Jun 04, 2026 at 09:06:37AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The `data-lanes` property is defined strictly for the `endpoint` node name, causing schema validation to fail or miss constraints if applied to valid unit-addressed nodes like `endpoint@0`.
> --
> 
> commit c6792a12ea4c785878c85982c6a4537b6a1901d6
> Author: Damon Ding <damon.ding@rock-chips.com>
> 
> dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint
>     
> This commit adds data-lanes property support to the port@1 endpoint in
> the analogix-dp bridge binding to allow physical lane mapping configuration.
> It also updates the Rockchip eDP DT node example to demonstrate its usage.
> 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> > index 62f0521b09245..ecf206871cdde 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,dp.yaml
> > @@ -42,13 +42,22 @@ properties:
> >      properties:
> >        port@0:
> 
> [ ... ]
> 
> >        port@1:
> > -        $ref: /schemas/graph.yaml#/properties/port
> > -        description:
> > -          Port node with one endpoint connected to a dp-connector node.
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        unevaluatedProperties: false
> > +        description: Port node with one endpoint connected to sink device node.
> > +        properties:
> > +          endpoint:
>              ^^^^^^^^
> [Severity: Medium]
> Does restricting this to the exact node name 'endpoint' cause validation
> issues for unit-addressed nodes?
> 
> Because JSON Schema strictly matches object keys, a node named endpoint@0
> under port@1 will bypass this block, meaning the video-interfaces $ref
> and data-lanes validation constraints are never applied.

That could be an issue...

> Should this use patternProperties: "^endpoint(@[0-9a-f]+)?$": instead
> of properties: endpoint: to ensure unit-addressed endpoint nodes don't
> bypass these constraints?

But no, this is fine as-is because it defines that only a single 
endpoint is valid and we'd need to fix everywhere as this is a common 
pattern.

Rob

Claude review: Add eDP lane mapping support

[Claude Code Review Bot]

Overall Series Review

Subject: Add eDP lane mapping support
Author: Damon Ding <damon.ding@rock-chips.com>
Patches: 9
Reviewed: 2026-06-05T06:34:42.550041

---

This is a well-structured 4-patch series adding configurable eDP physical lane mapping via the `data-lanes` device tree property for the analogix_dp bridge driver. The series is cleanly organized: DT bindings first, then a shared helper, then a preparatory validation patch, and finally the feature implementation.

The overall approach is sound. The code is clean, the DT binding changes are reasonable, and the lane mapping logic is correct. There are a few issues worth flagging, primarily around potential regression risk in patch 3 and some minor cleanups in patch 4.

The series has appropriate Reviewed-by and Acked-by tags for the DT binding and implementation patches.

---
Generated by Claude Code Patch Reviewer

Claude review: dt-bindings: display: bridge: analogix-dp: Add data-lanes support for endpoint

[Claude Code Review Bot]

Patch Review

**Status: Looks good**

The binding change is straightforward. The `port@1` is updated from `$ref: /schemas/graph.yaml#/properties/port` to `$ref: /schemas/graph.yaml#/$defs/port-base` with `unevaluatedProperties: false`, which is the correct pattern for extending a port with endpoint properties.

The `data-lanes` property is appropriately constrained:
```yaml
data-lanes:
  minItems: 1
  maxItems: 4
  items:
    enum: [ 0, 1, 2, 3 ]
```

The Rockchip example addition of `data-lanes = <0 1>;` is a nice touch showing a 2-lane configuration.

No issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/dp: Add helper to validate DP lane counts

[Claude Code Review Bot]

Patch Review

**Status: Looks good, minor note**

The helper is clean and correct:
```c
static inline bool
drm_dp_lane_count_is_valid(int lane_count)
{
	return lane_count == 1 || lane_count == 2 || lane_count == 4;
}
```

This correctly reflects the DP spec (1, 2, or 4 lanes only). Using a `static inline` in the header is appropriate for such a trivial function.

One minor observation: there's no `drm_dp_lane_count_is_valid` user beyond this series. It might be worth mentioning in the commit message that other drivers could adopt it to replace ad-hoc checks, but that's a commit message nit, not a code issue.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: analogix_dp: Add validation for samsung, lane-count property

[Claude Code Review Bot]

Patch Review

**Status: Concern — potential regression for existing DTs without samsung,lane-count**

The patch changes the `EXYNOS_DP` case from silently tolerating a missing `samsung,lane-count` to hard-failing with `dev_err_probe`:

```c
ret = of_property_read_u32(dp_node, "samsung,lane-count",
			   &video_info->max_lane_count);
if (ret || !drm_dp_lane_count_is_valid(video_info->max_lane_count))
	return dev_err_probe(dp->dev, ret ? ret : -EINVAL,
			     "failed to parse samsung,lane-count\n");
```

**Issue: `max_lane_count` is zero-initialized** (via `devm_drm_bridge_alloc` which zeroes the struct). Previously, if `samsung,lane-count` was missing, `max_lane_count` stayed 0 and the driver would continue (perhaps not ideally, but it wouldn't fail probe). Now it's a hard probe failure. While I confirmed all in-tree Exynos DTs have `samsung,lane-count`, there could be out-of-tree DTs that relied on the old permissive behavior. This is a kernel regression risk.

Additionally, `samsung,link-rate` is NOT validated in the same way — it's still silently read with no error check. This asymmetry is a bit inconsistent. If `samsung,lane-count` is truly mandatory, perhaps both should be validated; if the intent is defensive validation, perhaps a `dev_warn` + fallback to a sane default (like 4 lanes) would be safer than a hard failure.

Consider either:
1. Making this a warning with fallback rather than a hard probe error, or
2. Also validating `samsung,link-rate` for consistency.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[Claude Code Review Bot]

Patch Review

**Status: Mostly good, a few observations**

The overall implementation is correct. The parsing logic properly validates lane values and checks for duplicates. The fill-in logic for unused lanes when fewer than 4 data-lanes are specified is reasonable:

```c
for (i = 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) {
	if (!used[i])
		map[num_lanes++] = i;
}
```

The `analogix_dp_lane_mapping()` register write using shift constants is cleaner than the old hardcoded approach.

**Observations:**

1. **Double endpoint lookup**: `analogix_dp_dt_parse_lanes_map()` first calls `drm_of_get_data_lanes_count_ep()` to get the count, then calls `of_graph_get_endpoint_by_regs()` + `of_property_read_u32_array()` to read the values. `drm_of_get_data_lanes_count_ep` already looks up the endpoint internally and reads `data-lanes`. Consider whether it's possible to just do a single endpoint lookup and read `data-lanes` directly, getting both count and values in one pass. This would also avoid the subtle inconsistency that `drm_of_get_data_lanes_count_ep` uses port_reg=1,reg=0 while `of_graph_get_endpoint_by_regs` uses port=1,reg=-1 (first endpoint).

2. **Error return on missing data-lanes**: `drm_of_get_data_lanes_count_ep` returns a negative error when no `data-lanes` property exists. The function returns `-EINVAL` in this case, which is then silently caught by the caller:
   ```c
   if (analogix_dp_dt_parse_lanes_map(dp))
       dev_dbg(dp->dev, "No valid data-lanes found, using default lane map\n");
   ```
   The initial `memcpy(video_info->lane_map, map, sizeof(map))` at the top of the function ensures the default `{0, 1, 2, 3}` is always set, which is correct.

3. **Parentheses in shift defines**: Minor style nit — the new defines use unnecessary parentheses around the values:
   ```c
   #define LANE3_MAP_SHIFT				(6)
   #define LANE2_MAP_SHIFT				(4)
   ```
   While not wrong, this is inconsistent with many other shift defines in the same file that don't use parentheses (e.g., the existing `LANE*_MAP_LOGIC_LANE_*` defines use `(0x1 << 6)` syntax where the parens wrap the expression, not bare integers). Very minor.

4. **`LANE_COUNT4` as array size**: Using the enum value `LANE_COUNT4 = 4` as an array dimension and loop bound works since its value is 4, but it mixes enum semantics (a lane-count option) with "number of DP lanes total." A named constant like `DP_MAX_LANES` or `ANALOGIX_DP_MAX_LANES` would be self-documenting, though this is a pre-existing pattern in the codebase.

The register-level change from `analogix_dp_lane_swap()` to `analogix_dp_lane_mapping()` is clean — the old hardcoded swap/no-swap is replaced by a generic mapping write that produces the same result for the default `{0,1,2,3}` map.

---
Generated by Claude Code Patch Reviewer