Re: [PATCH v9 3/4] iio: accel: adxl345: add FIFO with watermark events

[Andy Shevchenko <andy.shevchenko@gmail.com>]

Sat, Dec 28, 2024 at 11:29:48PM +0000, Lothar Rubusch kirjoitti:
> Add a basic setup for FIFO with configurable watermark. Add a handler
> for watermark interrupt events and extend the channel for the
> scan_index needed for the iio channel. The sensor is configurable to use
> a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> a watermark can be configured, or disabled by setting 0. Further features
> require a working FIFO setup.

...

>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>

Why not keep it ordered?

...

> +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	unsigned int fifo_mask = 0x1F;

GENMASK() ?
(BIT(5) - 1) ? 

> +	int ret;
> +
> +	value = min(value, ADXL345_FIFO_SIZE - 1);
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, fifo_mask, value);
> +	if (ret)
> +		return ret;
> +
> +	st->watermark = value;
> +	st->int_map |= ADXL345_INT_WATERMARK;
> +
> +	return 0;
> +}

...

> +	int i, ret = 0;

Why is i signed?

> +
> +	/* count is the 3x the fifo_buf element size, hence 6B */
> +	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> +	for (i = 0; i < samples; i++) {
> +		/* read 3x 2 byte elements from base address into next fifo_buf position */
> +		ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> +				       st->fifo_buf + (i * count / 2), count);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * To ensure that the FIFO has completely popped, there must be at least 5
> +		 * us between the end of reading the data registers, signified by the
> +		 * transition to register 0x38 from 0x37 or the CS pin going high, and the
> +		 * start of new reads of the FIFO or reading the FIFO_STATUS register. For
> +		 * SPI operation at 1.5 MHz or lower, the register addressing portion of the
> +		 * transmission is sufficient delay to ensure the FIFO has completely
> +		 * popped. It is necessary for SPI operation greater than 1.5 MHz to
> +		 * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us
> +		 * at 5 MHz operation.
> +		 */
> +		if (st->fifo_delay && samples > 1)
> +			udelay(3);
> +	}

MIssed blank line.

> +	return ret;

...

> +static int adxl345_fifo_push(struct iio_dev *indio_dev,
> +			     int samples)

This can be effectively a single line.

> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int i, ret;

> +	if (samples <= 0)
> +		return -EINVAL;

This is strange. Why the heck somebody could translate the input parameter to
an error? Can't the function simply take the positive input only? Otherwise,
what is the meaning of the samples < 0 ?

> +	ret = adxl345_fifo_transfer(st, samples);
> +	if (ret)
> +		return ret;

> +	for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS)

Why doing mulpiplication here and step != 1 there instead of just one
multiplication...

> +		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);

...in the above line?

> +	return 0;
> +}

...

> +/**
> + * adxl345_irq_handler() - Handle irqs of the ADXL345.
> + * @irq: The irq being handled.
> + * @p: The struct iio_device pointer for the device.
> + *
> + * Return: The interrupt was handled.
> + */
> +static irqreturn_t adxl345_irq_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int int_stat;
> +	int samples;
> +
> +	int_stat = adxl345_get_status(st);
> +	if (int_stat <= 0)
> +		return IRQ_NONE;
> +
> +	if (int_stat & ADXL345_INT_OVERRUN)
> +		goto err;
> +
> +	if (int_stat & ADXL345_INT_WATERMARK) {
> +		samples = adxl345_get_samples(st);
> +		if (samples < 0)
> +			goto err;

You already seem to guarantee no negative samples, and TBH this has to be

		ret = ...
		...

		samples = ret;

which will make more sense and better types.


> +		if (adxl345_fifo_push(indio_dev, samples) < 0)
> +			goto err;
> +	}
> +	return IRQ_HANDLED;
> +
> +err:
> +	adxl345_fifo_reset(st);
> +
> +	return IRQ_HANDLED;
> +}

...

> +	if (st->intio != ADXL345_INT_NONE) {

What's wrong with positive conditional?
Negative is slightly harder to read and parse at a glance.

> +		/* FIFO_STREAM mode is going to be activated later */
> +		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, st->irq, NULL,
> +						&adxl345_irq_handler,
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* FIFO_BYPASS mode */
> +		fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> +		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> +		if (ret < 0)
> +			return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko