Hi Viresh,

If I send patches in reply, it will produce new patches, instead of answers
in the thread. Please find below the file dump.

->cat drivers/cpufreq/qcom-cpufreq-kryo.c
// SPDX-License-Identifier: GPL-2.0
/*
 * Copyright (c) 2018, The Linux Foundation. All rights reserved.
 */

/*
 * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
 * the CPU frequency subset and voltage value of each OPP varies
 * based on the silicon variant in use. Qualcomm Process Voltage Scaling
Tables
 * defines the voltage and frequency value based on the msm-id in SMEM
 * and speedbin blown in the efuse combination.
 * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the
SoC
 * to provide the OPP framework with required information.
 * This is used to determine the voltage and frequency value for each OPP of
 * operating-points-v2 table when it is parsed by the OPP framework.
 */

#include <linux/cpu.h>
#include <linux/err.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/nvmem-consumer.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/pm_opp.h>
#include <linux/slab.h>
#include <linux/soc/qcom/smem.h>

#define MSM_ID_SMEM     137
#define SILVER_LEAD     0
#define GOLD_LEAD       2

enum _msm_id {
        MSM8996V3 = 0xF6ul,
        APQ8096V3 = 0x123ul,
        MSM8996SG = 0x131ul,
        APQ8096SG = 0x138ul,
};

enum _msm8996_version {
        MSM8996_V3,
        MSM8996_SG,
        NUM_OF_MSM8996_VERSIONS,
};

static enum _msm8996_version __init qcom_cpufreq_kryo_get_msm_id(void)
{
        size_t len;
        u32 *msm_id;
        enum _msm8996_version version;

        msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
        /* The first 4 bytes are format, next to them is the actual msm-id
*/
        msm_id++;

        switch ((enum _msm_id)*msm_id) {
        case MSM8996V3:
        case APQ8096V3:
                version = MSM8996_V3;
                break;
        case MSM8996SG:
        case APQ8096SG:
                version = MSM8996_SG;
                break;
        default:
                version = NUM_OF_MSM8996_VERSIONS;
        }

        return version;
}

static int __init qcom_cpufreq_kryo_driver_init(void)
{
        struct device *cpu_dev_silver, *cpu_dev_gold;
        struct opp_table *opp_silver, *opp_gold;
        enum _msm8996_version msm8996_version;
        struct nvmem_cell *speedbin_nvmem;
        struct platform_device *pdev;
        struct device_node *np;
        u8 *speedbin;
        u32 versions;
        size_t len;
        int ret;

        cpu_dev_silver = get_cpu_device(SILVER_LEAD);
        if (IS_ERR_OR_NULL(cpu_dev_silver))
                return PTR_ERR(cpu_dev_silver);

        cpu_dev_gold = get_cpu_device(SILVER_LEAD);
        if (IS_ERR_OR_NULL(cpu_dev_gold))
                return PTR_ERR(cpu_dev_gold);

        msm8996_version = qcom_cpufreq_kryo_get_msm_id();
        if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
                dev_err(cpu_dev_silver, "Not Snapdragon 820/821!");
                return -ENODEV;
        }

        np = dev_pm_opp_of_get_opp_desc_node(cpu_dev_silver);
        if (IS_ERR_OR_NULL(np))
                return PTR_ERR(np);

        if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
                ret = -ENOENT;
                goto free_np;
        }

        speedbin_nvmem = of_nvmem_cell_get(np, NULL);
        if (IS_ERR(speedbin_nvmem)) {
                ret = PTR_ERR(speedbin_nvmem);
                dev_err(cpu_dev_silver, "Could not get nvmem cell: %d\n",
ret);
                goto free_np;
        }

        speedbin = nvmem_cell_read(speedbin_nvmem, &len);
        nvmem_cell_put(speedbin_nvmem);

        switch (msm8996_version) {
        case MSM8996_V3:
                versions = 1 << (unsigned int)(*speedbin);
                break;
        case MSM8996_SG:
                versions = 1 << ((unsigned int)(*speedbin) + 4);
                break;
        default:
                BUG();
                break;
        }

        opp_silver =
dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1);
        if (IS_ERR(opp_silver)) {
                dev_err(cpu_dev_silver, "Failed to set supported
hardware\n");
                ret = PTR_ERR(opp_silver);
                goto free_np;
        }

        opp_gold = dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1);
        if (IS_ERR(opp_gold)) {
                dev_err(cpu_dev_gold, "Failed to set supported hardware\n");
                ret = PTR_ERR(opp_gold);
                goto free_opp_silver;
        }

        pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
        if (!IS_ERR_OR_NULL(pdev))
                return 0;

        ret = PTR_ERR(pdev);
        dev_err(cpu_dev_silver, "Failed to register platform device\n");
        dev_pm_opp_put_supported_hw(opp_gold);

free_opp_silver:
        dev_pm_opp_put_supported_hw(opp_silver);

free_np:
        of_node_put(np);

        return ret;
}
late_initcall(qcom_cpufreq_kryo_driver_init);

MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver");
MODULE_LICENSE("GPL v2");

-----Original Message-----
From: Viresh Kumar <viresh.kumar@linaro.org>
Sent: Friday, May 18, 2018 04:46
To: Ilia Lin <redacted>
Cc: mturquette at baylibre.com; sboyd at kernel.org; robh at kernel.org;
mark.rutland at arm.com; nm at ti.com; lgirdwood at gmail.com;
broonie at kernel.org; andy.gross at linaro.org; david.brown at linaro.org;
catalin.marinas at arm.com; will.deacon at arm.com; rjw at rjwysocki.net; linux-
clk at vger.kernel.org; devicetree at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-pm at vger.kernel.org; linux-arm-
msm at vger.kernel.org; linux-soc at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; rnayak at codeaurora.org;
amit.kucheria at linaro.org; nicolas.dechesne at linaro.org;
celster at codeaurora.org; tfinkel at codeaurora.org
Subject: Re: [PATCH v8 10/15] cpufreq: Add Kryo CPU scaling driver

On 17-05-18, 14:19, Ilia Lin wrote:

quoted

+static int __init qcom_cpufreq_kryo_driver_init(void)
+{
+	size_t len;
+	int ret = 0;
+	u32 versions;
+	enum _msm8996_version msm8996_version;
+	u8 *speedbin;
+	struct device *cpu_dev_silver, *cpu_dev_gold;
+	struct device_node *np;
+	struct nvmem_cell *speedbin_nvmem;
+	struct platform_device *pdev;
+	struct opp_table *opp_silver = NULL;
+	struct opp_table *opp_gold = NULL;

No need to initialize them and you may want to arrange all above in
decreasing order of their length.

quoted

+
+	cpu_dev_silver = get_cpu_device(SILVER_LEAD);
+	if (IS_ERR_OR_NULL(cpu_dev_silver))
+		return PTR_ERR(cpu_dev_silver);
+
+	cpu_dev_gold = get_cpu_device(SILVER_LEAD);
+	if (IS_ERR_OR_NULL(cpu_dev_gold))
+		return PTR_ERR(cpu_dev_gold);
+
+	msm8996_version = qcom_cpufreq_kryo_get_msm_id();
+	if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
+		dev_err(cpu_dev_silver, "Not Snapdragon 820/821!");
+		return -ENODEV;
+	}
+
+	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev_silver);
+	if (IS_ERR_OR_NULL(np))
+		return PTR_ERR(np);
+
+	if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
+		ret = -ENOENT;
+		goto free_np;
+	}
+
+	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+	if (IS_ERR(speedbin_nvmem)) {
+		ret = PTR_ERR(speedbin_nvmem);
+		dev_err(cpu_dev_silver, "Could not get nvmem cell: %d\n",

ret);

quoted

+		goto free_np;
+	}
+
+	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
+	nvmem_cell_put(speedbin_nvmem);
+
+	switch (msm8996_version) {
+	case MSM8996_V3:
+		versions = 1 << (unsigned int)(*speedbin);
+		break;
+	case MSM8996_SG:
+		versions = 1 << ((unsigned int)(*speedbin) + 4);
+		break;
+	default:
+		BUG();
+		break;
+	}
+
+	opp_silver =

dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1);

quoted

+	if (IS_ERR_OR_NULL(opp_silver)) {

This API doesn't return NULL and so IS_ERR() would be sufficient.

quoted

+		dev_err(cpu_dev_silver, "Failed to set supported

hardware\n");

quoted

+		ret = PTR_ERR(opp_silver);
+		goto free_np;
+	}
+
+	opp_gold =

dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1);

quoted

+	if (IS_ERR_OR_NULL(opp_gold)) {

same here.

quoted

+		dev_err(cpu_dev_gold, "Failed to set supported

hardware\n");

quoted

+		ret = PTR_ERR(opp_gold);
+		goto free_opp_silver;
+	}
+
+	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	if (!IS_ERR_OR_NULL(pdev))
+		goto out;

Simply return from here and remove the useless label out.

quoted

+
+	ret = PTR_ERR(pdev);
+	dev_err(cpu_dev_silver, "Failed to register platform device\n");
+	dev_pm_opp_put_supported_hw(opp_gold);
+
+free_opp_silver:
+	dev_pm_opp_put_supported_hw(opp_silver);
+
+free_np:
+	of_node_put(np);
+
+out:
+	return ret;
+}
+late_initcall(qcom_cpufreq_kryo_driver_init);

Please resend only this patch now or just paste the new code in a mail

here

so that I can review it quickly and then you can resend the final version.

Most

of the patches aren't changing anyway.

--
viresh