Re: [PATCH v8 5/5] drm/bridge: analogix_dp: Add support for optional data-lanes mapping

[sashiko-bot@kernel.org]

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 <damon.ding@rock-chips.com>
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/20260828065153.590802-1-damon.ding@rock-chips.com?part=5