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

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

On Sat, 2023-12-02 at 09:46 +0100, Nuno Sá wrote:
> On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> > On Fri, Dec 1, 2023 at 3:08 AM 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....
> > > 
> > > - Nuno Sá
> > > > 
> > > 
> > 
> > I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
> > Engine offload support, so I hope that is the right way to go. ;-)
> > 
> > [1]:
> > https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5
> 
> I don't really want to extend much on this since this is still out of tree code so
> I'm not sure we should be discussing it much in here. But there a couple of
> concerns
> already I'm seeing:
> 
> * AFAIU, you export the function so you can use it in your pwm trigger. And you
> don't
> want to attach the buffer to a device. That looks very questionable. If you don't
> attach to a device, how do you have the userspace interface working on that buffer?
> How can you fetch samples from it? Also hiding the buffer allocation in pure
> trigger
> device is at the very least questionable. But the point is, in the end of the day,
> the buffer should belong to a device.
> 
> * Your PWM trigger seems to be highly focused on the spi_engine offload feature.
> You

OTOH, it also seems there are some stm focused triggers. So maybe we can also have
something more oriented to spi_engine...

- Nuno Sá