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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect physical-to-logical lane mapping programmed into hardware register.
- [Medium] Inconsistent endpoint resolution when validating and parsing the 'data-lanes' property.
--

commit b0396fb13d86cb1835c8181f73f2c91f756c7665
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 falls 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 699a7f380c56c..b2e729850391b 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)
> +{
[ ... ]
> +	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]
Is there a risk of inconsistent endpoint resolution here?

The call to drm_of_get_data_lanes_count_ep() strictly searches for the
endpoint with reg = 0 under port 1. However,
of_graph_get_endpoint_by_regs(..., 1, -1) matches the first available
endpoint regardless of its reg value.

If a device tree defines multiple endpoints under port 1, could these calls
resolve to different endpoints? This might cause the driver to validate the
lane count against one endpoint but parse the mapping from another.

> +	if (!endpoint)
> +		return -EINVAL;
[ ... ]
> 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)
[ ... ]
> -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 code program the hardware register with the correct mapping?

The data-lanes device tree property defines the physical lane for each
logical lane, meaning lane_map[i] holds the physical lane assigned to
logical lane i.

However, the hardware register ANALOGIX_DP_LANE_MAP appears to expect the
reverse mapping, where it is divided into bit fields per physical lane that
must be programmed with the desired logical lane.

Writing lane_map[0] << LANE0_MAP_SHIFT assigns the physical lane index into
the configuration field for Physical Lane 0. While this might work for
self-inverse mappings, could it produce a scrambled configuration for
non-symmetric mappings?

>  
>  	writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529040530.741336-1-damon.ding@rock-chips.com?part=3