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

[Damon Ding]

Hi Luca,

On 8/19/2026 8:54 PM, Luca Ceresoli wrote:
> Hello,
>
> > 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 (0, 1, 2, 3) automatically and keep the driver running.
> >
> > Lane mapping is mainly used for below scenarios:
> > 1. Correct PCB lane swap and differential line routing crossover
> >     without hardware changes;
> > 2. Adapt mismatched lane pin definitions between SoC and eDP panel;
> > 3. Support multiple panel hardware variants on the same board
> >     by configuring data-lanes in device tree only.
> >
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > Signed-off-by: Damon Ding <damon.ding@xxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 117448b854db..4aa444858bb6 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)
> > +{
> > +	struct video_info *video_info = &dp->video_info;
> > +	struct device_node *endpoint;
> > +	u32 tmp[LANE_COUNT4];
>
> The 'tmp' name is not very useful to understand what it's for. Based on the
> code I'd say it stores a lane index, so what about 'lane_idx', unless you
> can propose a better name.

Good idea, will fix in v8.

> > +	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);
>
>
> > +	if (!endpoint)
> > +		return -EINVAL;
> > +
> > +	ret = of_property_read_u32_array(endpoint, "data-lanes", tmp, num_lanes);
> > +	of_node_put(endpoint);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < num_lanes; i++) {
> > +		if (tmp[i] >= LANE_COUNT4) {
> > +			dev_dbg(dp->dev, "data-lanes[%d] = %u is out of range\n", i, tmp[i]);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (used[tmp[i]]) {
> > +			dev_dbg(dp->dev, "data-lanes[%d] = %u is duplicate\n", i, tmp[i]);
> > +			return -EINVAL;
> > +		}
> > +
> > +		used[tmp[i]] = true;
> > +		map[i] = tmp[i];
> > +	}
> > +
> > +	for (i = 0; i < LANE_COUNT4 && num_lanes < LANE_COUNT4; i++) {
> > +		if (!used[i])
> > +			map[num_lanes++] = i;
> > +	}
>
> This loop is a bit obscure to me. After reading it a few times I _think_ it
> does the following:
>
>   /*
>    * Fill the unused map[] entries with the unused lane indices <reason?>. E.g.:
>    * used[] values = {0,1,0,1} // Only lanes 1 and 3 are used
>    * map[] before  = {3,1,x,x} // x = unassigned
>    * map[] after   = {3,1,0,2} // filled last 2 entries with the unused lane indices
>    */
>
> Is my understanding correct? If it is, please fill <reason?> and add the
> above comment (possibly improved) before the loop.

You are right.

The hardware register defaults to 0xE4, which corresponds to map = {0, 
1, 2, 3}. Functionally, writing duplicate mappings for unused lanes 
would not cause issues.

Unused lanes are populated with non‑duplicated physical lane indices to 
support DP HPD scenarios where different devices with varying lane 
counts may be connected, as well as to keep the register content reasonable.

The comment will be added in v8.

Best regards,
Damon