Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Jonathan Cameron <jic23@kernel.org>]

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

> Prepare the interrupt handler. Add register entries to evaluate the
> incoming interrupt. Add functions to clear status registers and reset the
> FIFO.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

A few comments inline.

> ---
>  drivers/iio/accel/adxl313.h      |  16 ++++
>  drivers/iio/accel/adxl313_core.c | 134 +++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+)



>  struct adxl313_chip_info {
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 9db318a03eea..1e085f0c61a0 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -10,15 +10,24 @@
>  #include <linux/bitfield.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>

This is an odd selection of headers to add now. Why do we need them but didn't
before?  Some of these aren't used yet so drop them (events.h, sysfs.h I think)

> +

>  static const struct regmap_range adxl312_readable_reg_range[] = {
>  	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
>  	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -62,6 +71,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
>  	case ADXL313_REG_DATA_AXIS(4):
>  	case ADXL313_REG_DATA_AXIS(5):
>  	case ADXL313_REG_FIFO_STATUS:
> +	case ADXL313_REG_INT_SOURCE:
>  		return true;
>  	default:
>  		return false;
> @@ -363,6 +373,118 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxl313_get_samples(struct adxl313_data *data)

I doubt this gets called from multiple places. I'd just put
the code inline and no have this helper at all.

> +{
> +	unsigned int regval = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> +}
> +
> +static int adxl313_set_fifo(struct adxl313_data *data)
> +{
> +	unsigned int int_line;
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, ADXL313_REG_INT_MAP, &int_line);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, data->fifo_mode));

Check ret.

> +
> +	return adxl313_set_measure_en(data, true);
> +}
> +
> +static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
> +{
> +	size_t count;
> +	unsigned int i;
> +	int ret;
> +
> +	count = array_size(sizeof(data->fifo_buf[0]), ADXL313_NUM_AXIS);
> +	for (i = 0; i < samples; i++) {
> +		ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
> +				       data->fifo_buf + (i * count / 2), count);

that 2 is I'd guessed based on size of some data store element?  
I'd guess sizeof(data->fifo_buf[0]) is appropriate.


> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> + * @data: The device data.
> + *
> + * Reset the FIFO status registers. Reading out status registers clears the

I think you already read it before calling this. So how is it ever set?

> + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> + * Ignore particular read register content. Register content is not processed
> + * any further. Therefore the function returns void.
> + */
> +static void adxl313_fifo_reset(struct adxl313_data *data)

As below.  This isn't a reset.  Fifo reset is normally the term used
for when we have lost tracking of what is in the fifo and drop all data,
not normal readback.

> +{
> +	unsigned int regval;
> +	int samples;
> +
> +	adxl313_set_measure_en(data, false);
Disabling measurement to read a fifo is unusual -  is this really necessary
as it presumably puts a gap in the data, which is what we are trying
to avoid by using a fifo.

> +
> +	samples = adxl313_get_samples(data);
> +	if (samples > 0)
> +		adxl313_fifo_transfer(data, samples);
> +
> +	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);

Not processing the convents of INT_SOURCE every time you read it
introduces race conditions.  This logic needs a rethink so that
never happens.  I guess this is why you are disabling measurement
to stop the status changing?  Just whatever each read of INT_SOURCE
tells us we need to handle and all should be fine without disabling
measurement.  That read should only clear bits that are set, so no
race conditions.

> +
> +	adxl313_set_measure_en(data, true);
> +}
> +
> +static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +
> +	data->fifo_mode = ADXL313_FIFO_STREAM;

If you always set fifo_mode before calling _set_fifo() probably better
to pass the value in as a separate parameter and store it as necessary
inside that function.

> +	return adxl313_set_fifo(data);
> +}
> +
> +static int adxl313_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	data->fifo_mode = ADXL313_FIFO_BYPASS;
> +	ret = adxl313_set_fifo(data);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->regmap, ADXL313_REG_INT_ENABLE, 0);
> +}
> +
> +static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
> +	.postenable = adxl313_buffer_postenable,
> +	.predisable = adxl313_buffer_predisable,
> +};
> +
> +static irqreturn_t adxl313_irq_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int int_stat;
> +
> +	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))

Failure to read is one thing we should handle, but also we should handle
int_stat telling us there were no interrupts set for this device.

> +		return IRQ_NONE;
> +
> +	adxl313_fifo_reset(data);

Given we don't know it had anything to do with the fifo at this point
resetting the fifo doesn't make much sense.  I'd expect a check
on int_status, probably for overrun, before doing this.

Ah. On closer inspection this isn't resetting the fifo, it's just
reading it.  Rename that function and make it dependent on what
was in int_stat.


> +
> +	return IRQ_HANDLED;
> +}