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

From: "Nuno Sá" <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>
Cc: nuno.sa@analog.com, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
	Olivier MOYSAN <olivier.moysan@foss.st.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>
Subject: Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework
Date: Sat, 02 Dec 2023 09:46:26 +0100	[thread overview]
Message-ID: <863114fa44cda4ca58e17a47191f0388df39cc80.camel@gmail.com> (raw)
In-Reply-To: <CAMknhBGKinZB==QHLazZ9ZkfALyj2N=rVfZfsOk22p6X9SZSrQ@mail.gmail.com>

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
should go for a generic pwm trigger which is something that was already discussed on
the list and AFAIR, completely acceptable. That said, not so sure how much will we
win (in terms of code simplification) by having devices under the spi_engine
controller using a pwm trigger. But conceptually it is correct and we do have a mode
for hardware triggered buffers.

- Nuno Sá