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

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

Patch Review

**Generally good, but has a few issues:**

1. **Redundant endpoint lookup**: `analogix_dp_dt_parse_lanes_map()` calls `drm_of_get_data_lanes_count_ep()` which internally does `of_graph_get_endpoint_by_regs()` + `of_node_put()`, and then the function immediately does the same `of_graph_get_endpoint_by_regs()` call again to read the actual `data-lanes` values:

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

    endpoint = of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1);
    ```

    This looks up the endpoint twice. Consider using `drm_of_get_data_lanes_count()` (the non-`_ep` variant) on the already-obtained endpoint node, or simply use `of_property_count_u32_elems()` + manual validation to avoid the double lookup. It's not a bug, but it's unnecessary work.

2. **Inconsistent endpoint reg parameter**: The first lookup uses reg `0` (via `drm_of_get_data_lanes_count_ep(..., 1, 0, ...)`), while the second uses reg `-1` (via `of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1)`). Reg `-1` means "any endpoint in port 1", while reg `0` means "endpoint with reg=0 in port 1". If the endpoint doesn't have `reg = <0>` explicitly, these could potentially find different endpoints, or `_count_ep` could fail while `get_endpoint_by_regs` succeeds. This should be made consistent — either use `0` for both or `-1` for both (the `_count_ep` variant doesn't accept `-1`, so the proper fix would be to get the endpoint once with `-1` and then call `drm_of_get_data_lanes_count()` on it directly).

3. **Error semantics when `data-lanes` is absent**: When no `data-lanes` property exists in DT, `drm_of_get_data_lanes_count_ep()` returns `-ENODEV` (property missing). The function returns `-EINVAL` for this case, and the caller prints a debug message:

    ```c
    if (analogix_dp_dt_parse_lanes_map(dp))
        dev_dbg(dp->dev, "No valid data-lanes found, using default lane map\n");
    ```

    This works correctly since the default map `{0, 1, 2, 3}` is written via `memcpy` at the top of the function before any error paths. However, the function returns `-EINVAL` for all error cases (missing property, invalid values, missing endpoint), which is slightly imprecise — returning the actual error code from the helpers would be more idiomatic, even though the caller doesn't distinguish them.

4. **Lane fill logic for partial specification**: When fewer than 4 lanes are specified, the remaining slots are filled with unused physical lane indices in ascending order:

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

    This is reasonable behavior, but worth documenting somewhere (commit message or binding doc) what the semantics are for partial lane specifications. For example, `data-lanes = <1 0>` means lanes 0→1, 1→0, 2→2, 3→3. Is this the intended hardware behavior? The commit message says "fall back to the default lane map" for absent configuration but doesn't describe partial specification behavior.

5. **New shift defines have unnecessary parentheses**: Minor style nit — the kernel style generally doesn't parenthesize simple integer constants:

    ```c
    #define LANE3_MAP_SHIFT				(6)
    #define LANE2_MAP_SHIFT				(4)
    ```

    These should be just `6`, `4`, `2`, `0` without parentheses, matching other simple constant defines in the same file. The existing `LANEx_MAP_LOGIC_LANE_y` defines use parentheses around the shift *expression* `(0x1 << 6)` which is different and appropriate.

6. **The `analogix_dp_lane_swap` function removal is clean**: The old boolean swap API is replaced with the general mapping API. The only caller was `analogix_dp_reset()` which called it with `enable=0` (identity mapping), so this is a correct generalization. The function rename from `analogix_dp_lane_swap` to `analogix_dp_lane_mapping` is appropriate.

---
Generated by Claude Code Patch Reviewer