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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux