Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <026fa80d29054750937cd077b7f4f689de4e18f2.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>, nuno.sa@analog.com
Cc:     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:   Fri, 01 Dec 2023 10:08:27 +0100
In-Reply-To: <CAMknhBFbLju8UQJ7Uz85kHKrbK4mzt=wTRdnp40+PwWCJa5dsA@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>
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 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 removed
> > 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 AD9=
467 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

Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now th=
at I'm
thinking, I'm not so sure if this is just some legacy thing we had in ADI t=
ree. I
wonder if it actually makes sense for a device like with no buffering suppo=
rt?!
=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_string(d=
ev, "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_buffer_=
setup(dev, indio_dev, dma_name);
>=20
> 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?
>=20

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, s=
ince we
want to have the IIO interface in the frontend, it would be hard to do that=
 without
hacking devm_iio_dmaengine_buffer_setup().=C2=A0I mean, lifetime wise it wo=
uld be far from
wise to have the DMA buffer associated to a completely different device tha=
n the IIO
parent device. I mean, one way could just be export iio_dmaengine_buffer_fr=
ee() 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 i=
t....

- Nuno S=C3=A1
>=20