On Mon, Mar 22 2021, Klemens Nanni <k...@openbsd.org> wrote:
> Better diff at the end thanks to jca's eyeballing, see comments inline.
>
> kettenis: I see room for improvement in our subsystems and their
> interactions, but I don't think the current situation is bad enough to
> leave those bits out for now.
>
> Feedback? OK?
>
[...]
>> > @@ -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.
> Oops. Both `val' and `rtt' can be "useless" and we could end up with
> a partially updated `cwfg_power'.
With the current code, rtt is always meaningful. I was really talking
only about bat_percent being possibly uninitialized.
> If we always reset all values up front and then update whatever is
> possible, we can avoid the intermediate variable completely and end up
> with `cwfg_power' providing consistent readings.
Except if reading one register fails and we take the goto done shortcut.
In that case sensors and apm get out of sync, which is bad IMHO.
[...]
>> 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 did `sc->sc_sensor[CWFG_SENSOR_SOC].value / 1000' first but ended up
> with bogus values, hence the `bat_percent' buffer to avoid arithmetics.
>
>
>> 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.
> But that isn't the case? From apm(4):
>
> APM_BATTERY_ABSENT
> No battery installed.
>
> The driver just can't tell us enough, but the battery is still there
> (we get good percentage/liftime readings), so
>
> APM_BATT_UNKNOWN
> Cannot read the current battery state.
>
> is only appropiate here imho.
>
>
>> What's the underlying hardware, does it involve a pluggable battery?
> Nope, Pinebook Pro with one internal battery and AC.
Since the battery can't be removed in your pinebook, then obviously
APM_BATTERY_ABSENT isn't very meaningful. But maybe the CellWise
hardware supports pluggable batteries. Anyway...
The diff below is mostly your diff, except:
- statically initialize cwfg_power with proper values. That way there's
a reason less for initializing it early by forcefully calling
cwfg_update_sensors().
- only update cwfg_power when we already fetched new values for sensors.
I hope that this approach will make changing the cwfg_update_sensors()
code easier.
If it works for you and you agree with this tweaked proposal, ok jca@
Index: cwfg.c
===================================================================
RCS file: /d/cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.2
diff -u -p -p -u -r1.2 cwfg.c
--- cwfg.c 22 Mar 2021 18:37:26 -0000 1.2
+++ cwfg.c 23 Mar 2021 23:49:22 -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,22 @@ struct cfdriver cwfg_cd = {
NULL, "cwfg", DV_DULL
};
+#if NAPM > 0
+struct apm_power_info cwfg_power = {
+ .battery_state = APM_BATT_UNKNOWN,
+ .ac_state = APM_AC_UNKNOWN,
+ .battery_life = 0,
+ .minutes_left = -1,
+};
+
+int
+cwfg_apminfo(struct apm_power_info *info)
+{
+ memcpy(info, &cwfg_power, sizeof(*info));
+ return 0;
+}
+#endif
+
int
cwfg_match(struct device *parent, void *match, void *aux)
{
@@ -202,6 +221,10 @@ cwfg_attach(struct device *parent, struc
sensor_task_register(sc, cwfg_update_sensors, 5);
+#if NAPM > 0
+ apm_setinfohook(cwfg_apminfo);
+#endif
+
printf("\n");
}
@@ -350,6 +373,11 @@ cwfg_update_sensors(void *arg)
if (val != 0xff) {
sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
sc->sc_sensor[CWFG_SENSOR_SOC].flags &= ~SENSOR_FINVALID;
+#if NAPM > 0
+ cwfg_power.battery_state = val > sc->sc_alert_level ?
+ APM_BATT_HIGH : APM_BATT_LOW;
+ cwfg_power.battery_life = val;
+#endif
}
/* RTT */
@@ -362,6 +390,9 @@ cwfg_update_sensors(void *arg)
if (rtt != 0x1fff) {
sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
+#if NAPM > 0
+ cwfg_power.minutes_left = rtt;
+#endif
}
done:
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
