lkml.org 
[lkml]   [2006]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch 04/19] Add a framework to manage clock event devices.
From
Date
On Thu, 2006-11-09 at 23:38 +0000, Thomas Gleixner wrote:
> + * struct clock_event_device - clock event descriptor
> + *
> + * @name: ptr to clock event name
> + * @capabilities: capabilities of the event chip
> + * @max_delta_ns: maximum delta value in ns
> + * @min_delta_ns: minimum delta value in ns
> + * @mult: nanosecond to cycles multiplier
> + * @shift: nanoseconds to cycles divisor (power of two)
> + * @set_next_event: set next event
> + * @set_mode: set mode function
> + * @suspend: suspend function (optional)
> + * @resume: resume function (optional)
> + * @evthandler: Assigned by the framework to be called by the low
> + * level handler of the event source
> + */
it would be nice if the datastructure was "pure"; eg entirely owned by
the source (and could be made const), however the only way I can see
that done is by having a private duplicate datastructure in each clock
driver... which is way overkill for one single function pointer ;(


well you could do

struct clock_event_device
{
const struct clock_ops *ops;
void *instance;
evthndlr_t *evthandler;
}
that way if you have, say, 3 hpet channels you can use the same
"ops" (or maybe "props") structure for all three, but still register the
per channel state as well..


you maybe also want to have a "costs" member, so that you can pick the
cheapest available timer..


> +struct clock_event_device {
> + const char *name;
> + unsigned int capabilities;
> + unsigned long max_delta_ns;
> + unsigned long min_delta_ns;
> + unsigned long mult;
> + int shift;
> + void (*set_next_event)(unsigned long evt,
> + struct clock_event_device *);
> + void (*set_mode)(enum clock_event_mode mode,
> + struct clock_event_device *);
> + void (*event_handler)(struct pt_regs *regs);

is pt_regs really really needed here? We got rid of it in most places
(and made it a per tast struct thing), I wonder if it can be made to go
away here too.


> +/*
> + * Start up an event device
> + */
> +static void startup_event(struct clock_event_device *evt, unsigned int caps)
> +{
> + int mode;
> +
> + if (caps == CLOCK_CAP_NEXTEVT)

isn't caps a bitfield ? if so, shouldn't this be a & ?


> + */
> +int clockevents_set_next_event(ktime_t expires, int force)
> +{
> + struct local_events *devices = &__get_cpu_var(local_eventdevices);
> + struct clock_event_device *nextevt = devices->nextevt;
> + int64_t delta = ktime_to_ns(ktime_sub(expires, ktime_get()));
> +
> + if (delta <= 0 && !force) {
> + devices->expires_next.tv64 = KTIME_MAX;
> + return -ETIME;
> + }

hmmm so if I set a timer 10 nsec in the future, and then I get an
interrupt, I suddenly get an infinite time timer? Sounds more like a
case of "please just run it right away"


> + * Resume the cpu local clock events
> + */
> +static void clockevents_resume_local_events(void *arg)
> +{
> + struct local_events *devices = &__get_cpu_var(local_eventdevices);
> + int i;
> +
> + for (i = 0; i < devices->installed; i++) {
> + if (devices->events[i].real_caps)
> + startup_event(devices->events[i].event,
> + devices->events[i].real_caps);
> + }
> + touch_softlockup_watchdog();
> +}

what is this watchdog touching for?

> +static int clockevents_cpu_notify(struct notifier_block *self,
> + unsigned long action, void *hcpu)
> +{
> + switch(action) {
> + case CPU_UP_PREPARE:
> + break;

don't you want to start the per cpu timer in such a case?




--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

-
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: 2006-11-10 10:51    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site