Re: [PATCH v3 08/12] iio: accel: adxl313: add FIFO watermark

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 May 2025 22:35:19 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add FIFO watermark configuration and evaluation. Let a watermark to be
> configured. Evaluate the interrupt accordingly. Read out the FIFO content
> and push the values to the IIO channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

I'd rethink the patch split a little and combine this with the initial interrupt
support.  That will avoid confusion over when the reset fires.

> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 1e085f0c61a0..80991cd9bd79 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -373,6 +373,25 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,

> +
> +static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
I'd avoid the term 'event' for this as your only use so far is something
we'd not consider an 'event' in IIO terms.

I'd be tempted to just keep this as code directly in the _irq_handler()
but maybe it will become less manageable in later patches.

> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int samples;
> +
> +	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);
> +	}
> +
> +	/* Return error if no event data was pushed to the IIO channel. */
> +	return -ENOENT;
> +}
> +
>  static irqreturn_t adxl313_irq_handler(int irq, void *p)
>  {
>  	struct iio_dev *indio_dev = p;
> @@ -480,6 +533,15 @@ static irqreturn_t adxl313_irq_handler(int irq, void *p)
>  	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
>  		return IRQ_NONE;
>  
> +	if (adxl313_push_event(indio_dev, int_stat))
> +		goto err;
> +
> +	if (FIELD_GET(ADXL313_INT_OVERRUN, int_stat))
> +		goto err;
> +
> +	return IRQ_HANDLED;
> +
> +err:
>  	adxl313_fifo_reset(data);

Ah. This is all making a little more sense.  I think partly the issue
here is this got split up into a few too many patches.  FIFO plus
initial interrupt support in one would have been better perhaps.

Jonathan