On 04-08-17, 15:31, Sudeep Holla wrote:

I don't think its the Microsoft exchange server which screwed up tabs and
spaces, but you.

You have used spaces here instead of tab and at multiple other places, can you
please fix them all ?

This stores the same handle pointer which is stored in the global variable
below. Right? Why keep a local variable here at all ?

+};
+
+static const struct scmi_handle *handle;
+
+unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
+{
+	int ret;
+	unsigned long rate;
+	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
+	struct scmi_data *priv = policy->driver_data;
+	struct scmi_perf_ops *perf_ops = priv->handle->perf_ops;

Normally people prefer to keep these definitions in decreasing order of their
lengths. i.e. ret and rate would be defined in the last line. Though I would
leave it to you to decide.

+
+	ret = perf_ops->freq_get(priv->handle, priv->domain_id, &rate, false);
+	if (ret)
+		return CPUFREQ_ENTRY_INVALID;

This is something special which is used only when we are returning indexes and
I am not sure if this will have benefit here. I will rather return 0 here.
That's what other drivers are doing.

+	return rate / 1000;
+}
+
+static int
+scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
+{
+	struct scmi_data *priv = policy->driver_data;
+	struct scmi_perf_ops *perf_ops = priv->handle->perf_ops;
+	u64 freq = policy->freq_table[index].frequency * 1000;
+
+	return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false);
+}

I suppose any CPU can change the frequency of any other CPU here, right? You
must set policy->dvfs_possible_from_any_cpu = true, from ->init() then.

+static int
+scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
+{
+	int cpu, domain, ret = 0;

You don't need to initialize ret here and I would rather name it tdomain or
something else. ret is a lot used to store error/success values, which isn't
your case.

+	struct device *tcpu_dev;
+
+	domain = handle->perf_ops->device_domain_id(cpu_dev);
+	if (domain < 0)
+		return domain;
+
+	cpumask_set_cpu(cpu_dev->id, cpumask);

The mask already have this set from the core, you don't need to do it again.

+	for_each_possible_cpu(cpu) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		tcpu_dev = get_cpu_device(cpu);
+		if (!tcpu_dev)
+			continue;
+
+		ret = handle->perf_ops->device_domain_id(tcpu_dev);
+		if (ret == domain)
+			cpumask_set_cpu(cpu, cpumask);
+	}
+
+	return 0;
+}
+
+static int scmi_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+	unsigned int latency;
+	struct device *cpu_dev;
+	struct scmi_data *priv;
+	struct cpufreq_frequency_table *freq_table;
+
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu%d device\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	ret = handle->perf_ops->add_opps_to_device(cpu_dev);
+	if (ret) {
+		dev_warn(cpu_dev, "failed to add opps to the device\n");
+		return ret;
+	}
+
+	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
+	if (ret) {
+		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
+		return ret;
+	}
+
+	ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
+	if (ret) {
+		dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	/*
+	 * But we need OPP table to function so if it is not there let's
+	 * give platform code chance to provide it for us.
+	 */

How are we getting the OPPs? DT or non DT ?

+	ret = dev_pm_opp_get_opp_count(cpu_dev);
+	if (ret <= 0) {
+		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
+		ret = -EPROBE_DEFER;
+		goto out_free_opp;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto out_free_opp;
+	}
+
+	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
+		goto out_free_priv;
+	}
+
+	priv->handle = handle;
+	priv->cpu_dev = cpu_dev;
+	priv->domain_id = handle->perf_ops->device_domain_id(cpu_dev);
+
+	policy->driver_data = priv;
+
+	ret = cpufreq_table_validate_and_show(policy, freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
+			ret);
+		goto out_free_cpufreq_table;
+	}
+
+	latency = handle->perf_ops->get_transition_latency(cpu_dev);
+	if (!latency)
+		latency = CPUFREQ_ETERNAL;
+
+	policy->cpuinfo.transition_latency = latency;
+
+	return 0;
+
+out_free_cpufreq_table:
+	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_free_priv:
+	kfree(priv);
+out_free_opp:
+	dev_pm_opp_cpumask_remove_table(policy->cpus);
+
+	return ret;
+}
+
+static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	struct scmi_data *priv = policy->driver_data;
+
+	cpufreq_cooling_unregister(priv->cdev);
+	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
+	dev_pm_opp_cpumask_remove_table(policy->related_cpus);
+	kfree(priv);

I would rather swap the above two lines to keep the same order as in probe.
Though nothing would fail with the current code as well.

+
+	return 0;
+}
+
+static void scmi_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct scmi_data *priv = policy->driver_data;
+	struct device_node *np = of_node_get(priv->cpu_dev->of_node);
+
+	if (WARN_ON(!np))
+		return;
+
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		u32 pcoeff = 0;
+
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &pcoeff);
+
+		priv->cdev = of_cpufreq_power_cooling_register(np, policy,
+							       pcoeff, NULL);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(priv->cpu_dev,
+				"running cpufreq without cooling device: %ld\n",
+				PTR_ERR(priv->cdev));
+
+			priv->cdev = NULL;
+		}
+	}
+
+	of_node_put(np);
+}
+
+static struct cpufreq_driver scmi_cpufreq_driver = {
+	.name			= "scmi",
+	.flags			= CPUFREQ_STICKY |
+					CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
+					CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.verify			= cpufreq_generic_frequency_table_verify,
+	.attr			= cpufreq_generic_attr,
+	.target_index		= scmi_cpufreq_set_target,
+	.get			= scmi_cpufreq_get_rate,
+	.init			= scmi_cpufreq_init,
+	.exit			= scmi_cpufreq_exit,
+	.ready			= scmi_cpufreq_ready,
+};

Above block has lots of space/tab issues. Can you please use tabs before "="
instead?

+static int scmi_cpufreq_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	handle = devm_scmi_handle_get(&pdev->dev);

What code is creating this pdev ?

+
+	if (IS_ERR_OR_NULL(handle) || !handle->perf_ops)
+		return -EPROBE_DEFER;
+
+	ret = cpufreq_register_driver(&scmi_cpufreq_driver);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: registering cpufreq failed, err: %d\n",
+			__func__, ret);
+	}
+
+	return ret;
+}
+
+static int scmi_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&scmi_cpufreq_driver);
+	return 0;
+}
+
+static struct platform_driver scmi_cpufreq_platdrv = {
+	.driver = {
+		.name	= "scmi-cpufreq",
+	},
+	.probe		= scmi_cpufreq_probe,
+	.remove		= scmi_cpufreq_remove,
+};
+module_platform_driver(scmi_cpufreq_platdrv);
+
+MODULE_AUTHOR("Sudeep Holla [off-list ref]");
+MODULE_DESCRIPTION("ARM SCMI CPUFreq interface driver");
+MODULE_LICENSE("GPL v2");

-- 
viresh