Hi Andrew,
Few more comments from my side :(
On 26 January 2013 21:13, Andrew Lunn <andrew@xxxxxxx> wrote:
> diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/cpufreq.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <asm/proc-fns.h>
> +
> +#define CPU_SW_INT_BLK BIT(28)
> +
> +
> +#include <linux/clk-private.h>
Any specific reason for keeping it together with other #includes?
> +static void kirkwood_cpufreq_set_cpu_state(unsigned int index)
> +{
> +
Can remove extra blank line.
> + if (freqs.old != freqs.new) {
> + local_irq_disable();
> +
> + /* Disable interrupts to the CPU */
> + reg = readl_relaxed(priv.base);
> + reg |= CPU_SW_INT_BLK;
> + writel(reg, priv.base);
Any specific reason for calling writel() instead of writel_relaxed()?
relaxed one would take less time and would be much more efficient.
same for all other writel's too.
> +};
> +
> +static int kirkwood_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> + return cpufreq_frequency_table_verify(policy, &kirkwood_freq_table[0]);
return cpufreq_frequency_table_verify(policy, kirkwood_freq_table); ??
> +}
> +
> +static int kirkwood_cpufreq_target(struct cpufreq_policy *policy,
> + unsigned int target_freq,
> + unsigned int relation)
> +{
> + unsigned int index = 0;
> +
> + if (cpufreq_frequency_table_target(policy, kirkwood_freq_table,
> + target_freq, relation, &index))
> + return -EINVAL;
> +
> + kirkwood_cpufreq_set_cpu_state(index);
This routine does have error cases, like: wrong value of "state"
variable, so returning int from it might make sense. Though i believe
state can't be anything else then STATE_CPU_FREQ or STATE_DDR_FREQ.
In which case, switch must be modified?
> + return 0;
> +}
> +
> +/*
> + * Module init and exit code
> + */
Can be converted to a single line comment if you want. :)
> +static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> +{
> + cpufreq_frequency_table_put_attr(policy->cpu);
> + return 0;
> +}
Hmm.. Exit is normally called in two cases. Either cpufreq driver is
unregistered
or cpu is removed. In that case i am not sure if this routine makes sense? As
we have a uniprocessor SoC here.
@Rafael?
> +static struct freq_attr *kirkwood_cpufreq_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> +};
> +
> +
Can remove extra blank line.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
