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

On 8/19/2026 4:04 PM, [email protected] wrote:
> 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);
> 

Hi Luca,

Thanks for taking a look at previous v6 patch series. This Sashiko 
warning was also present in v6. Sorry for CC'ing you out‑of‑the‑blue 
without prior heads‑up.

> [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?
> 

For Analogix DP, port@1 will only ever have a single endpoint. 
Multi‑output configurations are not supported by the hardware, so 
passing -1 cannot select a mismatched endpoint. There is no risk of 
retrieving the 'data‑lanes' property from an unintended 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);
>>   }
> 

lane_map[] uses index as physical lane, value as logical lane.

This matches training_lane[] / cr_loop[] in struct link_train, whose 
indexes are physical lanes for DPCD access during link training.

LANE<i>_MAP_SHIFT configures which logical lane is routed onto physical 
lane i, so current register logic is correct.

Best regards,
Damon