Dear Chander Kashyap,
On 5 January 2012 19:31, Chander Kashyap chander.kashyap@linaro.org wrote:
Hi Minkyu Kang,
On 5 January 2012 12:13, Minkyu Kang promsoft@gmail.com wrote:
Dear Chander Kashyap,
On 27 December 2011 17:48, Chander Kashyap chander.kashyap@linaro.org wrote:
Torsten Koschorrek koschorrek@synertronixx.de scb9328 ARM920T (i.MXL) diff --git a/arch/arm/cpu/armv7/exynos/clock.c b/arch/arm/cpu/armv7/exynos/clock.c index b101f96..88e2fc0 100644 --- a/arch/arm/cpu/armv7/exynos/clock.c +++ b/arch/arm/cpu/armv7/exynos/clock.c @@ -125,10 +125,14 @@ static unsigned long exynos_get_pwm_clk(void)
if (s5p_get_cpu_rev() == 0) { /*
- * CLK_SRC_PERIL0
- * CLK_SRC_{PERIL0 | PERIC0}
* PWM_SEL [27:24] */ +#ifdef CONFIG_EXYNOS5
- sel = readl(&clk->src_peric0);
+#else sel = readl(&clk->src_peril0); +#endif
NAK. We don't allow to using ifdef for separating SoCs. Please refer s5pc1xx case for solve it. This comment apply to this patch globally. Please remove '#ifdef CONFIG_EXYNOS5'.
I have tried to reuse the code. It is possible to remove #ifdef CONFIG_EXYNOS5' in clock.c with cpu_is_s5pcXXX check. Is it a acceptable solution? Or is it necessary to write SoC specific function in clock.c as done in case of s5pc1xx/clock.c.
Please Advice
Removing CONFIG_EXYNOS5 and following s5pc1xx case will not allow to reuse the code in clock.c. What is the technical hindrance of not using ifdefs?
No need to reuse the code, if SoCs are different. If need, please separate the functions.
Yes, though SoC's are different, the functionality in clock.c is similar. The only difference is the clock name in the clock structure for Exynos4 and Exynos5 but the functionality is exactly same in clock.c. To accommodate this change in clock name #ifdef is used.
Following is the function in clock .c which uses #ifdef to accommodate the different clock name in SoC's.
static unsigned long exynos_get_pwm_clk(void) { struct exynos_clock *clk = (struct exynos_clock *)samsung_get_base_clock(); unsigned long pclk, sclk; unsigned int sel; unsigned int ratio;
if (s5p_get_cpu_rev() == 0) { /* * CLK_SRC_{PERIL0 | PERIC0} * PWM_SEL [27:24] */ #ifdef CONFIG_EXYNOS5 sel = readl(&clk->src_peric0); #else sel = readl(&clk->src_peril0); #endif sel = (sel >> 24) & 0xf;
if (sel == 0x6) sclk = get_pll_clk(MPLL); else if (sel == 0x7) sclk = get_pll_clk(EPLL); else if (sel == 0x8) sclk = get_pll_clk(VPLL); else return 0;
/* * CLK_DIV_{PERIL3 | PERIC3} * PWM_RATIO [3:0] */ #ifdef CONFIG_EXYNOS5 ratio = readl(&clk->div_peric3); #else ratio = readl(&clk->div_peril3); #endif ratio = ratio & 0xf; } else if (s5p_get_cpu_rev() == 1) { sclk = get_pll_clk(MPLL); ratio = 8; } else return 0;
pclk = sclk / (ratio + 1);
return pclk; }
As listed above, the exynos_get_pwm_clk(() function is fully reusable for both Exynos4 and Exynos5. The #ifdef was used to get around the different clock names of Exynos4 and Exynos5.
Another option here could be, that the differing clock name (src_peril0 for Exynos4 and src_peric0 for Exynos5) is resolved by change the src_peric0 to src_peril0 for Exynos5, and clearly commenting the reason for the deviation from the user manual. Would this approach be acceptable ?
Using same clock's name is acceptable.
But exynos4 clock structure and exynos5 clock structure are different. I requested removing all ifdefs in your patch. So, I will not allow such cases.
+#ifdef CONFIG_EXYNOS4 +#include<asm/arch/clock_exynos4.h> +#elif defined CONFIG_EXYNOS5 +#include<asm/arch/clock_exynos5.h> #endif
Then, we should do like this,
struct exynos4_clock *clk = (struct exynos4_clock *)samsung_get_base_clock();
struct exynos5_clock *clk = (struct exynos5_clock *)samsung_get_base_clock();
how solve it?
And you should consider variation by cpu revision. If Exynos5 revision1 is different from Exynos4 revision1, then?
Thanks. Minkyu Kang.