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

+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.

+
+	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_OR_NULL(opp_silver)) {

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

+		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_OR_NULL(opp_gold)) {

same here.

+		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))
+		goto out;

Simply return from here and remove the useless label out.

+
+	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