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

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Status: Has issues to address.**

**1. Error handling in `analogix_dp_dt_parse_lanes_map` returns `-EINVAL` for non-error conditions:**

```c
+	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;
```

`drm_of_get_data_lanes_count_ep` returns `-ENODEV` when the `data-lanes` property is simply absent, which is the normal case for most boards. Returning `-EINVAL` here loses the distinction between "property not present" (normal) and "property present but invalid" (an error). 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");
```

...treats all failures the same — silently falling back. This means a genuinely malformed `data-lanes` property (e.g., `data-lanes = <0 0 1 1>;` with duplicates, or `data-lanes = <5>;` with out-of-range values) is silently ignored and falls back to the default. This is a **data integrity concern**: a board designer who configures lane mapping but makes a typo will get no indication that their configuration was ignored.

**Suggestion**: Return the actual error. In the caller, only fall back silently on `-ENODEV`; propagate or at minimum `dev_warn()` on other errors so that DT mistakes are visible.

**2. Redundant `memcpy` — the default map is written twice on the success path:**

```c
+	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
+	...
+	memcpy(video_info->lane_map, map, sizeof(map));  // <-- sets defaults
+	...
+	// parse and modify map[]
+	...
+	memcpy(video_info->lane_map, map, sizeof(map));  // <-- sets final
```

The first `memcpy` at the start is only needed for the early-return error paths. Consider moving the initial default write to the caller, or just removing the first `memcpy` and only doing it in the error path. Minor style nit.

**3. `LANE_COUNT4` is an enum value of `4`, not a `#define`:**

```c
+	u32 tmp[LANE_COUNT4];
+	u32 map[LANE_COUNT4] = {0, 1, 2, 3};
+	bool used[LANE_COUNT4] = {false};
```

Using an enum value as an array size is technically fine in C (it's a compile-time constant), but `LANE_COUNT4 = 4` is semantically about link lane count negotiation, not "the fixed number of physical lanes on the chip." A `#define ANALOGIX_DP_MAX_LANES 4` would be clearer in intent, but this is a minor style preference. The existing codebase already uses `LANE_COUNT4` in some places as a max, so this is consistent.

**4. The fill-remaining-lanes logic:**

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

This fills in unmapped physical lanes beyond what `data-lanes` specifies. For example, if `data-lanes = <2 3>`, the final map becomes `{2, 3, 0, 1}`. This behavior seems reasonable for 2-lane configurations where lanes 2 and 3 are unused but the register needs all 4 entries. However, it's not documented anywhere — consider adding a brief comment or noting this in the commit message.

**5. Register shift defines — minor style:**

```c
+#define LANE3_MAP_SHIFT				(6)
+#define LANE2_MAP_SHIFT				(4)
+#define LANE1_MAP_SHIFT				(2)
+#define LANE0_MAP_SHIFT				(0)
```

Parentheses around single integer constants are unnecessary per kernel coding style (though this matches the existing defines above like `(0x1 << 6)`). Very minor.

**6. The refactoring from `analogix_dp_lane_swap(dp, bool enable)` to `analogix_dp_lane_mapping(dp)` is clean:**

The old function only supported two states (identity or full reversal). The new function reads from `video_info->lane_map` which supports arbitrary permutations. The only caller was `analogix_dp_reset()` which always passed `enable=0`, so the "swap" functionality was never actually used. The refactoring is safe.

**Summary of patch 3 action items:**
- Differentiate between missing `data-lanes` (normal, silent) and malformed `data-lanes` (should warn)
- Consider removing the redundant first `memcpy`
- Document or mention the fill-remaining-lanes behavior

---
Generated by Claude Code Patch Reviewer