* Kevin Hilman <khilman@xxxxxx> [110331 14:32]:
> Tony Lindgren <tony@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
