[PATCH 2/3] cpufreq: exynos: Adding cpufreq driver for exynos5440

[Viresh Kumar]

On 8 February 2013 00:38, amit kachhap <amit.kachhap at gmail.com> wrote:
> Hi Viresh,
>
> Thanks for the detailed review. Will try to handle them in the next version,

np. I haven't seen reply to few questions, you missed them or accept them.

General tip: Leave a blank line before and after your comment, it makes it more
readable. :)

> On Thu, Feb 7, 2013 at 3:17 AM, Viresh Kumar <viresh.kumar at linaro.org> wrote:
>> On Thu, Feb 7, 2013 at 1:09 AM, Amit Daniel Kachhap
>>> +Required properties:
>>> +- interrupts: Interrupt to know the completion of cpu frequency change.
>>> +- cpufreq_tbl: Table of frequencies and voltage CPU could be transitioned into,
>>
>> This has to be "operating-points" as in cpufreq-cpu0 driver.
> Yes I will check if opp table is beneficial. In my case it is just one
> time parsing of cpufreq table and those values(freq, volt) are not
> used later so did not use opp libraries.

Its one time parsing for everybody, nobody do it twice :)

>>> +       for (old_index = 0;
>>> +               freq_table[old_index].frequency != CPUFREQ_TABLE_END;
>>> +               old_index++)
>>> +               if (freq_table[old_index].frequency == freqs.old)
>>> +                       break;
>>> +
>>> +       if (freq_table[old_index].frequency == CPUFREQ_TABLE_END) {
>>
>> How can this be true?
> This is error scenario

We have given cpufreq core a valid table and it has to set frequency from
this table only. How can cpu have any other freq here ? And you have done
something similar in your init() too, where you check if cpu has freq from the
table or not.

>>> +       dvfs_info = kzalloc(sizeof(struct exynos_dvfs_data), GFP_KERNEL);
>>
>> sizeof(*dvfs_info) ?? why don't make it static too, as you have other
>> stuff too.. ?
>> The better option for single image solution to allocate everything
>> dynamically, so that
>> unused drivers don't occupy any space.

?

>>> +       if ((len == 0) || (len / 2 > CPUFREQ_LEVEL_END)) {
>>
>> I really didn't like this limit you have put at the number of dvfs
>> points. Better
>> would be to use:

>>> +       dvfs_info->dvfs_init = true;
>>
>> why do you need this ?
> This is added to synchronize the interrupts.

How? You are setting it once in init() and not touching it afterwards. :)