Add support for configuring an activity detection threshold. Extend the
interrupt handler to process activity-related interrupts, and provide
functions to set the threshold as well as to enable or disable activity
sensing. Additionally, introduce a virtual channel that represents the
logical AND of the x, y, and z axes in the IIO channel.
This patch serves as a preparatory step; some definitions and functions
introduced here are intended to be extended later to support inactivity
detection.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
1 file changed, 326 insertions(+)
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index ac4cc16399fc..d2c625f27555 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -13,8 +13,10 @@
#include <linux/overflow.h>
#include <linux/property.h>
#include <linux/regmap.h>
+#include <linux/units.h>
#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
#include <linux/iio/kfifo_buf.h>
#include "adxl313.h"
@@ -25,6 +27,21 @@
#define ADXL313_REG_XYZ_BASE ADXL313_REG_DATA_AXIS(0)
+#define ADXL313_ACT_XYZ_EN GENMASK(6, 4)
+
+/* activity/inactivity */
+enum adxl313_activity_type {
+ ADXL313_ACTIVITY,
+};
+
+static const unsigned int adxl313_act_int_reg[] = {
+ [ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
+};
+
+static const unsigned int adxl313_act_thresh_reg[] = {
+ [ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
+};
+
static const struct regmap_range adxl312_readable_reg_range[] = {
regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -227,6 +244,15 @@ static const int adxl313_odr_freqs[][2] = {
}, \
}
+static const struct iio_event_spec adxl313_activity_events[] = {
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+ },
+};
+
enum adxl313_chans {
chan_x, chan_y, chan_z,
};
@@ -235,6 +261,14 @@ static const struct iio_chan_spec adxl313_channels[] = {
ADXL313_ACCEL_CHANNEL(0, chan_x, X),
ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
+ {
+ .type = IIO_ACCEL,
+ .modified = 1,
+ .channel2 = IIO_MOD_X_OR_Y_OR_Z,
+ .scan_index = -1, /* Fake channel for axis OR'ing */
+ .event_spec = adxl313_activity_events,
+ .num_event_specs = ARRAY_SIZE(adxl313_activity_events),
+ },
};
static const unsigned long adxl313_scan_masks[] = {
@@ -297,6 +331,81 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
}
}
+static int adxl313_is_act_inact_en(struct adxl313_data *data,
+ enum adxl313_activity_type type)
+{
+ unsigned int axis_ctrl;
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
+ if (ret)
+ return ret;
+
+ /* Check if axis for activity are enabled */
+ switch (type) {
+ case ADXL313_ACTIVITY:
+ if (!FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl))
+ return false;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Check if specific interrupt is enabled */
+ ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
+ if (ret)
+ return ret;
+
+ return adxl313_act_int_reg[type] & regval;
+}
+
+static int adxl313_set_act_inact_en(struct adxl313_data *data,
+ enum adxl313_activity_type type,
+ bool cmd_en)
+{
+ unsigned int axis_ctrl;
+ unsigned int threshold;
+ int ret;
+
+ if (cmd_en) {
+ /* When turning on, check if threshold is valid */
+ ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type],
+ &threshold);
+ if (ret)
+ return ret;
+
+ if (!threshold) /* Just ignore the command if threshold is 0 */
+ return 0;
+ }
+
+ /* Start modifying configuration registers */
+ ret = adxl313_set_measure_en(data, false);
+ if (ret)
+ return ret;
+
+ /* Enable axis according to the command */
+ switch (type) {
+ case ADXL313_ACTIVITY:
+ axis_ctrl = ADXL313_ACT_XYZ_EN;
+ break;
+ default:
+ return -EINVAL;
+ }
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+ axis_ctrl, cmd_en);
+ if (ret)
+ return ret;
+
+ /* Enable the interrupt line, according to the command */
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+ adxl313_act_int_reg[type], cmd_en);
+ if (ret)
+ return ret;
+
+ return adxl313_set_measure_en(data, true);
+}
+
static int adxl313_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -370,6 +479,177 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
}
}
+static int adxl313_read_mag_config(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum adxl313_activity_type type_act)
+{
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return !!adxl313_is_act_inact_en(data, type_act);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_write_mag_config(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum adxl313_activity_type type_act,
+ bool state)
+{
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_set_act_inact_en(data, type_act, state);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct adxl313_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ return adxl313_read_mag_config(data, dir,
+ ADXL313_ACTIVITY);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
+{
+ struct adxl313_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ return adxl313_write_mag_config(data, dir,
+ ADXL313_ACTIVITY,
+ state);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int _adxl313_read_mag_value(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum adxl313_activity_type type_act,
+ int *val, int *val2)
+{
+ unsigned int threshold;
+ int ret;
+
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_read(data->regmap,
+ adxl313_act_thresh_reg[type_act],
+ &threshold);
+ if (ret)
+ return ret;
+ *val = threshold * 15625;
+ *val2 = MICRO;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int _adxl313_write_mag_value(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum adxl313_activity_type type_act,
+ int val, int val2)
+{
+ unsigned int regval;
+
+ /* Scale factor 15.625 mg/LSB */
+ regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return regmap_write(data->regmap,
+ adxl313_act_thresh_reg[type_act],
+ regval);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_read_mag_value(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ enum adxl313_activity_type type_act,
+ int *val, int *val2)
+{
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return _adxl313_read_mag_value(data, dir,
+ type_act,
+ val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_write_mag_value(struct adxl313_data *data,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ enum adxl313_activity_type type_act,
+ int val, int val2)
+{
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return _adxl313_write_mag_value(data, dir,
+ type_act,
+ val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct adxl313_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ return adxl313_read_mag_value(data, dir, info,
+ ADXL313_ACTIVITY,
+ val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl313_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct adxl313_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_MAG:
+ return adxl313_write_mag_value(data, dir, info,
+ ADXL313_ACTIVITY,
+ val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
{
struct adxl313_data *data = iio_priv(indio_dev);
@@ -508,6 +788,25 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
return 0;
}
+static int adxl313_push_events(struct iio_dev *indio_dev, int int_stat)
+{
+ s64 ts = iio_get_time_ns(indio_dev);
+ 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);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
static irqreturn_t adxl313_irq_handler(int irq, void *p)
{
struct iio_dev *indio_dev = p;
@@ -517,6 +816,16 @@ static irqreturn_t adxl313_irq_handler(int irq, void *p)
if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
return IRQ_NONE;
+ /*
+ * In cases of sensor events not handled (still not implemented) by
+ * this driver, the FIFO needs to be drained to become operational
+ * again. In general the sensor configuration only should issue events
+ * which were configured by this driver. Anyway a miss-configuration
+ * easily might end up in a hanging sensor FIFO.
+ */
+ if (adxl313_push_events(indio_dev, int_stat))
+ goto err;
+
if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
samples = adxl313_get_samples(data);
if (samples < 0)
@@ -550,6 +859,10 @@ static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
static const struct iio_info adxl313_info = {
.read_raw = adxl313_read_raw,
.write_raw = adxl313_write_raw,
+ .read_event_config = adxl313_read_event_config,
+ .write_event_config = adxl313_write_event_config,
+ .read_event_value = adxl313_read_event_value,
+ .write_event_value = adxl313_write_event_value,
.read_avail = adxl313_read_freq_avail,
.hwfifo_set_watermark = adxl313_set_watermark,
.debugfs_reg_access = &adxl313_reg_access,
@@ -687,6 +1000,19 @@ int adxl313_core_probe(struct device *dev,
if (ret)
return ret;
+ /*
+ * Reset or configure the registers with reasonable default
+ * values. As having 0 in most cases may result in undesirable
+ * behavior if the interrupts are enabled.
+ */
+ ret = regmap_write(data->regmap, ADXL313_REG_ACT_INACT_CTL, 0x00);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
+ if (ret)
+ return ret;
+
ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
&adxl313_buffer_ops);
if (ret)
--
2.39.5On Sun, 22 Jun 2025 12:29:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add support for configuring an activity detection threshold. Extend the
> interrupt handler to process activity-related interrupts, and provide
> functions to set the threshold as well as to enable or disable activity
> sensing. Additionally, introduce a virtual channel that represents the
> logical AND of the x, y, and z axes in the IIO channel.
>
> This patch serves as a preparatory step; some definitions and functions
> introduced here are intended to be extended later to support inactivity
> detection.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar.
I think this is suffering from function naming evolution and we need
to rethink (slightly) what we ended up with. See inline.
I walked into the same trap recently so was on the look out for it.
> ---
> drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
> 1 file changed, 326 insertions(+)
>
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index ac4cc16399fc..d2c625f27555 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -13,8 +13,10 @@
> +
> +static int _adxl313_read_mag_value(struct adxl313_data *data,
> + enum iio_event_direction dir,
> + enum adxl313_activity_type type_act,
> + int *val, int *val2)
> +{
> + unsigned int threshold;
> + int ret;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_read(data->regmap,
> + adxl313_act_thresh_reg[type_act],
> + &threshold);
> + if (ret)
> + return ret;
> + *val = threshold * 15625;
> + *val2 = MICRO;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int _adxl313_write_mag_value(struct adxl313_data *data,
> + enum iio_event_direction dir,
> + enum adxl313_activity_type type_act,
> + int val, int val2)
> +{
> + unsigned int regval;
> +
> + /* Scale factor 15.625 mg/LSB */
> + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return regmap_write(data->regmap,
> + adxl313_act_thresh_reg[type_act],
> + regval);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl313_read_mag_value(struct adxl313_data *data,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + enum adxl313_activity_type type_act,
> + int *val, int *val2)
> +{
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + return _adxl313_read_mag_value(data, dir,
Same issue as below for read functions.
> + type_act,
> + val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl313_write_mag_value(struct adxl313_data *data,
This has me a little confused. It's called
adxl313_write_mag_value() which seems pretty specific but
then calls another level of _adxl313_write_mag_value.
In the next patch (assuming diff isn't leading me astray) we have
@@ -600,13 +687,17 @@ static int adxl313_write_mag_value(struct adxl313_data *data,
enum iio_event_direction dir,
enum iio_event_info info,
enum adxl313_activity_type type_act,
+ enum adxl313_activity_type type_inact,
int val, int val2)
{
switch (info) {
case IIO_EV_INFO_VALUE:
return _adxl313_write_mag_value(data, dir,
type_act,
+ type_inact,
val, val2);
+ case IIO_EV_INFO_PERIOD:
+ return adxl313_set_inact_time_s(data, val);
default:
return -EINVAL;
}
Which is adding PERIOD to something called write_mag_value()
Whilst I can see why you ended up with naming as:
adxl313_write_mag_value() as the magnitude event specific part of
adxl13_event_write_value()
and indeed
_adxl313_write_mag_value() as the thing that writes IIO_EV_INFO_VALUE
value (i.e. the threshold) for the magnitude events.
Last time I hit a similar naming stack, I spinkled in some _info
So have the inner one called something slightly more specific like
adxl313_write_mag_info_value()
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + enum adxl313_activity_type type_act,
> + int val, int val2)
> +{
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + return _adxl313_write_mag_value(data, dir,
> + type_act,
> + val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct adxl313_data *data = iio_priv(indio_dev);
> +
> + switch (type) {
> + case IIO_EV_TYPE_MAG:
> + return adxl313_read_mag_value(data, dir, info,
> + ADXL313_ACTIVITY,
> + val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct adxl313_data *data = iio_priv(indio_dev);
> +
> + switch (type) {
> + case IIO_EV_TYPE_MAG:
> + return adxl313_write_mag_value(data, dir, info,
> + ADXL313_ACTIVITY,
> + val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
Otherwise LGTM
Jonathan
On Sat, Jun 28, 2025 at 7:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 22 Jun 2025 12:29:33 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add support for configuring an activity detection threshold. Extend the
> > interrupt handler to process activity-related interrupts, and provide
> > functions to set the threshold as well as to enable or disable activity
> > sensing. Additionally, introduce a virtual channel that represents the
> > logical AND of the x, y, and z axes in the IIO channel.
> >
> > This patch serves as a preparatory step; some definitions and functions
> > introduced here are intended to be extended later to support inactivity
> > detection.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar.
>
> I think this is suffering from function naming evolution and we need
> to rethink (slightly) what we ended up with. See inline.
> I walked into the same trap recently so was on the look out for it.
>
> > ---
> > drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
> > 1 file changed, 326 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index ac4cc16399fc..d2c625f27555 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
> > @@ -13,8 +13,10 @@
>
> > +
> > +static int _adxl313_read_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum adxl313_activity_type type_act,
> > + int *val, int *val2)
> > +{
> > + unsigned int threshold;
> > + int ret;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + ret = regmap_read(data->regmap,
> > + adxl313_act_thresh_reg[type_act],
> > + &threshold);
> > + if (ret)
> > + return ret;
> > + *val = threshold * 15625;
> > + *val2 = MICRO;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int _adxl313_write_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum adxl313_activity_type type_act,
> > + int val, int val2)
> > +{
> > + unsigned int regval;
> > +
> > + /* Scale factor 15.625 mg/LSB */
> > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return regmap_write(data->regmap,
> > + adxl313_act_thresh_reg[type_act],
> > + regval);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_read_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + enum adxl313_activity_type type_act,
> > + int *val, int *val2)
> > +{
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + return _adxl313_read_mag_value(data, dir,
>
> Same issue as below for read functions.
>
> > + type_act,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_write_mag_value(struct adxl313_data *data,
>
> This has me a little confused. It's called
> adxl313_write_mag_value() which seems pretty specific but
> then calls another level of _adxl313_write_mag_value.
>
> In the next patch (assuming diff isn't leading me astray) we have
>
> @@ -600,13 +687,17 @@ static int adxl313_write_mag_value(struct adxl313_data *data,
> enum iio_event_direction dir,
> enum iio_event_info info,
> enum adxl313_activity_type type_act,
> + enum adxl313_activity_type type_inact,
> int val, int val2)
> {
> switch (info) {
> case IIO_EV_INFO_VALUE:
> return _adxl313_write_mag_value(data, dir,
> type_act,
> + type_inact,
> val, val2);
> + case IIO_EV_INFO_PERIOD:
> + return adxl313_set_inact_time_s(data, val);
> default:
> return -EINVAL;
> }
>
>
> Which is adding PERIOD to something called write_mag_value()
>
> Whilst I can see why you ended up with naming as:
>
> adxl313_write_mag_value() as the magnitude event specific part of
> adxl13_event_write_value()
>
> and indeed
>
> _adxl313_write_mag_value() as the thing that writes IIO_EV_INFO_VALUE
> value (i.e. the threshold) for the magnitude events.
>
> Last time I hit a similar naming stack, I spinkled in some _info
>
> So have the inner one called something slightly more specific like
>
> adxl313_write_mag_info_value()
>
>
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + enum adxl313_activity_type type_act,
> > + int val, int val2)
> > +{
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + return _adxl313_write_mag_value(data, dir,
> > + type_act,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_MAG:
> > + return adxl313_read_mag_value(data, dir, info,
> > + ADXL313_ACTIVITY,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_MAG:
> > + return adxl313_write_mag_value(data, dir, info,
> > + ADXL313_ACTIVITY,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
> Otherwise LGTM
>
Hi, I'm about to wrap this up for the final version (let's see...).
I understand that three levels of switch/case are not good. Instead
here I did a particular function/helper per switch/case level.
Finally, I ended up with, e.g.
adxl313_write_event_value() // calls
\-> adxl313_write_mag_value() // calls
\-> _adxl313_write_mag_value()
Personally, I think, why not just having the following calls hierarchy:
adxl313_write_event_value() // calls
\-> adxl313_write_mag_value()
First question: Regarding the adxl345 driver, with a little higher
level of complexity, I adopted such a solution keeping still 2 levels
of switch case inside the helper. Would this be ok for the ADXL313,
too? I mean, having just one helper, but that one containing one level
of nested switch case inside a switch/case?
Another question about the naming of the helper. As you saw, I went
"creative" and used the commonly used name for such functions
replacing "_event_" by "_mag_". I see this can be confusing, but also
it might make clear where the (only locally used) helper belongs to.
I understand names with leading '_' are not likely to be a decent
choice here. But in general in case of adxl313_write_mag_value() -like
names. What would be a better name for it, or would it be ok?
By the answers given to the above, and if you don't object I would
like to prepare the single level of helper approach (then having one
nested switch/case) and keep just the adxl313_*_mag_value() or
..._config() functions. Let me know what you think.
> Jonathan
On Tue, 1 Jul 2025 00:32:57 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Sat, Jun 28, 2025 at 7:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 22 Jun 2025 12:29:33 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add support for configuring an activity detection threshold. Extend the
> > > interrupt handler to process activity-related interrupts, and provide
> > > functions to set the threshold as well as to enable or disable activity
> > > sensing. Additionally, introduce a virtual channel that represents the
> > > logical AND of the x, y, and z axes in the IIO channel.
> > >
> > > This patch serves as a preparatory step; some definitions and functions
> > > introduced here are intended to be extended later to support inactivity
> > > detection.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > Hi Lothar.
> >
> > I think this is suffering from function naming evolution and we need
> > to rethink (slightly) what we ended up with. See inline.
> > I walked into the same trap recently so was on the look out for it.
> >
> > > ---
> > > drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
> > > 1 file changed, 326 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > > index ac4cc16399fc..d2c625f27555 100644
> > > --- a/drivers/iio/accel/adxl313_core.c
> > > +++ b/drivers/iio/accel/adxl313_core.c
> > > @@ -13,8 +13,10 @@
> >
> > > +
> > > +static int _adxl313_read_mag_value(struct adxl313_data *data,
> > > + enum iio_event_direction dir,
> > > + enum adxl313_activity_type type_act,
> > > + int *val, int *val2)
> > > +{
> > > + unsigned int threshold;
> > > + int ret;
> > > +
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = regmap_read(data->regmap,
> > > + adxl313_act_thresh_reg[type_act],
> > > + &threshold);
> > > + if (ret)
> > > + return ret;
> > > + *val = threshold * 15625;
> > > + *val2 = MICRO;
> > > + return IIO_VAL_FRACTIONAL;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int _adxl313_write_mag_value(struct adxl313_data *data,
> > > + enum iio_event_direction dir,
> > > + enum adxl313_activity_type type_act,
> > > + int val, int val2)
> > > +{
> > > + unsigned int regval;
> > > +
> > > + /* Scale factor 15.625 mg/LSB */
> > > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + return regmap_write(data->regmap,
> > > + adxl313_act_thresh_reg[type_act],
> > > + regval);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int adxl313_read_mag_value(struct adxl313_data *data,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + enum adxl313_activity_type type_act,
> > > + int *val, int *val2)
> > > +{
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + return _adxl313_read_mag_value(data, dir,
> >
> > Same issue as below for read functions.
> >
> > > + type_act,
> > > + val, val2);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int adxl313_write_mag_value(struct adxl313_data *data,
> >
> > This has me a little confused. It's called
> > adxl313_write_mag_value() which seems pretty specific but
> > then calls another level of _adxl313_write_mag_value.
> >
> > In the next patch (assuming diff isn't leading me astray) we have
> >
> > @@ -600,13 +687,17 @@ static int adxl313_write_mag_value(struct adxl313_data *data,
> > enum iio_event_direction dir,
> > enum iio_event_info info,
> > enum adxl313_activity_type type_act,
> > + enum adxl313_activity_type type_inact,
> > int val, int val2)
> > {
> > switch (info) {
> > case IIO_EV_INFO_VALUE:
> > return _adxl313_write_mag_value(data, dir,
> > type_act,
> > + type_inact,
> > val, val2);
> > + case IIO_EV_INFO_PERIOD:
> > + return adxl313_set_inact_time_s(data, val);
> > default:
> > return -EINVAL;
> > }
> >
> >
> > Which is adding PERIOD to something called write_mag_value()
> >
> > Whilst I can see why you ended up with naming as:
> >
> > adxl313_write_mag_value() as the magnitude event specific part of
> > adxl13_event_write_value()
> >
> > and indeed
> >
> > _adxl313_write_mag_value() as the thing that writes IIO_EV_INFO_VALUE
> > value (i.e. the threshold) for the magnitude events.
> >
> > Last time I hit a similar naming stack, I spinkled in some _info
> >
> > So have the inner one called something slightly more specific like
> >
> > adxl313_write_mag_info_value()
> >
> >
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + enum adxl313_activity_type type_act,
> > > + int val, int val2)
> > > +{
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + return _adxl313_write_mag_value(data, dir,
> > > + type_act,
> > > + val, val2);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int *val, int *val2)
> > > +{
> > > + struct adxl313_data *data = iio_priv(indio_dev);
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_MAG:
> > > + return adxl313_read_mag_value(data, dir, info,
> > > + ADXL313_ACTIVITY,
> > > + val, val2);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int val, int val2)
> > > +{
> > > + struct adxl313_data *data = iio_priv(indio_dev);
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_MAG:
> > > + return adxl313_write_mag_value(data, dir, info,
> > > + ADXL313_ACTIVITY,
> > > + val, val2);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> >
> > Otherwise LGTM
> >
>
> Hi, I'm about to wrap this up for the final version (let's see...).
>
> I understand that three levels of switch/case are not good. Instead
> here I did a particular function/helper per switch/case level.
> Finally, I ended up with, e.g.
>
> adxl313_write_event_value() // calls
> \-> adxl313_write_mag_value() // calls
> \-> _adxl313_write_mag_value()
>
> Personally, I think, why not just having the following calls hierarchy:
>
> adxl313_write_event_value() // calls
> \-> adxl313_write_mag_value()
>
> First question: Regarding the adxl345 driver, with a little higher
> level of complexity, I adopted such a solution keeping still 2 levels
> of switch case inside the helper. Would this be ok for the ADXL313,
> too? I mean, having just one helper, but that one containing one level
> of nested switch case inside a switch/case?
I think a bit of nesting is fine but it depends on whether we end
up with conditionals etc in the inner most nest. If that's going
on or the code is otherwise complex then breaking it up into single
layers makes sense.
>
>
> Another question about the naming of the helper. As you saw, I went
> "creative" and used the commonly used name for such functions
> replacing "_event_" by "_mag_". I see this can be confusing, but also
> it might make clear where the (only locally used) helper belongs to.
>
> I understand names with leading '_' are not likely to be a decent
> choice here. But in general in case of adxl313_write_mag_value() -like
> names. What would be a better name for it, or would it be ok?
mag_value was fine, it was only when you then use the same *write_mag_value
postfix to mean the IIO_EV_INFO_VALUE of the outer *write_mag_value
that things got problematic. Hence suggestion to use
write_mag_info_value postfix for that inner most call. For the outer calls
the write_mag_value() postfix was fine.
>
> By the answers given to the above, and if you don't object I would
> like to prepare the single level of helper approach (then having one
> nested switch/case) and keep just the adxl313_*_mag_value() or
> ..._config() functions. Let me know what you think.
The nesting comments were from Andy (IIRC), so perhaps he can offer
some feedback on what he feels is reasonable.
Thanks,
Jonathan
>
> > Jonathan
>
© 2016 - 2025 Red Hat, Inc.