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

[Justin Weiss <justin@justinweiss.com>]

Jonathan Cameron <jic23@kernel.org> writes:

> On Sat, 12 Oct 2024 19:43:19 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Jonathan Cameron <jic23@kernel.org> writes:
>> 
>> > 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.
>> >  
>> >> ...
>> >> 
>> >> +	__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.
>

Got it. I see that the BMI323 driver does the regmap_read into the
DMA-aligned buffer, and then copies it to the timestamp-aligned buffer,
which it then sends to iio_push_to_buffers_with_timestamp. Is that a
good way to handle this?

I think the timestamp-aligned buffer could still be stack-allocated in
that case. Or maybe a second buffer isn't even necessary, if
DMA_MINALIGN is at least the correct alignment for
iio_push_to_buffers_with_timestamp and I could pass the DMA-aligned
buffer in.

Justin

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