Re: [PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Carsten Emde <C.Emde@osadl.org>]

Hi Thomas,

>>   static void __iomem *tcaddr;
>> +static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);
>
> what's that for? Indirection for indirection sake?
The code apparently provides two different functions to read the current 
clock - depending on whether the clock is setup in single-channel mode 
or not.

The default function is predefined in the struct clocksource clksrc:
   .read = tc_get_cycles
This structure element may later on be overwritten, if the counter is 
configured in single-channel mode:
   clksrc.read = tc_get_cycles32;

This is why my code sets do_sched_clock_get_cycles to clksrc.read before 
sched_clock is registered to make sure the appropriate read function 
will be used.

>> +u64 sched_clock_get_cycles(void)
> Static?
Yes, sure, should be static. Will fix it.

> What's wrong with implementing this like:
>
> static u64 sched_clock_get_cycles(void)
> {
> 	return tc_get_cycles(NULL);
> }
> Hmm?
See above.

>> +	local_irq_save(flags);
> What's the purpose of this?
The sched_clock_register() function contains
   WARN_ON(!irqs_disabled());
which I did not want to trigger.

Thanks,
	-Carsten.