From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: analogix_dp: Add support for optional data-lanes mapping Date: Mon, 25 May 2026 20:15:52 +1000 Message-ID: In-Reply-To: <20260521114459.1394264-4-damon.ding@rock-chips.com> References: <20260521114459.1394264-1-damon.ding@rock-chips.com> <20260521114459.1394264-4-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 **Status: Needs revision =E2=80=94 potential mapping inversion bug and mino= r issues** #### 1. Potential lane mapping semantic bug (important) The `data-lanes` DT standard (from `video-interfaces.yaml`) defines: **posi= tion =3D logical lane index, value =3D physical lane index**. So `data-lane= s =3D <1 2 3 0>` means logical lane 0 is on physical lane 1, etc. The register `ANALOGIX_DP_LANE_MAP` maps in the opposite direction: `LANE_N= _MAP` specifies which **logical** lane **physical** lane N carries. The code writes the `data-lanes` array directly into the register: ```c reg =3D lane_map[0] << LANE0_MAP_SHIFT; reg |=3D lane_map[1] << LANE1_MAP_SHIFT; reg |=3D lane_map[2] << LANE2_MAP_SHIFT; reg |=3D lane_map[3] << LANE3_MAP_SHIFT; ``` This treats `data-lanes[physical] =3D logical`, but the standard says `data= -lanes[logical] =3D physical`. These interpretations yield the same result = only for self-inverse permutations (involutions) like `<1 0 3 2>` or `<3 2 = 1 0>`. For a permutation like `<1 2 3 0>`, the mapping would be wrong. For example, `data-lanes =3D <1 2 3 0>` means: - Logical 0 =E2=86=92 Physical 1, Logical 1 =E2=86=92 Physical 2, Logical 2= =E2=86=92 Physical 3, Logical 3 =E2=86=92 Physical 0 The correct register values should be the **inverse** permutation: - Physical 0 carries Logical 3 =E2=86=92 LANE0_MAP =3D 3 - Physical 1 carries Logical 0 =E2=86=92 LANE1_MAP =3D 0 But the code writes LANE0_MAP =3D 1, LANE1_MAP =3D 2, which is incorrect. **Recommendation**: Either compute the inverse permutation when programming= the register, or document clearly in the binding that this driver interpre= ts `data-lanes` as physical-to-logical (non-standard). The former is prefer= red for DT convention compliance. #### 2. Redundant endpoint lookup The endpoint node is fetched twice =E2=80=94 once inside `drm_of_get_data_l= anes_count_ep()` and once explicitly: ```c num_lanes =3D drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, 1, video_info->max_lane_count); ... endpoint =3D of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1); ``` Consider using `of_graph_get_endpoint_by_regs()` once, then calling `drm_of= _get_data_lanes_count()` (the non-`_ep` variant) on the resulting endpoint = node directly, reading the property from the same node. #### 3. Inconsistent endpoint reg parameter The first call uses `reg=3D0`: ```c drm_of_get_data_lanes_count_ep(dp->dev->of_node, 1, 0, ...) ``` The second uses `reg=3D-1` (wildcard): ```c of_graph_get_endpoint_by_regs(dp->dev->of_node, 1, -1) ``` If there were multiple endpoints under port@1, these could find different e= ndpoints. Use the same value in both calls to ensure consistency. #### 4. Early default initialization is partially redundant ```c u32 map[LANE_COUNT4] =3D {0, 1, 2, 3}; ... memcpy(video_info->lane_map, map, sizeof(map)); ``` The `memcpy` at line 230 sets the default immediately, which is good for th= e early-return paths. But the `map` array itself is initialized at declarat= ion with the same values, so this is fine functionally =E2=80=94 just sligh= tly redundant since the two copies serve the same purpose. #### 5. Minor style: unnecessary parentheses on shift defines ```c #define LANE3_MAP_SHIFT (6) #define LANE2_MAP_SHIFT (4) ``` Kernel coding style doesn't require parentheses around simple integer const= ants. Consider `#define LANE3_MAP_SHIFT 6` for consistency with the existin= g defines in the same file (e.g., `(0x0 << 6)` uses parentheses around expr= essions, not bare constants). #### 6. The rename from `analogix_dp_lane_swap` to `analogix_dp_lane_mappin= g` is clean The old boolean `enable` parameter is replaced by reading from `dp->video_i= nfo.lane_map`, and the single call site in `analogix_dp_reset()` is updated= . No external callers exist. This is a clean API evolution. --- Generated by Claude Code Patch Reviewer