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: Fri, 05 Jun 2026 06:34:43 +1000
Message-ID: <review-patch4-20260604085220.2862986-5-damon.ding@rock-chips.com>
In-Reply-To: <20260604085220.2862986-5-damon.ding@rock-chips.com>
References: <20260604085220.2862986-1-damon.ding@rock-chips.com> <20260604085220.2862986-5-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: 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 =3D 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) {
	if (!used[i])
		map[num_lanes++] =3D i;
}
```

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

**Observations:**

1. **Double endpoint lookup**: `analogix_dp_dt_parse_lanes_map()` first cal=
ls `drm_of_get_data_lanes_count_ep()` to get the count, then calls `of_grap=
h_get_endpoint_by_regs()` + `of_property_read_u32_array()` to read the valu=
es. `drm_of_get_data_lanes_count_ep` already looks up the endpoint internal=
ly and reads `data-lanes`. Consider whether it's possible to just do a sing=
le endpoint lookup and read `data-lanes` directly, getting both count and v=
alues in one pass. This would also avoid the subtle inconsistency that `drm=
_of_get_data_lanes_count_ep` uses port_reg=3D1,reg=3D0 while `of_graph_get_=
endpoint_by_regs` uses port=3D1,reg=3D-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 functio=
n returns `-EINVAL` in this case, which is then silently caught by the call=
er:
   ```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 =E2=80=94 the new defi=
nes 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 t=
he same file that don't use parentheses (e.g., the existing `LANE*_MAP_LOGI=
C_LANE_*` defines use `(0x1 << 6)` syntax where the parens wrap the express=
ion, not bare integers). Very minor.

4. **`LANE_COUNT4` as array size**: Using the enum value `LANE_COUNT4 =3D 4=
` as an array dimension and loop bound works since its value is 4, but it m=
ixes 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 se=
lf-documenting, though this is a pre-existing pattern in the codebase.

The register-level change from `analogix_dp_lane_swap()` to `analogix_dp_la=
ne_mapping()` is clean =E2=80=94 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