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 | 237 ++++++++++++++++++++++++++++++-
1 file changed, 235 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 8a0b5542fb40..e647c40575ab 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,68 @@ 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 axis_en, int_en, ret;
+
+ ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
+ if (ret)
+ 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);
+
+ /* The axis are enabled, now check if specific interrupt is enabled */
+ ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
+ if (ret)
+ return ret;
+
+ int_en = adxl313_act_int_reg[type] & regval;
+
+ return axis_en && int_en;
+}
+
+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 (type != ADXL313_ACTIVITY)
+ return 0;
+
+ axis_ctrl = ADXL313_ACT_XYZ_EN;
+
+ ret = adxl313_set_measure_en(data, false);
+ if (ret)
+ return ret;
+
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+ axis_ctrl, cmd_en);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
+ if (ret)
+ return ret;
+
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+ adxl313_act_int_reg[type],
+ cmd_en && threshold);
+ 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 +466,113 @@ 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);
+
+ if (type != IIO_EV_TYPE_MAG)
+ return -EINVAL;
+
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_is_act_inact_en(data, 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);
+
+ if (type != IIO_EV_TYPE_MAG)
+ return -EINVAL;
+
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
+ 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 */
+
+ if (type != IIO_EV_TYPE_MAG)
+ return -EINVAL;
+
+ if (info != IIO_EV_INFO_VALUE)
+ return -EINVAL;
+
+ 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;
+ 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;
+
+ 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);
+ if (ret)
+ return ret;
+ return adxl313_set_measure_en(data, true);
+ default:
+ return -EINVAL;
+ }
+}
+
static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
{
struct adxl313_data *data = iio_priv(indio_dev);
@@ -502,19 +705,32 @@ 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_OR_Y_OR_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)
return samples;
- return adxl313_fifo_push(indio_dev, samples);
+ ret = adxl313_fifo_push(indio_dev, samples);
}
/* Return error if no event data was pushed to the IIO channel. */
- return -ENOENT;
+ return ret;
}
static irqreturn_t adxl313_irq_handler(int irq, void *p)
@@ -553,6 +769,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,
@@ -666,6 +886,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, 1 Jun 2025 17:21:35 +0000
Lothar Rubusch <l.rubusch@gmail.com> 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.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,
My main question from this read through is whether we need to be quite
so careful on disabling measurement when configuring events. It is rather
unusual if that is necessary and I'm not sure that's what the datasheet
is implying with the vague bit of advice.
> static const unsigned long adxl313_scan_masks[] = {
> @@ -297,6 +331,68 @@ 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 axis_en, int_en, ret;
> +
> + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> + if (ret)
> + return ret;
> +
> + /* Check if axis for activity are enabled */
> + if (type != ADXL313_ACTIVITY)
As below - only one value possible, so don't check it.
> + return 0;
> +
> + axis_en = FIELD_GET(ADXL313_ACT_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);
> + if (ret)
> + return ret;
> +
> + int_en = adxl313_act_int_reg[type] & regval;
> +
> + return axis_en && int_en;
> +}
> +
> +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 (type != ADXL313_ACTIVITY)
As the enum only has one value you can drop this check.
Obviously it's dropped in next patch anyway but better to never
introduce it.
> + return 0;
> +
> + axis_ctrl = ADXL313_ACT_XYZ_EN;
> +
> + ret = adxl313_set_measure_en(data, false);
> + if (ret)
> + return ret;
> +
> + ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
> + axis_ctrl, cmd_en);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> + if (ret)
> + return ret;
> +
> + ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> + adxl313_act_int_reg[type],
> + cmd_en && threshold);
> + 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 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> }
> }
> +
> +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 */
If it isn't safe to read whilst measurements are in progress (as opposed
to maybe getting a small variation in timing) then this seems more
fragile than I'd like (to future code changes for example).
Might need an explicit check on it being cached regcache_reg_cached()
for example though that is very rarely used which makes me dubious
about using it here.
> +
> + if (type != IIO_EV_TYPE_MAG)
> + return -EINVAL;
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + 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;
> + 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;
> +
> + 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);
I'm surprised this can only be set with measurement disabled.
Maybe a spec reference. It's common to tweak event values as events
come in and we generally don't have to stop data flow whilst we do.
There are a few specific bits where the datasheet suggests updating
them has unwanted side effects in measurement mode. + there is a general
suggestion to do configuration before enabling measurement mode.
I don't see anything saying it is a problem for this register.
> + if (ret)
> + return ret;
> + return adxl313_set_measure_en(data, true);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> {
> struct adxl313_data *data = iio_priv(indio_dev);
> @@ -502,19 +705,32 @@ 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)
Ah. This does not also have events. Still it's a mix, so maybe
adxl313_handle_interrupts() or something like that.
> {
> + 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_OR_Y_OR_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)
> return samples;
>
> - return adxl313_fifo_push(indio_dev, samples);
> + ret = adxl313_fifo_push(indio_dev, samples);
> }
>
> /* Return error if no event data was pushed to the IIO channel. */
> - return -ENOENT;
> + return ret;
This handling works, but as Andy observed maybe the comment is now confusing
given ret is mostly not an error. Perhaps put that where ret is declared
instead, or use a separate mask check at the start to quickly
error out if no bits that we handle are set.
> }
On Sun, Jun 8, 2025 at 6:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 1 Jun 2025 17:21:35 +0000
> Lothar Rubusch <l.rubusch@gmail.com> 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.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> Hi Lothar,
>
> My main question from this read through is whether we need to be quite
> so careful on disabling measurement when configuring events. It is rather
> unusual if that is necessary and I'm not sure that's what the datasheet
> is implying with the vague bit of advice.
>
> > static const unsigned long adxl313_scan_masks[] = {
> > @@ -297,6 +331,68 @@ 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 axis_en, int_en, ret;
> > +
> > + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + /* Check if axis for activity are enabled */
> > + if (type != ADXL313_ACTIVITY)
>
> As below - only one value possible, so don't check it.
>
> > + return 0;
> > +
> > + axis_en = FIELD_GET(ADXL313_ACT_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);
> > + if (ret)
> > + return ret;
> > +
> > + int_en = adxl313_act_int_reg[type] & regval;
> > +
> > + return axis_en && int_en;
> > +}
> > +
> > +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 (type != ADXL313_ACTIVITY)
>
> As the enum only has one value you can drop this check.
> Obviously it's dropped in next patch anyway but better to never
> introduce it.
>
> > + return 0;
> > +
> > + axis_ctrl = ADXL313_ACT_XYZ_EN;
> > +
> > + ret = adxl313_set_measure_en(data, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
> > + axis_ctrl, cmd_en);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > + adxl313_act_int_reg[type],
> > + cmd_en && threshold);
> > + 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 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> > }
> > }
>
> > +
> > +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 */
>
> If it isn't safe to read whilst measurements are in progress (as opposed
> to maybe getting a small variation in timing) then this seems more
> fragile than I'd like (to future code changes for example).
>
> Might need an explicit check on it being cached regcache_reg_cached()
> for example though that is very rarely used which makes me dubious
> about using it here.
>
>
> > +
> > + if (type != IIO_EV_TYPE_MAG)
> > + return -EINVAL;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + 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;
> > + 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;
> > +
> > + 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);
>
> I'm surprised this can only be set with measurement disabled.
> Maybe a spec reference. It's common to tweak event values as events
> come in and we generally don't have to stop data flow whilst we do.
>
> There are a few specific bits where the datasheet suggests updating
> them has unwanted side effects in measurement mode. + there is a general
> suggestion to do configuration before enabling measurement mode.
> I don't see anything saying it is a problem for this register.
>
AFAIK there is no issue, nor a big side effect. Changing config
registers might lead to initially wrong measurements. Just the first
measurements might be wrong. I guess this could be a problem if the
sensor had more features and, say, any kind of threshold for some
event then triggered an event wrongly. In case of the ADXL313, there
should not be such a risk. It's then rather about initial wrong
measurements. (Exception: changing the FIFO modes, where turning to
standby is explicitely recommended).
Unfortunately, I could not recall the exact page in the datasheet, but
it matched pretty much my observation also with the ADXL345. So, I'll
give it a try and remove turning off measurement for the
write_event_config()
> > + if (ret)
> > + return ret;
> > + return adxl313_set_measure_en(data, true);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > {
> > struct adxl313_data *data = iio_priv(indio_dev);
> > @@ -502,19 +705,32 @@ 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)
>
> Ah. This does not also have events. Still it's a mix, so maybe
> adxl313_handle_interrupts() or something like that.
I also could break it up into:
- handle interrupt source register events
- drain fifo watermark samples
?
> > {
> > + 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_OR_Y_OR_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)
> > return samples;
> >
> > - return adxl313_fifo_push(indio_dev, samples);
> > + ret = adxl313_fifo_push(indio_dev, samples);
> > }
> >
> > /* Return error if no event data was pushed to the IIO channel. */
> > - return -ENOENT;
> > + return ret;
> This handling works, but as Andy observed maybe the comment is now confusing
> given ret is mostly not an error. Perhaps put that where ret is declared
> instead, or use a separate mask check at the start to quickly
> error out if no bits that we handle are set.
> > }
Yes. Andy also pointed out here. I already developed a feeling for
"something's smelly" with this code, but cannot really think of a
better approach. Actually it works, and for me it is somehow logical.
Probably there are better ways to solve this situation in a cleaner
way?
> > > static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > > {
> > > struct adxl313_data *data = iio_priv(indio_dev);
> > > @@ -502,19 +705,32 @@ 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)
> >
> > Ah. This does not also have events. Still it's a mix, so maybe
> > adxl313_handle_interrupts() or something like that.
>
> I also could break it up into:
> - handle interrupt source register events
> - drain fifo watermark samples
Sure - if that makes more logical sense that break up is fine.
> ?
>
> > > {
> > > + 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_OR_Y_OR_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)
> > > return samples;
> > >
> > > - return adxl313_fifo_push(indio_dev, samples);
> > > + ret = adxl313_fifo_push(indio_dev, samples);
> > > }
> > >
> > > /* Return error if no event data was pushed to the IIO channel. */
> > > - return -ENOENT;
> > > + return ret;
> > This handling works, but as Andy observed maybe the comment is now confusing
> > given ret is mostly not an error. Perhaps put that where ret is declared
> > instead, or use a separate mask check at the start to quickly
> > error out if no bits that we handle are set.
> > > }
>
> Yes. Andy also pointed out here. I already developed a feeling for
> "something's smelly" with this code, but cannot really think of a
> better approach. Actually it works, and for me it is somehow logical.
> Probably there are better ways to solve this situation in a cleaner
> way?
The check against a mask at the start is pretty common way to deal with
no status bits (that we understand) set. Then update that mask
define as you support more bits.
That deals with the odd error case. It is technically an additional
conditional but the compiler may squash it anyway or if on a decent CPU
it'll be a very easy to predict branch as it should never happen.
Jonathan
>
On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> 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.
IIO
And what does the 'anded' mean?
> 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)
> +{
> + unsigned int axis_ctrl;
> + unsigned int regval;
> + int axis_en, int_en, ret;
> +
> + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> + if (ret)
> + 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 it's false, it will be false anyway. No need to defer the check:
if (!axis_en)
return false;
> + /* The axis are enabled, now check if specific interrupt is enabled */
> + ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
> + if (ret)
> + return ret;
> +
> + int_en = adxl313_act_int_reg[type] & regval;
> +
> + return axis_en && int_en;
return ... & regval;
> +}
I have already commented on this a couple of times.
...
> + /* Scale factor 15.625 mg/LSB */
> + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
I would rather do
val * MICRO + val2
which is read more naturally (we will easily get that the expression
uses MICRO scale).
...
> + 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;
> + }
>
> if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> samples = adxl313_get_samples(data);
> if (samples < 0)
> return samples;
>
> - return adxl313_fifo_push(indio_dev, samples);
> + ret = adxl313_fifo_push(indio_dev, samples);
This is not needed...
> }
>
> /* Return error if no event data was pushed to the IIO channel. */
> - return -ENOENT;
> + return ret;
...and this looks wrong.
Before the case was clear, if we have no respective bit set in the
int_stat, we return ENOENT. Now it depends on the other bit. If this
is correct behaviour, it needs a comment.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Add 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.
>
> IIO
>
> And what does the 'anded' mean?
>
> > 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)
> > +{
> > + unsigned int axis_ctrl;
> > + unsigned int regval;
> > + int axis_en, int_en, ret;
> > +
> > + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > + if (ret)
> > + 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 it's false, it will be false anyway. No need to defer the check:
>
> if (!axis_en)
> return false;
>
> > + /* The axis are enabled, now check if specific interrupt is enabled */
> > + ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + int_en = adxl313_act_int_reg[type] & regval;
> > +
> > + return axis_en && int_en;
>
> return ... & regval;
>
> > +}
>
> I have already commented on this a couple of times.
>
> ...
>
> > + /* Scale factor 15.625 mg/LSB */
> > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
>
> I would rather do
>
> val * MICRO + val2
>
> which is read more naturally (we will easily get that the expression
> uses MICRO scale).
>
> ...
>
> > + 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;
> > + }
> >
> > if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > samples = adxl313_get_samples(data);
> > if (samples < 0)
> > return samples;
> >
> > - return adxl313_fifo_push(indio_dev, samples);
> > + ret = adxl313_fifo_push(indio_dev, samples);
>
> This is not needed...
>
IMHO this will be needed, or shall be needed in the follow up context.
The [going to be renamed] function push_events() shall evaluate the
interrupt status register for the events the driver can handle and
also eventually drain the FIFO in case of watermark. It shall
distinguish between failure, events / drain the FIFO which can be
handled, and events which cannot be handled so far. It's not a if /
else, there can be some event, and some fifo data. Therefore I'd like
not a simple return here, but init a ret var.
I interpreted your reviews, to change the particular implementation as
if there was just activity. Then in a follow up patch, rewrite it
again, now to distinguish just bewteen just activity and inactivity
e.g. by if/else. Eventually rewrite it by a third approach to
distinghish activity, inactivity, AC-coupled activity and AC-coupled
inactivity, might be now switch/case. Eventually you might complain
that my patches contain way too much modification of every line in
every patch.
I'd rather like to start right away with the final structure with just
the first element - e.g. "activity" - leads to results like the above.
Less churn among patches, but having just one element looks like
having taken an over-complicated approach.
Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.
Perhaps it needs more feedback here?
Another example is seting up the read/write_event_config() or
read/write_event_value() functions. I mean, eventually this will
become a switch/case implementation. Of course with just one element
switch/case seems to be obvious overkill. Going by your advice, I
changed it to if(!..) return, it's definitely cleaner. Definitely in
the follow up patches this will be rewritten, though.
Please, let me know what is the best approach or what I can improve to
avoid such "ping pong patching" as you name it?
Might be that you're right here in this particular case, but then it
would be better to discuss the final structure, isn't it?
> > }
> >
> > /* Return error if no event data was pushed to the IIO channel. */
> > - return -ENOENT;
> > + return ret;
>
> ...and this looks wrong.
Well, as I said. Each separate if-condition (not just if-else), could
be ok or not. If ok, the function still shall continue, might be at
the end, also a watermark flag is in the status reg and the FIFO needs
to be drained. It also might be, that some event comes which the
driver does still not handle, but not necessarily an error
(missconfiguration). So, draining the FIFO helps in most cases to
bring a derailed sensor back on track. If not doing so, it silmply
stops working, you would need to turn off and on again, or even power
cycle the setup.
Probably you have a better idea here, but pls have a look into the
final setup. I really appreciate your feedbacks. I understand this is
a rather problematic part of the code. To me it makes sense like this,
but I'd highly appreciate your advice.
>
> Before the case was clear, if we have no respective bit set in the
> int_stat, we return ENOENT. Now it depends on the other bit. If this
> is correct behaviour, it needs a comment.
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, 11 Jun 2025 16:49:34 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Hi Andy,
>
> On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > Add 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.
> >
> > IIO
> >
> > And what does the 'anded' mean?
> >
> > > 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)
> > > +{
> > > + unsigned int axis_ctrl;
> > > + unsigned int regval;
> > > + int axis_en, int_en, ret;
> > > +
> > > + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > > + if (ret)
> > > + 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 it's false, it will be false anyway. No need to defer the check:
> >
> > if (!axis_en)
> > return false;
> >
> > > + /* The axis are enabled, now check if specific interrupt is enabled */
> > > + ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + int_en = adxl313_act_int_reg[type] & regval;
> > > +
> > > + return axis_en && int_en;
> >
> > return ... & regval;
> >
> > > +}
> >
> > I have already commented on this a couple of times.
> >
> > ...
> >
> > > + /* Scale factor 15.625 mg/LSB */
> > > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> >
> > I would rather do
> >
> > val * MICRO + val2
> >
> > which is read more naturally (we will easily get that the expression
> > uses MICRO scale).
> >
> > ...
> >
> > > + 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;
> > > + }
> > >
> > > if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > > samples = adxl313_get_samples(data);
> > > if (samples < 0)
> > > return samples;
> > >
> > > - return adxl313_fifo_push(indio_dev, samples);
> > > + ret = adxl313_fifo_push(indio_dev, samples);
> >
> > This is not needed...
> >
>
> IMHO this will be needed, or shall be needed in the follow up context.
>
> The [going to be renamed] function push_events() shall evaluate the
> interrupt status register for the events the driver can handle and
> also eventually drain the FIFO in case of watermark. It shall
> distinguish between failure, events / drain the FIFO which can be
> handled, and events which cannot be handled so far. It's not a if /
> else, there can be some event, and some fifo data. Therefore I'd like
> not a simple return here, but init a ret var.
>
> I interpreted your reviews, to change the particular implementation as
> if there was just activity. Then in a follow up patch, rewrite it
> again, now to distinguish just bewteen just activity and inactivity
> e.g. by if/else. Eventually rewrite it by a third approach to
> distinghish activity, inactivity, AC-coupled activity and AC-coupled
> inactivity, might be now switch/case. Eventually you might complain
> that my patches contain way too much modification of every line in
> every patch.
>
> I'd rather like to start right away with the final structure with just
> the first element - e.g. "activity" - leads to results like the above.
> Less churn among patches, but having just one element looks like
> having taken an over-complicated approach.
I'd do the from the first but with the comment up with where ret is
declared.
>
> Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> Perhaps it needs more feedback here?
>
> Another example is seting up the read/write_event_config() or
> read/write_event_value() functions. I mean, eventually this will
> become a switch/case implementation. Of course with just one element
> switch/case seems to be obvious overkill. Going by your advice, I
> changed it to if(!..) return, it's definitely cleaner. Definitely in
> the follow up patches this will be rewritten, though.
Don't do that. Just use the switch from the start.
Sometimes we will give review feedback that doesn't take the whole
series into account (because it takes much longer to review a full series
then reread the feedback to spot anything that turned out to be due
to a later change) In those cases it is fine to just reply to the
comment with - "The switch gathers additional elements in patches X,Y,Z
and so is introduced in this first patch to reduce churn.
>
> Please, let me know what is the best approach or what I can improve to
> avoid such "ping pong patching" as you name it?
>
> Might be that you're right here in this particular case, but then it
> would be better to discuss the final structure, isn't it?
>
>
> > > }
> > >
> > > /* Return error if no event data was pushed to the IIO channel. */
> > > - return -ENOENT;
> > > + return ret;
> >
> > ...and this looks wrong.
>
> Well, as I said. Each separate if-condition (not just if-else), could
> be ok or not. If ok, the function still shall continue, might be at
> the end, also a watermark flag is in the status reg and the FIFO needs
> to be drained. It also might be, that some event comes which the
> driver does still not handle, but not necessarily an error
> (missconfiguration). So, draining the FIFO helps in most cases to
> bring a derailed sensor back on track. If not doing so, it silmply
> stops working, you would need to turn off and on again, or even power
> cycle the setup.
>
> Probably you have a better idea here, but pls have a look into the
> final setup. I really appreciate your feedbacks. I understand this is
> a rather problematic part of the code. To me it makes sense like this,
> but I'd highly appreciate your advice.
The code is correct I think as I said in my review, but it is a little
unusual. One option is sanity check and return early if none of the
events we support is set. That removes this path from consideration.
>
> >
> > Before the case was clear, if we have no respective bit set in the
> > int_stat, we return ENOENT. Now it depends on the other bit. If this
> > is correct behaviour, it needs a comment.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
On Wed, Jun 11, 2025 at 04:15:04PM +0100, Jonathan Cameron wrote: > On Wed, 11 Jun 2025 16:49:34 +0200 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote: ... > > > > - return adxl313_fifo_push(indio_dev, samples); > > > > + ret = adxl313_fifo_push(indio_dev, samples); > > > > > > This is not needed... > > > > > > > IMHO this will be needed, or shall be needed in the follow up context. > > > > The [going to be renamed] function push_events() shall evaluate the > > interrupt status register for the events the driver can handle and > > also eventually drain the FIFO in case of watermark. It shall > > distinguish between failure, events / drain the FIFO which can be > > handled, and events which cannot be handled so far. It's not a if / > > else, there can be some event, and some fifo data. Therefore I'd like > > not a simple return here, but init a ret var. > > > > I interpreted your reviews, to change the particular implementation as > > if there was just activity. Then in a follow up patch, rewrite it > > again, now to distinguish just bewteen just activity and inactivity > > e.g. by if/else. Eventually rewrite it by a third approach to > > distinghish activity, inactivity, AC-coupled activity and AC-coupled > > inactivity, might be now switch/case. Eventually you might complain > > that my patches contain way too much modification of every line in > > every patch. > > > > I'd rather like to start right away with the final structure with just > > the first element - e.g. "activity" - leads to results like the above. > > Less churn among patches, but having just one element looks like > > having taken an over-complicated approach. > > I'd do the from the first but with the comment up with where ret is > declared. > > > Perhaps it's my patch split? Unsure, I tried to note in the commit message: > > > This is a preparatory patch. Some of the definitions and functions are > > > supposed to be extended for inactivity later on. > > Perhaps it needs more feedback here? > > > > Another example is seting up the read/write_event_config() or > > read/write_event_value() functions. I mean, eventually this will > > become a switch/case implementation. Of course with just one element > > switch/case seems to be obvious overkill. Going by your advice, I > > changed it to if(!..) return, it's definitely cleaner. Definitely in > > the follow up patches this will be rewritten, though. > Don't do that. Just use the switch from the start. But at the same time if switch becomes nested and 2+ levels, it's better to split the inner parts to the helpr functions or so. Doing a switch with 2+ levels looks ugly independently on the approach taken. > Sometimes we will give review feedback that doesn't take the whole > series into account (because it takes much longer to review a full series > then reread the feedback to spot anything that turned out to be due > to a later change) In those cases it is fine to just reply to the > comment with - "The switch gathers additional elements in patches X,Y,Z > and so is introduced in this first patch to reduce churn. Indeed. > > Please, let me know what is the best approach or what I can improve to > > avoid such "ping pong patching" as you name it? > > > > Might be that you're right here in this particular case, but then it > > would be better to discuss the final structure, isn't it? -- With Best Regards, Andy Shevchenko
On Wed, Jun 11, 2025 at 04:49:34PM +0200, Lothar Rubusch wrote:
> On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
...
> > > + 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;
> > > + }
> > >
> > > if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > > samples = adxl313_get_samples(data);
> > > if (samples < 0)
> > > return samples;
> > >
> > > - return adxl313_fifo_push(indio_dev, samples);
> > > + ret = adxl313_fifo_push(indio_dev, samples);
> >
> > This is not needed...
>
> IMHO this will be needed, or shall be needed in the follow up context.
Right, but wouldn't be better to update at the same time when this new context
appears?
> The [going to be renamed] function push_events() shall evaluate the
> interrupt status register for the events the driver can handle and
> also eventually drain the FIFO in case of watermark. It shall
> distinguish between failure, events / drain the FIFO which can be
> handled, and events which cannot be handled so far. It's not a if /
> else, there can be some event, and some fifo data. Therefore I'd like
> not a simple return here, but init a ret var.
>
> I interpreted your reviews, to change the particular implementation as
> if there was just activity. Then in a follow up patch, rewrite it
> again, now to distinguish just bewteen just activity and inactivity
> e.g. by if/else. Eventually rewrite it by a third approach to
> distinghish activity, inactivity, AC-coupled activity and AC-coupled
> inactivity, might be now switch/case. Eventually you might complain
> that my patches contain way too much modification of every line in
> every patch.
>
> I'd rather like to start right away with the final structure with just
> the first element - e.g. "activity" - leads to results like the above.
> Less churn among patches, but having just one element looks like
> having taken an over-complicated approach.
>
> Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> Perhaps it needs more feedback here?
>
> Another example is seting up the read/write_event_config() or
> read/write_event_value() functions. I mean, eventually this will
> become a switch/case implementation. Of course with just one element
> switch/case seems to be obvious overkill. Going by your advice, I
> changed it to if(!..) return, it's definitely cleaner. Definitely in
> the follow up patches this will be rewritten, though.
>
> Please, let me know what is the best approach or what I can improve to
> avoid such "ping pong patching" as you name it?
>
> Might be that you're right here in this particular case, but then it
> would be better to discuss the final structure, isn't it?
Basically I use the following rule of thumb: I made an approach and look at
the each patch separately and at the series as a whole (with the end result).
If it's too much of rewriting (yes, I admit, that in some cases it's
unavoidable to have some changes as we do feature-by-feature incremental
changes), I try to rethink. Repeat, until the result looks good enough.
I.o.w. you, as the author of this code, can propose something better based on
your knowledge of the HW and vision of what you want at the end.
Yeah, this might require time and a few attempts which one can argue would be
waste of time. But at least this is my personal experience and flow with my
own patches.
> > > }
> > >
> > > /* Return error if no event data was pushed to the IIO channel. */
> > > - return -ENOENT;
> > > + return ret;
> >
> > ...and this looks wrong.
>
> Well, as I said. Each separate if-condition (not just if-else), could
> be ok or not. If ok, the function still shall continue, might be at
> the end, also a watermark flag is in the status reg and the FIFO needs
> to be drained. It also might be, that some event comes which the
> driver does still not handle, but not necessarily an error
> (missconfiguration). So, draining the FIFO helps in most cases to
> bring a derailed sensor back on track. If not doing so, it silmply
> stops working, you would need to turn off and on again, or even power
> cycle the setup.
>
> Probably you have a better idea here, but pls have a look into the
> final setup. I really appreciate your feedbacks. I understand this is
> a rather problematic part of the code. To me it makes sense like this,
> but I'd highly appreciate your advice.
I will do my best. Thanks for your patience!
> > Before the case was clear, if we have no respective bit set in the
> > int_stat, we return ENOENT. Now it depends on the other bit. If this
> > is correct behaviour, it needs a comment.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.