On Sun, Mar 21 2021, Mark Kettenis <mark.kette...@xs4all.nl> wrote: >> Date: Sun, 21 Mar 2021 17:22:05 +0100 >> From: Klemens Nanni <k...@openbsd.org> >> >> On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote: >> > > Date: Sun, 21 Mar 2021 01:02:53 +0100 >> > > From: Klemens Nanni <k...@openbsd.org> >> > > >> > > apm(4/arm64) merely provides an all zero/unknown stub for those values, >> > > e.g. apm(8) output is useless. >> > > >> > > Hardware sensors however provide the information: >> > > >> > > $ sysctl hw.sensors >> > > hw.sensors.rktemp0.temp0=32.22 degC (CPU) >> > > hw.sensors.rktemp0.temp1=33.89 degC (GPU) >> > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage) >> > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes) >> > > hw.sensors.cwfg0.percent0=58.00% (battery percent) >> > > >> > > Diff below simply copies them over using apm_setinfohook() >> > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same): >> > > >> > > $ apm >> > > Battery state: high, 58% remaining, 259 minutes life estimate >> > > A/C adapter state: not known >> > > Performance adjustment mode: auto (408 MHz) >> > > >> > > Feedback? OK? >> > >> > This doesn't scale. >> What exactly does not scale? The way how various drivers hook into >> apm(4)? I'm not familiar enough (yet) with the framework(s) to judge >> their overall design and/or interop strategy off hand. > > Since there is no firmware abstraction like on i386 (and to some > extent amd64) there will be many drivers that provide battery > informarion on armv7/arm64. > >> > The whole apm(4) interface is too closely tied to the original APM >> > BIOS model. Even with acpi(4) it is a bit of a square peg for a round >> > hole, for example on machines with more than one battery. >> Right, there is hardware which doesn't fit this model and our apm* is >> not exactly up to speed with those, but I still don't see actual issues >> (as per above). >> >> > I'm not even sure apm(8) should bother reporting some sort of battery >> > status. >> apm* showing battery information seems only naturally to me since that >> is the driver/daemon/tool one issues power commands with, often based >> on such information - think of `apmd -z 15': given that I want my box >> to safely suspend around 15% remaining battery, it seems only >> consequential that apm* is the same tool I query such information with. > > apmd(8) and apm(8) are not the same tool though. Having a deamon to > run scripts on certain power-state changes makes some sense, but I'd > argue that sensorsd(8) could be a reasonable place to implement this > as well.
When I dived into the apm(4) and apmd(8) code I noted how redundant it was considering sensorsd(8). But the user interface of sensorsd(8) needs a fair bit of pondering before it can be proposed as a replacement. sensorsd(8) is fairly generic and its configuration format, while extensible, isn't easy to use. To be fair I didn't even try to make it execute stuff based on battery level. I suspect it would just be useful to execute commands based on changes in one driver, when what you need to mimic apmd(8) is to merge the values coming from different drivers. > The need to run apmd(8) in order to suspend/hibernate is a bit of an > historical wart. It's not clear to me right now why this was needed and why it's not needed anymore. (I'm just talking about basic suspend/hibernate, not about the nifty -z/-Z additions in apmd(8)). > And we already removed powermanagement state support > from apmd(8). These days, users should probably just use > /etc/sysctl.conf to set the powermanagement state instead of starting > apmd(8) with the appropriate -A, -H or -L option. Indeed, -A/-H/-L are just syntactical sugar. I wouldn't mind seeing them go. >> > But if it does, my suggestion would be to make it use the >> > sensors framework. That would need some work though such that drivers >> > can mark sensors as providing battery information. Maybe add a >> > SENSOR_FBATTERY flag? Why the 'F' and not just SENSOR_BATTERY? >> You mean apm(8) directly using sensor_find(9) or so to look for >> suitable sensors instead of relaying values throuh apm(4)? > > That's a possibility. But having apm(4) do that might be a useful > middle ground as it would mean you don't have to change the userland > tools just yet. I'm not familiar at all with the sensors framework, but if presenting the same simple apm(4) interface is doable using sensors, then you can count me as a reviewer! :) (I don't know where the apm(4) interface comes from, but it looks like we have a bunch of ports also using it.) > My main concern with your diff is having to add APM > hooks in all the drivers... Or maybe we need a nicer way to register APM hooks. IIUC apm_setinfohook was copied from miod's work on loongson where a single driver is in charge, depending on the underlying hardware. So it probably can't solve all use cases as is. >> It sounds simpler in principal but that's just my naive guess; >> I'll take a curious look into whether/how this could work. > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
