[PATCH v2] cpufreq: mt8173: move resources allocation into ->probe()
From: Daniel Kurtz <hidden>
Date: 2015-12-07 02:44:42
Also in:
linux-mediatek, linux-pm
Hi Pi-Cheng, On Sat, Dec 5, 2015 at 2:01 PM, Pi-Cheng Chen [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Since the return value of ->init() of cpufreq driver is not propagated to the device driver model now, move resources allocation into ->probe() to handle -EPROBE_DEFER properly. Signed-off-by: Pi-Cheng Chen <redacted> --- v1->v2: - fix error handling path in ->probe() --- drivers/cpufreq/mt8173-cpufreq.c | 98 ++++++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 28 deletions(-)diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c index 9d0fe37..257bcb9 100644 --- a/drivers/cpufreq/mt8173-cpufreq.c +++ b/drivers/cpufreq/mt8173-cpufreq.c@@ -41,16 +41,35 @@ * the original PLL becomes stable at target frequency. */ struct mtk_cpu_dvfs_info { + struct cpumask cpus; struct device *cpu_dev; struct regulator *proc_reg; struct regulator *sram_reg; struct clk *cpu_clk; struct clk *inter_clk; struct thermal_cooling_device *cdev; + struct list_head list_head; int intermediate_voltage; bool need_voltage_tracking; }; +static LIST_HEAD(dvfs_info_list);
This struct is specific to this driver, so: mtk_cpu_dvfs_info_list
+ +struct mtk_cpu_dvfs_info *get_dvfs_info(int cpu)
static Also, it would be clearer to name this function something like "mtk_cpu_dfs_info_lookup" since "mtk_cpu_dfs_info" is the type this function looks up in the list. It would also match the other functions such as "mtk_cpu_dvfs_info_init()".
quoted hunk ↗ jump to hunk
+{ + struct mtk_cpu_dvfs_info *info; + struct list_head *list; + + list_for_each(list, &dvfs_info_list) { + info = list_entry(list, struct mtk_cpu_dvfs_info, list_head); + + if (cpumask_test_cpu(cpu, &info->cpus)) + return info; + } + + return NULL; +} + static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, int new_vproc) {@@ -402,6 +421,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) */ info->need_voltage_tracking = !IS_ERR(sram_reg); + /* CPUs in the same cluster share a clock and power domain. */ + cpumask_copy(&info->cpus, &cpu_topology[cpu].core_sibling); + return 0; out_free_opp_table:@@ -440,47 +462,32 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy) struct cpufreq_frequency_table *freq_table; int ret; - info = kzalloc(sizeof(*info), GFP_KERNEL); - if (!info) - return -ENOMEM; - - ret = mtk_cpu_dvfs_info_init(info, policy->cpu); - if (ret) { - pr_err("%s failed to initialize dvfs info for cpu%d\n", - __func__, policy->cpu); - goto out_free_dvfs_info; + info = get_dvfs_info(policy->cpu); + if (!info) { + pr_err("dvfs info for cpu%d is not initialized.\n", + policy->cpu); + return -EINVAL; } ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
Hmm. Does this change at init() time? Or can we move this to probe(), too?
if (ret) {
pr_err("failed to init cpufreq table for cpu%d: %d\n",
policy->cpu, ret);
- goto out_release_dvfs_info;
+ return ret;
}
ret = cpufreq_table_validate_and_show(policy, freq_table);
if (ret) {
pr_err("%s: invalid frequency table: %d\n", __func__, ret);
- goto out_free_cpufreq_table;
+ dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table);
+ return ret;If dev_pm_opp_init_cpufreq_table() really is needed at init() time, then please don't change this; still use "goto out_free_cpufreq_table" here.
quoted hunk ↗ jump to hunk
} - /* CPUs in the same cluster share a clock and power domain. */ - cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling); + cpumask_copy(policy->cpus, &info->cpus); policy->driver_data = info; policy->clk = info->cpu_clk; return 0; - -out_free_cpufreq_table: - dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table); - -out_release_dvfs_info: - mtk_cpu_dvfs_info_release(info); - -out_free_dvfs_info: - kfree(info); - - return ret; } static int mtk_cpufreq_exit(struct cpufreq_policy *policy)@@ -489,8 +496,6 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy) cpufreq_cooling_unregister(info->cdev); dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table); - mtk_cpu_dvfs_info_release(info); - kfree(info); return 0; }@@ -510,11 +515,48 @@ static struct cpufreq_driver mt8173_cpufreq_driver = { static int mt8173_cpufreq_probe(struct platform_device *pdev) { - int ret; + struct mtk_cpu_dvfs_info *info; + struct list_head *list, *tmp; + int cpu, ret; + + for_each_possible_cpu(cpu) {
Is there an iterator for cpu clusters? Would be useful here. Or, even better: * Is there a way to way to map cpu to cpu cluster ID? * Is there a way to count the number of clusters? If so you can just use an array of mtk_cpu_dvfs_info, and use a cpu's cluster ID as its index into the array.
+ info = get_dvfs_info(cpu); + if (info) + continue;
How could this ever return non-NULL? This is probe; it won't be called twice - not unless a prior probe() failed (and/or after .remove()), right?
+ + info = kzalloc(sizeof(*info), GFP_KERNEL);
devm_kzalloc() (and you can then remove the kfree()'s below).
+ if (!info) {
+ ret = -ENOMEM;
+ goto release_dvfs_info_list;
+ }
+
+ ret = mtk_cpu_dvfs_info_init(info, cpu);
+ if (ret) {
+ pr_err("%s failed to initialize dvfs info for cpu%d\n",
+ __func__, cpu);dev_err(&pdev->dev, ) here and elsewhere. -Dan
+ kfree(info);
+ goto release_dvfs_info_list;
+ }
+
+ list_add(&info->list_head, &dvfs_info_list);
+ }
ret = cpufreq_register_driver(&mt8173_cpufreq_driver);
- if (ret)
+ if (ret) {
pr_err("failed to register mtk cpufreq driver\n");
+ goto release_dvfs_info_list;
+ }
+
+ return 0;
+
+release_dvfs_info_list:
+ list_for_each_safe(list, tmp, &dvfs_info_list) {
+ info = list_entry(list, struct mtk_cpu_dvfs_info, list_head);
+
+ mtk_cpu_dvfs_info_release(info);
+ list_del(list);
+ kfree(info);
+ }
return ret;
}
--
1.9.1