Re: [PATCH RFC v2 8/8] iio: adc: ad7944: add support for SPI offload

[David Lechner <dlechner@baylibre.com>]

On Sun, May 12, 2024 at 6:52 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 11 May 2024 13:41:09 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On Sat, May 11, 2024 at 11:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Fri, 10 May 2024 19:44:31 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> > > > This adds support for SPI offload to the ad7944 driver. This allows
> > > > reading data at the max sample rate of 2.5 MSPS.
> > > >
> > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > > ---
> > > >
> > > > v2 changes:
> > > >
> > > > In the previous version, there was a new separate driver for the PWM
> > > > trigger and DMA hardware buffer. This was deemed too complex so they
> > > > are moved into the ad7944 driver.
> > > >
> > > > It has also been reworked to accommodate for the changes described in
> > > > the other patches.
> > > >
> > > > RFC: This isn't very polished yet, just FYI. A few things to sort out:
> > > >
> > > > Rather than making the buffer either triggered buffer or hardware buffer,
> > > > I'm considering allowing both, e.g. buffer0 will always be the triggered
> > > > buffer and buffer1 will will be the hardware buffer if connected to a SPI
> > > > controller with offload support, otherwise buffer1 is absent. But since
> > > > multiple buffers haven't been used much so far, more investigation is
> > > > needed to see how that would work in practice. If we do that though, then
> > > > we would always have the sampling_frequency attribute though even though
> > > > it only applies to one buffer.
> > >
> > > Why would someone who has this nice IP in the path want the conventional
> > > triggered buffer?  I'm not against the two buffer option, but I'd like to know
> > > the reasoning not to just provide the hardware buffer if this SPI offload
> > > is available.
> > >
> > > I can conjecture reasons but would like you to write them out for me :)
> > > This feels like if someone has paid for the expensive hardware they probably
> > > only want the best performance.
> > >
> >
> > For me, it was more of a question of if we need to keep the userspace
> > interface consistent between both with or without offload support. But
> > if you are happy with it this way where we have only one or the other,
> > it is less work for me. :-)
>
> So inconsistency in userspace interfaces can occur for many reasons like
> whether the interrupt is wired or not, but in this particularly
> case I guess we have ABI stability issue because there are boards out there
> today and people using the driver without this offload functionality.

FWIW, the ad7944 driver will be landing in 6.10, so no users to speak of yet.

> I'd not really thought that bit through, so I think you are correct that
> we need to maintain the triggered buffer interface and 'add' the new
> ABI for the offloaded case.  The multibuffer approach should work for this.
> Will be interesting if any problem surface from having two very different
> types of buffer on the same device.
>

In this particular case, I think the issues will be:
1. The hardware buffer can't allow the soft timestamp. Otherwise, the
buffer layout is the same (on other chips, we won't always be so
lucky).
2. The hardware trigger (sampling_frequency attribute) will only apply
if there is the SPI offload support.