On Wed, Mar 31, 2021 at 01:35:12PM +0200, Patrick Wildt wrote:
> If there's no alert-level property, then maybe we should just remove it.
> Then you could hard code a value for "below will be critical", like you
> now do with the 50%?
This makes sense, I did not remove it when writing the diff due to my
uncertainty about the design, intention and usage of properties (which
are not part of linux upstream's bindings) -- more reading and help from
patrick have changed that.
No longer look for the nonexistent "alert-level" property and hardcode
thresholds the same way acpipat(4) does.
Also don't bother with this threshold in the CONFIG register: our driver
and overall sensor framework is designed for polling values every N
seconds, so there is no need for the hardware to tell us whether the
threshold has been reached -- cwfg closely watches SOC readings and does
its own threshold metrics (APM_BATT_HIGH/LOW/CRITICAL).
NetBSD (from which this driver was ported) also handles "alert-level",
but they also do the same sensors polling we so, so I don't see why that
is done in the first place.
Linux does not look for "alert-level" property but still updates the
register -- although I fail to see any code that updates their driver's
`alert_level' value with anything, the seem to simply update the CONFIG
register with the zero initialised value. Their capacity calculations
are then also based on SOC readings like ours and do not use the
hardware threshold support at all.
Feedback? Objections? OK?
Index: cwfg.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.4
diff -u -p -r1.4 cwfg.c
--- cwfg.c 26 Mar 2021 22:54:41 -0000 1.4
+++ cwfg.c 31 Mar 2021 12:09:59 -0000
@@ -58,8 +58,6 @@
#define RTT_LO_MASK 0xff
#define RTT_LO_SHIFT 0
#define CONFIG_REG 0x08
-#define CONFIG_ATHD_MASK 0x1f
-#define CONFIG_ATHD_SHIFT 3
#define CONFIG_UFG (1 << 1)
#define MODE_REG 0x0a
#define MODE_SLEEP_MASK (0x3 << 6)
@@ -91,7 +89,6 @@ struct cwfg_softc {
uint8_t sc_batinfo[BATINFO_SIZE];
- uint32_t sc_alert_level;
uint32_t sc_monitor_interval;
uint32_t sc_design_capacity;
@@ -101,7 +98,6 @@ struct cwfg_softc {
#define CWFG_MONITOR_INTERVAL_DEFAULT 5000
#define CWFG_DESIGN_CAPACITY_DEFAULT 2000
-#define CWFG_ALERT_LEVEL_DEFAULT 0
int cwfg_match(struct device *, void *, void *);
void cwfg_attach(struct device *, struct device *, void *);
@@ -189,8 +185,6 @@ cwfg_attach(struct device *parent, struc
"cellwise,monitor-interval-ms", CWFG_MONITOR_INTERVAL_DEFAULT);
sc->sc_design_capacity = OF_getpropint(sc->sc_node,
"cellwise,design-capacity", CWFG_DESIGN_CAPACITY_DEFAULT);
- sc->sc_alert_level = OF_getpropint(sc->sc_node,
- "cellwise,alert-level", CWFG_ALERT_LEVEL_DEFAULT);
if (cwfg_init(sc) != 0) {
printf(": failed to initialize device\n");
@@ -271,7 +265,6 @@ done:
int
cwfg_set_config(struct cwfg_softc *sc)
{
- uint32_t alert_level;
uint8_t config, mode, val;
int need_update;
int error, n;
@@ -280,19 +273,6 @@ cwfg_set_config(struct cwfg_softc *sc)
if ((error = cwfg_read(sc, CONFIG_REG, &config)) != 0)
return error;
- /* Update alert level, if necessary */
- alert_level = (config >> CONFIG_ATHD_SHIFT) & CONFIG_ATHD_MASK;
- if (alert_level != sc->sc_alert_level) {
- config &= ~(CONFIG_ATHD_MASK << CONFIG_ATHD_SHIFT);
- config |= (sc->sc_alert_level << CONFIG_ATHD_SHIFT);
- if ((error = cwfg_write(sc, CONFIG_REG, config)) != 0)
- return error;
- }
-
- /* Re-read current config */
- if ((error = cwfg_read(sc, CONFIG_REG, &config)) != 0)
- return error;
-
/*
* We need to upload a battery profile if either the UFG flag
* is unset, or the current battery profile differs from the
@@ -387,9 +367,13 @@ cwfg_update_sensors(void *arg)
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;
+ if (val > 50)
+ cwfg_power.battery_state = APM_BATT_HIGH;
+ else if (val > 25)
+ cwfg_power.battery_state = APM_BATT_LOW;
+ else
+ cwfg_power.battery_state = APM_BATT_CRITICAL;
#endif
}
