Hi Viresh,
Thanks for the review.
On 21.05.2014 13:22, Viresh Kumar wrote:
> On 21 May 2014 16:47, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
>
> Mostly nitpicks ..
>
>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>> config ARM_EXYNOS4X12_CPUFREQ
>>> bool "SAMSUNG EXYNOS4x12"
>>> - depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412) && !ARCH_MULTIPLATFORM
>>> + depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
>
> Get rid of () as well..
Right.
>
>>> diff --git a/drivers/cpufreq/exynos-cpufreq.h b/drivers/cpufreq/exynos-cpufreq.h
>>> index a28ee9d..8dfebac 100644
>>> --- a/drivers/cpufreq/exynos-cpufreq.h
>>> +++ b/drivers/cpufreq/exynos-cpufreq.h
>>> @@ -50,6 +50,7 @@ struct exynos_dvfs_info {
>>> struct cpufreq_frequency_table *freq_table;
>>> void (*set_freq)(unsigned int, unsigned int);
>>> bool (*need_apll_change)(unsigned int, unsigned int);
>>> + void __iomem *cmu_regs;
>
> s/tab/space ? before *cmu_regs ..
Other fields in this struct have their names aligned with a tab as well.
>
>>> diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
>>> @@ -143,6 +160,8 @@ int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
>>> info->freq_table = exynos4210_freq_table;
>>> info->set_freq = exynos4210_set_frequency;
>>>
>>> + cpufreq = info;
>
> I couldn't find this variable .. i.e. 'cpufreq'
It is a static global variable that is being added at the top of the file.
>
>>> +
>>> return 0;
>>>
>>> err_mout_apll:
>>> diff --git a/drivers/cpufreq/exynos4x12-cpufreq.c b/drivers/cpufreq/exynos4x12-cpufreq.c
>
>>> int exynos4x12_cpufreq_init(struct exynos_dvfs_info *info)
>>> {
>>> + struct device_node *np;
>>> unsigned long rate;
>>>
>>> + np = of_find_compatible_node(NULL, NULL, "samsung,exynos4412-clock");
>>> + if (!np) {
>>> + pr_err("%s: failed to find clock controller DT node\n",
>>> + __func__);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + info->cmu_regs = of_iomap(np, 0);
>>> + if (!info->cmu_regs) {
>>> + pr_err("%s: failed to map CMU registers\n", __func__);
>>> + return -EFAULT;
>>> + }
>>> +
>
> Don't replicate. Create a routine for all this..
>
While I agree that all three drivers basically use the same look-up and
mapping code replicated, this patch is a temporary hack, until all those
three drivers are completely removed, most likely in 3.17, so I would
prefer doing this in the most ugly way, so that people don't follow this.
Still, I think a comment added before of_find_compatible_node() in each
driver saying that this is a hack and why it is there would be nice, though.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
