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

[Jonathan Cameron]

On Sat, 12 Oct 2024 19:43:19 -0700
Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:

> Jonathan Cameron <jic23@xxxxxxxxxx> writes:
> 
> > On Fri, 11 Oct 2024 08:37:48 -0700
> > Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:
> >  
> >> Set up a triggered buffer for the accel and angl_vel values.
> >> 
> >> Signed-off-by: Justin Weiss <justin@xxxxxxxxxxxxxxx>  
> > 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.  
> 
> That's convenient :-)
> 
> It should make this much simpler. To clarify, I'll use regmap_bulk_read
> to read all of the registers at once into a stack-allocated buffer, and
> then push that buffer. Then I can remove bmi270_device->buf entirely,
> and avoid the DMA problem that way.

Given this supports SPI. The target buffer can't be on the stack.
You still need the __aligned(IIO_DMA_MINALIGN) element in your iio_priv()
structure.

> 
> Then I'll provide one avail_mask that sets all of the
> BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.
Otherwise your description is what I'd expect to see.

> 
> > 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?
This would only be correct for the case where you aren't DMAing directly into it.
I guess I confused the message above with this point!

Jonathan

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