Page MenuHomeFreeBSD

New driver pchtherm(4), PCH installed thermal sensor.
ClosedPublic

Authored by takawata on Mar 15 2020, 7:40 AM.
Tags
None
Referenced Files
F103810613: D24077.id69672.diff
Fri, Nov 29, 5:59 PM
F103810470: D24077.id70047.diff
Fri, Nov 29, 5:56 PM
Unknown Object (File)
Tue, Nov 26, 6:48 AM
Unknown Object (File)
Tue, Nov 26, 4:39 AM
Unknown Object (File)
Tue, Nov 26, 3:40 AM
Unknown Object (File)
Sun, Nov 17, 2:17 PM
Unknown Object (File)
Sun, Nov 17, 7:43 AM
Unknown Object (File)
Sun, Nov 17, 12:29 AM

Details

Summary

This driver is for thermal sensor in Intel PCH chipset.
This exports temperature and some threshold temperature values via sysctl .

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29951
Build 27768: arc lint + arc unit

Event Timeline

Support autoload
Change driver source path.

Nice! Looking forward to this :-).

share/man/man4/pchtherm.4
51

templature/s/temperature

fix man page.
Add files.x86 entry to static configuration.

Nice. Tested on Kaby Lake-Y (Google Pixelbook, i7-7Y75).

Maybe only spam all the info into dmesg when bootverbose is on? I like cool new dmesg lines but 7 lines for a thermal sensor does seem like a bit too much.

share/man/man4/pchtherm.4
76

missing space "andunder"

sys/dev/intel/pchtherm.c
89 ↗(On Diff #69638)

"Haswel" → "Haswell"

172 ↗(On Diff #69638)

alart? Alert? (this same mistake/typo is in all the places)

194 ↗(On Diff #69638)

"Throtting" → "Throttling"

234 ↗(On Diff #69638)

typo "Cucrrent" → "Current"

262 ↗(On Diff #69638)

typo "Catastorophic" → "Catastrophic"

Fix typo.
Hide normal informational message under bootverbose.
Add comment on alert register handling.

danfe added inline comments.
share/man/man4/pchtherm.4
70

When reached ... will shutdown — sequence of tenses seems wrong here, suggest getting advice from a native speaker. Also, each sentence should start on a new line (after period).

74

Should it be spelled as .Va t0temp?

76

Should it be spelled as .Va t1temp?

79

Missing newline after dot (end of sentence) and space before opening parenthesis.

81

Missing newline after dot (end of sentence) and space before opening parenthesis.

83

Sentence break => new line; should it be spelled as .Va pmtime (as this is a reference to adjacent variable)?

93

Perhaps should be: read-only, do not support...

106

I think you've also spelled your name wrong. :-)

sys/dev/intel/pchtherm.c
74 ↗(On Diff #69672)

Should #include block be visually separated with an extra blank line?

101 ↗(On Diff #69672)

Local variables are typically separated from the function body with a blank line.

125 ↗(On Diff #69672)

Inconsistent vertical spacing in local variables block.

151 ↗(On Diff #69672)

Missing space after if and before { (ditto for the line below).

155 ↗(On Diff #69672)

Missing space after else.

160 ↗(On Diff #69672)

Missing spaces.

168 ↗(On Diff #69672)

Superfluous spaces (ditto below).

242 ↗(On Diff #69672)

Missing space after | (also below).

267 ↗(On Diff #69672)

Missing spaces again here and below.

289 ↗(On Diff #69672)

Functions should be visually separated from each other for better readability.

296 ↗(On Diff #69672)

Ditto (and below).

Fix man pages wording.
Source coude style fix.

@takawata wrote:

Source coude style fix.

Thanks. Couple of style bugs are still there (search for if( without the space), but I don't want to cherry pick each particular case and just leave it at your discretion.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 31 2020, 6:25 AM
This revision was automatically updated to reflect the committed changes.

fix man page.
Add files.x86 entry to static configuration.

It seems that you never actually added the files.x86 change.
At least, I do not see it.