Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <863114fa44cda4ca58e17a47191f0388df39cc80.camel@gmail.com>
Subject: Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework
From:   Nuno =?ISO-8859-1?Q?S=E1?= <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>
Date:   Sat, 02 Dec 2023 09:46:26 +0100
In-Reply-To: <CAMknhBGKinZB==QHLazZ9ZkfALyj2N=rVfZfsOk22p6X9SZSrQ@mail.gmail.com>
References: <20231121-dev-iio-backend-v1-0-6a3d542eba35@analog.com>
         <20231121-dev-iio-backend-v1-10-6a3d542eba35@analog.com>
         <CAMknhBFbLju8UQJ7Uz85kHKrbK4mzt=wTRdnp40+PwWCJa5dsA@mail.gmail.com>
         <026fa80d29054750937cd077b7f4f689de4e18f2.camel@gmail.com>
         <CAMknhBGKinZB==QHLazZ9ZkfALyj2N=rVfZfsOk22p6X9SZSrQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 3:08=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail.co=
m> wrote:
> >=20
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17=E2=80=AFAM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > >=20
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > >=20
> > > > Convert the driver to use the new IIO backend framework. The device
> > > > functionality is expected to be the same (meaning no added or remov=
ed
> > > > features).
> > >=20
> > > Missing a devicetree bindings patch before this one?
> > >=20
> > > >=20
> > > > 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.
> > >=20
> > > Can you be more specific about what is actually breaking?
> > >=20
> > > >=20
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > > =C2=A0drivers/iio/adc/Kconfig=C2=A0 |=C2=A0=C2=A0 2 +-
> > > > =C2=A0drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++-=
---------------
> > > > --
> > > > =C2=A02 files changed, 157 insertions(+), 101 deletions(-)
> > > >=20
> > > > 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
> > > > =C2=A0config AD9467
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tristate "Analog Devices=
 AD9467 High Speed ADC driver"
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 depends on SPI
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 depends on ADI_AXI_ADC
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select IIO_BACKEND
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 help
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Say yes here=
 to build support for Analog Devices:
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * 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
> > >=20
> > > <snip>
> > >=20
> > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)
> > >=20
> > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > >=20
> >=20
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, no=
w that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in A=
DI tree. I
> > wonder if it actually makes sense for a device like with no buffering s=
upport?!
> >=20
> > > > +{
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct device *dev =3D indio_=
dev->dev.parent;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const char *dma_name;
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!device_property_present(=
dev, "dmas"))
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 return 0;
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (device_property_read_stri=
ng(dev, "dma-names", &dma_name))
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 dma_name =3D "rx";
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return devm_iio_dmaengine_buf=
fer_setup(dev, indio_dev, dma_name);
> > >=20
> > > The device tree bindings for "adi,ad9467" don't include dma propertie=
s
> > > (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?
> > >=20
> >=20
> > Oh yes, I need to update the bindings. In the link I sent you we can se=
e 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. Howeve=
r, 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 wou=
ld 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_buffe=
r_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 f=
or it....
> >=20
> > - Nuno S=C3=A1
> > >=20
> >=20
>=20
> 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. ;-)
>=20
> [1]:
> https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e=
9d0a5ddb9ffa331871edd6bd2a5

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 pur=
e trigger
device is at the very least questionable. But the point is, in the end of t=
he day,
the buffer should belong to a device.

* Your PWM trigger seems to be highly focused on the spi_engine offload fea=
ture. You
should go for a generic pwm trigger which is something that was already dis=
cussed 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_engin=
e
controller using a pwm trigger. But conceptually it is correct and we do ha=
ve a mode
for hardware triggered buffers.

- Nuno S=C3=A1