Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 1 Aug 2018 17:31:11 +0200 (CEST)
From:   Peter Meerwald-Stadler <pmeerw@pmeerw.net>
To:     Stefan Popa <stefan.popa@analog.com>
cc:     jic23@kernel.org, lars@metafoo.de, Michael.Hennerich@analog.com,
        knaack.h@gmx.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/6] iio:adxl372: Add FIFO and interrupts support
In-Reply-To: <1533136692-31935-1-git-send-email-stefan.popa@analog.com>
Message-ID: <alpine.DEB.2.21.1808011721040.22146@vps.pmeerw.net>
References: <1533136692-31935-1-git-send-email-stefan.popa@analog.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> 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@analog.com>
> ---
> 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