Fixes: f87ba4522ec9e7 ("acpi_system76: Add support for ...")
Reported by: olce, jhb
Details
Details
- Reviewers
olce jhb - Commits
- rGf91464171d61: acpi_system76: Improve sysctl names
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 71391 Build 68274: arc lint + arc unit
Event Timeline
| sys/dev/acpi_support/acpi_system76.c | ||
|---|---|---|
| 32–34 | Sort headers (but not sys/param.h, see style(9)). | |
| 40 | Move this line up in the #include <sys/XXX.h> block. | |
| 42 | Please regroup this #include <dev/XXX> with the other above. I'd also put the <contrib/dev/XXX> group after the <dev/XXX> one. | |
| 130–133 | While here, there's only one low threshold. I'd suggest also to change the knob name as indicated, at least if the level passed is easy to correlate to the charge level. "min" (or "minimum") sees more explicit than "low", even if that is their EC's terminology. | |
| 134–140 | Same as above. | |
| sys/dev/acpi_support/acpi_system76.c | ||
|---|---|---|
| 133 | For another change/revision, but while here: It would be nice to have the unit of this value in the description (percentage?), as well as some check that the value is in range when setting (if the range can be reliably known; if not, just let setting fail, hopefully). For the latter, you can use either another type than SYSCTL_UINT or just add a range test in the handler for S76_CTRL_BCTL (set direction; same for S76_CTRL_BCTH). | |
| sys/dev/acpi_support/acpi_system76.c | ||
|---|---|---|
| 42 | I can't. <dev/acpica/acpivar.h> requires <contrib/...> to be included first. | |
| 130–133 | Done. Yes, it's their terminology. | |
| 133 | Yes, it's in percentage. should I add " (percentage)" at the end of its description? | |
| 399–410 | @olce I'm validating the sysctl inputs here. | |