On 8 February 2013 00:38, amit kachhap <amit.kachhap@xxxxxxxxx> 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@xxxxxxxxxx> 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. :)
--
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
