Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <5043c6729e1ffd2dac84b6908b26db11347521b0.camel@gmail.com>
Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
From:   Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To:     Arnd Bergmann <arnd@arndb.de>, Eliza Balas <Eliza.Balas@analog.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        "derek.kiernan@amd.com" <derek.kiernan@amd.com>,
        "dragan.cvetic@amd.com" <dragan.cvetic@amd.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Date:   Thu, 28 Sep 2023 18:35:29 +0200
In-Reply-To: <94110af1-2b32-4eb2-81be-2a79fc6973d8@app.fastmail.com>
References: <20230928092804.22612-1-eliza.balas@analog.com>
         <20230928092804.22612-3-eliza.balas@analog.com>
         <839638d2-7502-4925-8b7f-6b15779a6840@app.fastmail.com>
         <BN7PR03MB4545DC903A0D62639085591697C1A@BN7PR03MB4545.namprd03.prod.outlook.com>
         <94110af1-2b32-4eb2-81be-2a79fc6973d8@app.fastmail.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-09-28 at 10:19 -0400, Arnd Bergmann wrote:
> On Thu, Sep 28, 2023, at 06:54, Balas, Eliza wrote:
> > > <conor+dt@kernel.org>; derek.kiernan@amd.com; dragan.cvetic@amd.com; =
Greg
> > > Kroah-Hartman <gregkh@linuxfoundation.org>;
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> > > Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engin=
e
> > >=20
> > > [External]
> > >=20
> > > On Thu, Sep 28, 2023, at 11:28, Eliza Balas wrote:
> > > > This patch introduces the driver for the new ADI TDD engine HDL.
> > > > The generic TDD controller is in essence a waveform generator
> > > > capable of addressing RF applications which require Time Division
> > > > Duplexing, as well as controlling other modules of general
> > > > applications through its dedicated 32 channel outputs.
> > > >=20
> > > > The reason of creating the generic TDD controller was to reduce
> > > > the naming confusion around the existing repurposed TDD core
> > > > built for AD9361, as well as expanding its number of output
> > > > channels for systems which require more than six controlling signal=
s.
> > > >=20
> > > > Signed-off-by: Eliza Balas <eliza.balas@analog.com>
> > >=20
> > > Thanks for your submission, I've had a first look at the driver
> > > and the implementation of the interface you have chosen looks
> > > all good to me, so I have no detailed comments on that.
> > >=20
> > > It would however help to explain the ideas you had for the
> > > user-space interface design and summarize them in the changelog
> > > text.
> > >=20
> > > You have chosen a low-level interface that wraps the individual
> > > device registers and gives user space direct control over them.
> > > The risk here is to lock yourself into the first design,
> > > giving you less flexibility for future extensions, so it would
> > > help to understand what the usage model is here.
> > >=20
> > > One risk is that there may be an in-kernel user in the future
> > > when the TDD engine interacts with another device, so you
> > > need a driver level interface, which would in turn break
> > > if any user pokes the registers directly.
> > >=20
> > > Another possible problem I see is that an application written
> > > for this driver would be incompatible with similar hardware
> > > that has the same functionality but a different register-level
> > > interface, or even a minor revision of the device that ends up
> > > breaking one of the assumptions about the hardware design.
> > >=20
> > > In both cases, the likely answer is to have a higher-level
> > > interface of some sort, but the downside of that would be
> > > that it is much harder to come up with a good interface that
> > > covers all possible use cases.
> > >=20
> > > Another question is whether you could fit into some
> > > existing subsystem instead of creating a single-driver
> > > interface. drivers/iio/ might be a good choice, as
> > > it already handles both in-kernel and userspace users,
> > > and provides a common abstraction for multiple classes
> > > of devices that (without any domain knowledge in my case)
> > > look similar enough that this could be added there.
> > >=20
> >=20
> > We are using this driver with an iio-fake device=20
> > https://github.com/analogdevicesinc/linux/blob/master/Documentation/dev=
icetree/bindings/iio/jesd204/adi%2Ciio-fakedev.yaml
> > =C2=A0
> > =C2=A0so we can take advantage of the iio user-space interface.
>=20
> I don't understand how that works yet: Do you mean that there
> is=C2=A0 user-space application that uses the tdd sysfs interface to
> export an IIO device back into the kernel, or do you mean there
> is a regular IIO device in with a kernel driver that is used
> as the back-end for the tdd device, or something else?
>=20

Well, I never used this myself but the iio-fakedev is an out of tree driver=
 that
receives a phandle to a device and a string list of sysfs attributes of tha=
t same
device. It then symlinks those to an IIO fake device so they seem like IIO =
device
attributes. As a said, this is __very__, __very__ hackish and the solely re=
ason it's
being done (I believe), is to use libiio on that fakedev so you can access =
these kind
of devices (like this TDD core) remotely through the network for example (o=
r USB). In
the past, we would put drivers that are not IIO in IIO for the same reason.=
 So, at
least now, it's a one time ugly hack :sweat_smile: but then we can put driv=
ers in
their right places. Not saying this justifies this fakedev but it is what i=
t is :).

Well, maybe this one is not really in here but the IIO maintainer was also =
not to
keen to have in there. So I'm not really sure where else it can go.
>=20

- Nuno S=C3=A1