Page MenuHomeFreeBSD

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

Authored by takawata on Mar 15 2020, 7:40 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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

takawata created this revision.Mar 15 2020, 7:40 AM
takawata updated this revision to Diff 69595.Mar 17 2020, 1:19 PM

Support autoload
Change driver source path.

Nice! Looking forward to this :-).

share/man/man4/pchtherm.4
50 ↗(On Diff #69595)

templature/s/temperature

takawata updated this revision to Diff 69638.Mar 18 2020, 2:52 PM

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
75 ↗(On Diff #69638)

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"

takawata updated this revision to Diff 69672.Mar 19 2020, 2:55 AM

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

danfe added a subscriber: danfe.Mar 19 2020, 6:33 AM
danfe added inline comments.
share/man/man4/pchtherm.4
69 ↗(On Diff #69672)

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).

73 ↗(On Diff #69672)

Should it be spelled as .Va t0temp?

75 ↗(On Diff #69672)

Should it be spelled as .Va t1temp?

78 ↗(On Diff #69672)

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

80 ↗(On Diff #69672)

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

82 ↗(On Diff #69672)

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

92 ↗(On Diff #69672)

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

105 ↗(On Diff #69672)

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).

takawata updated this revision to Diff 69684.Mar 19 2020, 3:10 PM

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.