Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The logic-to-physical lane mapping is mathematically inverted when programming the hardware, breaking support for asymmetric lane swap configurations. - [Medium] Inconsistent endpoint resolution when reading the data-lanes property. --
commit b6abf6d746df1b04ca60b36cd37e77da204d915a 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 f23c2ae6e10d0..2dfe7de5d3101 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1234,6 +1234,59 @@ static const struct drm_bridge_funcs > analogix_dp_bridge_funcs = { [ ... ] > +static int analogix_dp_dt_parse_lanes_map(struct analogix_dp_device *dp) > +{ > + struct video_info *video_info = &dp->video_info; > + struct device_node *endpoint; > + u32 tmp[LANE_COUNT4]; > + u32 map[LANE_COUNT4] = {0, 1, 2, 3}; > + bool used[LANE_COUNT4] = {false}; > + int num_lanes; > + int ret, i; > + > + memcpy(video_info->lane_map, map, sizeof(map)); > + > + 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: Medium] Will this result in reading from an inconsistent endpoint if port 1 has multiple endpoints? The call to drm_of_get_data_lanes_count_ep() above explicitly passes 0 for the endpoint reg, but this call uses -1. If there are multiple endpoints, this could return the first one it finds rather than endpoint@0, which might lead to reading the lane map from a different endpoint than the one used for the count. > + if (!endpoint) > + return -EINVAL; > + > + ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, > num_lanes); [ ... ] > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index 38fd8d5014d29..45c7652645a13 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 mathematically invert the lane mapping for asymmetric configurations? The device tree data-lanes property specifies a mapping from logical to physical lanes, so that lane_map[logical_lane] = physical_lane. However, when programming the hardware, it expects the logical lane index to be placed into the bit-field corresponding to the physical lane. Here, the code shifts the physical lane number by the logical lane's bit-shift, effectively programming the logical lane into the physical lane's field. This happens to work by coincidence for symmetric mappings like <3 2 1 0> or <0 1 2 3>, but it will corrupt asymmetric configurations like <1 2 3 0>. Should the logical lane index instead be shifted into the physical lane's bit-field? For example, using 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=4
