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

[Nuno Sá]

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@xxxxxxxxxx> wrote:
> > 
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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....

- Nuno Sá
>