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.
The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
is now optional and skipped if not defined in DT.
Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
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");
+
+ 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.1On 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@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
> 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
>
Hi Bjorn,
Thank you for review
On 11/26/2025 8:37 PM, Bjorn Andersson wrote:
> 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.
The "qup-memory" interconnect path is optional and may not be defined
in all device trees. Unroll the loop-based ICC path initialization to
allow specific error handling for each path type.
The "qup-core" and "qup-config" paths remain mandatory and will fail
probe if missing, while "qup-memory" is now handled as optional and
skipped when not present in the device tree.
I hope the commit text above should be appropriate
Thanks,
Praveen
>
>> Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>> 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.
Sure, will do it in next version.
Thanks,
Praveen
>
> 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
>>
© 2016 - 2025 Red Hat, Inc.