Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-958-g1b1b911df8-fm-20230927.002-g1b1b911d
MIME-Version: 1.0
Message-Id: <839638d2-7502-4925-8b7f-6b15779a6840@app.fastmail.com>
In-Reply-To: <20230928092804.22612-3-eliza.balas@analog.com>
References: <20230928092804.22612-1-eliza.balas@analog.com>
 <20230928092804.22612-3-eliza.balas@analog.com>
Date:   Thu, 28 Sep 2023 12:07:26 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "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, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
Content-Type: text/plain
Precedence: bulk

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.
>
> 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 signals.
>
> Signed-off-by: Eliza Balas <eliza.balas@analog.com>

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.

It would however help to explain the ideas you had for the
user-space interface design and summarize them in the changelog
text.

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.

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.

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.

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.

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.

     Arnd