linux-kernel - Re: [PATCH v2 3/4] clk: qcom: add IPQ9574 interconnect clocks support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org>
Date: Mon, 25 Mar 2024 20:42:21 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Varadarajan Narayanan <quic_varada@...cinc.com>, andersson@...nel.org,
 mturquette@...libre.com, sboyd@...nel.org, robh@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, djakov@...nel.org,
 quic_anusha@...cinc.com, linux-arm-msm@...r.kernel.org,
 linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 3/4] clk: qcom: add IPQ9574 interconnect clocks support

On 25.03.2024 11:20 AM, Varadarajan Narayanan wrote:
> Unlike MSM platforms that manage NoC related clocks and scaling
> from RPM, IPQ SoCs dont involve RPM in managing NoC related
> clocks and there is no NoC scaling.
> 
> However, there is a requirement to enable some NoC interface
> clocks for accessing the peripheral controllers present on
> these NoCs. Though exposing these as normal clocks would work,
> having a minimalistic interconnect driver to handle these clocks
> would make it consistent with other Qualcomm platforms resulting
> in common code paths.  This is similar to msm8996-cbf's usage of
> icc-clk framework.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@...cinc.com>
> ---

[...]

>  
> +
> +static struct icc_clk_data *icc_ipq9574;
> +

What does this help achieve?

[...]

> +static int noc_clks[] = {

We could probably use indexed identifiers here to avoid confusion:
[ICC_BINDING_NAME] = CLK_BINDING_NAME

>  static int gcc_ipq9574_probe(struct platform_device *pdev)
>  {
> -	return qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> +	int ret = qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> +	struct icc_provider *provider;
> +	struct icc_clk_data *icd;
> +	int i;
> +
> +	if (ret)

I'd personally prefer if you left ret uninitialized and assigned it
above the if-statement.

> +		return dev_err_probe(&pdev->dev, ret, "%s failed\n", __func__);

Please avoid the use of __func__ throughout your change and write
a more useful error message.

> +
> +	icd = devm_kmalloc(&pdev->dev, ARRAY_SIZE(noc_clks) * sizeof(*icd),
> +			   GFP_KERNEL);

devm_kcalloc

> +
> +	if (IS_ERR_OR_NULL(icd))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(icd),
> +				     "%s malloc failed\n", __func__);

ditto

> +
> +	icc_ipq9574 = icd;
> +
> +	for (i = 0; i < ARRAY_SIZE(noc_clks); i++, icd++) {
> +		icd->clk = gcc_ipq9574_clks[noc_clks[i]]->hw.clk;
> +		if (IS_ERR_OR_NULL(icd->clk)) {
> +			dev_err(&pdev->dev, "%s: %d clock not found\n",
> +				__func__, noc_clks[i]);
> +			return -ENOENT;

return dev_err_probe

> +		}
> +		icd->name = clk_hw_get_name(&gcc_ipq9574_clks[noc_clks[i]]->hw);
> +	}
> +
> +	provider = icc_clk_register(&pdev->dev, IPQ_APPS_ID,
> +				    ARRAY_SIZE(noc_clks), icc_ipq9574);
> +	if (IS_ERR_OR_NULL(provider))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(provider),
> +				     "%s: icc_clk_register failed\n", __func__);

ditto

On a second thought, since I'm assuming you're going to expand this to other
IPQ SoCs, it might be useful to factor this out into drivers/clk/qcom/common.c

Konrad