[U-Boot] [RFC][Timer API] Revised Specification - Implementation details

[Simon Glass]

Hi Graeme,

On Wed, Jun 15, 2011 at 1:38 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> On 16/06/11 02:03, Simon Glass wrote:

>> - the common case is min I think, so let's get rid of the min prefix
>> so everyone will use it without question or needing to read screeds of
>> doc
>
> I don't like this idea - Extrapolate this to dropping the 'ms' common case
> and you end up with:
>
> u32 time_delta(u32 from, u32 to)
> u32 time_delta_max(u32 from, u32 to)
> u32 time_delta_raw(u32 from, u32 to)
>
> u32 time_us_delta(u32 from, u32 to)
> u32 time_us_delta_max(u32 from, u32 to)
> u32 time_us_delta_raw(u32 from, u32 to)
>
> Not as grep'able IMHO

The reason I suggested this is not because it is common, in that ms is
more common than us. It is that users will find time_ms_delta_min
confusing. There is a min, max and raw - which should they use? While
everyone on this list is a genius there will be other people who will
choose one at random and run with it.

To my mind 'min' is the safe one and the others will be rarely used.

I agree that removing ms is a bad idea. But this is pretty grep'able
for sensible values of grep:

u32 time_delta_ms(u32 from, u32 to)
u32 time_delta_us(u32 from, u32 to)

[hide these away in the closet:
u32 time_delta_max_ms(u32 from, u32 to)
u32 time_delta_raw_ms(u32 from, u32 to)
u32 time_delta_max_us(u32 from, u32 to)
u32 time_delta_raw_us(u32 from, u32 to)
]

int time_since_ms(u32 from)

Also bear in mind that _us() will be seldom used. I will cheerfully
use it for boot timing though.

BTW should the deltas return a signed value?

>
>> - would prefer the ms and us at the end as I think it reads better.
>> Getting the time is the important bit - the units are generally at the
>> end
>
> Hmm, time_since_ms or time_ms_since - I suppose either reads OK - But if I
> keep min/max/raw, I find time_min_ms_since (barely) easier in the eye than
> time_min_since_ms - I'm not bothered either way and will go with the
> general consensus

No I mean _ms at the end, so it is always last. time_min_ms_since()
sounds like a martian trying to learn English. See my comment above:
time_since_ms() - we shouldn't even define time_since_raw_ms() and
time_since_max_ms() in my view.

>
>> This code from your excellent wiki page bothers me. Can we find a way
>> to shrink it?
>>
>>                 now = timer_ms_now();
>>                 if (time_ms_delta_min(start, now)> timeout)
>>
>> How about:
>>
>>                 if (time_since_ms(start) > timeout)
>>
>> The idea of the time 'since' an event is more natural than the delta
>> between then and now which seems more abstract.
>
> I tend to agree - I have been thinking about 'since' functions for a while now

Cool.

>
> [snip]
>
>>>
>>> With the 'time_ms_' prefix, it's starting to get rather long, and I'm
>>> pushing a lot of timeout checks beyond 80 columns - especially when
>>> existing code has variables like 'start_time_tx' - I'm starting to consider
>>> dropping the time_ prefix (all time functions will still have a ms_ or us_
>>> prefix anyway) and rename a lot of variables
>>
>> Now I see why you want units at the start. Seems a bit ugly to me -
>> which lines are getting long - do you mean the time delta check line?
>> If so see above, or perhaps it is shorter without _min.
>
> An example from drivers/net/ns7520_eth.c:
>
>        ulStartJiffies = time_ms_now();
>        while (time_ms_delta_min(ulStartJiffies, time_ms_now())
>                        < NS7520_MII_NEG_DELAY) {
>
> Could be reduced to:
>
>        ulStartJiffies = time_ms_now();
>        while (time_min_ms_since(ulStartJiffies) < NS7520_MII_NEG_DELAY) {
>
> And with a rename:
>
>        start_ms = time_ms_now();
>        while (time_min_ms_since(start_ms) < NS7520_MII_NEG_DELAY) {
>

or:

        start_ms = time_now_ms();
        while (time_since_ms(start_ms) < NS7520_MII_NEG_DELAY) {

Regards,
Simon

> Regards,
>
> Graeme
>