Extend the interrupt handler to process interrupts as inactivity events.
Add functions to set threshold and period registers for inactivity. Add
functions to enable / disable inactivity. Extend the fake iio channel to
deal with inactivity events on x, y and z combined with AND.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl313.h | 2 +
drivers/iio/accel/adxl313_core.c | 159 +++++++++++++++++++++++++------
2 files changed, 131 insertions(+), 30 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 e647c40575ab..c5767d56b0cb 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 = 255;
+ 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)
{
@@ -343,10 +374,10 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
return ret;
/* Check if axis for activity are enabled */
- if (type != ADXL313_ACTIVITY)
- return 0;
-
- axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+ if (type == ADXL313_ACTIVITY)
+ axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+ else
+ axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
/* The axis are enabled, now check if specific interrupt is enabled */
ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
@@ -364,12 +395,14 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
{
unsigned int axis_ctrl;
unsigned int threshold;
+ unsigned int inact_time_s;
+ bool en;
int ret;
- if (type != ADXL313_ACTIVITY)
- return 0;
-
- axis_ctrl = ADXL313_ACT_XYZ_EN;
+ if (type == ADXL313_ACTIVITY)
+ axis_ctrl = ADXL313_ACT_XYZ_EN;
+ else
+ axis_ctrl = ADXL313_INACT_XYZ_EN;
ret = adxl313_set_measure_en(data, false);
if (ret)
@@ -384,9 +417,17 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
if (ret)
return ret;
+ en = cmd_en && threshold;
+ if (type == ADXL313_INACTIVITY) {
+ ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
+ if (ret)
+ return ret;
+
+ en = en && inact_time_s;
+ }
+
ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
- adxl313_act_int_reg[type],
- cmd_en && threshold);
+ adxl313_act_int_reg[type], en);
if (ret)
return ret;
@@ -479,6 +520,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;
}
@@ -498,6 +541,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;
}
@@ -512,6 +557,8 @@ 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;
/* Measurement stays enabled, reading from regmap cache */
@@ -519,19 +566,38 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
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;
}
@@ -555,16 +621,30 @@ 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(MICRO * val + val2, 15625);
- switch (dir) {
- case IIO_EV_DIR_RISING:
- ret = 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:
+ 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);
@@ -721,6 +801,17 @@ static int adxl313_push_event(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;
+ }
+
if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
samples = adxl313_get_samples(data);
if (samples < 0)
@@ -895,6 +986,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.5On Sun, 1 Jun 2025 17:21:36 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Extend the interrupt handler to process interrupts as inactivity events.
> Add functions to set threshold and period registers for inactivity. Add
> functions to enable / disable inactivity. Extend the fake iio channel to
> deal with inactivity events on x, y and z combined with AND.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> @@ -555,16 +621,30 @@ 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(MICRO * val + val2, 15625);
> - switch (dir) {
> - case IIO_EV_DIR_RISING:
> - ret = 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:
> + 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);
Having the enable in the case statement but he disable outside is misbalanced.
Do it one way or the other (either always disable / enable outside, or both
inside each relevant case statement. If we need to do this, I'd have
a helper function do_adxl313_write_event_value() or similar that is called
between disabling and enabling measurement mode and contains all the other
stuff in this function (or along those lines anyway). You can chose
whether or not to reenable measurement mode depending on what is returned
by the helper function.
On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Extend the interrupt handler to process interrupts as inactivity events.
> Add functions to set threshold and period registers for inactivity. Add
> functions to enable / disable inactivity. Extend the fake iio channel to
IIO
> deal with inactivity events on x, y and z combined with AND.
...
> +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> + unsigned int val_s)
> +{
> + unsigned int max_boundary = 255;
This is unclear how it's defined. What is the limit behind? Size of a
bit field? Decimal value from the datasheet?
The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
the answers to the above questions.
> + unsigned int val = min(val_s, max_boundary);
> +
> + return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> +}
...
> - axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> + if (type == ADXL313_ACTIVITY)
> + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> + else
> + axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
Even with this change my previous comment stays.
...
> + en = cmd_en && threshold;
> + if (type == ADXL313_INACTIVITY) {
> + ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> + if (ret)
> + return ret;
> +
> + en = en && inact_time_s;
> + }
...
> - if (info != IIO_EV_INFO_VALUE)
> - return -EINVAL;
> -
> - /* 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);
Hmm... This was added by the previous patches, right? Why can't it be
done as a switch case to begin with? I remember one of the previous
versions had some nested switch-cases, perhaps you need to rethink on
how to split the code between functions to avoid too much nesting (add
some helper functions?).
> + 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);
--
With Best Regards,
Andy Shevchenko
On Sun, Jun 1, 2025 at 9:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Extend the interrupt handler to process interrupts as inactivity events.
> > Add functions to set threshold and period registers for inactivity. Add
> > functions to enable / disable inactivity. Extend the fake iio channel to
>
> IIO
>
> > deal with inactivity events on x, y and z combined with AND.
>
> ...
>
> > +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> > + unsigned int val_s)
> > +{
> > + unsigned int max_boundary = 255;
>
> This is unclear how it's defined. What is the limit behind? Size of a
> bit field? Decimal value from the datasheet?
>
> The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
> the answers to the above questions.
>
> > + unsigned int val = min(val_s, max_boundary);
> > +
> > + return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> > +}
>
> ...
>
> > - axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > + if (type == ADXL313_ACTIVITY)
> > + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > + else
> > + axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
>
> Even with this change my previous comment stays.
>
> ...
>
> > + en = cmd_en && threshold;
> > + if (type == ADXL313_INACTIVITY) {
> > + ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> > + if (ret)
> > + return ret;
> > +
> > + en = en && inact_time_s;
> > + }
>
> ...
>
> > - if (info != IIO_EV_INFO_VALUE)
> > - return -EINVAL;
> > -
> > - /* 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);
>
> Hmm... This was added by the previous patches, right? Why can't it be
> done as a switch case to begin with? I remember one of the previous
> versions had some nested switch-cases, perhaps you need to rethink on
> how to split the code between functions to avoid too much nesting (add
> some helper functions?).
The point here is, as I mentioned in the other mail:
Initially, I wanted to build up the final switch/case struct i.e.
going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
PERIOD.
This will distinguish properties for four different types of events,
of course it then also will use separate functions. As I uderstood
your review, why starting with switch/case, do
if (!MAG event) then, return right away. I implemented that as I
understood. For further switch/case-ing, I did the same.
Now, patch by patch, it grows. Thus the if-not-back-out lines will be
moved out and replaced by switch/case. Worse, with every level switch
case, all existing code needs indention, thus reading through the
patches show (too) many changes.
How can I improve to help you reviewing this or make the feedback more
useful for me? Or is my approach wrong? I'd like to start with the
switch case right away, then just add up what comes in with every
other patch. If so, you'd only see the changes, since the final
structure of this is already clear, because very similar to all
iio/accel drivers at least (as you probably know better than me).
>
> > + 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);
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, 11 Jun 2025 17:36:49 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Sun, Jun 1, 2025 at 9:46 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > Extend the interrupt handler to process interrupts as inactivity events.
> > > Add functions to set threshold and period registers for inactivity. Add
> > > functions to enable / disable inactivity. Extend the fake iio channel to
> >
> > IIO
> >
> > > deal with inactivity events on x, y and z combined with AND.
> >
> > ...
> >
> > > +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> > > + unsigned int val_s)
> > > +{
> > > + unsigned int max_boundary = 255;
> >
> > This is unclear how it's defined. What is the limit behind? Size of a
> > bit field? Decimal value from the datasheet?
> >
> > The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
> > the answers to the above questions.
> >
> > > + unsigned int val = min(val_s, max_boundary);
> > > +
> > > + return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> > > +}
> >
> > ...
> >
> > > - axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > + if (type == ADXL313_ACTIVITY)
> > > + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > + else
> > > + axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
> >
> > Even with this change my previous comment stays.
> >
> > ...
> >
> > > + en = cmd_en && threshold;
> > > + if (type == ADXL313_INACTIVITY) {
> > > + ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + en = en && inact_time_s;
> > > + }
> >
> > ...
> >
> > > - if (info != IIO_EV_INFO_VALUE)
> > > - return -EINVAL;
> > > -
> > > - /* 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);
> >
> > Hmm... This was added by the previous patches, right? Why can't it be
> > done as a switch case to begin with? I remember one of the previous
> > versions had some nested switch-cases, perhaps you need to rethink on
> > how to split the code between functions to avoid too much nesting (add
> > some helper functions?).
>
> The point here is, as I mentioned in the other mail:
> Initially, I wanted to build up the final switch/case struct i.e.
> going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
> PERIOD.
>
> This will distinguish properties for four different types of events,
> of course it then also will use separate functions. As I uderstood
> your review, why starting with switch/case, do
> if (!MAG event) then, return right away. I implemented that as I
> understood. For further switch/case-ing, I did the same.
> Now, patch by patch, it grows. Thus the if-not-back-out lines will be
> moved out and replaced by switch/case. Worse, with every level switch
> case, all existing code needs indention, thus reading through the
> patches show (too) many changes.
>
> How can I improve to help you reviewing this or make the feedback more
> useful for me? Or is my approach wrong? I'd like to start with the
> switch case right away, then just add up what comes in with every
> other patch. If so, you'd only see the changes, since the final
> structure of this is already clear, because very similar to all
> iio/accel drivers at least (as you probably know better than me).
As mentioned earlier, reviewers tend to look at patches in a linear
fashion (and forget them more or less entirely between versions posted!)
So feel free to push back on earlier review comments that say 'you could
simplify this as X' with 'I did this because it becomes more complex in patch 4
and this reduces churn'.
Maybe here factoring out some elements into helpers will reduce the churn
anyway (to a couple of lines) and that discussion become unnecessary.
J
>
> >
> > > + 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);
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
© 2016 - 2026 Red Hat, Inc.