On Thu, Aug 04 2016 at 09:59 -0600, Brendan Jackman wrote:
Hi Lina, I spotted a couple more nits..+static struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn, + const struct cpu_pd_ops *ops) +{ + struct cpu_pm_domain *pd = NULL; + struct generic_pm_domain *genpd = NULL; + int ret = -ENOMEM; + + if (!of_device_is_available(dn)) + return ERR_PTR(-ENODEV); + + genpd = kzalloc(sizeof(*genpd), GFP_KERNEL); + if (!genpd) + goto fail; + + genpd->name = kstrndup(dn->full_name, CPU_PD_NAME_MAX, GFP_KERNEL); + if (!genpd->name) + goto fail; + + pd = kzalloc(sizeof(*pd), GFP_KERNEL); + if (!pd) + goto fail; + + if (!zalloc_cpumask_var(&pd->cpus, GFP_KERNEL)) + goto fail; + + genpd->power_off = cpu_pd_power_off; + genpd->power_on = cpu_pd_power_on; + genpd->flags |= GENPD_FLAG_IRQ_SAFE; + genpd->of_node = dn; + pd->genpd = genpd; + pd->ops.power_on = ops->power_on; + pd->ops.power_off = ops->power_off; + + INIT_LIST_HEAD_RCU(&pd->link); + mutex_lock(&cpu_pd_list_lock); + list_add_rcu(&pd->link, &of_cpu_pd_list); + mutex_unlock(&cpu_pd_list_lock); + + /* Populate platform specific states from DT */ + if (ops->populate_state_data) { + struct device_node *np; + int i; + + /* Initialize the arm,idle-state properties */ + ret = pm_genpd_of_parse_power_states(genpd); + if (ret) { + pr_warn("%s domain states not initialized (%d)\n", + dn->full_name, ret); + goto fail; + } + for (i = 0; i < genpd->state_count; i++) { + np = of_parse_phandle(dn, "domain-idle-states", i); + ret = ops->populate_state_data(np, + &genpd->states[i].param); + of_node_put(np); + if (ret) + goto fail; + } + }It seems a bit unfortunate to do of_parse_phandle for "domain-idle-states" again, when pm_genpd_of_parse_power_states has just done that. Maybe we could could add an `of_node` member to `struct genpd_power_state` and have pm_genpd_of_parse_power_states populate that?
Hmm.. that could be done.
+ + /* Register the CPU genpd */ + pr_debug("adding %s as CPU PM domain\n", pd->genpd->name); + ret = pm_genpd_init(pd->genpd, &simple_qos_governor, false); + if (ret) { + pr_err("Unable to initialize domain %s\n", dn->full_name); + goto fail; + } + + ret = of_genpd_add_provider_simple(dn, pd->genpd); + if (ret) + pr_warn("Unable to add genpd %s as provider\n", + pd->genpd->name); + + return pd->genpd; +fail: + + kfree(genpd->name); + kfree(genpd); + if (pd) + kfree(pd->cpus); + kfree(pd); + return ERR_PTR(ret); +} ++int of_setup_cpu_pd_single(int cpu, const struct cpu_pd_ops *ops) +{ + + struct device_node *dn; + struct generic_pm_domain *genpd; + struct cpu_pm_domain *cpu_pd; + + dn = of_get_cpu_node(cpu, NULL); + if (!dn) + return -ENODEV;of_get_cpu_node increments the refcount so we need an of_node_put at some point. There are loads of instances around the kernel of of_get_cpu_node without corresponding of_node_put, though, I could be misunderstanding...
It should be done. An easily missed statementr, I suppose. Thanks for all the nits. This is the best time for that. Thanks, Lina
+ + dn = of_parse_phandle(dn, "power-domains", 0); + if (!dn) + return -ENODEV; + + /* Find the genpd for this CPU, create if not found */ + genpd = of_get_cpu_domain(dn, ops, cpu); + if (IS_ERR(genpd)) + return PTR_ERR(genpd); + + of_node_put(dn); + cpu_pd = to_cpu_pd(genpd); + if (!cpu_pd) { + pr_err("%s: Genpd was created outside CPU PM domains\n", + __func__); + return -ENOENT; + } + + return cpu_pd_attach_cpu(cpu_pd, cpu); +}Cheers, Brendan
-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
