[PATCH v4] cpufreq: powerpc: Add cpufreq driver for Freescale e500mc SoCs

[Tang Yuantian-B29983]

Thanks, you make my code look better each review.

Thanks,
Yuantian


> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar at linaro.org]
> Sent: 2013年4月9日 17:47
> To: Tang Yuantian-B29983
> Cc: rjw at sisk.pl; cpufreq at vger.kernel.org; linux-pm at vger.kernel.org;
> linux-kernel at vger.kernel.org; linuxppc-dev at lists.ozlabs.org; Li Yang-
> R58472
> Subject: Re: [PATCH v4] cpufreq: powerpc: Add cpufreq driver for
> Freescale e500mc SoCs
> 
> Mostly good now, V5 should be the final one.
> 
> On 9 April 2013 14:02,  <Yuantian.Tang at freescale.com> wrote:
> > diff --git a/drivers/cpufreq/ppc-corenet-cpufreq.c
> > b/drivers/cpufreq/ppc-corenet-cpufreq.c
> 
> > +static int corenet_cpufreq_target(struct cpufreq_policy *policy,
> > +               unsigned int target_freq, unsigned int relation) {
> > +       struct cpufreq_freqs freqs;
> > +       unsigned int new;
> > +       struct clk *parent;
> > +       int ret;
> > +       struct cpu_data *data = per_cpu(cpu_data, policy->cpu);
> > +
> > +       cpufreq_frequency_table_target(policy, data->table,
> > +                       target_freq, relation, &new);
> > +
> > +       if (policy->cur == data->table[new].frequency)
> > +               return 0;
> > +
> > +       freqs.old = policy->cur;
> > +       freqs.new = data->table[new].frequency;
> > +       freqs.cpu = policy->cpu;
> 
> You don't need to set freqs.cpu..
> 
> > +       mutex_lock(&cpufreq_lock);
> > +       cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> > +
> > +       parent = of_clk_get(data->parent, new);
> > +       ret = clk_set_parent(data->clk, parent);
> > +       if (ret) {
> > +               freqs.new = freqs.old;
> > +               cpufreq_notify_transition(policy, &freqs,
> CPUFREQ_POSTCHANGE);
> > +               mutex_unlock(&cpufreq_lock);
> > +               return ret;
> > +       }
> > +
> > +       cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> > +       mutex_unlock(&cpufreq_lock);
> 
> What about writing it as:
> 
> +       ret = clk_set_parent(data->clk, parent);
> +       if (ret)
> +               freqs.new = freqs.old;
> +
> +       cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> +       mutex_unlock(&cpufreq_lock);
> 
> return ret;
> 
> > +       return 0;
> > +}