On Sun, Mar 21 2021, Klemens Nanni <k...@openbsd.org> wrote:
> 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?
Feedback:
> Index: dev/fdt/cwfg.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 cwfg.c
> --- dev/fdt/cwfg.c 10 Jun 2020 17:51:21 -0000 1.1
> +++ dev/fdt/cwfg.c 20 Mar 2021 23:43:13 -0000
> @@ -32,12 +32,15 @@
> #include <sys/malloc.h>
> #include <sys/sensors.h>
>
> +#include <machine/apmvar.h>
> #include <machine/fdt.h>
>
> #include <dev/ofw/openfirm.h>
>
> #include <dev/i2c/i2cvar.h>
>
> +#include "apm.h"
> +
> #define VERSION_REG 0x00
> #define VCELL_HI_REG 0x02
> #define VCELL_HI_MASK 0x3f
> @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
> NULL, "cwfg", DV_DULL
> };
>
> +int cwfg_apminfo(struct apm_power_info *info);
Why out of the #if NAPM > 0 check?
> +#if NAPM > 0
> +struct apm_power_info cwfg_power;
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + bcopy(&cwfg_power, info, sizeof(*info));
There's no overlap between source and destination, better use memcpy.
(Else, better use memmove.)
> + return 0;
> +}
> +#endif
> +
> int
> cwfg_match(struct device *parent, void *match, void *aux)
> {
> @@ -202,6 +217,12 @@ cwfg_attach(struct device *parent, struc
>
> sensor_task_register(sc, cwfg_update_sensors, 5);
>
> +#if NAPM > 0
> + /* make sure we have the apm state initialized before apm attaches */
> + cwfg_update_sensors(sc);
> + apm_setinfohook(cwfg_apminfo);
> +#endif
> +
> printf("\n");
> }
>
> @@ -324,6 +345,7 @@ cwfg_update_sensors(void *arg)
> uint32_t vcell, rtt, tmp;
> uint8_t val;
> int error, n;
> + u_char bat_percent;
>
> if ((error = cwfg_lock(sc)) != 0)
> return;
> @@ -348,6 +370,7 @@ cwfg_update_sensors(void *arg)
> if ((error = cwfg_read(sc, SOC_HI_REG, &val)) != 0)
> goto done;
> if (val != 0xff) {
> + bat_percent = val;
> sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
> sc->sc_sensor[CWFG_SENSOR_SOC].flags &= ~SENSOR_FINVALID;
> }
If val == 0xff bat_percent is unitialized. Note that in this error
condition the sensors code leaves sc->sc_sensor[CWFG_SENSOR_SOC].value
alone.
> @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
> sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
> sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
> }
> +
> +#if NAPM > 0
> + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;
It's cool that this driver keeps track of the "alert level". acpibat(4)
also does that on amd64, but the apm(4)-through-acpi(4) userland
interface just hardcodes levels:
/* running on battery */
pi->battery_life = remaining * 100 / capacity;
if (pi->battery_life > 50)
pi->battery_state = APM_BATT_HIGH;
else if (pi->battery_life > 25)
pi->battery_state = APM_BATT_LOW;
else
pi->battery_state = APM_BATT_CRITICAL;
Maybe the levels reported by hardware couldn't be trusted? Or maybe
those hardcoded values were deemed good enough, back then. Anyway, on
this new hardware I think it makes sense to just trust the exported
values and later act if we get reports.
> + cwfg_power.ac_state = APM_AC_UNKNOWN;
This kinda points out the need for an AC driver on this platform.
If done in another driver, the code will need to be changed. But this
looks acceptable to me for now.
> + cwfg_power.battery_life = bat_percent;
So to keep apm and sensors in sync I think it would be better to reuse
the cached sc->sc_sensor[CWFG_SENSOR_SOC].value.
I don't know whether the error condition mentioned above happens
frequently with this driver. Reporting APM_BATT_ABSENT (and similar for
sensors) would be useful, and could be done in a subsequent step.
What's the underlying hardware, does it involve a pluggable battery?
> + cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
> +#endif
>
> done:
> cwfg_unlock(sc);
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
