Message-ID: <5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com>
Date:   Thu, 28 Sep 2023 13:52:25 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Arnd Bergmann <arnd@...db.de>, Eliza Balas <eliza.balas@...log.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "derek.kiernan@....com" <derek.kiernan@....com>,
        "dragan.cvetic@....com" <dragan.cvetic@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

Hi Arnd,

On Thu, 2023-09-28 at 12:07 +0200, Arnd Bergmann wrote:
> 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@...log.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.
> 

Yeah, the same question was done in v1 [1]:

For some reason (I guess a simple mistake :)) Jonathan only replied to me: Here is
it's response:

"
> We do have tons of specific attributes (non IIO ones) for this device. The ones
> resembling IIO is likely because it feels familiar to us in ADI. Anyways, I have my
> doubts this fits (at least as IIO stands right now) but maybe Jonathan thinks
> otherwise.

>From a superficial look, it's a stretch but we have stretched IIO in the past
(things like very high frequency synthesizers)

In some ways this looks like a complex PWM device though it would probably be hard
to force it into that framework.

This is weird enough that I'd be surprised to see it fit well anywhere so misc
might be the best option.

Jonathan

> 
> +cc Jonathan...
"

That said, I do agree that some of the interface should likely not be part of the
"main" ABI. Like anything that states "debugging purposes"  in the ABI file should
probably go to debugfs. I'm also not sure about those dual "*_ms" vs "*_raw" files.
Ideally, we would only provide the non "raw" ones. At least in the beginning... 

+cc Jonathan again so he can confirm that I'm not putting words in his mouth :)

[1]: https://lore.kernel.org/all/b2379aadad95d68ca9605bb9566ce6a70121a409.camel@gmail.com/

- Nuno Sá

Powered by blists - more mailing lists