Re: [PATCH v2 4/6] iio:adxl372: Add FIFO and interrupts support

From: Peter Meerwald-Stadler
Date: Wed Aug 01 2018 - 11:31:16 EST



> This patch adds support for the adxl372 FIFO. In order to accomplish this,
> triggered buffers were used.
>
> The number of FIFO samples which trigger the watermark interrupt can be
> configured by using the buffer watermark. The FIFO format is determined by
> configuring the scan elements for each axis. Moreover, the FIFO can also
> be configured in the XYZ peak mode.
>
> The FIFO data is pushed to the IIO device's buffer.

some minor comments below

>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> ---
> Changes in v2:
> - removed DT bindings changes from this patch.
> - removed the linux/gpio/consumer.h header.
> - used regmap_pipe_read() instead of regmap_bulk_read() when
> reading data from the FIFO.
> - used multiple regmap_write() calls instead of a single
> regmap_bulk_write() if there is no fast path.
> - used be32_to_cpu() inside the adxl372_get_status() function.
> - added a new in_accel_x&y&z_peak scan element which allows the
> user to set the FIFO into XYZ peak mode.
> - used a adxl372_axis_lookup_table() to determine the fifo format
> from the active_scan_mask.
> - made IRQ optional.
>
> drivers/iio/accel/adxl372.c | 362 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 361 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index 7d5092d..68862bf 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -6,12 +6,19 @@
> */
>
> #include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/regmap.h>
> #include <linux/spi/spi.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> /* ADXL372 registers definition */
> #define ADXL372_DEVID 0x00
> @@ -123,6 +130,8 @@
> #define ADXL372_INT1_MAP_LOW_MSK BIT(7)
> #define ADXL372_INT1_MAP_LOW_MODE(x) (((x) & 0x1) << 7)
>
> +#define ADXL372_FIFO_SIZE 512

is this bytes or entries?
seems to be entries

> +
> /*
> * At +/- 200g with 12-bit resolution, scale is computed as:
> * (200 + 200) * 9.81 / (2^12 - 1) = 0.958241
> @@ -164,6 +173,44 @@ enum adxl372_bandwidth {
> ADXL372_BW_3200HZ,
> };
>
> +enum adxl372_fifo_format {
> + ADXL372_XYZ_FIFO,
> + ADXL372_X_FIFO,
> + ADXL372_Y_FIFO,
> + ADXL372_XY_FIFO,
> + ADXL372_Z_FIFO,
> + ADXL372_XZ_FIFO,
> + ADXL372_YZ_FIFO,
> + ADXL372_XYZ_PEAK_FIFO,
> +};
> +
> +enum adxl372_fifo_mode {
> + ADXL372_FIFO_BYPASSED,
> + ADXL372_FIFO_STREAMED,
> + ADXL372_FIFO_TRIGGERED,
> + ADXL372_FIFO_OLD_SAVED
> +};
> +
> +static const int adxl372_samp_freq_tbl[5] = {
> + 400, 800, 1600, 3200, 6400,
> +};
> +
> +struct adxl372_axis_lookup {
> + unsigned int bits;
> + enum adxl372_fifo_format fifo_format;
> +};
> +
> +static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = {
> + { BIT(0), ADXL372_X_FIFO },
> + { BIT(1), ADXL372_Y_FIFO },
> + { BIT(2), ADXL372_Z_FIFO },
> + { BIT(0) | BIT(1), ADXL372_XY_FIFO },
> + { BIT(0) | BIT(2), ADXL372_XZ_FIFO },
> + { BIT(1) | BIT(2), ADXL372_YZ_FIFO },
> + { BIT(0) | BIT(1) | BIT(2), ADXL372_XYZ_FIFO },
> + { BIT(3), ADXL372_XYZ_PEAK_FIFO }

maybe end with ,

> +};
> +
> #define ADXL372_ACCEL_CHANNEL(index, reg, axis) { \
> .type = IIO_ACCEL, \
> .address = reg, \
> @@ -171,25 +218,69 @@ enum adxl372_bandwidth {
> .channel2 = IIO_MOD_##axis, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .shift = 4, \
> + }, \
> }
>
> static const struct iio_chan_spec adxl372_channels[] = {
> ADXL372_ACCEL_CHANNEL(0, ADXL372_X_DATA_H, X),
> ADXL372_ACCEL_CHANNEL(1, ADXL372_Y_DATA_H, Y),
> ADXL372_ACCEL_CHANNEL(2, ADXL372_Z_DATA_H, Z),
> + {
> + .type = IIO_ACCEL,
> + .modified = 1,
> + .channel2 = IIO_MOD_X_AND_Y_AND_Z,
> + .extend_name = "peak",
> + .scan_index = 3,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 12,
> + .storagebits = 16,
> + .shift = 4,
> + }
> + }

end with ,

> };
>
> struct adxl372_state {
> struct spi_device *spi;
> struct regmap *regmap;
> + struct iio_trigger *dready_trig;
> + enum adxl372_fifo_mode fifo_mode;
> + enum adxl372_fifo_format fifo_format;
> enum adxl372_op_mode op_mode;
> enum adxl372_act_proc_mode act_proc_mode;
> enum adxl372_odr odr;
> enum adxl372_bandwidth bw;
> u32 act_time_ms;
> u32 inact_time_ms;
> + u8 fifo_set_size;
> + u8 int1_bitmask;
> + u8 int2_bitmask;
> + u16 watermark;
> + __be16 fifo_buf[512];

maybe use ADXL372_FIFO_SIZE here?

> +};
> +
> +static const unsigned long adxl372_channel_masks[] = {
> + BIT(0), BIT(1), BIT(2),
> + BIT(0) | BIT(1),
> + BIT(0) | BIT(2),
> + BIT(1) | BIT(2),
> + BIT(0) | BIT(1) | BIT(2),
> + BIT(3),
> + 0
> };
>
> +static int adxl372_read_fifo(struct adxl372_state *st, u16 fifo_entries)
> +{
> + return regmap_pipe_read(st->regmap, ADXL372_FIFO_DATA,
> + st->fifo_buf, fifo_entries * 2);

sizeof(be16) instead of 2?

> +}
> +
> static int adxl372_read_axis(struct adxl372_state *st, u8 addr)
> {
> __be16 regval;
> @@ -364,6 +455,107 @@ static int adxl372_set_inactivity_time_ms(struct adxl372_state *st,
> return ret;
> }
>
> +static int adxl372_set_interrupts(struct adxl372_state *st,
> + unsigned char int1_bitmask,
> + unsigned char int2_bitmask)
> +{
> + int ret;
> +
> + ret = regmap_write(st->regmap, ADXL372_INT1_MAP, int1_bitmask);
> + if (ret < 0)
> + return ret;
> +
> + return regmap_write(st->regmap, ADXL372_INT2_MAP, int2_bitmask);
> +}
> +
> +static int adxl372_configure_fifo(struct adxl372_state *st)
> +{
> + unsigned int fifo_samples, fifo_ctl;
> + int ret;
> +
> + /* FIFO must be configured while in standby mode */
> + ret = adxl372_set_op_mode(st, ADXL372_STANDBY);
> + if (ret < 0)
> + return ret;
> +
> + fifo_samples = st->watermark & 0xFF;
> + fifo_ctl = ADXL372_FIFO_CTL_FORMAT_MODE(st->fifo_format) |
> + ADXL372_FIFO_CTL_MODE_MODE(st->fifo_mode) |
> + ADXL372_FIFO_CTL_SAMPLES_MODE(st->watermark);
> +
> + ret = regmap_write(st->regmap, ADXL372_FIFO_SAMPLES, fifo_samples);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write(st->regmap, ADXL372_FIFO_CTL, fifo_ctl);
> + if (ret < 0)
> + return ret;
> +
> + return adxl372_set_op_mode(st, ADXL372_FULL_BW_MEASUREMENT);
> +}
> +
> +static int adxl372_get_status(struct adxl372_state *st,
> + u8 *status1, u8 *status2,
> + u16 *fifo_entries)
> +{
> + __be32 buf;

the buf may be a packed struct probably?

> + u32 val;
> + int ret;
> +
> + /* STATUS, STATUS2, FIFO_ENTRIES2 and FIFO_ENTRIES are adjacent regs */

STATUS1 maybe?

> + ret = regmap_bulk_read(st->regmap, ADXL372_STATUS_1,
> + &buf, sizeof(buf));
> + if (ret < 0)
> + return ret;
> +
> + val = be32_to_cpu(buf);
> +
> + *status1 = val >> 24;
> + *status2 = val >> 16;

mask missing?

> + /*
> + * FIFO_ENTRIES contains the least significant byte, and FIFO_ENTRIES2
> + * contains the two most significant bits
> + */
> + *fifo_entries = val & 0x3FF;
> +
> + return ret;
> +}
> +
> +static irqreturn_t adxl372_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adxl372_state *st = iio_priv(indio_dev);
> + u8 status1, status2;
> + u16 fifo_entries, i;

i should be just unsigned, not u16

> + int ret;
> +
> + ret = adxl372_get_status(st, &status1, &status2, &fifo_entries);
> + if (ret < 0)
> + goto err;
> +
> + if ((st->fifo_mode != ADXL372_FIFO_BYPASSED) &&
> + (ADXL372_STATUS_1_FIFO_FULL(status1))) {

parenthesis not needed

> + /*
> + * When reading data from multiple axes from the FIFO,
> + * to ensure that data is not overwritten and stored out
> + * of order at least one sample set must be left in the
> + * FIFO after every read.
> + */
> + fifo_entries -= st->fifo_set_size;
> +
> + ret = adxl372_read_fifo(st, fifo_entries);
> + if (ret < 0)
> + goto err;
> +
> + for (i = 0; i < fifo_entries * 2; i += st->fifo_set_size * 2)

this confuses me; 2 is what? use sizeof()

> + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> + }
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> static int adxl372_setup(struct adxl372_state *st)
> {
> unsigned int regval;
> @@ -443,7 +635,12 @@ static int adxl372_read_raw(struct iio_dev *indio_dev,
>
> switch (info) {
> case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> ret = adxl372_read_axis(st, chan->address);
> + iio_device_release_direct_mode(indio_dev);
> if (ret < 0)
> return ret;
>
> @@ -459,9 +656,136 @@ static int adxl372_read_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static ssize_t adxl372_get_fifo_enabled(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct adxl372_state *st = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%d\n", st->fifo_mode);
> +}
> +
> +static ssize_t adxl372_get_fifo_watermark(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct adxl372_state *st = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%d\n", st->watermark);
> +}
> +
> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
> +static IIO_CONST_ATTR(hwfifo_watermark_max,
> + __stringify(ADXL372_FIFO_SIZE));
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> + adxl372_get_fifo_watermark, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> + adxl372_get_fifo_enabled, NULL, 0);
> +
> +static const struct attribute *adxl372_fifo_attributes[] = {
> + &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> + &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> + NULL,
> +};
> +
> +static int adxl372_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct adxl372_state *st = iio_priv(indio_dev);
> +
> + if (val > ADXL372_FIFO_SIZE)
> + val = ADXL372_FIFO_SIZE;
> +
> + st->watermark = val;
> +
> + return 0;
> +}
> +
> +static int adxl372_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct adxl372_state *st = iio_priv(indio_dev);
> + unsigned int mask;
> + int i, ret;
> +
> + ret = adxl372_set_interrupts(st, ADXL372_INT1_MAP_FIFO_FULL_MSK, 0);
> + if (ret < 0)
> + return ret;
> +
> + mask = *indio_dev->active_scan_mask;
> +
> + for (i = 0; i < ARRAY_SIZE(adxl372_axis_lookup_table); i++) {
> + if (mask == adxl372_axis_lookup_table[i].bits)
> + break;
> + }
> +
> + st->fifo_format = adxl372_axis_lookup_table[i].fifo_format;
> +
> + if (st->fifo_format == ADXL372_XYZ_PEAK_FIFO)
> + st->fifo_set_size = 3;
> + else
> + st->fifo_set_size = bitmap_weight(indio_dev->active_scan_mask,
> + indio_dev->masklength);
> + /*
> + * 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
> + */
> + if ((st->watermark * st->fifo_set_size) > ADXL372_FIFO_SIZE)
> + st->watermark = (ADXL372_FIFO_SIZE / st->fifo_set_size);
> +
> + st->fifo_mode = ADXL372_FIFO_STREAMED;
> +
> + ret = adxl372_configure_fifo(st);
> + if (ret < 0) {
> + st->fifo_mode = ADXL372_FIFO_BYPASSED;
> + adxl372_set_interrupts(st, 0, 0);
> + return ret;
> + }
> +
> + return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +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->fifo_mode = ADXL372_FIFO_BYPASSED;
> + adxl372_configure_fifo(st);
> +
> + return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static const struct iio_buffer_setup_ops adxl372_buffer_ops = {
> + .postenable = adxl372_buffer_postenable,
> + .predisable = adxl372_buffer_predisable,
> +};
> +
> +static int adxl372_dready_trig_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + 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;
> +
> + return adxl372_set_interrupts(st, mask, 0);
> +}
> +
> +static const struct iio_trigger_ops adxl372_trigger_ops = {
> + .set_trigger_state = adxl372_dready_trig_set_state,
> +};
> +
> static const struct iio_info adxl372_info = {
> .read_raw = adxl372_read_raw,
> .debugfs_reg_access = &adxl372_reg_access,
> + .hwfifo_set_watermark = adxl372_set_watermark,
> };
>
> static const struct regmap_config adxl372_spi_regmap_config = {
> @@ -495,10 +819,11 @@ static int adxl372_probe(struct spi_device *spi)
>
> indio_dev->channels = adxl372_channels;
> indio_dev->num_channels = ARRAY_SIZE(adxl372_channels);
> + indio_dev->available_scan_masks = adxl372_channel_masks;
> indio_dev->dev.parent = &spi->dev;
> indio_dev->name = spi_get_device_id(spi)->name;
> indio_dev->info = &adxl372_info;
> - indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>
> ret = adxl372_setup(st);
> if (ret < 0) {
> @@ -506,6 +831,41 @@ static int adxl372_probe(struct spi_device *spi)
> return ret;
> }
>
> + ret = devm_iio_triggered_buffer_setup(&st->spi->dev,
> + indio_dev, NULL,
> + adxl372_trigger_handler,
> + &adxl372_buffer_ops);
> + if (ret < 0)
> + return ret;
> +
> + iio_buffer_set_attrs(indio_dev->buffer, adxl372_fifo_attributes);
> +
> + if (st->spi->irq) {
> + st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + indio_dev->id);
> + if (st->dready_trig == NULL)
> + return -ENOMEM;
> +
> + st->dready_trig->ops = &adxl372_trigger_ops;
> + st->dready_trig->dev.parent = &st->spi->dev;
> + iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> + ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
> + if (ret < 0)
> + return ret;
> +
> + indio_dev->trig = iio_trigger_get(st->dready_trig);
> +
> + ret = devm_request_threaded_irq(&st->spi->dev, st->spi->irq,
> + iio_trigger_generic_data_rdy_poll,
> + NULL,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + indio_dev->name, st->dready_trig);
> + if (ret < 0)
> + return ret;
> + }
> +
> return devm_iio_device_register(&st->spi->dev, indio_dev);
> }
>
>

--

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418