Jonathan Cameron <jic23@xxxxxxxxxx> writes:
> 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.
>> >
>> >> ...
>> >>
>> >> + __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;
>> >> +}
