Re: [PATCH v2 14/22] iio: accel: adxl345: read FIFO entries

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 17 Nov 2024 18:26:43 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a function to read the elements of the adxl345 FIFO. This will
> flush the FIFO, and brings it into a ready state. The read out
> is used to read the elemnts and to reset the fifo again. The cleanup
> equally needs a read on the INT_SOURCE register.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 40 ++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 508de81bb9..40e78dbdb0 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -140,6 +140,8 @@ struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
> +
> +	__le16 fifo_buf[3 * ADXL34x_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
That doesn't work.  Look at why we have IIO_DMA_MINALIGN().
Hint is that the buffer needs to end up in a cacheline with nothing that might
be written by software in the same line.  int_map and below will be so this
is not doing it's job.

Also not used in this patch.

>  	u8 int_map;
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
> @@ -323,6 +325,37 @@ static const struct attribute_group adxl345_attrs_group = {
>  	.attrs = adxl345_attrs,
>  };
>  
> +/**
> + * adxl345_get_fifo_entries() - Read number of FIFO entries into *fifo_entries.
> + * @st: The initialized state instance of this driver.
> + * @fifo_entries: A field to be initialized by this function with the number of
> + * FIFO entries.
> + *
> + * Since one read on the FIFO is reading all three axis, X, Y and Z in one, the
> + * number of FIFO entries corresponds to the number of triples of those values.
> + * Note, this sensor does not support treating any axis individually, or
> + * exclude them from measuring.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
> +{
> +	unsigned int regval = 0;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
> +	if (ret) {
> +		pr_warn("%s(): Failed to read FIFO_STATUS register\n", __func__);
As before, seems overkill to warn int his path but not really anywhere else.
dev_err() if you do want to.
> +		*fifo_entries = 0;
> +		return ret;
> +	}
> +
> +	*fifo_entries = 0x3f & regval;
> +	pr_debug("%s(): fifo_entries %d\n", __func__, *fifo_entries);
> +
> +	return 0;
return the value (similar comment to previous patch review.)
> +}
> +
>  static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  };
>  
> @@ -363,6 +396,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = ((struct iio_poll_func *) p)->indio_dev;
>  	struct adxl34x_state *st = iio_priv(indio_dev);
>  	u8 int_stat;
> +	int fifo_entries;
>  	int ret;
>  
>  	ret = adxl345_get_status(st, &int_stat);
> @@ -380,9 +414,11 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  		goto done;
>  	}
>  
> -	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK))
> +	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
>  		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
> -
> +		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
Do it all in one go. I want to see interrupt through to data hitting the kfifo +
registration of that kfifo all in one patch.

It is not complicated enough to need to jump through this very slow introduction
of code.

Jonathan

> +			goto done;
> +	}
>  	goto done;
>  done:
>