[haiku-gsoc] Re: New timer patch (again) (Was: Re: First timer.diff (from private e-mails))
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Wed, 25 Jun 2008 12:41:13 +0200 CEST
"Dustin Howett" <alaricx@xxxxxxxxx> wrote:
> I have yet another patch for consideration (sending to haiku-gsoc so
> all discussion on it can get captured here).
>
> I haven't tried it on native hardware but it boots in qemu (in both
> uniprocessor mode and multiprocessor mode, the PIT and APIC timers
> get
> used)
>
> It addresses many of Axel's concerns (anything that remains, please
> let me know)
Looks much better now, thanks!
I still have a few comments mostly on the coding style, and I would
like to see them resolved before applying the patch:
> +++ src/system/kernel/arch/x86/arch_timer.c (working copy)
> -//#define TRACE_TIMER
> +#define TRACE_TIMER
That is probably not intended.
> + if(sBestTimer != NULL)
Just one of many occasions: there is always a space after if/while/for/
select/... keywords
> +static timer_info *timers[] = {
Global static, hence it should be called sTimers.
> + int cur_timer = 0;
> + int cur_prio = -1;
> + char cur_best_name[B_FILE_NAME_LENGTH];
>
> + cpu_status state = disable_interrupts();
>
> + while(true) {
> + timer_info *timer = timers[cur_timer++];
>
> + if(timer == NULL) break;
Same if/while issue, please always put the statement after "if" on the
next line. Naming cur_timer: first of all, it would be currentTimer,
and then, why use such a specific name (only to abbreviate it), when
"i" or "index" is not used? cur_best_name seems to be unused, and
doesn't make much sense either.
BTW a for loop would be more appropriate here.
> +/*
> + * Copyright 2008, Dustin Howett, dustin.howett@xxxxxxxxxx All
> rights reserved.
> + * Distributed under the terms of the MIT License.
> +
> + * Copyright 2005, Axel Dörfler, axeld@xxxxxxxxxxxxxxxxx All rights
> reserved.
> + * Distributed under the terms of the MIT License.
First of all, there would be a '*' in the separation line. Then, since
both licenses are MIT, you would fold the license together like this:
+ * Copyright 2008, Dustin Howett, dustin.howett@xxxxxxxxxx All rights
reserved.
+ * Copyright 2005, Axel Dörfler, axeld@xxxxxxxxxxxxxxxxx All rights
reserved.
+ * Distributed under the terms of the MIT License.
> +++ src/system/kernel/arch/x86/timers/pit.h (revision 0)
> +static status_t pit_clear_hardware_timer();
Always use (void) in plain C rather than this form; () means (...) in
C, not actually ().
> +++ src/system/kernel/arch/x86/timers/hpet.h (revision 0)
> +#endif
Please add the closing comment like:
+#endif // _KERNEL_ARCH_x86_TIMERS_HPET_H
(this is not the only place you left that out)
> +static int
> +hpet_set_legacy(struct hpet_regs *regs, bool enabled)
> +{
> + if(!HPET_IS_LEGACY_CAPABLE(regs)) return B_NOT_SUPPORTED;
> + if(enabled == true) regs->config |= HPET_CONF_MASK_LEGACY;
> + else regs->config &= ~HPET_CONF_MASK_LEGACY;
> + return B_OK;
> +}
Following our style guide a bit this would be:
static int
hpet_set_legacy(struct hpet_regs *regs, bool enabled)
{
if (!HPET_IS_LEGACY_CAPABLE(regs))
return B_NOT_SUPPORTED;
if (enabled)
regs->config |= HPET_CONF_MASK_LEGACY;
else
regs->config &= ~HPET_CONF_MASK_LEGACY;
return B_OK;
}
It's also not a crime to utilize spacing to make functions clearer and
cleaner :-)
> +++ src/system/kernel/arch/x86/arch_smp.c (working copy)
> if (args->num_cpus > 1) {
> +
> // setup some globals
Please don't do that.
> +++ headers/private/kernel/timer.h (working copy)
> +#include <module.h>
It obviously doesn't need this header anymore.
> +/* Timer info structure */
> +struct timer_info
> +{
That would be:
struct timer_info {
...
};
> + bool initialized;
Any particular reason to have this field in there?
It doesn't really belong there, and you could also make the structs
const without this.
Bye,
Axel.
Other related posts:
