Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Logic error in hardware lane mapping configuration causes inverted assignments for non-symmetric maps. - [Low] Device Tree endpoint lookup inconsistency when querying lane counts versus reading properties. --
--- Patch [5]: [PATCH v7 5/5] drm/bridge: analogix_dp: Add support for optional data-lanes mapping --- commit 1c15eb6132eedc3d606168644e5f97918c8e3651 Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Add support for optional data-lanes mapping Parse the optional 'data-lanes' device tree property to support custom physical lane mapping configuration. If no valid configuration is found, fall back to the default lane map automatically. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 039e076d2b8ab..cb06347196cef 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c [ ... ] > @@ -1240,6 +1240,59 @@ static const struct drm_bridge_funcs > analogix_dp_bridge_funcs = { > .detect = analogix_dp_bridge_detect, > }; > > +static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp) > +{ [ ... ] > + 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); [Severity: Low] Does passing -1 here potentially retrieve a different endpoint from the one validated with reg=0 above? If multiple endpoints exist on the DP port and the target endpoint is not positionally first, could this mismatch result in reading the data-lanes property from the wrong endpoint? > + if (!endpoint) > + return -EINVAL; > + > + ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, > num_lanes); > + of_node_put(endpoint); [ ... ] > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index ea8401293a23a..c1344a3f013ab 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c [ ... ] > @@ -48,16 +48,15 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp) > writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1); > } > > -void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable) > +void analogix_dp_lane_mapping(struct analogix_dp_device *dp) > { > + u32 *lane_map = dp->video_info.lane_map; > u32 reg; > > - if (enable) > - reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 | > - LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3; > - else > - reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 | > - LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0; > + reg = lane_map[0] << LANE0_MAP_SHIFT; > + reg |= lane_map[1] << LANE1_MAP_SHIFT; > + reg |= lane_map[2] << LANE2_MAP_SHIFT; > + reg |= lane_map[3] << LANE3_MAP_SHIFT; [Severity: High] Does this logic incorrectly assign the physical lane value to the logical lane's field? By shifting the physical lane index (lane_map[i]) into the register field assigned to the logical lane index (LANE<i>_MAP_SHIFT), it appears this inherently applies the inverse of the requested mapping. While this silently succeeds for symmetric maps (e.g., swapping 0<->3, 1<->2), will it fail for any non-symmetric map (e.g., <1 2 3 0>) by causing physical lanes to carry the wrong logical lanes? Should the bitwise construction instead be reg |= i << (lane_map[i] * 2)? > > writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
