Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
up an additional set of threshold and period handles, verifies matching
disabling functionality and extends setting the link bit to complementary
event configurations.
This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
enabled is ignored, since it does not match (should be disabling
ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
bit will be set. Note, having the link bit and auto-sleep in place activity
and inactivity indicate the power save state change and thus will only be
triggered once a state transition occurs. Since there is a separate AC bit
for ACTIVITY and for INACTIVITY, events can be linked independently from
each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
When one of both is disabled, the link bit will be removed. Hence, the
remaining event will not indicate a plain state change anymore, but occur
as a periodically triggered inactivity event or for each activity event
above the threshold.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl313_core.c | 414 +++++++++++++++++++++++++------
1 file changed, 333 insertions(+), 81 deletions(-)
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 1598562a38e2..9a0905e30de3 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -30,20 +30,38 @@
#define ADXL313_ACT_XYZ_EN GENMASK(6, 4)
#define ADXL313_INACT_XYZ_EN GENMASK(2, 0)
+#define ADXL313_REG_ACT_ACDC_MSK BIT(7)
+#define ADXL313_REG_INACT_ACDC_MSK BIT(3)
+#define ADXL313_COUPLING_DC 0
+#define ADXL313_COUPLING_AC 1
+
/* activity/inactivity */
enum adxl313_activity_type {
ADXL313_ACTIVITY,
ADXL313_INACTIVITY,
+ ADXL313_ACTIVITY_AC,
+ ADXL313_INACTIVITY_AC,
};
static const unsigned int adxl313_act_int_reg[] = {
[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
[ADXL313_INACTIVITY] = ADXL313_INT_INACTIVITY,
+ [ADXL313_ACTIVITY_AC] = ADXL313_INT_ACTIVITY,
+ [ADXL313_INACTIVITY_AC] = ADXL313_INT_INACTIVITY,
};
static const unsigned int adxl313_act_thresh_reg[] = {
[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
[ADXL313_INACTIVITY] = ADXL313_REG_THRESH_INACT,
+ [ADXL313_ACTIVITY_AC] = ADXL313_REG_THRESH_ACT,
+ [ADXL313_INACTIVITY_AC] = ADXL313_REG_THRESH_INACT,
+};
+
+static const unsigned int adxl313_act_acdc_msk[] = {
+ [ADXL313_ACTIVITY] = ADXL313_REG_ACT_ACDC_MSK,
+ [ADXL313_INACTIVITY] = ADXL313_REG_INACT_ACDC_MSK,
+ [ADXL313_ACTIVITY_AC] = ADXL313_REG_ACT_ACDC_MSK,
+ [ADXL313_INACTIVITY_AC] = ADXL313_REG_INACT_ACDC_MSK,
};
static const struct regmap_range adxl312_readable_reg_range[] = {
@@ -255,6 +273,13 @@ static const struct iio_event_spec adxl313_activity_events[] = {
.mask_separate = BIT(IIO_EV_INFO_ENABLE),
.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
},
+ {
+ /* activity, AC bit set */
+ .type = IIO_EV_TYPE_MAG_ADAPTIVE,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+ },
};
static const struct iio_event_spec adxl313_inactivity_events[] = {
@@ -265,6 +290,14 @@ static const struct iio_event_spec adxl313_inactivity_events[] = {
.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
BIT(IIO_EV_INFO_PERIOD),
},
+ {
+ /* inactivity, AC bit set */
+ .type = IIO_EV_TYPE_MAG_ADAPTIVE,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD),
+ },
};
enum adxl313_chans {
@@ -362,11 +395,58 @@ static int adxl313_set_inact_time_s(struct adxl313_data *data,
return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
}
+/**
+ * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.
+ *
+ * @data: The device data.
+ * @type: The activity or inactivity type.
+ *
+ * Provide a type of activity or inactivity, combined with either AC coupling
+ * set, or default to DC coupling. This function verifies, if the combination is
+ * currently enabled or not.
+ *
+ * Return if the provided activity type has AC coupling enabled or a negative
+ * error value.
+ */
+static int adxl313_is_act_inact_ac(struct adxl313_data *data,
+ enum adxl313_activity_type type)
+{
+ unsigned int regval;
+ bool coupling;
+ int ret;
+
+ ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, ®val);
+ if (ret)
+ return ret;
+
+ coupling = adxl313_act_acdc_msk[type] & regval;
+
+ if (type == ADXL313_ACTIVITY || type == ADXL313_INACTIVITY)
+ return coupling == ADXL313_COUPLING_DC;
+ else
+ return coupling == ADXL313_COUPLING_AC;
+}
+
+static int adxl313_set_act_inact_ac(struct adxl313_data *data,
+ enum adxl313_activity_type type)
+{
+ unsigned int coupling;
+
+ if (type == ADXL313_ACTIVITY_AC || type == ADXL313_INACTIVITY_AC)
+ coupling = ADXL313_COUPLING_AC;
+ else
+ coupling = ADXL313_COUPLING_DC;
+
+ return regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+ adxl313_act_acdc_msk[type], coupling);
+}
+
static int adxl313_is_act_inact_en(struct adxl313_data *data,
enum adxl313_activity_type type)
{
unsigned int axis_ctrl;
unsigned int regval;
+ int coupling;
int axis_en, int_en, ret;
ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
@@ -374,7 +454,7 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
return ret;
/* Check if axis for activity are enabled */
- if (type == ADXL313_ACTIVITY)
+ if (type == ADXL313_ACTIVITY || type == ADXL313_ACTIVITY_AC)
axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
else
axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
@@ -386,7 +466,12 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
int_en = adxl313_act_int_reg[type] & regval;
- return axis_en && int_en;
+ /* Return true if configured coupling matches provided type */
+ coupling = adxl313_is_act_inact_ac(data, type);
+ if (coupling < 0)
+ return coupling;
+
+ return axis_en && int_en && coupling;
}
static int adxl313_set_act_inact_en(struct adxl313_data *data,
@@ -396,15 +481,26 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
unsigned int axis_ctrl;
unsigned int threshold;
unsigned int inact_time_s;
- int act_en, inact_en;
- bool en;
+ int act_en, inact_en, act_ac_en, inact_ac_en;
+ bool en, act_inact_ac;
int ret;
- if (type == ADXL313_ACTIVITY)
+ /*
+ * In case of turning off, assure turning off a correspondent coupling
+ * event. In case of not matching coupling, simply return.
+ */
+ if (!cmd_en) {
+ /* Expected positive true if coupling matches coupling type */
+ if (adxl313_is_act_inact_ac(data, type) <= 0)
+ return 0;
+ }
+
+ if (type == ADXL313_ACTIVITY || type == ADXL313_ACTIVITY_AC)
axis_ctrl = ADXL313_ACT_XYZ_EN;
else
axis_ctrl = ADXL313_INACT_XYZ_EN;
+ /* Start modifying configuration registers */
ret = adxl313_set_measure_en(data, false);
if (ret)
return ret;
@@ -414,12 +510,16 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
if (ret)
return ret;
+ act_inact_ac = type == ADXL313_ACTIVITY_AC || ADXL313_INACTIVITY_AC;
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+ adxl313_act_acdc_msk[type], act_inact_ac);
+
ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
if (ret)
return ret;
en = cmd_en && threshold;
- if (type == ADXL313_INACTIVITY) {
+ if (type == ADXL313_INACTIVITY || type == ADXL313_INACTIVITY_AC) {
ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
if (ret)
return ret;
@@ -427,6 +527,10 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
en = en && inact_time_s;
}
+ ret = adxl313_set_act_inact_ac(data, type);
+ if (ret)
+ return ret;
+
ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
adxl313_act_int_reg[type], en);
if (ret)
@@ -439,10 +543,22 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
if (act_en < 0)
return act_en;
+ act_ac_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY_AC);
+ if (act_ac_en < 0)
+ return act_ac_en;
+
+ act_en = act_en || act_ac_en;
+
inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
if (inact_en < 0)
return inact_en;
+ inact_ac_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY_AC);
+ if (inact_ac_en < 0)
+ return inact_ac_en;
+
+ inact_en = inact_en || inact_ac_en;
+
en = en && act_en && inact_en;
ret = regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
@@ -534,14 +650,25 @@ static int adxl313_read_event_config(struct iio_dev *indio_dev,
{
struct adxl313_data *data = iio_priv(indio_dev);
- if (type != IIO_EV_TYPE_MAG)
- return -EINVAL;
-
- switch (dir) {
- case IIO_EV_DIR_RISING:
- return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
- case IIO_EV_DIR_FALLING:
- return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+ case IIO_EV_DIR_FALLING:
+ return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY_AC);
+ case IIO_EV_DIR_FALLING:
+ return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY_AC);
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -555,14 +682,33 @@ static int adxl313_write_event_config(struct iio_dev *indio_dev,
{
struct adxl313_data *data = iio_priv(indio_dev);
- if (type != IIO_EV_TYPE_MAG)
- return -EINVAL;
-
- switch (dir) {
- case IIO_EV_DIR_RISING:
- return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
- case IIO_EV_DIR_FALLING:
- return adxl313_set_act_inact_en(data, ADXL313_INACTIVITY, state);
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_set_act_inact_en(data,
+ ADXL313_ACTIVITY,
+ state);
+ case IIO_EV_DIR_FALLING:
+ return adxl313_set_act_inact_en(data,
+ ADXL313_INACTIVITY,
+ state);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_set_act_inact_en(data,
+ ADXL313_ACTIVITY_AC,
+ state);
+ case IIO_EV_DIR_FALLING:
+ return adxl313_set_act_inact_en(data,
+ ADXL313_INACTIVITY_AC,
+ state);
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -583,41 +729,79 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
/* Measurement stays enabled, reading from regmap cache */
- if (type != IIO_EV_TYPE_MAG)
- return -EINVAL;
-
- switch (info) {
- case IIO_EV_INFO_VALUE:
- switch (dir) {
- case IIO_EV_DIR_RISING:
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_read(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+ &act_threshold);
+ if (ret)
+ return ret;
+ *val = act_threshold * 15625;
+ *val2 = MICRO;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_read(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+ &inact_threshold);
+ if (ret)
+ return ret;
+ *val = inact_threshold * 15625;
+ *val2 = MICRO;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_PERIOD:
ret = regmap_read(data->regmap,
- adxl313_act_thresh_reg[ADXL313_ACTIVITY],
- &act_threshold);
+ ADXL313_REG_TIME_INACT,
+ &inact_time_s);
if (ret)
return ret;
- *val = act_threshold * 15625;
- *val2 = MICRO;
- return IIO_VAL_FRACTIONAL;
- case IIO_EV_DIR_FALLING:
+ *val = inact_time_s;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_read(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_ACTIVITY_AC],
+ &act_threshold);
+ if (ret)
+ return ret;
+ *val = act_threshold * 15625;
+ *val2 = MICRO;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_read(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_INACTIVITY_AC],
+ &inact_threshold);
+ if (ret)
+ return ret;
+ *val = inact_threshold * 15625;
+ *val2 = MICRO;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_PERIOD:
ret = regmap_read(data->regmap,
- adxl313_act_thresh_reg[ADXL313_INACTIVITY],
- &inact_threshold);
+ ADXL313_REG_TIME_INACT,
+ &inact_time_s);
if (ret)
return ret;
- *val = inact_threshold * 15625;
- *val2 = MICRO;
- return IIO_VAL_FRACTIONAL;
+ *val = inact_time_s;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
- case IIO_EV_INFO_PERIOD:
- ret = regmap_read(data->regmap,
- ADXL313_REG_TIME_INACT,
- &inact_time_s);
- if (ret)
- return ret;
- *val = inact_time_s;
- return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -638,36 +822,69 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
if (ret)
return ret;
- if (type != IIO_EV_TYPE_MAG)
- return -EINVAL;
-
- switch (info) {
- case IIO_EV_INFO_VALUE:
- /* Scale factor 15.625 mg/LSB */
- regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
- switch (dir) {
- case IIO_EV_DIR_RISING:
- ret = regmap_write(data->regmap,
- adxl313_act_thresh_reg[ADXL313_ACTIVITY],
- regval);
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /* Scale factor 15.625 mg/LSB */
+ regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_write(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+ regval);
+ if (ret)
+ return ret;
+ return adxl313_set_measure_en(data, true);
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_write(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+ regval);
+ if (ret)
+ return ret;
+ return adxl313_set_measure_en(data, true);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_PERIOD:
+ ret = adxl313_set_inact_time_s(data, val);
if (ret)
return ret;
return adxl313_set_measure_en(data, true);
- case IIO_EV_DIR_FALLING:
- ret = regmap_write(data->regmap,
- adxl313_act_thresh_reg[ADXL313_INACTIVITY],
- regval);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /* Scale factor 15.625 mg/LSB */
+ regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_write(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_ACTIVITY_AC],
+ regval);
+ if (ret)
+ return ret;
+ return adxl313_set_measure_en(data, true);
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_write(data->regmap,
+ adxl313_act_thresh_reg[ADXL313_INACTIVITY_AC],
+ regval);
+ if (ret)
+ return ret;
+ return adxl313_set_measure_en(data, true);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_PERIOD:
+ ret = adxl313_set_inact_time_s(data, val);
if (ret)
return ret;
return adxl313_set_measure_en(data, true);
default:
return -EINVAL;
}
- case IIO_EV_INFO_PERIOD:
- ret = adxl313_set_inact_time_s(data, val);
- if (ret)
- return ret;
- return adxl313_set_measure_en(data, true);
default:
return -EINVAL;
}
@@ -807,29 +1024,64 @@ static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
{
s64 ts = iio_get_time_ns(indio_dev);
struct adxl313_data *data = iio_priv(indio_dev);
+ unsigned int regval;
int samples;
int ret = -ENOENT;
if (FIELD_GET(ADXL313_INT_ACTIVITY, int_stat)) {
- ret = iio_push_event(indio_dev,
- IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
- IIO_MOD_X_OR_Y_OR_Z,
- IIO_EV_TYPE_MAG,
- IIO_EV_DIR_RISING),
- ts);
+ ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, ®val);
if (ret)
return ret;
+
+ if (FIELD_GET(ADXL313_REG_ACT_ACDC_MSK, regval)) {
+ /* AC coupled */
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG_ADAPTIVE,
+ IIO_EV_DIR_RISING),
+ ts);
+ if (ret)
+ return ret;
+ } else {
+ /* DC coupled, relying on THRESH */
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_RISING),
+ ts);
+ if (ret)
+ return ret;
+ }
}
if (FIELD_GET(ADXL313_INT_INACTIVITY, int_stat)) {
- ret = iio_push_event(indio_dev,
- IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
- IIO_MOD_X_AND_Y_AND_Z,
- IIO_EV_TYPE_MAG,
- IIO_EV_DIR_FALLING),
- ts);
+ ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, ®val);
if (ret)
return ret;
+
+ if (FIELD_GET(ADXL313_REG_INACT_ACDC_MSK, regval)) {
+ /* AC coupled */
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_AND_Y_AND_Z,
+ IIO_EV_TYPE_MAG_ADAPTIVE,
+ IIO_EV_DIR_FALLING),
+ ts);
+ if (ret)
+ return ret;
+ } else {
+ /* DC coupled, relying on THRESH */
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_AND_Y_AND_Z,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_FALLING),
+ ts);
+ if (ret)
+ return ret;
+ }
}
if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
--
2.39.5On Sun, 1 Jun 2025 17:21:38 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > up an additional set of threshold and period handles, verifies matching > disabling functionality and extends setting the link bit to complementary > event configurations. > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > enabled is ignored, since it does not match (should be disabling > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > bit will be set. Note, having the link bit and auto-sleep in place activity > and inactivity indicate the power save state change and thus will only be > triggered once a state transition occurs. Since there is a separate AC bit > for ACTIVITY and for INACTIVITY, events can be linked independently from > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > When one of both is disabled, the link bit will be removed. Hence, the > remaining event will not indicate a plain state change anymore, but occur > as a periodically triggered inactivity event or for each activity event > above the threshold. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> Minor thought on rereading this. If we don't have the link bit set (and the paired event) the AC events are more accurately described as MAG_REFERENCED as they are referenced simply to whatever acceleration was going on when they were first enabled. Only with the link bit set (and the other event type enabled) are they actually adapting (so MAG_ADAPTIVE). Maybe there is room to use that to ultimately control whether the link bit is set or not (putting aside the power aspect of that).
Hi Jonathan, On Sun, Jun 8, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sun, 1 Jun 2025 17:21:38 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > > up an additional set of threshold and period handles, verifies matching > > disabling functionality and extends setting the link bit to complementary > > event configurations. > > > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > > enabled is ignored, since it does not match (should be disabling > > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > > bit will be set. Note, having the link bit and auto-sleep in place activity > > and inactivity indicate the power save state change and thus will only be > > triggered once a state transition occurs. Since there is a separate AC bit > > for ACTIVITY and for INACTIVITY, events can be linked independently from > > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > > > When one of both is disabled, the link bit will be removed. Hence, the > > remaining event will not indicate a plain state change anymore, but occur > > as a periodically triggered inactivity event or for each activity event > > above the threshold. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > Minor thought on rereading this. If we don't have the link bit set > (and the paired event) the AC events are more accurately described as > MAG_REFERENCED as they are referenced simply to whatever acceleration > was going on when they were first enabled. Only with the link bit > set (and the other event type enabled) are they actually adapting > (so MAG_ADAPTIVE). > Going by examples, I can follow you as practically I'm aware of the difference between a plain inactivity setup and a link-bit enabled inactivity(and activity) setup. Initially I thought of MAG and the AC-coupled equivalent being MAG_REFERENCED. By your explanation I understand why you preferred MAG_ADAPTIVE rather. But still all three configurations are possible. My idea is, the driver implementation supports all cases in parallel, at least to a certain extent. I mean, at the current implementation someone can configure plain activity, or AC-coupled activity, or respectively, their inactivity equivalents - when both, an activity type together with an inactivity type are enabled, they will be linked counter events. I.e. "adaptive" - and auto-sleep will be turned on for the inactivity periods. Built on using just plain IIO API w/o custom API calls. Due to all the possible combinations, this comes at a certain complexity. In terms of configuration and for instance mapping to MAG, MAG_REFERNCED or MAG_ADAPTIVE. Here I rely on your feedback. On my side, I'll try to recycle the automation setup to verify registers are configured as I like them to be using the sysfs handles (that's btw the reason why I'm glad to have debugfs on board). So, if you tell me, to change it rather to MAG_REFERENCED, I'll do it, but then AC-coupled events will be all MAG_REFRENCED w/ or w/o link bit. Or we come up with a total different approach, like putting link-bit AC on MAG_ADAPTIVE and plain AC-coupled on MAG_REFERENCED, but then what about MAG events w/ or w/o link-bit? hmm, I think current approach seems to be a good compromise. Let me know what you think. Best, L > > Maybe there is room to use that to ultimately control whether the > link bit is set or not (putting aside the power aspect of that). >
On Wed, 11 Jun 2025 21:58:36 +0200 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Hi Jonathan, > > On Sun, Jun 8, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sun, 1 Jun 2025 17:21:38 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > > > up an additional set of threshold and period handles, verifies matching > > > disabling functionality and extends setting the link bit to complementary > > > event configurations. > > > > > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > > > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > > > enabled is ignored, since it does not match (should be disabling > > > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > > > bit will be set. Note, having the link bit and auto-sleep in place activity > > > and inactivity indicate the power save state change and thus will only be > > > triggered once a state transition occurs. Since there is a separate AC bit > > > for ACTIVITY and for INACTIVITY, events can be linked independently from > > > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > > > > > When one of both is disabled, the link bit will be removed. Hence, the > > > remaining event will not indicate a plain state change anymore, but occur > > > as a periodically triggered inactivity event or for each activity event > > > above the threshold. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > > > Minor thought on rereading this. If we don't have the link bit set > > (and the paired event) the AC events are more accurately described as > > MAG_REFERENCED as they are referenced simply to whatever acceleration > > was going on when they were first enabled. Only with the link bit > > set (and the other event type enabled) are they actually adapting > > (so MAG_ADAPTIVE). > > > > Going by examples, I can follow you as practically I'm aware of the > difference between a plain inactivity setup and a link-bit enabled > inactivity(and activity) setup. Initially I thought of MAG and the > AC-coupled equivalent being MAG_REFERENCED. By your explanation I > understand why you preferred MAG_ADAPTIVE rather. But still all three > configurations are possible. > > My idea is, the driver implementation supports all cases in parallel, > at least to a certain extent. I mean, at the current implementation > someone can configure plain activity, or AC-coupled activity, or > respectively, their inactivity equivalents - when both, an activity > type together with an inactivity type are enabled, they will be linked > counter events. I.e. "adaptive" - and auto-sleep will be turned on for > the inactivity periods. Built on using just plain IIO API w/o custom > API calls. > > Due to all the possible combinations, this comes at a certain > complexity. In terms of configuration and for instance mapping to MAG, > MAG_REFERNCED or MAG_ADAPTIVE. Here I rely on your feedback. On my > side, I'll try to recycle the automation setup to verify registers are > configured as I like them to be using the sysfs handles (that's btw > the reason why I'm glad to have debugfs on board). So, if you tell me, > to change it rather to MAG_REFERENCED, I'll do it, but then AC-coupled > events will be all MAG_REFRENCED w/ or w/o link bit. Or we come up > with a total different approach, like putting link-bit AC on > MAG_ADAPTIVE and plain AC-coupled on MAG_REFERENCED, but then what > about MAG events w/ or w/o link-bit? hmm, I think current approach > seems to be a good compromise. Let me know what you think. I don't have a good answer for how to handle this complexity :( This was more about an earlier question I think you asked about whether there was a way to have the user opt in to the link bit or not. I'm fine with treating them all as adaptive and glossing over exactly how it is adapting but that doesn't get us to link bit control if we decide that wants to be explicitly exposed at all. J > > Best, > L > > > > > Maybe there is room to use that to ultimately control whether the > > link bit is set or not (putting aside the power aspect of that). > >
On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote: > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > up an additional set of threshold and period handles, verifies matching > disabling functionality and extends setting the link bit to complementary > event configurations. > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > enabled is ignored, since it does not match (should be disabling > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > bit will be set. Note, having the link bit and auto-sleep in place activity > and inactivity indicate the power save state change and thus will only be > triggered once a state transition occurs. Since there is a separate AC bit > for ACTIVITY and for INACTIVITY, events can be linked independently from > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > When one of both is disabled, the link bit will be removed. Hence, the > remaining event will not indicate a plain state change anymore, but occur > as a periodically triggered inactivity event or for each activity event > above the threshold. ... > +/** > + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled. > + * Unneeded blank line. > + * @data: The device data. > + * @type: The activity or inactivity type. > + * > + * Provide a type of activity or inactivity, combined with either AC coupling > + * set, or default to DC coupling. This function verifies, if the combination is > + * currently enabled or not. > + * > + * Return if the provided activity type has AC coupling enabled or a negative > + * error value. Missing Return section. Always try kernel-doc validation when adding new kernel-doc descriptions. > + */ ... > unsigned int regval; > + int coupling; Why? Doesn't 'ret' suffice? > int axis_en, int_en, ret; ... > - int act_en, inact_en; > - bool en; > + int act_en, inact_en, act_ac_en, inact_ac_en; > + bool en, act_inact_ac; > int ret; For all your patches: try really hard to avoid the ping-pong coding, i.e. when you add something in one patch in the series and change in the other for no reason. I.o.w. when the initial code may be written already in a form that doesn't need further changes (e.g., switch-case vs. if). This patch is *very* noisy due to the above. So, just slow down, try a new approach that you have less '-' lines in the diff:s all over the code. -- With Best Regards, Andy Shevchenko
Hi Andy, On Sun, Jun 1, 2025 at 9:54 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > > up an additional set of threshold and period handles, verifies matching > > disabling functionality and extends setting the link bit to complementary > > event configurations. > > > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > > enabled is ignored, since it does not match (should be disabling > > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > > bit will be set. Note, having the link bit and auto-sleep in place activity > > and inactivity indicate the power save state change and thus will only be > > triggered once a state transition occurs. Since there is a separate AC bit > > for ACTIVITY and for INACTIVITY, events can be linked independently from > > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > > > When one of both is disabled, the link bit will be removed. Hence, the > > remaining event will not indicate a plain state change anymore, but occur > > as a periodically triggered inactivity event or for each activity event > > above the threshold. > > ... > > > +/** > > + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled. > > > + * > > Unneeded blank line. > > > + * @data: The device data. > > + * @type: The activity or inactivity type. > > + * > > + * Provide a type of activity or inactivity, combined with either AC coupling > > + * set, or default to DC coupling. This function verifies, if the combination is > > + * currently enabled or not. > > + * > > + * Return if the provided activity type has AC coupling enabled or a negative > > + * error value. > > Missing Return section. Always try kernel-doc validation when adding > new kernel-doc descriptions. > > > + */ > > ... > > > unsigned int regval; > > + int coupling; > > Why? Doesn't 'ret' suffice? > the coupling variable here is rather meant to provide kind of a semantic context. It shall be checked for being negative (error), or used in binary decision logic. In fact, could be done with ret as well, but then in case I'd need to comment that in this case the value of 'ret' carries either error, or the bool if we have coupling or not. I'd like to leave it like this, but let me know if better replace it by ret here. > > int axis_en, int_en, ret; > > ... > > > - int act_en, inact_en; > > - bool en; > > + int act_en, inact_en, act_ac_en, inact_ac_en; > > + bool en, act_inact_ac; > > int ret; > > For all your patches: try really hard to avoid the ping-pong coding, > i.e. when you add something in one patch in the series and change in > the other for no reason. I.o.w. when the initial code may be written > already in a form that doesn't need further changes (e.g., switch-case > vs. if). > > This patch is *very* noisy due to the above. So, just slow down, try a > new approach that you have less '-' lines in the diff:s all over the > code. Agree. I tried to follow the review comments. Probably, IMHO it's mostly about how to separate the patches. Your reviews seem to be quite focussed on the particular patch w/o taking the context of follow up patches so much into account. [At least by the way you gave me feed back here. Actually, by your vast experience I'm pretty sure you have the context of how such a driver shall look and have an excellent overview well in mind.] So, I guess you'd like to stress on certain points. I'm wondering if it might probably be better to send you this all first in one big patch, or say rather bigger patches, and then separate pieces out? Let me know what you think. Thank you so much for the reviews, let's see how this can be improved here in a v5. Best, L > > -- > With Best Regards, > Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.