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.
Then I'll provide one avail_mask that sets all of the
BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.
> 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;
>> +}
