http://defect.opensolaris.org/bz/show_bug.cgi?id=6499
Summary: Implement code review feedback for Dan Mick
Classification: Development
Product: power-mgmt
Version: unspecified
Platform: Other
OS/Version: Solaris
Status: NEW
Severity: minor
Priority: P3
Component: c-state
AssignedTo: bill.holler at sun.com
ReportedBy: bill.holler at sun.com
CC: tesla-dev at opensolaris.org
hpet.h:
Copyright date
SCCS IDs
43 * Using the top bit of a 64-bit hrtime_t reduces the max cyclic timeout
44 * we can track to about 260 thousand years after boot.
What does "using the top bit" mean? You mean it's used for something
else, or are you talking about signed int, or?...
HPET_NOT_SUPPORT presumably should be "no support"
HPET_INTRRUPT_SUPPORT needs an E
All the "/* support" comments there should presumably be "supports"
*configure is used ... at some CPR callback time? It'd be nice
if its name indicated that; configure is pretty generic.
The reason I was asking about gethpethrtime is the XXX comment by it.
That should be removed, or the function should, one of the two.
Why are the lapic_ functions called "schedule"? They actually do the
transition, right? and for that matter, why are they called hpet_lapic_?
Which one's being scheduled?...or used?...
why are some of the entry points called non-hpet names, some have hpet
embedded, and some start with hpet_?
hpet_acpi.h:
39 #include <sys/acpi/acresrc.h> /* XXX: needed? */
should probably figure that out pre-putback
"femto" is spelled without a 'p', despite what some idiot who wrote the
HPET spec, and half the Internet, think
cyclic.h: probably shouldn't be included in the push
hpet_acpi.c:
37 * This code is in pcplusmp.
seems odd.
I'd have put the large comment explaining HPET in an include file, with
a reference to it here if you wish, but along with the hpet definition
68 * replacement" for the PIC and RTC hardware timers. These first two
69 * timers are reserved for that purpose.
does that mean this file avoids using the first two timers, or the spec
69 * timers are reserved for that purpose.
does that mean this file avoids using the first two timers, or the spec
requires it? I think you are not using them for PIC and RTC, but
one could interpret this statement as implying that.
74 * Mutual exclusion of HPET access is provided at the caller level.
doesn't really tell me something I can act on; are you saying "make your
own mutex before calling routines in this file"?
78 * HPET Specification 1.0a defines the HPET to be memory mapped to 1024
bytes
"be memory mapped to" should be "occupy"
I'd much prefer the #defines to be in hpet_acpi.h, at least, if not hpet.h, or
if you really want them private, hpet_impl.h, but not in the .c file.
static prototypes: eh. There are a whole lot of them, and they clutter
the file, but I'm OK I guess. 50/50 on moving them to hpet_acpi.h.
236 * Setup pointers to access symbols in pcplusmp.
"Set up". (Setup is a noun.)
274 hpet.supported = 0;
is superfluous, although I'm not very offended by it
276 if (idle_cpu_no_deep_c)
277 return (DDI_FAILURE);
278
279 if (!cpuid_deep_cstates_supported())
280 return (DDI_FAILURE);
Why here? Why not set up the info, and then let the configure() callback
enable conditionally?
hpet_memory_map() doesn't seem worth the extra function, and I wonder
about hpet_read_gen_cap and gen_config; are they there for dtrace/kmdb, or?..
=== stop here on 6 Feb
--
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
