lkml.org 
[lkml]   [2007]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 07/23] clocksource: rating sorted list

* Daniel Walker <dwalker@mvista.com> wrote:

> Converts the original plain list into a sorted list based on the clock
> rating. Later in my tree this allows some of the variables to be
> dropped since the highest rated clock is always at the front of the
> list. This also does some other nice things like allow the sysfs files
> to print the clocks in a more interesting order. It's forward looking.

Unfortunately this seems to be a step backwards to me. Please consider:

> if (clocksource_tsc.rating != 0 && check_tsc_unstable()) {
> clocksource_tsc.rating = 0;
> - clocksource_reselect();
> + clocksource_rating_change(&clocksource_tsc);
> change = 1;

this should be the following API:

clocksource_rating_change(&clocksource_tsc, 0);

a rating change can occur due to other things as well, not only due to
'tsc unstable' events. So keeping an API around that changes the rating
(and propagates all related changes), makes more sense to me.

also, a pure function call is the most natural (and most flexible) API,
for a centrally registered resource like a clock source driver. And a
rating change should imply a reselect too, obviously. That way we
replace 2 lines with 1 line - that's a real API improvement and a real
cleanup patch.

and that's precisely what Thomas' patch in -mm that your queue undoes
implements. (see: simplify-the-registration-of-clocksources.patch in
-mm) Have you considered Thomas' change when you dropped it?

Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-01-31 10:39    [W:0.442 / U:17.228 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site