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

[Luca Ceresoli]

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 at collabora.com>
> Signed-off-by: Damon Ding <damon.ding at rock-chips.com>
>
> 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.

> +	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.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com