Re: [PATCH 1/2] cpufreq: exynos4x12: Use the common clock framework to set APLL clock rate

Hi Sachin,

Hi Lukasz,

On 25 September 2013 16:52, Lukasz Majewski [off-list ref]
wrote:

quoted

 static void exynos4x12_set_apll(unsigned int index)
 {
-       unsigned int tmp, pdiv;
+       unsigned int tmp, freq = apll_freq_4x12[index].freq;

nit: It is better to put the 'freq' assignment on a new line.

checkpatch.pl wasn't complaining :-).
Also please consider below comment.

quoted

-       /* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
+       /* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
        clk_set_parent(moutcore, mout_mpll);

        do {

@@ -140,24 +140,9 @@ static void exynos4x12_set_apll(unsigned int

index) tmp &= 0x7;
        } while (tmp != 0x2);

-       /* 2. Set APLL Lock time */
-       pdiv = ((apll_freq_4x12[index].mps >> 8) & 0x3f);
+       clk_set_rate(mout_apll, freq * 1000);

Don't we need to check the return value of this?

The broken code isn't handling errors now (*_set_apll() function is
defined as void). Since this patch is a regression fix (for v3.12) I
just wanted to change as little as possible to provide a functional fix.

I think that regression fix shall not change much functionality -
therefore the exynosXXXX-cpufreq.c cleanup will be done for next kernel
release.

Same comments for the second patch too.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group