Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incorrect hardware lane mapping due to logic-to-physical vs physical-to-logic mismatch, effectively programming the inverse of the intended mapping. --
commit 83b65668f019b59b41d7d71bc5a8a34dd644922a Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Add support for optional data-lanes mapping This commit parses the optional 'data-lanes' device tree property to support custom physical lane mapping configurations. If no valid configuration is found, it automatically falls back to a default lane map. > 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 unintentionally program the inverse of the intended lane mapping? The data-lanes device tree property specifies the physical lane for each logical lane, where lane_map[logical_index] = physical_index. However, the ANALOGIX_DP_LANE_MAP register is structured to select a logical lane for each physical lane. In analogix_dp_lane_mapping(), the code uses the logical lane index to select the shift mask (e.g., LANE0_MAP_SHIFT) and assigns the physical lane index (lane_map[0]) as the value. This configures physical lane 0 to output logical lane lane_map[0], which results in the inverse of the intended mapping when an asymmetric configuration is used. Would it be more appropriate to write the logical lane index into the shift corresponding to the physical lane? For example, by doing something like: reg |= logical_index << (lane_map[logical_index] * 2); > > writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP); > } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
