Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 11 Oct 2024 08:37:48 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Set up a triggered buffer for the accel and angl_vel values.
> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
Hi Justin

A few suggestions inline. Other than the DMA safe buffer thing, looks good
but you might want to consider using a single bulk read.

My cynical view is that if someone paid for an IMU they probably want all
the channels, so optimizing for that case is a good plan.

> ---
>  drivers/iio/imu/bmi270/Kconfig       |  1 +
>  drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
>  drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
> index 0ffd29794fda..6362acc706da 100644
> --- a/drivers/iio/imu/bmi270/Kconfig
> +++ b/drivers/iio/imu/bmi270/Kconfig
> @@ -6,6 +6,7 @@
>  config BMI270
>  	tristate
>  	select IIO_BUFFER

Hmm. The IIO_BUFFER select shouldn't have been here as no obvious use
in the driver. Ah well - this patch 'fixes' that :)

> +	select IIO_TRIGGERED_BUFFER
>  
>  config BMI270_I2C
>  	tristate "Bosch BMI270 I2C driver"
> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
> index 51e374fd4290..335400c34b0d 100644
> --- a/drivers/iio/imu/bmi270/bmi270.h
> +++ b/drivers/iio/imu/bmi270/bmi270.h
> @@ -11,6 +11,14 @@ struct bmi270_data {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	const struct bmi270_chip_info *chip_info;
> +
> +	/*
> +	 * Ensure natural alignment for timestamp if present.
> +	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
> +	 * If fewer channels are enabled, less space may be needed, as
> +	 * long as the timestamp is still aligned to 8 bytes.
> +	 */
> +	__le16 buf[12] __aligned(8);
>  };
>  
>  enum bmi270_device_type {
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index e5ee80c12166..f49db5d1bffd 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -7,6 +7,8 @@
>  #include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #include "bmi270.h"
>  
> @@ -66,6 +68,7 @@ enum bmi270_scan {
>  	BMI270_SCAN_GYRO_X,
>  	BMI270_SCAN_GYRO_Y,
>  	BMI270_SCAN_GYRO_Z,
> +	BMI270_SCAN_TIMESTAMP,
>  };
>  
>  const struct bmi270_chip_info bmi270_chip_info[] = {
> @@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>  };
>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>  
> +static irqreturn_t bmi270_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
> +	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
> +	__le16 sample;
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		ret = regmap_bulk_read(bmi270_device->regmap,
> +				       base + i * sizeof(sample),
> +				       &sample, sizeof(sample));

This is always a fun corner.
regmap doesn't guarantee to bounce buffer the data used by the underlying
transport. In the case of SPI that means we need a DMA safe buffer for bulk
accesses.  In practice it may well bounce the data today but there are optmizations
that would make it zero copy that might get applied in future.

Easiest way to do that is put your sample variable in the iio_priv structure
at the end and mark it __aligned(IIO_DMA_MINALIGN)

Given you are reading a bunch of contiguous registers here it may well make
sense to do a single bulk read directly into buf and then use
the available_scan_masks to let the IIO core know it always gets a full set
of samples. Then if the user selects a subset the IIO core will reorganize
the data that they get presented with.

Whether that makes sense from a performance point of view depends on
the speed of the spi transfers vs the cost of setting up the individual ones.

You could optimize contiguous reads in here, but probably not worth that
complexity.


> +		if (ret)
> +			goto done;
> +		bmi270_device->buf[j++] = sample;

It's not a huge buffer and you aren't DMAing into it, so maybe just put this
on the stack?

> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}