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?
On Mon, Mar 22, 2021 at 01:21:02AM +0100, Jeremie Courreges-Anglas wrote:
> > 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?
Copied over from the loongson driver; the declaration doesn't hurt
outside but isn't needed either. I've removed it.
> > +#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.)
Right, I also copied this over from loongson (we can dust that one off
as well later on).
>
> > + 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.
Oops. Both `val' and `rtt' can be "useless" and we could end up with
a partially updated `cwfg_power'.
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.
>
> > @@ -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.
Yes, I explicitly want to use what hardware provides and we can still
tweak it to more sensible values if need be.
>
> > + 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.
Yup.
>
> > + 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 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.
Index: cwfg.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.1
diff -u -p -r1.1 cwfg.c
--- cwfg.c 10 Jun 2020 17:51:21 -0000 1.1
+++ cwfg.c 22 Mar 2021 16:36:47 -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,17 @@ struct cfdriver cwfg_cd = {
NULL, "cwfg", DV_DULL
};
+#if NAPM > 0
+struct apm_power_info cwfg_power;
+
+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 +216,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");
}
@@ -325,6 +345,14 @@ cwfg_update_sensors(void *arg)
uint8_t val;
int error, n;
+#if NAPM > 0
+ /* reset previous for consistent information */
+ cwfg_power.battery_state = APM_BATT_UNKNOWN;
+ cwfg_power.ac_state = APM_AC_UNKNOWN;
+ cwfg_power.battery_life = 0;
+ cwfg_power.minutes_left = -1;
+#endif
+
if ((error = cwfg_lock(sc)) != 0)
return;
@@ -350,6 +378,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 +395,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:
