From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: analogix_dp: Add support for optional data-lanes mapping
Date: Tue, 26 May 2026 07:20:31 +1000
Message-ID: <review-patch2-20260525094731.121380-3-damon.ding@rock-chips.com>
In-Reply-To: <20260525094731.121380-3-damon.ding@rock-chips.com>
References: <20260525094731.121380-1-damon.ding@rock-chips.com> <20260525094731.121380-3-damon.ding@rock-chips.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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_endp=
oint_by_regs()` + `of_node_put()`, and then the function immediately does t=
he same `of_graph_get_endpoint_by_regs()` call again to read the actual `da=
ta-lanes` values:

    ```c
    num_lanes =3D 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 =3D 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 th=
e 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 use=
s 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=3D0 in port 1". If the endpoint doesn't have `reg =3D <0>` explicitly,=
 these could potentially find different endpoints, or `_count_ep` could fai=
l while `get_endpoint_by_regs` succeeds. This should be made consistent =E2=
=80=94 either use `0` for both or `-1` for both (the `_count_ep` variant do=
esn'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` pr=
operty exists in DT, `drm_of_get_data_lanes_count_ep()` returns `-ENODEV` (=
property missing). The function returns `-EINVAL` for this case, and the ca=
ller 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 vi=
a `memcpy` at the top of the function before any error paths. However, the =
function returns `-EINVAL` for all error cases (missing property, invalid v=
alues, missing endpoint), which is slightly imprecise =E2=80=94 returning t=
he 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 a=
re specified, the remaining slots are filled with unused physical lane indi=
ces in ascending order:

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

    This is reasonable behavior, but worth documenting somewhere (commit me=
ssage or binding doc) what the semantics are for partial lane specification=
s. For example, `data-lanes =3D <1 0>` means lanes 0=E2=86=921, 1=E2=86=920=
, 2=E2=86=922, 3=E2=86=923. Is this the intended hardware behavior? The com=
mit message says "fall back to the default lane map" for absent configurati=
on but doesn't describe partial specification behavior.

5. **New shift defines have unnecessary parentheses**: Minor style nit =E2=
=80=94 the kernel style generally doesn't parenthesize simple integer const=
ants:

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

    These should be just `6`, `4`, `2`, `0` without parentheses, matching o=
ther simple constant defines in the same file. The existing `LANEx_MAP_LOGI=
C_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 boole=
an swap API is replaced with the general mapping API. The only caller was `=
analogix_dp_reset()` which called it with `enable=3D0` (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