Re: [PATCH V3 4/4] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 20 May 2014 22:19, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> Note that in the old code you used to set the "emc" clock before the
> transition to the intermediate clock.  Now you don't.  Are you sure
> it's OK to change this order?

Yeah, I have seen that and as Stephen didn't had any objection to the
change I thought it is probably fine.

>> @@ -98,10 +88,21 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>>         else
>>                 clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
>>
>> -       ret = tegra_cpu_clk_set_rate(rate * 1000);
>> +       if (rate == clk_get_rate(pll_p_clk))
>
> Shouldn't this be "rate * 1000 =="?

Yes.

>> +               goto out;
>> +
>> +       ret = clk_set_rate(pll_x_clk, rate * 1000);
>> +       if (ret) {
>> +               pr_err("Failed to change pll_x to %lu\n", rate);
>> +               goto out;
>> +       }
>
> I feel like this should be in tegra_target_intermediate(), since it
> could fail (right?).  Essentially we want to be as sure as possible
> that tegra_target() doesn't return an error code.
>
>
>> +
>> +       ret = clk_set_parent(cpu_clk, pll_x_clk);
>
> Presumably this won't actually ever fail, since that violates the
> rules that target() shouldn't fail if you used an intermediate
> frequency.

Don't know, it looks this rule isn't going to last long.. Let me see
if I can improve it a bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/