Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing

Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

Hi Andy, also here I copied from the MARC mailing list.. some questions below.

> List:       linux-iio
> Subject:    Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing
> From:       Andy Shevchenko <andy () kernel ! org>
> Date:       2025-05-19 12:15:17
> Message-ID: aCsg1XddkT6sGjev () smile ! fi ! intel ! com
> [Download RAW message or body]
>
> On Sun, May 18, 2025 at 11:13:18AM +0000, Lothar Rubusch wrote:
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
> >
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
>
> ...
>
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > +                              enum adxl313_activity_type type,
> > +                              bool *en)
> > +{
> > +   unsigned int axis_ctrl;
> > +   unsigned int regval;
> > +   int ret;
>
> > +   *en = false;
>
> Even in case of an error? The rule of thumb is to avoid assigning output when
> we know that the error will be returned to the caller.
>
> > +   ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > +   if (ret)
> > +           return ret;
>
> > +   if (type == ADXL313_ACTIVITY)
> > +           *en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > +
> > +   if (*en) {
>
> This doesn't need to re-write the value of *en. Just declare local boolean
> temporary variable and use it and only assign it on success.
>
> > +           ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > +           if (ret)
> > +                   return ret;
> > +
> > +           *en = adxl313_act_int_reg[type] & regval;
> > +   }
> > +
> > +   return 0;
> > +}
>
> ...
>
> > +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> > +                               enum adxl313_activity_type type,
> > +                               bool cmd_en)
> > +{
> > +   unsigned int axis_ctrl = 0;
> > +   unsigned int threshold;
> > +   bool en;
> > +   int ret;
> > +
> > +   if (type == ADXL313_ACTIVITY)
> > +           axis_ctrl = ADXL313_ACT_XYZ_EN;
> > +
> > +   ret = regmap_update_bits(data->regmap,
> > +                            ADXL313_REG_ACT_INACT_CTL,
> > +                            axis_ctrl,
> > +                            cmd_en ? 0xff : 0x00);
> > +   if (ret)
> > +           return ret;
> > +
> > +   ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > +   if (ret)
> > +           return ret;
>
> > +   en = false;
>
> Instead...
>
> > +   if (type == ADXL313_ACTIVITY)
> > +           en = cmd_en && threshold;
>
>       else
>               en = false;
>
> > +   return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > +                             adxl313_act_int_reg[type],
> > +                             en ? adxl313_act_int_reg[type] : 0);
> > +}
>
> ...

The above is a good example for the following. From time to time, I
face the situation in a
function where I'd like to end up with something like

    if (foo = A) {
        var = thenDoA();
    } else {
        var = thenDoB();
    }
    doSomething(var);

In a first patch I'll introduce only the following and remark in the
commit message, that this will be extended. Since smatch/sparse tool
will complain, I'll need to fiddle around with initializations
(becoming obsolete in the end), e.g. I'll end up with something like
this in a first patch A:

    var = nonsense;
    if (foo = A) {
        var = thenDoA();
    }
    doSomething(var);

This is the case for switch(type) case IIO_...MAG: as only type (for
now). This is the case for this is_act_inact_enabled(),
set_act_inact(), etc.

I assume it's better to simplify each commit individually and don't
leave the "churn" around which might make sense in combination with a
follow patch? Is this a general approach I should follow?

Or, can it be legitimate to just split an if/else and add if-clause in
a patch A and the else clause in the other patch B, since both are
probably actually not complex. Such that patch A for itself looks a
bit odd, but will make sense together with patch B?


> > +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);
>
> > +   bool int_en;
>
> Why? You return the int here... I would expect rather to see unsigned int...
>
> > +   int ret;
> > +
> > +   switch (type) {
> > +   case IIO_EV_TYPE_MAG:
> > +           switch (dir) {
> > +           case IIO_EV_DIR_RISING:
> > +                   ret = adxl313_is_act_inact_en(data,
> > +                                                 ADXL313_ACTIVITY,
> > +                                                 &int_en);
> > +                   if (ret)
> > +                           return ret;
> > +                   return int_en;
>
> ...or even simply
>
>                       return adx1313...(...);
>
> > +           default:
> > +                   return -EINVAL;
> > +           }
> > +   default:
> > +           return -EINVAL;
> > +   }
> > +}
>
> ...

This one here is interesting, to my understanding I followed here e.g.
the approach of the ADXL380 which is supposed to be a quite recent
driver [the _read/write_event_config() there.]

Now, your remark made me think: I'm unsure, can I actually I implement
the following approach here?
- return >0 : true
- return =0 : false
- return <0 : error

It seems to work (unsure about the  error cases, though), but much
cleaner and simpler! I'll send
that in v2, pls let me know if I missunderstood you.

[...]
>
> --
> With Best Regards,
> Andy Shevchenko

Best,
L

Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Tue, May 20, 2025 at 10:25:03PM +0200, Lothar Rubusch wrote:
> > On Sun, May 18, 2025 at 11:13:18AM +0000, Lothar Rubusch wrote:

...

> > > +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> > > +                               enum adxl313_activity_type type,
> > > +                               bool cmd_en)
> > > +{
> > > +   unsigned int axis_ctrl = 0;
> > > +   unsigned int threshold;
> > > +   bool en;
> > > +   int ret;
> > > +
> > > +   if (type == ADXL313_ACTIVITY)
> > > +           axis_ctrl = ADXL313_ACT_XYZ_EN;
> > > +
> > > +   ret = regmap_update_bits(data->regmap,
> > > +                            ADXL313_REG_ACT_INACT_CTL,
> > > +                            axis_ctrl,
> > > +                            cmd_en ? 0xff : 0x00);
> > > +   if (ret)
> > > +           return ret;
> > > +
> > > +   ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > > +   if (ret)
> > > +           return ret;
> >
> > > +   en = false;
> >
> > Instead...
> >
> > > +   if (type == ADXL313_ACTIVITY)
> > > +           en = cmd_en && threshold;
> >
> >       else
> >               en = false;
> >
> > > +   return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > > +                             adxl313_act_int_reg[type],
> > > +                             en ? adxl313_act_int_reg[type] : 0);
> > > +}
> 
> The above is a good example for the following. From time to time, I face
> the situation in a function where I'd like to end up with something like
> 
>     if (foo = A) {
>         var = thenDoA();
>     } else {
>         var = thenDoB();
>     }
>     doSomething(var);
> 
> In a first patch I'll introduce only the following and remark in the
> commit message, that this will be extended. Since smatch/sparse tool
> will complain, I'll need to fiddle around with initializations
> (becoming obsolete in the end), e.g. I'll end up with something like
> this in a first patch A:
> 
>     var = nonsense;
>     if (foo = A) {
>         var = thenDoA();
>     }
>     doSomething(var);
> 
> This is the case for switch(type) case IIO_...MAG: as only type (for
> now). This is the case for this is_act_inact_enabled(),
> set_act_inact(), etc.
> 
> I assume it's better to simplify each commit individually and don't
> leave the "churn" around which might make sense in combination with a
> follow patch? Is this a general approach I should follow?

I believe so.

> Or, can it be legitimate to just split an if/else and add if-clause in
> a patch A and the else clause in the other patch B, since both are
> probably actually not complex. Such that patch A for itself looks a
> bit odd, but will make sense together with patch B?

Yes, but just make sure the each of the patches (after being applied) give the
plausible result.

...

> > > +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);
> >
> > > +   bool int_en;
> >
> > Why? You return the int here... I would expect rather to see unsigned int...
> >
> > > +   int ret;
> > > +
> > > +   switch (type) {
> > > +   case IIO_EV_TYPE_MAG:
> > > +           switch (dir) {
> > > +           case IIO_EV_DIR_RISING:
> > > +                   ret = adxl313_is_act_inact_en(data,
> > > +                                                 ADXL313_ACTIVITY,
> > > +                                                 &int_en);
> > > +                   if (ret)
> > > +                           return ret;
> > > +                   return int_en;
> >
> > ...or even simply
> >
> >                       return adx1313...(...);
> >
> > > +           default:
> > > +                   return -EINVAL;
> > > +           }
> > > +   default:
> > > +           return -EINVAL;
> > > +   }
> > > +}
> 
> This one here is interesting, to my understanding I followed here e.g.
> the approach of the ADXL380 which is supposed to be a quite recent
> driver [the _read/write_event_config() there.]

> Now, your remark made me think: I'm unsure, can I actually I implement
> the following approach here?
> - return >0 : true

=1, but yes. We have plenty of functions like this in the kernel.

> - return =0 : false
> - return <0 : error
> 
> It seems to work (unsure about the  error cases, though),
> but much cleaner and simpler! I'll send that in v2,
> pls let me know if I missunderstood you.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 00/12] iio: accel: adxl313: add power-save on activity/inactivity

[Lothar Rubusch]

The patch set covers the following topics:
- add debug register and regmap cache
- prepare iio channel scan_type and scan_index
- prepare interrupt handling
- implement fifo with watermark
- add activity/inactivity together with auto-sleep with link bit
- documentation

Similar situation and approach as for the ADXL345. AC/DC coupling might be
a pending option as it is quite the same as for ADXL345.

Since activity and inactivity here are implemented covering all axis, I
assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Lothar Rubusch (12):
  iio: accel: adxl313: add debug register
  iio: accel: adxl313: introduce channel scan_index
  iio: accel: adxl313: configure scan type for buffer
  iio: accel: adxl313: make use of regmap cache
  iio: accel: adxl313: add function to enable measurement
  iio: accel: adxl313: prepare interrupt handling
  iio: accel: adxl313: add basic interrupt handling
  iio: accel: adxl313: add FIFO watermark
  iio: accel: adxl313: add activity sensing
  iio: accel: adxl313: add inactivity sensing
  iio: accel: adxl313: implement power-save on inactivity
  docs: iio: add ADXL313 accelerometer

 Documentation/iio/adxl313.rst    | 196 +++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl313.h      |  35 +-
 drivers/iio/accel/adxl313_core.c | 658 ++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl313_i2c.c  |   6 +
 drivers/iio/accel/adxl313_spi.c  |   6 +
 6 files changed, 893 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/iio/adxl313.rst

-- 
2.39.5

[PATCH v1 09/12] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

Add possibilities to set a threshold for activity sensing. Extend the
interrupt handler to process activity interrupts. Provide functions to set
the activity threshold and to enable/disable activity sensing. Further add
a fake channel for having x, y and z axis anded on the iio channel.

This is a preparatory patch. Some of the definitions and functions are
supposed to be extended for inactivity later on.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313_core.c | 252 +++++++++++++++++++++++++++++++
 1 file changed, 252 insertions(+)

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 0a93af89ba19..49914ccc92cf 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -27,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)),
@@ -231,6 +246,16 @@ static const int adxl313_odr_freqs[][2] = {
 	},								\
 }
 
+static const struct iio_event_spec adxl313_fake_chan_events[] = {
+	{
+		/* activity */
+		.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
 };
@@ -239,6 +264,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_AND_Y_AND_Z,
+		.scan_index = -1, /* Fake channel for axis AND'ing */
+		.event_spec = adxl313_fake_chan_events,
+		.num_event_specs = ARRAY_SIZE(adxl313_fake_chan_events),
+	},
 };
 
 static const unsigned long adxl313_scan_masks[] = {
@@ -301,6 +334,67 @@ 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,
+				   bool *en)
+{
+	unsigned int axis_ctrl;
+	unsigned int regval;
+	int ret;
+
+	*en = false;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	if (type == ADXL313_ACTIVITY)
+		*en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+
+	if (*en) {
+		ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
+		if (ret)
+			return ret;
+
+		*en = adxl313_act_int_reg[type] & regval;
+	}
+
+	return 0;
+}
+
+static int adxl313_set_act_inact_en(struct adxl313_data *data,
+				    enum adxl313_activity_type type,
+				    bool cmd_en)
+{
+	unsigned int axis_ctrl = 0;
+	unsigned int threshold;
+	bool en;
+	int ret;
+
+	if (type == ADXL313_ACTIVITY)
+		axis_ctrl = ADXL313_ACT_XYZ_EN;
+
+	ret = regmap_update_bits(data->regmap,
+				 ADXL313_REG_ACT_INACT_CTL,
+				 axis_ctrl,
+				 cmd_en ? 0xff : 0x00);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
+	en = false;
+
+	if (type == ADXL313_ACTIVITY)
+		en = cmd_en && threshold;
+
+	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+				  adxl313_act_int_reg[type],
+				  en ? adxl313_act_int_reg[type] : 0);
+}
+
 static int adxl313_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -374,6 +468,135 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+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);
+	bool int_en;
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = adxl313_is_act_inact_en(data,
+						      ADXL313_ACTIVITY,
+						      &int_en);
+			if (ret)
+				return ret;
+			return int_en;
+		default:
+			return -EINVAL;
+		}
+	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:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl313_set_act_inact_en(data,
+							ADXL313_ACTIVITY,
+							state);
+		default:
+			return -EINVAL;
+		}
+	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);
+	unsigned int act_threshold;
+	int ret;
+
+	/* measurement stays enabled, reading from regmap cache */
+
+	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 = 1000000;
+				return IIO_VAL_FRACTIONAL;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+
+	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);
+	unsigned int regval;
+	int ret;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			/* The scale factor is 15.625 mg/LSB */
+			regval = DIV_ROUND_CLOSEST(1000000 * 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);
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	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,10 +731,22 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
 
 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);
 	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_AND_Y_AND_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_RISING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
 		samples = adxl313_get_samples(data);
 		if (samples < 0)
@@ -558,6 +793,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,
@@ -668,6 +907,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, 0);
+		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.5

Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Sun, May 18, 2025 at 11:13:18AM +0000, Lothar Rubusch wrote:
> Add possibilities to set a threshold for activity sensing. Extend the
> interrupt handler to process activity interrupts. Provide functions to set
> the activity threshold and to enable/disable activity sensing. Further add
> a fake channel for having x, y and z axis anded on the iio channel.
> 
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.

...

> +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> +				   enum adxl313_activity_type type,
> +				   bool *en)
> +{
> +	unsigned int axis_ctrl;
> +	unsigned int regval;
> +	int ret;

> +	*en = false;

Even in case of an error? The rule of thumb is to avoid assigning output when
we know that the error will be returned to the caller.

> +	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> +	if (ret)
> +		return ret;

> +	if (type == ADXL313_ACTIVITY)
> +		*en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +
> +	if (*en) {

This doesn't need to re-write the value of *en. Just declare local boolean
temporary variable and use it and only assign it on success.

> +		ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> +		if (ret)
> +			return ret;
> +
> +		*en = adxl313_act_int_reg[type] & regval;
> +	}
> +
> +	return 0;
> +}

...

> +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> +				    enum adxl313_activity_type type,
> +				    bool cmd_en)
> +{
> +	unsigned int axis_ctrl = 0;
> +	unsigned int threshold;
> +	bool en;
> +	int ret;
> +
> +	if (type == ADXL313_ACTIVITY)
> +		axis_ctrl = ADXL313_ACT_XYZ_EN;
> +
> +	ret = regmap_update_bits(data->regmap,
> +				 ADXL313_REG_ACT_INACT_CTL,
> +				 axis_ctrl,
> +				 cmd_en ? 0xff : 0x00);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;

> +	en = false;

Instead...

> +	if (type == ADXL313_ACTIVITY)
> +		en = cmd_en && threshold;

	else
		en = false;

> +	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				  adxl313_act_int_reg[type],
> +				  en ? adxl313_act_int_reg[type] : 0);
> +}

...

> +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);

> +	bool int_en;

Why? You return the int here... I would expect rather to see unsigned int...

> +	int ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = adxl313_is_act_inact_en(data,
> +						      ADXL313_ACTIVITY,
> +						      &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en;

...or even simply

			return adx1313...(...);

> +		default:
> +			return -EINVAL;
> +		}
> +	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);
> +	unsigned int act_threshold;
> +	int ret;
> +
> +	/* measurement stays enabled, reading from regmap cache */
> +
> +	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 = 1000000;

MICRO?

> +				return IIO_VAL_FRACTIONAL;
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	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);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:

This can be collapsed to the conditional, making indentation better overall.
Same applies to the other parts of the code outside of this function.

> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			/* The scale factor is 15.625 mg/LSB */
> +			regval = DIV_ROUND_CLOSEST(1000000 * val + val2, 15625);

MICRO?

> +			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);
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +		ret = regmap_write(data->regmap, ADXL313_REG_ACT_INACT_CTL, 0);

0x00 ?

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
> +		if (ret)
> +			return ret;

-- 
With Best Regards,
Andy Shevchenko