[PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing

Lothar Rubusch posted 8 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing
Posted by Lothar Rubusch 3 months, 3 weeks ago
Enhance the interrupt handler to process inactivity events. Introduce
functions to configure the threshold and period registers for
inactivity detection, as well as to enable or disable the inactivity
feature. Extend the fake IIO channel to handle inactivity events by
combining the x, y, and z axes using a logical AND operation.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |   2 +
 drivers/iio/accel/adxl313_core.c | 161 ++++++++++++++++++++++++++-----
 2 files changed, 137 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 4f4a9fd39f6d..d7e8cb44855b 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -18,6 +18,8 @@
 #define ADXL313_REG_SOFT_RESET		0x18
 #define ADXL313_REG_OFS_AXIS(index)	(0x1E + (index))
 #define ADXL313_REG_THRESH_ACT		0x24
+#define ADXL313_REG_THRESH_INACT	0x25
+#define ADXL313_REG_TIME_INACT		0x26
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index b0d14ce60f01..95f52c95682a 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -28,18 +28,22 @@
 #define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
 
 #define ADXL313_ACT_XYZ_EN			GENMASK(6, 4)
+#define ADXL313_INACT_XYZ_EN			GENMASK(2, 0)
 
 /* activity/inactivity */
 enum adxl313_activity_type {
 	ADXL313_ACTIVITY,
+	ADXL313_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_int_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
+	[ADXL313_INACTIVITY] = ADXL313_INT_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_thresh_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
+	[ADXL313_INACTIVITY] = ADXL313_REG_THRESH_INACT,
 };
 
 static const struct regmap_range adxl312_readable_reg_range[] = {
@@ -253,6 +257,16 @@ static const struct iio_event_spec adxl313_activity_events[] = {
 	},
 };
 
+static const struct iio_event_spec adxl313_inactivity_events[] = {
+	{
+		.type = IIO_EV_TYPE_MAG,
+		.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 {
 	chan_x, chan_y, chan_z,
 };
@@ -269,6 +283,14 @@ static const struct iio_chan_spec adxl313_channels[] = {
 		.event_spec = adxl313_activity_events,
 		.num_event_specs = ARRAY_SIZE(adxl313_activity_events),
 	},
+	{
+		.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_inactivity_events,
+		.num_event_specs = ARRAY_SIZE(adxl313_inactivity_events),
+	},
 };
 
 static const unsigned long adxl313_scan_masks[] = {
@@ -331,6 +353,15 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_set_inact_time_s(struct adxl313_data *data,
+				    unsigned int val_s)
+{
+	unsigned int max_boundary = U8_MAX; /* by register size */
+	unsigned int val = min(val_s, max_boundary);
+
+	return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
+}
+
 static int adxl313_is_act_inact_en(struct adxl313_data *data,
 				   enum adxl313_activity_type type)
 {
@@ -342,7 +373,17 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
 	if (ret)
 		return ret;
 
-	axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+	/* Check if axis for activity are enabled */
+	switch (type) {
+	case ADXL313_ACTIVITY:
+		axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+		break;
+	case ADXL313_INACTIVITY:
+		axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	if (!axis_en)
 		return false;
@@ -361,11 +402,9 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 {
 	unsigned int axis_ctrl;
 	unsigned int threshold;
+	unsigned int inact_time_s;
 	int ret;
 
-	if (type != ADXL313_ACTIVITY)
-		return 0;
-
 	if (cmd_en) {
 		/* When turning on, check if threshold is valid */
 		ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type],
@@ -375,15 +414,33 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 
 		if (!threshold) /* Just ignore the command if threshold is 0 */
 			return 0;
+
+		/* When turning on inactivity, check if inact time is valid */
+		if (type == ADXL313_INACTIVITY) {
+			ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
+			if (ret)
+				return ret;
+
+			if (!inact_time_s)
+				return 0;
+		}
 	}
 
-	/* Start modifying configuration registers */
 	ret = adxl313_set_measure_en(data, false);
 	if (ret)
 		return ret;
 
 	/* Enable axis according to the command */
-	axis_ctrl = ADXL313_ACT_XYZ_EN;
+	switch (type) {
+	case ADXL313_ACTIVITY:
+		axis_ctrl = ADXL313_ACT_XYZ_EN;
+		break;
+	case ADXL313_INACTIVITY:
+		axis_ctrl = ADXL313_INACT_XYZ_EN;
+		break;
+	default:
+		return -EINVAL;
+	}
 	ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
 				 axis_ctrl, cmd_en);
 	if (ret)
@@ -484,6 +541,8 @@ static int adxl313_read_event_config(struct iio_dev *indio_dev,
 	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;
 	}
@@ -503,6 +562,8 @@ static int adxl313_write_event_config(struct iio_dev *indio_dev,
 	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;
 	}
@@ -517,24 +578,45 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
 	unsigned int act_threshold;
+	unsigned int inact_threshold;
+	unsigned int inact_time_s;
 	int ret;
 
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
+	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;
+		*val = inact_time_s;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -553,16 +635,24 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	/* Scale factor 15.625 mg/LSB */
-	regval = DIV_ROUND_CLOSEST(val * MICRO + val2, 15625);
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		return regmap_write(data->regmap,
-				    adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-				    regval);
+	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:
+			return regmap_write(data->regmap,
+					    adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+					    regval);
+		case IIO_EV_DIR_FALLING:
+			return regmap_write(data->regmap,
+					    adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+					    regval);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		return adxl313_set_inact_time_s(data, val);
 	default:
 		return -EINVAL;
 	}
@@ -722,6 +812,17 @@ static int adxl313_push_events(struct iio_dev *indio_dev, int int_stat)
 			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);
+		if (ret)
+			return ret;
+	}
+
 	return ret;
 }
 
@@ -903,6 +1004,14 @@ int adxl313_core_probe(struct device *dev,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(data->regmap, ADXL313_REG_TIME_INACT, 5);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_INACT, 0x4f);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
 		if (ret)
 			return ret;
-- 
2.39.5
Re: [PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing
Posted by Andy Shevchenko 3 months, 3 weeks ago
On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Enhance the interrupt handler to process inactivity events. Introduce
> functions to configure the threshold and period registers for
> inactivity detection, as well as to enable or disable the inactivity
> feature. Extend the fake IIO channel to handle inactivity events by
> combining the x, y, and z axes using a logical AND operation.

...

> -       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +       /* Check if axis for activity are enabled */
> +       switch (type) {
> +       case ADXL313_ACTIVITY:
> +               axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +               break;
> +       case ADXL313_INACTIVITY:
> +               axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
>
>         if (!axis_en)
>                 return false;

So, this looks better without a variable?

  case FOO:
     if (!FIELD_GET(...))
         return false;
     break;

On the first glance it seems that next changes don't affect this.

...

> -       /* Start modifying configuration registers */

Stray change, the next patch restores this. So why change to  begin with?

>         ret = adxl313_set_measure_en(data, false);
>         if (ret)
>                 return ret;

...

>         /* Enable axis according to the command */
> -       axis_ctrl = ADXL313_ACT_XYZ_EN;
> +       switch (type) {

I was wondering if you can use switch-case earlier in the series.

> +       case ADXL313_ACTIVITY:
> +               axis_ctrl = ADXL313_ACT_XYZ_EN;
> +               break;
> +       case ADXL313_INACTIVITY:
> +               axis_ctrl = ADXL313_INACT_XYZ_EN;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

...

> -       if (info != IIO_EV_INFO_VALUE)
> -               return -EINVAL;
> -
> -       switch (dir) {
> -       case IIO_EV_DIR_RISING:
> +       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;
> +               *val = inact_time_s;
> +               return IIO_VAL_INT;
>         default:
>                 return -EINVAL;
>         }

I still don't get what's wrong with helpers for nested switches?
Instead of doing ping-pong with so many lines (due to indentation
changes), just create a helper from the beginning. In this case this
will look more like


  if (nfo == IIO_EV_INFO_VALUE)
    return my_cool_helper_for_THIS_case(...);

Note, I admit that not all the cases may be done like this, but just
look at this again and perhaps something can be optimised.

--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing
Posted by Lothar Rubusch 3 months, 3 weeks ago
Hi Andy / Hi Jonathan,

Two questions down below.

On Mon, Jun 16, 2025 at 12:59 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
[...]
> > +       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;
> > +               *val = inact_time_s;
> > +               return IIO_VAL_INT;
> >         default:
> >                 return -EINVAL;
> >         }
>
> I still don't get what's wrong with helpers for nested switches?
> Instead of doing ping-pong with so many lines (due to indentation
> changes), just create a helper from the beginning. In this case this
> will look more like
>
>
>   if (nfo == IIO_EV_INFO_VALUE)
>     return my_cool_helper_for_THIS_case(...);
>
> Note, I admit that not all the cases may be done like this, but just
> look at this again and perhaps something can be optimised.
>

First, about helpers dealing with nested switches. The resulting
structure then is like

switch (type) {
case IIO_EV_TYPE_MAG:
    switch (info) {
    case IIO_EV_INFO_VALUE:
        switch (dir) {
        case IIO_EV_DIR_RISING:  // activity
            ....
        case IIO_EV_DIR_FALLING: // inactivity
            ....
        }
        case IIO_EV_INFO_PERIOD:
            ...
    }
case IIO_EV_TYPE_MAG_ADAPTIVE:
      // same as above, again for _AC events
 ...
}

Actually I'm using a helper for nested switches. But currently I'm
adding it quite late, when I have cases for ACTIVITY, INACTIVITY and
ACTIVITY_AC and INACTIVITY_AC, since this results in code duplication.
The resulting structure the looks as follows.

helper_mag(...)
{
    switch (info) {
    case IIO_EV_INFO_VALUE:
        switch (dir) {
        case IIO_EV_DIR_RISING:  // activity
            ....
        case IIO_EV_DIR_FALLING: // inactivity
            ....
        }
        case IIO_EV_INFO_PERIOD:
            ...
    }
}

switch (type) {
case IIO_EV_TYPE_MAG:
    helper_mag(...);
case IIO_EV_TYPE_MAG_ADAPTIVE:
        // same as above, again for _AC events
    helper_mag(...);
}

Is this reasonable? For the v6 now, I plan on introducing this helper
when adding INACTIVITY sensing, having ACTIVITY already in place. This
is, because INACTIVITY as distinguishable type is still not available,
but would be needed as function argument as well. Does this make
sense? Or, should I start with the helper right at the beginning? Is
it ok to have once a nested switch in the helper?

Second question is about the adxl313_read_event_config() functions,
I'd like to have here 0 or 1 in regular cases (<0 for error). Is it ok
if I adjust the functions slightly to guarantee this? Currently it
generally returns >0 in cases of "true" which is correct. But this is
most of the times 1, in some cases can be 8 or something. I just like
it to be uniform for testing (which is not a valid argumentation). Is
this legitimate?

Best,
L

> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing
Posted by Andy Shevchenko 3 months, 3 weeks ago
On Tue, Jun 17, 2025 at 1:10 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Mon, Jun 16, 2025 at 12:59 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@gmail.com> wrote:

[...]

> > > +       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;
> > > +               *val = inact_time_s;
> > > +               return IIO_VAL_INT;
> > >         default:
> > >                 return -EINVAL;
> > >         }
> >
> > I still don't get what's wrong with helpers for nested switches?
> > Instead of doing ping-pong with so many lines (due to indentation
> > changes), just create a helper from the beginning. In this case this
> > will look more like
> >
> >   if (nfo == IIO_EV_INFO_VALUE)
> >     return my_cool_helper_for_THIS_case(...);
> >
> > Note, I admit that not all the cases may be done like this, but just
> > look at this again and perhaps something can be optimised.
>
> First, about helpers dealing with nested switches. The resulting
> structure then is like
>
> switch (type) {
> case IIO_EV_TYPE_MAG:
>     switch (info) {
>     case IIO_EV_INFO_VALUE:
>         switch (dir) {
>         case IIO_EV_DIR_RISING:  // activity
>             ....
>         case IIO_EV_DIR_FALLING: // inactivity
>             ....
>         }
>         case IIO_EV_INFO_PERIOD:
>             ...
>     }
> case IIO_EV_TYPE_MAG_ADAPTIVE:
>       // same as above, again for _AC events
>  ...
> }
>
> Actually I'm using a helper for nested switches. But currently I'm
> adding it quite late, when I have cases for ACTIVITY, INACTIVITY and
> ACTIVITY_AC and INACTIVITY_AC, since this results in code duplication.
> The resulting structure the looks as follows.
>
> helper_mag(...)
> {
>     switch (info) {
>     case IIO_EV_INFO_VALUE:
>         switch (dir) {
>         case IIO_EV_DIR_RISING:  // activity
>             ....
>         case IIO_EV_DIR_FALLING: // inactivity
>             ....
>         }
>         case IIO_EV_INFO_PERIOD:
>             ...
>     }
> }
>
> switch (type) {
> case IIO_EV_TYPE_MAG:
>     helper_mag(...);
> case IIO_EV_TYPE_MAG_ADAPTIVE:
>         // same as above, again for _AC events
>     helper_mag(...);
> }
>
> Is this reasonable? For the v6 now, I plan on introducing this helper
> when adding INACTIVITY sensing, having ACTIVITY already in place. This
> is, because INACTIVITY as distinguishable type is still not available,
> but would be needed as function argument as well. Does this make
> sense? Or, should I start with the helper right at the beginning? Is
> it ok to have once a nested switch in the helper?

Yes, that's what even would propose here, is to make a helper with the
current code (if it's not empty) and then fill it with the content. In
any case try and see.

The (end) idea is to have only one level of the switch-case per
function in this case and use helpers for the inner ones:

foo()
{
  if (...)
    return -EINVAL;
  switch (dir) {
  case BAZ:
    ...
    break;
  }
}

switch (type) {
case FOO:
  return _do_foo();
}

> Second question is about the adxl313_read_event_config() functions,
> I'd like to have here 0 or 1 in regular cases (<0 for error). Is it ok
> if I adjust the functions slightly to guarantee this? Currently it
> generally returns >0 in cases of "true" which is correct. But this is
> most of the times 1, in some cases can be 8 or something. I just like
> it to be uniform for testing (which is not a valid argumentation). Is
> this legitimate?

If this is an ABI, better to unify this to have the same meaning of
each of the returned values independently on the functions, if this is
just an internal, who cares? However, there is, of course, a corner
case if MSB is set and you will return a (positive) value as an error
code. So, just look at the functions and decide which path you take.

-- 
With Best Regards,
Andy Shevchenko