Re: [PATCH v3 2/4] iio: accel: adxl372: add event interface

Re: [PATCH v3 2/4] iio: accel: adxl372: add event interface

[Lars-Peter Clausen]

On 3/18/20 12:09 PM, Alexandru Tachici wrote:
> Currently the driver configures adxl372 to work in loop mode.
> The inactivity and activity timings  decide how fast the chip
> will loop through the awake and waiting states and the
> thresholds on x,y,z axis decide when activity or inactivity
> will be detected.
> 
> This patch adds standard events sysfs entries for the inactivity
> and activity timings: thresh_falling_period/thresh_rising_period
> and for the in_accel_x_thresh_falling/rising_value.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>

Hi,

Looks very good.

Since you are sending this from your gmail account there is a mismatch 
between the driver author and the signed-off-by. Either Add "From: 
Alexandru Tachici <alexandru.tachici@analog.com>" to the first line of 
the patch description or sign-off with your gmail address.

> ---
[...]
>   #define ADXL372_ACCEL_CHANNEL(index, reg, axis) {			\
>   	.type = IIO_ACCEL,						\
>   	.address = reg,							\
> @@ -241,6 +271,8 @@ static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = {
>   		.storagebits = 16,					\
>   		.shift = 4,						\
>   	},								\
> +	.event_spec = adxl372_events,					\
> +	.num_event_specs = 2						\

ARRAY_SIZE(adxl372_events)

>   }
>   
>   static const struct iio_chan_spec adxl372_channels[] = {
> @@ -280,6 +312,43 @@ static const unsigned long adxl372_channel_masks[] = {
>   	0
>   };
>   
> +static ssize_t adxl372_read_threshold_value(struct iio_dev *indio_dev,
> +					    unsigned int addr,
> +					    u16 *threshold)
> +{
> +	struct adxl372_state *st = iio_priv(indio_dev);
> +	__be16 __regval;

I'd avoid using the __ prefix for normal identifiers as those are 
supposed to be reserved. Maybe use raw_regval or something like that.

> +	u16 regval;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, addr, &__regval, sizeof(__regval));
> +	if (ret < 0)
> +		return ret;
> +
> +	regval = be16_to_cpu(__regval);
> +	regval >>= 5;
> +
> +	*threshold = regval;
> +
> +	return 0;
> +}
> +
> +static ssize_t adxl372_write_threshold_value(struct iio_dev *indio_dev,
> +					     unsigned int addr,
> +					     u16 threshold)
> +{
> +	struct adxl372_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, addr,
> +			   ADXL372_THRESH_VAL_H_SEL(threshold));
> +	if (ret < 0)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, addr + 1, GENMASK(7, 5),
> +				  ADXL372_THRESH_VAL_L_SEL(threshold) << 5);

I think this needs a lock to make sure that the update to the two 
registers is atomic.

> +}
> +
>   static int adxl372_read_axis(struct adxl372_state *st, u8 addr)
>   {
>   	__be16 regval;
> @@ -543,6 +612,27 @@ static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample)
>   	memcpy(sample, axis_sample, 3 * sizeof(__be16));
>   }
>   
> +static void adxl372_push_event(struct iio_dev *indio_dev, s64 timestamp,
> +			       u8 status2)
> +{
> +	unsigned int ev_dir;

= IIO_EV_DIR_NONE;

Otherwise it is uninitialized and you might send spurious events.

> +
> +	if (ADXL372_STATUS_2_ACT(status2))
> +		ev_dir = IIO_EV_DIR_RISING;
> +
> +	if (ADXL372_STATUS_2_INACT(status2))
> +		ev_dir = IIO_EV_DIR_FALLING;
> +
> +	if (ev_dir != IIO_EV_DIR_NONE)
> +		iio_push_event(indio_dev,
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  ev_dir),
> +						  timestamp);
> +}
[...]

Re: [PATCH v3 1/4] iio: accel: adxl372: Add support for FIFO peak mode

[Lars-Peter Clausen]

On 3/18/20 12:09 PM, Alexandru Tachici wrote:
> From: Stefan Popa <stefan.popa@analog.com>
> 
> By default, if all three channels (x, y, z) are enabled, sample sets of
> concurrent 3-axis data is stored in the FIFO. This patch adds the option
> to configure the FIFO to store peak acceleration (x, y and z) of every
> over-threshold event. When pushing to iio buffer we push only enabled
> axis data.
> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> ---
>   drivers/iio/accel/adxl372.c | 74 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index 67b8817995c0..90c37d6f10d3 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -133,6 +133,9 @@
>   
>   /* The ADXL372 includes a deep, 512 sample FIFO buffer */
>   #define ADXL372_FIFO_SIZE			512
> +#define ADXL372_X_AXIS_EN(x)			(((x) >> 0) & 0x1)
> +#define ADXL372_Y_AXIS_EN(x)			(((x) >> 1) & 0x1)
> +#define ADXL372_Z_AXIS_EN(x)			(((x) >> 2) & 0x1)
>   
>   /*
>    * At +/- 200g with 12-bit resolution, scale is computed as:
> @@ -253,6 +256,7 @@ struct adxl372_state {
>   	struct iio_trigger		*dready_trig;
>   	enum adxl372_fifo_mode		fifo_mode;
>   	enum adxl372_fifo_format	fifo_format;
> +	unsigned int			fifo_axis_mask;
>   	enum adxl372_op_mode		op_mode;
>   	enum adxl372_act_proc_mode	act_proc_mode;
>   	enum adxl372_odr		odr;
> @@ -264,6 +268,7 @@ struct adxl372_state {
>   	u8				int2_bitmask;
>   	u16				watermark;
>   	__be16				fifo_buf[ADXL372_FIFO_SIZE];
> +	bool				peak_fifo_mode_en;
>   };
>   
>   static const unsigned long adxl372_channel_masks[] = {
> @@ -522,6 +527,22 @@ static int adxl372_get_status(struct adxl372_state *st,
>   	return ret;
>   }
>   
> +static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample)
> +{
> +	__be16	axis_sample[3];
> +	int i = 0;
> +
> +	memset(axis_sample, 0, 3 * sizeof(__be16));
> +	if (ADXL372_X_AXIS_EN(st->fifo_axis_mask))
> +		axis_sample[i++] = sample[0];
> +	if (ADXL372_Y_AXIS_EN(st->fifo_axis_mask))
> +		axis_sample[i++] = sample[1];
> +	if (ADXL372_Z_AXIS_EN(st->fifo_axis_mask))
> +		axis_sample[i++] = sample[2];
> +
> +	memcpy(sample, axis_sample, 3 * sizeof(__be16));
> +}
> +
>   static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
>   {
>   	struct iio_poll_func *pf = p;
> @@ -553,8 +574,12 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
>   			goto err;
>   
>   		/* Each sample is 2 bytes */
> -		for (i = 0; i < fifo_entries; i += st->fifo_set_size)
> +		for (i = 0; i < fifo_entries; i += st->fifo_set_size) {
> +			/* filter peak detection data */
> +			if (st->peak_fifo_mode_en)
> +				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
>   			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> +		}
>   	}
>   err:
>   	iio_trigger_notify_done(indio_dev->trig);
> @@ -722,6 +747,43 @@ static int adxl372_write_raw(struct iio_dev *indio_dev,
>   	}
>   }
>   
> +static ssize_t adxl372_peak_fifo_en_get(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct adxl372_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", st->peak_fifo_mode_en);
> +}
> +
> +static ssize_t adxl372_peak_fifo_en_set(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adxl372_state *st = iio_priv(indio_dev);
> +	bool val;
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	st->peak_fifo_mode_en = val;
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_peak_mode_enable, 0644,
> +		       adxl372_peak_fifo_en_get,
> +		       adxl372_peak_fifo_en_set, 0);
> +

Rather than going with a non-standard attribute, I'd register a IIO 
trigger for the peak mode and switch between peak and normal mode by 
assigning the corresponding trigger.

At least that's how I understand how this mode works. Data capture is 
triggered by exceeding a threshold.

- Lars

Re: [PATCH v3 3/4] iio: accel: adxl372: add additional events ABIs

[Lars-Peter Clausen]

On 3/18/20 12:09 PM, Alexandru Tachici wrote:
> Adxl372 uses the standard event interface. The additional
> ABIs aim to explain to the user that the values set in
> ./events/thresh_falling_period and ./events/thresh_rising_period
> control the state of the device, not just the events timings.
> 

Hi,

I'm not convinced this is a good idea. Since this is non-standard ABI 
the user would likely have to consult the documentation to figure out 
what this means. So we might as well document in the documentation that 
for peak mode the thresholds are configured through the event API.

- Lars

[PATCH v3 0/6] iio: accel: adxl372: add peak mode

[Alexandru Tachici]

This series adds the possibility to configure
the device, from sysfs, to work in peak mode. This enables
adxl372 to capture only over threshold accelerations.

1. Create sysfs files for falling_period/rising_period
and thresh_falling_value/thresh_rising_value in events/ dir.
Set INT1 reg for activity/inactivity and push
event code in events fifo on irq.

2. Add additional ABIs in order to explain the
user that setting values in the events/ sysfs files
also changes devices behaviour.

3. Update sysfs docs: renames, added two new
entries for the activity_detect_event/
inactivity_detect_event

Alexandru Tachici (3):
  iio: accel: adxl372: add event interface
  iio: accel: adxl372: add additional events ABIs
  iio: accel: adxl372: Update sysfs docs

1. Device FIFO can now be set in peak mode and only over the
threshold accelerations will be stored.

Stefan Popa (1):
  iio: accel: adxl372: Add support for FIFO peak mode

Changelog v2 -> v3:
- now use iio_device_claim_direct_mode before setting peak mode
- if peak mode is enabled and not all axis are enabled, filter out disabled axis
data before iio_push_to_buffers
- add events enable attributes: (thresh_rising_en/thresh_falling_en)
- renamed attr: buffer_peak_mode_enable to hwfifo_peak_mode_enable
- squashed event interface related commits into one

 .../ABI/testing/sysfs-bus-iio-accel-adxl372   |  30 ++
 drivers/iio/accel/adxl372.c                   | 345 +++++++++++++++++-
 2 files changed, 368 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-accel-adxl372

-- 
2.20.1

[PATCH v3 1/4] iio: accel: adxl372: Add support for FIFO peak mode

[Alexandru Tachici]

From: Stefan Popa <stefan.popa@analog.com>

By default, if all three channels (x, y, z) are enabled, sample sets of
concurrent 3-axis data is stored in the FIFO. This patch adds the option
to configure the FIFO to store peak acceleration (x, y and z) of every
over-threshold event. When pushing to iio buffer we push only enabled
axis data.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
 drivers/iio/accel/adxl372.c | 74 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 67b8817995c0..90c37d6f10d3 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -133,6 +133,9 @@
 
 /* The ADXL372 includes a deep, 512 sample FIFO buffer */
 #define ADXL372_FIFO_SIZE			512
+#define ADXL372_X_AXIS_EN(x)			(((x) >> 0) & 0x1)
+#define ADXL372_Y_AXIS_EN(x)			(((x) >> 1) & 0x1)
+#define ADXL372_Z_AXIS_EN(x)			(((x) >> 2) & 0x1)
 
 /*
  * At +/- 200g with 12-bit resolution, scale is computed as:
@@ -253,6 +256,7 @@ struct adxl372_state {
 	struct iio_trigger		*dready_trig;
 	enum adxl372_fifo_mode		fifo_mode;
 	enum adxl372_fifo_format	fifo_format;
+	unsigned int			fifo_axis_mask;
 	enum adxl372_op_mode		op_mode;
 	enum adxl372_act_proc_mode	act_proc_mode;
 	enum adxl372_odr		odr;
@@ -264,6 +268,7 @@ struct adxl372_state {
 	u8				int2_bitmask;
 	u16				watermark;
 	__be16				fifo_buf[ADXL372_FIFO_SIZE];
+	bool				peak_fifo_mode_en;
 };
 
 static const unsigned long adxl372_channel_masks[] = {
@@ -522,6 +527,22 @@ static int adxl372_get_status(struct adxl372_state *st,
 	return ret;
 }
 
+static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample)
+{
+	__be16	axis_sample[3];
+	int i = 0;
+
+	memset(axis_sample, 0, 3 * sizeof(__be16));
+	if (ADXL372_X_AXIS_EN(st->fifo_axis_mask))
+		axis_sample[i++] = sample[0];
+	if (ADXL372_Y_AXIS_EN(st->fifo_axis_mask))
+		axis_sample[i++] = sample[1];
+	if (ADXL372_Z_AXIS_EN(st->fifo_axis_mask))
+		axis_sample[i++] = sample[2];
+
+	memcpy(sample, axis_sample, 3 * sizeof(__be16));
+}
+
 static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 {
 	struct iio_poll_func *pf = p;
@@ -553,8 +574,12 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 			goto err;
 
 		/* Each sample is 2 bytes */
-		for (i = 0; i < fifo_entries; i += st->fifo_set_size)
+		for (i = 0; i < fifo_entries; i += st->fifo_set_size) {
+			/* filter peak detection data */
+			if (st->peak_fifo_mode_en)
+				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
 			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+		}
 	}
 err:
 	iio_trigger_notify_done(indio_dev->trig);
@@ -722,6 +747,43 @@ static int adxl372_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static ssize_t adxl372_peak_fifo_en_get(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct adxl372_state *st = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", st->peak_fifo_mode_en);
+}
+
+static ssize_t adxl372_peak_fifo_en_set(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adxl372_state *st = iio_priv(indio_dev);
+	bool val;
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret < 0)
+		return ret;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	st->peak_fifo_mode_en = val;
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(hwfifo_peak_mode_enable, 0644,
+		       adxl372_peak_fifo_en_get,
+		       adxl372_peak_fifo_en_set, 0);
+
 static ssize_t adxl372_show_filter_freq_avail(struct device *dev,
 					      struct device_attribute *attr,
 					      char *buf)
@@ -815,13 +877,22 @@ static int adxl372_buffer_postenable(struct iio_dev *indio_dev)
 	}
 
 	st->fifo_format = adxl372_axis_lookup_table[i].fifo_format;
+	st->fifo_axis_mask = adxl372_axis_lookup_table[i].bits;
 	st->fifo_set_size = bitmap_weight(indio_dev->active_scan_mask,
 					  indio_dev->masklength);
+
+	/* Configure the FIFO to store sets of impact event peak. */
+	if (st->peak_fifo_mode_en) {
+		st->fifo_set_size = 3;
+		st->fifo_format = ADXL372_XYZ_PEAK_FIFO;
+	}
+
 	/*
 	 * The 512 FIFO samples can be allotted in several ways, such as:
 	 * 170 sample sets of concurrent 3-axis data
 	 * 256 sample sets of concurrent 2-axis data (user selectable)
 	 * 512 sample sets of single-axis data
+	 * 170 sets of impact event peak (x, y, z)
 	 */
 	if ((st->watermark * st->fifo_set_size) > ADXL372_FIFO_SIZE)
 		st->watermark = (ADXL372_FIFO_SIZE  / st->fifo_set_size);
@@ -894,6 +965,7 @@ static IIO_DEVICE_ATTR(in_accel_filter_low_pass_3db_frequency_available,
 static struct attribute *adxl372_attributes[] = {
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_filter_low_pass_3db_frequency_available.dev_attr.attr,
+	&iio_dev_attr_hwfifo_peak_mode_enable.dev_attr.attr,
 	NULL,
 };
 
-- 
2.20.1

[PATCH v3 2/4] iio: accel: adxl372: add event interface

[Alexandru Tachici]

Currently the driver configures adxl372 to work in loop mode.
The inactivity and activity timings  decide how fast the chip
will loop through the awake and waiting states and the
thresholds on x,y,z axis decide when activity or inactivity
will be detected.

This patch adds standard events sysfs entries for the inactivity
and activity timings: thresh_falling_period/thresh_rising_period
and for the in_accel_x_thresh_falling/rising_value.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/accel/adxl372.c | 247 +++++++++++++++++++++++++++++++++++-
 1 file changed, 241 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 90c37d6f10d3..d8f95c9f9587 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -5,6 +5,7 @@
  * Copyright 2018 Analog Devices Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -113,6 +114,11 @@
 #define ADXL372_STATUS_1_AWAKE(x)		(((x) >> 6) & 0x1)
 #define ADXL372_STATUS_1_ERR_USR_REGS(x)	(((x) >> 7) & 0x1)
 
+/* ADXL372_STATUS_2 */
+#define ADXL372_STATUS_2_INACT(x)		(((x) >> 4) & 0x1)
+#define ADXL372_STATUS_2_ACT(x)			(((x) >> 5) & 0x1)
+#define ADXL372_STATUS_2_AC2(x)			(((x) >> 6) & 0x1)
+
 /* ADXL372_INT1_MAP */
 #define ADXL372_INT1_MAP_DATA_RDY_MSK		BIT(0)
 #define ADXL372_INT1_MAP_DATA_RDY_MODE(x)	(((x) & 0x1) << 0)
@@ -131,6 +137,14 @@
 #define ADXL372_INT1_MAP_LOW_MSK		BIT(7)
 #define ADXL372_INT1_MAP_LOW_MODE(x)		(((x) & 0x1) << 7)
 
+/* ADX372_THRESH */
+#define ADXL372_THRESH_VAL_H_MSK		GENMASK(10, 3)
+#define ADXL372_THRESH_VAL_H_SEL(x)		\
+		FIELD_GET(ADXL372_THRESH_VAL_H_MSK, x)
+#define ADXL372_THRESH_VAL_L_MSK		GENMASK(2, 0)
+#define ADXL372_THRESH_VAL_L_SEL(x)		\
+		FIELD_GET(ADXL372_THRESH_VAL_L_MSK, x)
+
 /* The ADXL372 includes a deep, 512 sample FIFO buffer */
 #define ADXL372_FIFO_SIZE			512
 #define ADXL372_X_AXIS_EN(x)			(((x) >> 0) & 0x1)
@@ -225,6 +239,22 @@ static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = {
 	{ BIT(0) | BIT(1) | BIT(2), ADXL372_XYZ_FIFO },
 };
 
+static const struct iio_event_spec adxl372_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD) |
+				      BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD) |
+				      BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 #define ADXL372_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
 	.address = reg,							\
@@ -241,6 +271,8 @@ static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = {
 		.storagebits = 16,					\
 		.shift = 4,						\
 	},								\
+	.event_spec = adxl372_events,					\
+	.num_event_specs = 2						\
 }
 
 static const struct iio_chan_spec adxl372_channels[] = {
@@ -280,6 +312,43 @@ static const unsigned long adxl372_channel_masks[] = {
 	0
 };
 
+static ssize_t adxl372_read_threshold_value(struct iio_dev *indio_dev,
+					    unsigned int addr,
+					    u16 *threshold)
+{
+	struct adxl372_state *st = iio_priv(indio_dev);
+	__be16 __regval;
+	u16 regval;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, addr, &__regval, sizeof(__regval));
+	if (ret < 0)
+		return ret;
+
+	regval = be16_to_cpu(__regval);
+	regval >>= 5;
+
+	*threshold = regval;
+
+	return 0;
+}
+
+static ssize_t adxl372_write_threshold_value(struct iio_dev *indio_dev,
+					     unsigned int addr,
+					     u16 threshold)
+{
+	struct adxl372_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(st->regmap, addr,
+			   ADXL372_THRESH_VAL_H_SEL(threshold));
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(st->regmap, addr + 1, GENMASK(7, 5),
+				  ADXL372_THRESH_VAL_L_SEL(threshold) << 5);
+}
+
 static int adxl372_read_axis(struct adxl372_state *st, u8 addr)
 {
 	__be16 regval;
@@ -543,6 +612,27 @@ static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample)
 	memcpy(sample, axis_sample, 3 * sizeof(__be16));
 }
 
+static void adxl372_push_event(struct iio_dev *indio_dev, s64 timestamp,
+			       u8 status2)
+{
+	unsigned int ev_dir;
+
+	if (ADXL372_STATUS_2_ACT(status2))
+		ev_dir = IIO_EV_DIR_RISING;
+
+	if (ADXL372_STATUS_2_INACT(status2))
+		ev_dir = IIO_EV_DIR_FALLING;
+
+	if (ev_dir != IIO_EV_DIR_NONE)
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_X_OR_Y_OR_Z,
+						  IIO_EV_TYPE_THRESH,
+						  ev_dir),
+						  timestamp);
+}
+
 static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 {
 	struct iio_poll_func *pf = p;
@@ -556,6 +646,8 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 	if (ret < 0)
 		goto err;
 
+	adxl372_push_event(indio_dev, iio_get_time_ns(indio_dev), status2);
+
 	if (st->fifo_mode != ADXL372_FIFO_BYPASSED &&
 	    ADXL372_STATUS_1_FIFO_FULL(status1)) {
 		/*
@@ -747,6 +839,143 @@ static int adxl372_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl372_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 adxl372_state *st = iio_priv(indio_dev);
+	unsigned int addr;
+	u16 raw_value;
+	int ret;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = ADXL372_X_THRESH_ACT_H + 2 * chan->scan_index;
+			ret = adxl372_read_threshold_value(indio_dev, addr,
+							   &raw_value);
+			if (ret < 0)
+				return ret;
+			*val = raw_value * ADXL372_USCALE;
+			*val2 = 1000000;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_DIR_FALLING:
+			addr = ADXL372_X_THRESH_INACT_H + 2 * chan->scan_index;
+			ret =  adxl372_read_threshold_value(indio_dev, addr,
+							    &raw_value);
+			if (ret < 0)
+				return ret;
+			*val = raw_value * ADXL372_USCALE;
+			*val2 = 1000000;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			*val = st->act_time_ms;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_DIR_FALLING:
+			*val = st->inact_time_ms;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl372_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 adxl372_state *st = iio_priv(indio_dev);
+	unsigned int val_ms;
+	unsigned int addr;
+	u16 raw_val;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		raw_val = DIV_ROUND_UP(val * 1000000, ADXL372_USCALE);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = ADXL372_X_THRESH_ACT_H + 2 * chan->scan_index;
+			return adxl372_write_threshold_value(indio_dev, addr,
+							     raw_val);
+		case IIO_EV_DIR_FALLING:
+			addr = ADXL372_X_THRESH_INACT_H + 2 * chan->scan_index;
+			return adxl372_write_threshold_value(indio_dev, addr,
+							     raw_val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		val_ms = val * 1000 + DIV_ROUND_UP(val2, 1000);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl372_set_activity_time_ms(st, val_ms);
+		case IIO_EV_DIR_FALLING:
+			return adxl372_set_inactivity_time_ms(st, val_ms);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl372_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 adxl372_state *st = iio_priv(indio_dev);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return FIELD_GET(ADXL372_INT1_MAP_ACT_MSK, st->int1_bitmask);
+	case IIO_EV_DIR_FALLING:
+		return FIELD_GET(ADXL372_INT1_MAP_INACT_MSK, st->int1_bitmask);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl372_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      int state)
+{
+	struct adxl372_state *st = iio_priv(indio_dev);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		set_mask_bits(&st->int1_bitmask, ADXL372_INT1_MAP_ACT_MSK,
+			      ADXL372_INT1_MAP_ACT_MODE(state));
+		break;
+	case IIO_EV_DIR_FALLING:
+		set_mask_bits(&st->int1_bitmask, ADXL372_INT1_MAP_INACT_MSK,
+			      ADXL372_INT1_MAP_INACT_MODE(state));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return adxl372_set_interrupts(st, st->int1_bitmask, 0);
+}
+
 static ssize_t adxl372_peak_fifo_en_get(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -860,7 +1089,8 @@ static int adxl372_buffer_postenable(struct iio_dev *indio_dev)
 	if (ret < 0)
 		return ret;
 
-	ret = adxl372_set_interrupts(st, ADXL372_INT1_MAP_FIFO_FULL_MSK, 0);
+	st->int1_bitmask |= ADXL372_INT1_MAP_FIFO_FULL_MSK;
+	ret = adxl372_set_interrupts(st, st->int1_bitmask, 0);
 	if (ret < 0)
 		goto err;
 
@@ -902,7 +1132,8 @@ static int adxl372_buffer_postenable(struct iio_dev *indio_dev)
 	ret = adxl372_configure_fifo(st);
 	if (ret < 0) {
 		st->fifo_mode = ADXL372_FIFO_BYPASSED;
-		adxl372_set_interrupts(st, 0, 0);
+		st->int1_bitmask &= ~ADXL372_INT1_MAP_FIFO_FULL_MSK;
+		adxl372_set_interrupts(st, st->int1_bitmask, 0);
 		goto err;
 	}
 
@@ -917,7 +1148,8 @@ static int adxl372_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct adxl372_state *st = iio_priv(indio_dev);
 
-	adxl372_set_interrupts(st, 0, 0);
+	st->int1_bitmask &= ~ADXL372_INT1_MAP_FIFO_FULL_MSK;
+	adxl372_set_interrupts(st, st->int1_bitmask, 0);
 	st->fifo_mode = ADXL372_FIFO_BYPASSED;
 	adxl372_configure_fifo(st);
 
@@ -934,12 +1166,11 @@ static int adxl372_dready_trig_set_state(struct iio_trigger *trig,
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct adxl372_state *st = iio_priv(indio_dev);
-	unsigned long int mask = 0;
 
 	if (state)
-		mask = ADXL372_INT1_MAP_FIFO_FULL_MSK;
+		st->int1_bitmask |= ADXL372_INT1_MAP_FIFO_FULL_MSK;
 
-	return adxl372_set_interrupts(st, mask, 0);
+	return adxl372_set_interrupts(st, st->int1_bitmask, 0);
 }
 
 static int adxl372_validate_trigger(struct iio_dev *indio_dev,
@@ -978,6 +1209,10 @@ static const struct iio_info adxl372_info = {
 	.attrs = &adxl372_attrs_group,
 	.read_raw = adxl372_read_raw,
 	.write_raw = adxl372_write_raw,
+	.read_event_config = adxl372_read_event_config,
+	.write_event_config = adxl372_write_event_config,
+	.read_event_value = adxl372_read_event_value,
+	.write_event_value = adxl372_write_event_value,
 	.debugfs_reg_access = &adxl372_reg_access,
 	.hwfifo_set_watermark = adxl372_set_watermark,
 };
-- 
2.20.1

[PATCH v3 3/4] iio: accel: adxl372: add additional events ABIs

[Alexandru Tachici]

Adxl372 uses the standard event interface. The additional
ABIs aim to explain to the user that the values set in
./events/thresh_falling_period and ./events/thresh_rising_period
control the state of the device, not just the events timings.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/accel/adxl372.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index d8f95c9f9587..1122c27fc37e 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -239,6 +239,29 @@ static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = {
 	{ BIT(0) | BIT(1) | BIT(2), ADXL372_XYZ_FIFO },
 };
 
+static ssize_t adxl372_read_detect_event(struct iio_dev *indio_dev, uintptr_t p,
+					 const struct iio_chan_spec *chan,
+					 char *buf)
+{
+	return sprintf(buf, "%s", (const char *)p);
+}
+
+static const struct iio_chan_spec_ext_info adxl372_ext_info[] = {
+	{
+		.name = "activity_detect_event",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = adxl372_read_detect_event,
+		.private = (uintptr_t)"in_accel_thresh_x_rising\n",
+	},
+	{
+		.name = "inactivity_detect_event",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = adxl372_read_detect_event,
+		.private = (uintptr_t)"in_accel_thresh_x_falling\n",
+	},
+	{},
+};
+
 static const struct iio_event_spec adxl372_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -272,7 +295,8 @@ static const struct iio_event_spec adxl372_events[] = {
 		.shift = 4,						\
 	},								\
 	.event_spec = adxl372_events,					\
-	.num_event_specs = 2						\
+	.num_event_specs = 2,						\
+	.ext_info = adxl372_ext_info,					\
 }
 
 static const struct iio_chan_spec adxl372_channels[] = {
-- 
2.20.1

[PATCH v3 4/4] iio: accel: adxl372: Update sysfs docs

[Alexandru Tachici]

This patch adds entries in the syfs docs of ADXL372.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-accel-adxl372   | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-accel-adxl372

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-accel-adxl372 b/Documentation/ABI/testing/sysfs-bus-iio-accel-adxl372
new file mode 100644
index 000000000000..6be4f57f9520
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-accel-adxl372
@@ -0,0 +1,30 @@
+What:		/sys/bus/iio/devices/iio:deviceX/hwfifo_peak_mode_enable
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute allows to configure the FIFO to store sample
+		sets of impact event peak (x, y, z). As a precondition, all
+		three channels (x, y, z) need to be enabled.
+		Writing 1, peak fifo mode will be enabled, if cleared and
+		all three channels are enabled, sample sets of concurrent
+		3-axis data will be stored in the FIFO.
+
+What:		/sys/bus/iio/devices/iio:deviceX/activity_detect_event
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		adxl372 works in loop mode. It will loop  between activity
+		and inactivity detection mode. The thresh_rising sysfs files
+		found in events/ need to be configured in order to define when
+		the device will mark a sensed acceleration over a period of
+		time as activity. Reads "in_accel_thresh_x_rising".
+
+What:		/sys/bus/iio/devices/iio:deviceX/inactivity_detect_event
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		adxl372 works in loop mode. It will loop  between activity
+		and inactivity detection mode. The thresh_falling sysfs files
+		found in events/ need to be configured in order to define when
+		the device will mark a sensed acceleration over a period of
+		time as inactivity. Reads "in_accel_thresh_x_falling".
-- 
2.20.1

Re: [PATCH v3 1/4] iio: accel: adxl372: Add support for FIFO peak mode

[Jonathan Cameron]

On Wed, 18 Mar 2020 11:10:10 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/18/20 12:09 PM, Alexandru Tachici wrote:
> > From: Stefan Popa <stefan.popa@analog.com>
> > 
> > By default, if all three channels (x, y, z) are enabled, sample sets of
> > concurrent 3-axis data is stored in the FIFO. This patch adds the option
> > to configure the FIFO to store peak acceleration (x, y and z) of every
> > over-threshold event. When pushing to iio buffer we push only enabled
> > axis data.
> > 
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > ---
> >   drivers/iio/accel/adxl372.c | 74 ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 73 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> > index 67b8817995c0..90c37d6f10d3 100644
> > --- a/drivers/iio/accel/adxl372.c
> > +++ b/drivers/iio/accel/adxl372.c
> > @@ -133,6 +133,9 @@
> >   
> >   /* The ADXL372 includes a deep, 512 sample FIFO buffer */
> >   #define ADXL372_FIFO_SIZE			512
> > +#define ADXL372_X_AXIS_EN(x)			(((x) >> 0) & 0x1)
> > +#define ADXL372_Y_AXIS_EN(x)			(((x) >> 1) & 0x1)
> > +#define ADXL372_Z_AXIS_EN(x)			(((x) >> 2) & 0x1)
> >   
> >   /*
> >    * At +/- 200g with 12-bit resolution, scale is computed as:
> > @@ -253,6 +256,7 @@ struct adxl372_state {
> >   	struct iio_trigger		*dready_trig;
> >   	enum adxl372_fifo_mode		fifo_mode;
> >   	enum adxl372_fifo_format	fifo_format;
> > +	unsigned int			fifo_axis_mask;
> >   	enum adxl372_op_mode		op_mode;
> >   	enum adxl372_act_proc_mode	act_proc_mode;
> >   	enum adxl372_odr		odr;
> > @@ -264,6 +268,7 @@ struct adxl372_state {
> >   	u8				int2_bitmask;
> >   	u16				watermark;
> >   	__be16				fifo_buf[ADXL372_FIFO_SIZE];
> > +	bool				peak_fifo_mode_en;
> >   };
> >   
> >   static const unsigned long adxl372_channel_masks[] = {
> > @@ -522,6 +527,22 @@ static int adxl372_get_status(struct adxl372_state *st,
> >   	return ret;
> >   }
> >   
> > +static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample)
> > +{
> > +	__be16	axis_sample[3];
> > +	int i = 0;
> > +
> > +	memset(axis_sample, 0, 3 * sizeof(__be16));
> > +	if (ADXL372_X_AXIS_EN(st->fifo_axis_mask))
> > +		axis_sample[i++] = sample[0];
> > +	if (ADXL372_Y_AXIS_EN(st->fifo_axis_mask))
> > +		axis_sample[i++] = sample[1];
> > +	if (ADXL372_Z_AXIS_EN(st->fifo_axis_mask))
> > +		axis_sample[i++] = sample[2];
> > +
> > +	memcpy(sample, axis_sample, 3 * sizeof(__be16));
> > +}
> > +
> >   static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
> >   {
> >   	struct iio_poll_func *pf = p;
> > @@ -553,8 +574,12 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
> >   			goto err;
> >   
> >   		/* Each sample is 2 bytes */
> > -		for (i = 0; i < fifo_entries; i += st->fifo_set_size)
> > +		for (i = 0; i < fifo_entries; i += st->fifo_set_size) {
> > +			/* filter peak detection data */
> > +			if (st->peak_fifo_mode_en)
> > +				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
> >   			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> > +		}
> >   	}
> >   err:
> >   	iio_trigger_notify_done(indio_dev->trig);
> > @@ -722,6 +747,43 @@ static int adxl372_write_raw(struct iio_dev *indio_dev,
> >   	}
> >   }
> >   
> > +static ssize_t adxl372_peak_fifo_en_get(struct device *dev,
> > +					struct device_attribute *attr,
> > +					char *buf)
> > +{
> > +	struct adxl372_state *st = iio_priv(dev_to_iio_dev(dev));
> > +
> > +	return sprintf(buf, "%d\n", st->peak_fifo_mode_en);
> > +}
> > +
> > +static ssize_t adxl372_peak_fifo_en_set(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct adxl372_state *st = iio_priv(indio_dev);
> > +	bool val;
> > +	int ret;
> > +
> > +	ret = iio_device_claim_direct_mode(indio_dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = kstrtobool(buf, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->peak_fifo_mode_en = val;
> > +
> > +	iio_device_release_direct_mode(indio_dev);
> > +
> > +	return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(hwfifo_peak_mode_enable, 0644,
> > +		       adxl372_peak_fifo_en_get,
> > +		       adxl372_peak_fifo_en_set, 0);
> > +  
> 
> Rather than going with a non-standard attribute, I'd register a IIO 
> trigger for the peak mode and switch between peak and normal mode by 
> assigning the corresponding trigger.
> 
> At least that's how I understand how this mode works. Data capture is 
> triggered by exceeding a threshold.

I wondered about that, but my reading of the datasheet suggests this isn't
simply a trigger that runs when data is above a threshold but rather
specifically the highest value that occurs between crossing the threshold
on the way up and crossing it again on the way down.

We could still consider it a trigger though, be it a very strange one.

There is another mode where it moves out of lower power mode into
high power mode at the first crossing of a threshold.  That one I can't
see how to implement as a trigger type really as it wouldn't jump back
into low power mode without some form of user intervention.

Confusing device...

Jonathan



> 
> - Lars
>