Re: [PATCH 05/10] omap2+: Use dmtimer macros for clockevent

From: Tony Lindgren <tony@atomide.com>
To: Kevin Hilman <khilman@ti.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org
Subject: Re: [PATCH 05/10] omap2+: Use dmtimer macros for clockevent
Date: Thu, 31 Mar 2011 15:04:26 -0700	[thread overview]
Message-ID: <20110331220426.GC21887@atomide.com> (raw)
In-Reply-To: <8762qz9dnl.fsf@ti.com>

* Kevin Hilman <khilman@ti.com> [110331 14:32]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > This patch makes timer-gp.c to use only a subset of dmtimer
> > functions without the need to initialize dmtimer code early.
> >
> > Note that omap_dmtimer_init_one can eventually be moved to
> > omap2+ specific dmtimer.c.
> >
> > Also note that now with the inline functions, timer_set_next_event
> > becomes more efficient in the lines of assembly code.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> In looking closer at this patch, I have a few questions.
> 
> [...]
> 
> > @@ -75,7 +97,8 @@ static struct irqaction omap2_gp_timer_irq = {
> >  static int omap2_gp_timer_set_next_event(unsigned long cycles,
> >  					 struct clock_event_device *evt)
> >  {
> > -	omap_dm_timer_set_load_start(gptimer, 0, 0xffffffff - cycles);
> > +	__omap_dm_timer_load_start(clkev.io_base, OMAP_TIMER_CTRL_ST,
> > +						0xffffffff - cycles, 1);
> >  
> >  	return 0;
> 
> The creation of macros is causing some readability concern, at least for
> me...
> 
> In creating the macro versions of some of the functions, the macro
> version actually has different behavior which makes reading the code a
> little confusing.
> 
> I just noticed that the macro version of _load_start() doesn't actually
> do the "start", so you added the OMAP_TIMER_CTRL_ST when calling it.
> To do this, the macro version takes a 'ctrl' argument where as the
> "real" version only takes the autoreload argument.
> 
> If you're going to keep the same function name (but just add the __
> prefix, I would expect that the functionality of the function doesn't
> change.
> 
> If the functionality of the macro is different from the "real" function,
> it should probably just be given a different name.   In this case,
> probably dropping the _start suffix is probably enough.

OK good point.

> >  }
> > @@ -85,13 +108,13 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
> >  {
> >  	u32 period;
> >  
> > -	omap_dm_timer_stop(gptimer);
> > +	omap_dm_timer_stop(&clkev);
> >  
> >  	switch (mode) {
> >  	case CLOCK_EVT_MODE_PERIODIC:
> > -		period = clk_get_rate(omap_dm_timer_get_fclk(gptimer)) / HZ;
> > +		period = clkev.rate / HZ;
> >  		period -= 1;
> > -		omap_dm_timer_set_load_start(gptimer, 1, 0xffffffff - period);
> > +		omap_dm_timer_set_load_start(&clkev, 1, 0xffffffff - period);
> 
> Hmm, you're using the driver function here not the macro.  Is that
> intended?
> 
> Why not use the macro version here with OMAP_TIMER_CTRL_AR |
> OMAP_TIMER_CTRL_ST.

Hmm I guess I tried to change as little as possible here when I noticed
that there's some weirdness with the autoreload mode in the original
omap_dm_timer_set_load_start function that requires writing twice in
the autoreload case. Looks like that's not needed for the one-shot case.

Anyways, you're right, this should not be using the driver function to move
the rest of the timers to be under drivers/ eventually. Will post an
updated version after taking another look at the dmtimer hwmod series.

Regards,

Tony