Created attachment 256226 [details] diff for sys/dev/acpica/acpi_cmbat.c [PATCH] Affected file: sys/dev/acpica/acpi_cmbat.c This patch implements _BTP (battery trip point). This is basically for laptop users. You set a warning level, e.g. 20%. Then devd will notify you when the battery reaches 20%. Quoting the ACPI specification version 6.5 section 10.2.2.14 ("_BTP"): "This object is used to set a trip point to generate an SCI whenever the Battery Remaining Capacity reaches or crosses the value specified in the _BTP object." I have another explanation here: https://github.com/random532/FreeBSD_Battery_Trip_Point The _BTP method is optional, so not every battery supports it. -- What I did (aka design choices): 1. The cmbat driver has a unique device_attach() routine. It loops multipe times until the battery controller is ready. So once that process is finished (aka the battery is registered), I feel like that is a good place to add all the optional acpi methods. For now, the only optional method that is supported would be _BTP. I add a function acpi_initialize_btp() and a handler function acpi_cmbat_btp_sysctl(). 2. I chose a sysctl for the trip point. It is possible to use /dev/acpi and acpiconf for this, which at first glance is more consistent with the current design. However, the trip point is read-write. The current battery reporting is read-only. Also, we'd need a new argument for acpiconf, because right now, there is only acpiconf -i X, which reads battery information. I can see a design where the userland command would be "acpiconf -i 0 -w 20", where w would be the warning level. But I think the sysctl is just fine. 3. More information is on github. I just want to put this out there. 4. So far tested on: ThinkPad x220 Thanks everyone.
So there's a number of problems with the attached patch, though I think they are all style(9) issues: No space between if and ( in a few places. + if(ACPI_SUCCESS(acpi_GetHandleInScope(acpi_get_handle(dev), "_BTP", &tmp))) + { + sc->btp_warning_level = ACPI_BATTERY_BTP_WARNING_LEVEL; + + struct sysctl_oid *cmbat_oid = device_get_sysctl_tree(dev); + SYSCTL_ADD_PROC(NULL, SYSCTL_CHILDREN(cmbat_oid), OID_AUTO, + "warning_level", CTLTYPE_INT | CTLFLAG_RW, dev, 0, + acpi_cmbat_btp_sysctl, "I" ,"battery warning level"); + } So here the struct sysctl_oid line should be the first line of this, with a blank line after it before the rest. Also the { for this should be at the end of the if line (which is one example of the if complaint above) Also, the continuation of the SYSCTL_ADD_PROC macro should be indented by 4 spaces. + if(req->newptr && acpi_BatteryIsPresent(dev)) { + /* Write request. */ + + SYSCTL_IN(req, &sc->btp_warning_level, sizeof(sc->btp_warning_level)); + + /* Correct bogus writes. */ + if(sc->btp_warning_level < 0 || sc->btp_warning_level > 100) + sc->btp_warning_level = ACPI_BATTERY_BTP_WARNING_LEVEL; + + /* Call _BTP method */ + newtp = sc->bix.lfcap * sc->btp_warning_level / 100; + as = acpi_SetInteger(acpi_get_handle(dev), "_BTP", (uint32_t) newtp); + + /* Error checking. */ + if (ACPI_FAILURE(as)) { + ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev), + "error setting _BTP --%s\n", AcpiFormatException(as)); + sc->btp_warning_level = 0; + } + } + + else if(req->newptr) /* Write request w/o battery. */ + sc->btp_warning_level = 0; + + else /* Read request. */ + SYSCTL_OUT(req, &sc->btp_warning_level, sizeof(sc->btp_warning_level)); + (same if complaints) The last two clauses in the if else chain should have { } around them. The /* Write request. */ comment should be indented one more tab I think that the error case should cause a return of EINVAL instead of 0 when the ACPI call fails. +} \ No newline at end of file The file needs to end in a newline Would you be able to correct these issues with the patch?
Hey, thank you for the feedback. Yes, I will edit this. I will also look into style(9).