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;
> >> +}
