Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> On Fri, 01 Dec 2023 10:08:27 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > 
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > Convert the driver to use the new IIO backend framework. The device
> > > > functionality is expected to be the same (meaning no added or removed
> > > > features).  
> > > 
> > > Missing a devicetree bindings patch before this one?
> > >   
> > > > 
> > > > Also note this patch effectively breaks ABI and that's needed so we can
> > > > properly support this device and add needed features making use of the
> > > > new IIO framework.  
> > > 
> > > Can you be more specific about what is actually breaking?
> > >   
> > > > 
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/iio/adc/Kconfig  |   2 +-
> > > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++----------------
> > > > --
> > > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > index 1e2b7a2c67c6..af56df63beff 100644
> > > > --- a/drivers/iio/adc/Kconfig
> > > > +++ b/drivers/iio/adc/Kconfig
> > > > @@ -275,7 +275,7 @@ config AD799X
> > > >  config AD9467
> > > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > > >         depends on SPI
> > > > -       depends on ADI_AXI_ADC
> > > > +       select IIO_BACKEND
> > > >         help
> > > >           Say yes here to build support for Analog Devices:
> > > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > index 5db5690ccee8..8b0402e73ace 100644
> > > > --- a/drivers/iio/adc/ad9467.c
> > > > +++ b/drivers/iio/adc/ad9467.c  
> > > 
> > > <snip>
> > >   
> > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)  
> > > 
> > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > >   
> > 
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > wonder if it actually makes sense for a device like with no buffering support?!
> >  
> > > > +{
> > > > +       struct device *dev = indio_dev->dev.parent;
> > > > +       const char *dma_name;
> > > > +
> > > > +       if (!device_property_present(dev, "dmas"))
> > > > +               return 0;
> > > > +
> > > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > > +               dma_name = "rx";
> > > > +
> > > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);  
> > > 
> > > The device tree bindings for "adi,ad9467" don't include dma properties
> > > (nor should they). Perhaps the DMA lookup should be a callback to the
> > > backend? Or something similar to the SPI Engine offload that we are
> > > working on?
> > >   
> > 
> > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > thoughts
> > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > on
> > the backend device because that's where the connection is in HW. However, since
> > we
> > want to have the IIO interface in the frontend, it would be hard to do that
> > without
> > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > from
> > wise to have the DMA buffer associated to a completely different device than the
> > IIO
> > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > and
> > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > buffer
> > from the frontend device. If Jonathan is fine with this, I'm on board for it....
> 
> It is going to be fiddly but I'd kind of expect the front end to be using a more
> abstracted interface to tell the backend to start grabbing data.  Maybe that ends
> up being so slim it's just these interfaces and it's not sensible to wrap it
> though.
> 

Likely I'm missing your point but the discussion here is where the DMA buffer should
be allocated. In theory, in the backend (at least on ADI usecases - it's the proper
representation of the HW) but as we have the iio device in the frontend, it's more
appropriate to have the buffer there. Or at least to have a way to control the buffer
lifetime from there...

On the our usecases, it's not like we tell the backend to grab data, we just use the
normal .update_scan_mode() to enable/disable the channels in the backend so when we
enable the buffer (and the frontend starts receiving and sending data via the serial
interface) the data paths are enabaled/disabled accordingly. Bah, yeah, in a way is
another wording for "grab" or "grab not" :)

- Nuno Sá