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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 04 Dec 2023 17:23:12 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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...

My instinct was put it in the backened and proxy / interfaces as necessary but (based
on my vague recollection of how this works) at times these DMA buffers are handed off
to userspace which is a front end problem rather than the hardware.  So I guess it's
a question of who logically creates them?  Then as  you say provide the controls for
the other part to mess with their lifetimes or at least ensure the stick around whilst
it expects them to.

> 
> 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" :)

Yup. It's not as easily separated as simple always on analog only front ends. Someone
drives the clock ultimately and that could be either end in theory at least.

What fun.

J
> 
> - Nuno Sá