[PATCH 03/74] ST SPEAr: Formalized timer support

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:52 Mon 13 Sep     , Shiraz Hashim wrote:
> Dear Jean,
> 
> On 9/7/2010 4:25 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>  
> >> -#endif
> >> +static void __init spear3xx_timer_init(void)
> >> +{
> >> +	spear_setup_timer();
> > why not call it directly?
> 
> A single level of abstraction was provided so that if archs (spear3xx/6xx/13xx)
> required to do something extra.
it does not seems to be needed at all so for now call it directly is enough
> >> +/* following defines the parent clock to be used */
> >> +#ifdef CONFIG_ARCH_SPEAR13XX
> >> +static char pclk_name[] = "osc1_24m_clk";
> >> +#else
> >> +static char pclk_name[] = "pll3_48m_clk";
> >> +#endif
> > why 2 name?
> 
> spear13xx has different parent clock for gpt than spear3xx/6xx.
so use a generic name which will be an alias specify on the soc clock
so here no need to do the ifdef specially if you want to support both of them
in the same kernel
> 
> > it will be better to use clkdev and create an alias
> 
> This is specifically to chose parent clock, which can be different in different
> architectures. clkdev corresponding to parent clock is already defined. We use
> name here to get the clock.
> 
> > btw how about move the timer to drivers/clocksource?
> 
> I don't know whether this is the right place. Every body else has placed timer
> code in his respective mach/plat directory.
> 
> >> +
> >>  static void clockevent_set_mode(enum clock_event_mode mode,
> >>  				struct clock_event_device *clk_event_dev);
> >>  static int clockevent_next_event(unsigned long evt,
> >> @@ -215,7 +222,8 @@ static void __init spear_clockevent_init(void)
> >>  
> >>  void __init spear_setup_timer(void)
> >>  {
> >> -	struct clk *pll3_clk;
> >> +	struct clk *clk;
> >> +	int ret;
> >>  
> >>  	if (!request_mem_region(SPEAR_GPT0_BASE, SZ_1K, "gpt0")) {
> >>  		pr_err("%s:cannot get IO addr\n", __func__);
> >> @@ -234,26 +242,32 @@ void __init spear_setup_timer(void)
> >>  		goto err_iomap;
> >>  	}
> >>  
> >> -	pll3_clk = clk_get(NULL, "pll3_48m_clk");
> >> -	if (!pll3_clk) {
> >> -		pr_err("%s:couldn't get PLL3 as parent for gpt\n", __func__);
> >> +	/* get the parent clock */
> >> +	clk = clk_get(NULL, pclk_name);
> >> +
> >> +	if (!clk) {
> >> +		pr_err("%s:couldn't get %s as parent for gpt\n",
> >> +				__func__, pclk_name);
> >>  		goto err_iomap;
> >>  	}
> >>  
> >> -	clk_set_parent(gpt_clk, pll3_clk);
> >> +	clk_set_parent(gpt_clk, clk);
> > ??
> > why not do this in the clock file?
> 
> The functional clock of the timer very much depends on the parent clock it
> selects. This is a part of the timer itself, hence it is kept here.
it's seems to be more soc depends than timer

Best Regards,
J.