Re: [PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC path optional

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:07AM +0530, Praveen Talari wrote:
> Refactor the geni_icc_get() function to replace the loop-based ICC path
> initialization with explicit handling of each interconnect path. This
> improves code readability and allows for different error handling per
> path type.

I don't think this "improves code readability", IMO you're turning a
clean loop into a unrolled mess.


But then comes the least significant portion of your "problem
description" (i.e. the last words of it), where you indicate that this
would allow you to have different error handling for "qup-memory".

This is actually a valid reason to make this change, so say that!


> 
> The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
> is now optional and skipped if not defined in DT.
> 

Please rewrite this message to _start_ with the problem description.
Make it clear on the first line/sentence why the change should be done.

E.g. compare with something like this:

"""
"qup-memory" is an optional interconnect path, unroll the geni_icc_get()
loop in order to allow specific error handling for this path.
"""

You only need to read 4 words to understand exactly why this patch
exists.

> Co-developed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 36 +++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index cd1779b6a91a..b6167b968ef6 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -899,30 +899,32 @@ EXPORT_SYMBOL_GPL(geni_se_rx_dma_unprep);
>  
>  int geni_icc_get(struct geni_se *se, const char *icc_ddr)
>  {
> -	int i, err;
> -	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
> +	struct geni_icc_path *icc_paths = se->icc_paths;
>  
>  	if (has_acpi_companion(se->dev))
>  		return 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
> -		if (!icc_names[i])
> -			continue;
> -
> -		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
> -		if (IS_ERR(se->icc_paths[i].path))
> -			goto err;
> +	icc_paths[GENI_TO_CORE].path = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(icc_paths[GENI_TO_CORE].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_CORE].path),
> +				     "Failed to get 'qup-core' ICC path\n");

To taste, but I think a local variable would be helpful to make this
less dense.

	path = devm_of_icc_get(se->dev, "qup-core");
	if (IS_ERR(path))
		return dev_err_probe(se->dev, PTR_ERR(path), "Failed to get 'qup-core' ICC path\n");
	icc_paths[GENI_TO_CORE].path = path;

Regards,
Bjorn

> +
> +	icc_paths[CPU_TO_GENI].path = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(icc_paths[CPU_TO_GENI].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[CPU_TO_GENI].path),
> +				     "Failed to get 'qup-config' ICC path\n");
> +
> +	/* The DDR path is optional, depending on protocol and hw capabilities */
> +	icc_paths[GENI_TO_DDR].path = devm_of_icc_get(se->dev, "qup-memory");
> +	if (IS_ERR(icc_paths[GENI_TO_DDR].path)) {
> +		if (PTR_ERR(icc_paths[GENI_TO_DDR].path) == -ENODATA)
> +			icc_paths[GENI_TO_DDR].path = NULL;
> +		else
> +			return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_DDR].path),
> +					     "Failed to get 'qup-memory' ICC path\n");
>  	}
>  
>  	return 0;
> -
> -err:
> -	err = PTR_ERR(se->icc_paths[i].path);
> -	if (err != -EPROBE_DEFER)
> -		dev_err_ratelimited(se->dev, "Failed to get ICC path '%s': %d\n",
> -					icc_names[i], err);
> -	return err;
> -
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_get);
>  
> -- 
> 2.34.1
>