Details

Reviewers

rpaulo
avg
hrs

Group Reviewers

manpages

Commits

rS351930: Add embedded Managed Object Format blob access to acpi_wmi(4).

Summary

This patch extracts Managed Object Format object embedded to ACPI WMI devices, which appears some laptops like ThinkPad.

It is compiled binary form and needs extra tools to obtain human readable output. (http://github.com/pali/bmfdec)

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26282
Build 24764: arc lint + arc unit

Event Timeline

takawata created this revision.Sep 5 2019, 3:45 AM

Harbormaster completed remote builds in B26282: Diff 61664.Sep 5 2019, 3:45 AM

If this driver just provides a single sysctl, then I think that it would be simpler to add that sysctl to acpi_wmi.
Do you plan to have any other functionality in acpi_wmi_bmof?

Add the feature to acpi_wmi driver, instead of creating new driver.

Harbormaster completed remote builds in B26286: Diff 61671.Sep 5 2019, 9:00 AM

Free MOF buf only when successfilly detach.

Harbormaster completed remote builds in B26287: Diff 61672.Sep 5 2019, 9:10 AM

Some nit, pointed out by hrs@

Harbormaster completed remote builds in B26289: Diff 61693.Sep 5 2019, 3:37 PM

No features other than obtaining MOF is expected, so I add the feature in parent driver, instead of having new driver.

avg added inline comments.Sep 6 2019, 5:38 AM

share/man/man4/acpi_wmi.4
61 ↗	(On Diff #61693)	Are you sure about '-width indent' ? I am not an expert on this, it just looks unusual.
63 ↗	(On Diff #61693)	This line looks too long. Please break it up. Also, I am not sure if any special mark up is needed for URLs.
94 ↗	(On Diff #61693)	I would use something else rather than "blah blah" here. Maybe "..." or "etc".
116 ↗	(On Diff #61693)	No need for the space before the final '.'.
sys/dev/acpi_support/acpi_wmi.c
281 ↗	(On Diff #61693)	Please use the standard FreeBSD indentation for continuation lines (+4 spaces) here and in other new code.
282 ↗	(On Diff #61693)	Missing space before '}'.
286 ↗	(On Diff #61693)	Continuation
287 ↗	(On Diff #61693)	missing space
289 ↗	(On Diff #61693)	missing space
291 ↗	(On Diff #61693)	'(' should be on the original line. Also, wrong indentation.

Style nit.

Harbormaster completed remote builds in B26301: Diff 61717.Sep 6 2019, 7:57 AM

A couple more nits.

share/man/man4/acpi_wmi.4
63 ↗	(On Diff #61717)	I think that it is preferred that a new sentence is started on a new line. That is to say that there should be a line break after each period that ends a sentence.
sys/dev/acpi_support/acpi_wmi.c
281 ↗	(On Diff #61717)	Extra space before ';'
282 ↗	(On Diff #61717)	A blank line is needed after a variable definition block.
291 ↗	(On Diff #61717)	I think that we do not do a nested continuation, so this line should probably be indented the same as the previous one. Though, that would be less readable, so I don't insist on such change.