Re: [PATCH 2/7] of: factor out of_map_id() code

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 26/01/2026 12:25, Vikash Garodia wrote:
> From: Charan Teja Kalla <charan.kalla@oss.qualcomm.com>

This commit message is confusing and inaccurate.

First up, you're not factoring _out_ of_map_id() - factor out 
of_map_id() means to remove of_map_id() - you are refactoring of_map_id().

Your patch title should be something like "refactor of_map_id() to 
prepare for mapping of multiple IDs to a single device"

> Linux interprets multiple mappings for the same input ID as a set of
> equivalent choices to pick one. There exists usecases where these set
> must be maintained in parallel, ex: on ARM, a dynamically created child
> device(s) is referencing multiple input id's in parent iommu-map.
> 
> Factor out the code where multiple mappings needs to be maintained in
> parallel can be achieved through callback from this factored out code.

Which callback ? There is no ->function(pointer, here...); ?!

Just make some plain and straightforward statements about what you are 
doing and why. There's no need to resort to dissertation-speak.

> Signed-off-by: Charan Teja Kalla <charan.kalla@oss.qualcomm.com>
> Signed-off-by: Vijayanand Jitta <vijayanand.jitta@oss.qualcomm.com>
> Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> ---
>   drivers/of/base.c | 47 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0825f3dc93f2472e9947af09acdde72031ab85bc..606bef4f90e7d13bae4f7b0c45acd1755ad89826 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2122,6 +2122,32 @@ static bool of_check_bad_map(const __be32 *map, int len)
>   	return true;
>   }
>   
> +static int of_map_id_fill_output(struct of_map_id_arg *arg,
> +				 struct device_node *phandle_node, u32 id_or_offset,
> +				 const __be32 *out_base, u32 cells,
> +				 bool bypass)
> +{
> +	if (bypass) {
> +		arg->map_args.args[0] = id_or_offset;
> +		return 0;
> +	}
> +
> +	if (arg->map_args.np)
> +		of_node_put(phandle_node);
> +	else
> +		arg->map_args.np = phandle_node;
> +
> +	if (arg->map_args.np != phandle_node)
> +		return -EAGAIN;
> +
> +	for (int i = 0; i < cells; i++)
> +		arg->map_args.args[i] = (id_or_offset + be32_to_cpu(out_base[i]));
> +
> +	arg->map_args.args_count = cells;
> +
> +	return 0;
> +}
> +
>   /**
>    * of_map_id - Translate an ID through a downstream mapping.
>    * @np: root complex device node.
> @@ -2162,8 +2188,7 @@ int of_map_id(const struct device_node *np, u32 id, const char *map_name,
>   		if (arg->map_args.np)
>   			return -ENODEV;
>   		/* Otherwise, no map implies no translation */
> -		arg->map_args.args[0] = id;
> -		return 0;
> +		goto bypass_translation;
>   	}
>   
>   	if (map_bytes % sizeof(*map))
> @@ -2185,6 +2210,7 @@ int of_map_id(const struct device_node *np, u32 id, const char *map_name,
>   		struct device_node *phandle_node;
>   		u32 id_base, phandle, id_len, id_off, cells = 0;
>   		const __be32 *out_base;
> +		int ret;
>   
>   		if (map_len - offset < 2)
>   			goto err_map_len;
> @@ -2238,19 +2264,10 @@ int of_map_id(const struct device_node *np, u32 id, const char *map_name,
>   		if (masked_id < id_base || id_off >= id_len)
>   			continue;
>   
> -		if (arg->map_args.np)
> -			of_node_put(phandle_node);
> -		else
> -			arg->map_args.np = phandle_node;
> -
> -		if (arg->map_args.np != phandle_node)
> +		ret = of_map_id_fill_output(arg, phandle_node, id_off, out_base, cells, false);
> +		if (ret == -EAGAIN)
>   			continue;
>   
> -		for (int i = 0; i < cells; i++)
> -			arg->map_args.args[i] = (id_off + be32_to_cpu(out_base[i]));
> -
> -		arg->map_args.args_count = cells;
> -
>   		pr_debug("%pOF: %s, using mask %08x, id-base: %08x, out-base: %08x, length: %08x, id: %08x -> %08x\n",
>   			np, map_name, map_mask, id_base, be32_to_cpup(out_base),
>   			id_len, id, id_off + be32_to_cpup(out_base));
> @@ -2260,9 +2277,9 @@ int of_map_id(const struct device_node *np, u32 id, const char *map_name,
>   	pr_info("%pOF: no %s translation for id 0x%x on %pOF\n", np, map_name,
>   		id, arg->map_args.np  ? arg->map_args.np : NULL);
>   
> +bypass_translation:
>   	/* Bypasses translation */
> -	arg->map_args.args[0] = id;
> -	return 0;
> +	return of_map_id_fill_output(arg, NULL, id, 0, 0, true);
>   
>   err_map_len:
>   	pr_err("%pOF: Error: Bad %s length: %d\n", np, map_name, map_bytes);
>