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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231204154810.3f2f3f83@jic23-huawei>
Date:   Mon, 4 Dec 2023 15:48:10 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     David Lechner <dlechner@...libre.com>, nuno.sa@...log.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-iio@...r.kernel.org,
        Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

On Fri, 01 Dec 2023 10:08:27 +0100
Nuno Sá <noname.nuno@...il.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@...nel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@...log.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@...log.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.

The argument for the IIO device (you control) being associated with the analog/digital front end is that
is what happens with a consumer interface today. We think of the analog components (consuming device)
using the sevice of the ADC it is consuming to measure their state.

You can flip the whole thing and make the data grabbing engine the IIO device but
I guess that's just not how we did it and I'm sure I had many good reasons at the time
(one of them being that it needed to work for other cases such as a touchscreen driver
consuming the IIO device)  I that case it's the touchscreen driver that is responsible
for data scaling etc not the ADC.  

In my head, data acquisition is a service - what is interesting is what is being measured
and so I put the IIO device as near that as possible.

Jonathan