On Mon, Apr 06, 2009 at 06:08:48PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> Mark A. Greer wrote:
>> diff --git a/arch/arm/mach-davinci/include/mach/time.h
>> b/arch/arm/mach-davinci/include/mach/time.h
>> index 16cc89f..e6b0944 100644
>> --- a/arch/arm/mach-davinci/include/mach/time.h
>> +++ b/arch/arm/mach-davinci/include/mach/time.h
>> @@ -28,6 +28,16 @@ enum {
>> #define IS_TIMER_TOP(id) ((id & 0x1))
>> #define IS_TIMER_BOT(id) (!IS_TIMER_TOP(id))
>> +/* Offsets of the 8 compare registers */
>> +#define CMP12_0 0x60
>> +#define CMP12_1 0x64
>> +#define CMP12_2 0x68
>> +#define CMP12_3 0x6c
>> +#define CMP12_4 0x70
>> +#define CMP12_5 0x74
>> +#define CMP12_6 0x78
>> +#define CMP12_7 0x7c
>
> Why not:
>
> #define CMP12(n) (0x60 + (n) * 4)
For consistency with all the other regiser offset defines.
>> diff --git a/arch/arm/mach-davinci/time.c b/arch/arm/mach-davinci/time.c
>> index f07bac8..33ca090 100644
>> --- a/arch/arm/mach-davinci/time.c
>> +++ b/arch/arm/mach-davinci/time.c
>> @@ -84,6 +84,7 @@ struct timer_s {
>> unsigned int id;
>> unsigned long period;
>> unsigned long opts;
>> + unsigned long flags;
>> void __iomem *base;
>> unsigned long tim_off;
>> unsigned long prd_off;
>> @@ -97,6 +98,9 @@ static struct timer_s timers[];
>> #define TIMER_OPTS_ONESHOT 0x01
>> #define TIMER_OPTS_PERIODIC 0x02
>> +#define TIMER_FLAGS_USE_COMPARE 0x01
>> +#define USING_COMPARE(t) ((t)->flags & TIMER_FLAGS_USE_COMPARE)
>> +
>
> Why not add this flag to 'opts' field instead? I don't think we need
> another 4-byte flag field, just add one more flag...
> Also, why not add 'cmp_off' field since you've made it possible to use
> different compare registers?
Because it requires masking so the USE_COMPARE bit doesn't get lost
when 'opts' is set in davinci_set_mode(). I've already changed it
to use opts but I'm still not sure I like it.
>> + if (USING_COMPARE(t))
>> + /*
>> + * Next interrupt should be the current time reg value plus
>> + * the new period (using 32 bit unsigned addition/wrapping
>
> Shouldn't it be "32-bit"?
Yes, thanks.
>> @@ -218,9 +235,14 @@ static void __init timer_init(void)
>> /* Register interrupt */
>> t->irqaction.name = t->name;
>> t->irqaction.dev_id = (void *)t;
>> - if (t->irqaction.handler != NULL)
>> - setup_irq(davinci_soc_info->timer_info->
>> - timer_irqs[t->id], &t->irqaction);
>> +
>> + if (t->irqaction.handler != NULL) {
>> + irq = USING_COMPARE(t) ?
>> + davinci_soc_info->timer_info->compare_irq :
>> + davinci_soc_info->timer_info->timer_irqs[t->id];
>
> Why 'compare_irq' isn't an array as well? TIMER1 has the compare registers
> too, not only TIMER0.
This has all been changed.
>> + if (clockevent_id == davinci_soc_info->timer_info->clocksource_id) {
>> + /* Only bottom timers can use compare regs */
>> + if (IS_TIMER_TOP(clockevent_id))
>> + printk(KERN_WARNING "davinci_timer_init: Invalid use "
>> + "of system timers. Results unpredictable.\n");
>> + else if ((davinci_soc_info->timer_info->compare_off == 0) ||
>> + (davinci_soc_info->timer_info->compare_irq == 0))
>> + printk(KERN_WARNING "davinci_timer_init: Invalid "
>> + "setup. Results unpredictable.\n");
>
> Results are quite predictable -- it just won't work. :-)
:)
Mark
--
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
