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: Sat, 16 May 2026 11:13:14 +1000 Message-ID: In-Reply-To: <20260514070133.2275069-4-damon.ding@rock-chips.com> References: <20260514070133.2275069-1-damon.ding@rock-chips.com> <20260514070133.2275069-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: Has issues to address.** **1. Error handling in `analogix_dp_dt_parse_lanes_map` returns `-EINVAL` f= or non-error conditions:** ```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; ``` `drm_of_get_data_lanes_count_ep` returns `-ENODEV` when the `data-lanes` pr= operty is simply absent, which is the normal case for most boards. Returnin= g `-EINVAL` here loses the distinction between "property not present" (norm= al) 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 =E2=80=94 silently falling back. This means= a genuinely malformed `data-lanes` property (e.g., `data-lanes =3D <0 0 1 = 1>;` with duplicates, or `data-lanes =3D <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 w= ill get no indication that their configuration was ignored. **Suggestion**: Return the actual error. In the caller, only fall back sile= ntly on `-ENODEV`; propagate or at minimum `dev_warn()` on other errors so = that DT mistakes are visible. **2. Redundant `memcpy` =E2=80=94 the default map is written twice on the s= uccess path:** ```c + u32 map[LANE_COUNT4] =3D {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 p= aths. Consider moving the initial default write to the caller, or just remo= ving the first `memcpy` and only doing it in the error path. Minor style ni= t. **3. `LANE_COUNT4` is an enum value of `4`, not a `#define`:** ```c + u32 tmp[LANE_COUNT4]; + u32 map[LANE_COUNT4] =3D {0, 1, 2, 3}; + bool used[LANE_COUNT4] =3D {false}; ``` Using an enum value as an array size is technically fine in C (it's a compi= le-time constant), but `LANE_COUNT4 =3D 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` i= n some places as a max, so this is consistent. **4. The fill-remaining-lanes logic:** ```c + for (i =3D 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) { + if (!used[i]) + map[num_lanes++] =3D i; + } ``` This fills in unmapped physical lanes beyond what `data-lanes` specifies. F= or example, if `data-lanes =3D <2 3>`, the final map becomes `{2, 3, 0, 1}`= . This behavior seems reasonable for 2-lane configurations where lanes 2 an= d 3 are unused but the register needs all 4 entries. However, it's not docu= mented anywhere =E2=80=94 consider adding a brief comment or noting this in= the commit message. **5. Register shift defines =E2=80=94 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 codi= ng 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 `anal= ogix_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 pe= rmutations. The only caller was `analogix_dp_reset()` which always passed `= enable=3D0`, so the "swap" functionality was never actually used. The refac= toring 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