On Thu, Sep 28, 2023, at 06:54, Balas, Eliza wrote: >> <conor+dt@xxxxxxxxxx>; derek.kiernan@xxxxxxx; dragan.cvetic@xxxxxxx; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; >> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engine >> >> [External] >> >> 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@xxxxxxxxxx> >> >> 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. >> > > We are using this driver with an iio-fake device > https://github.com/analogdevicesinc/linux/blob/master/Documentation/devicetree/bindings/iio/jesd204/adi%2Ciio-fakedev.yaml > so we can take advantage of the iio user-space interface. I don't understand how that works yet: Do you mean that there is 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? > We talked in the previous v1 patch emails about adding this driver to > an existing subsystem, and I raised the question if we should add it to > the iio subsystem, but the driver is not registered into the IIO device > tree, and does not rely on IIO kernel APIs, so I concluded that misc is > a better choice. > What do you think? My feeling is that if you can make it fit into IIO, then this is likely the better choice, unless you can guarantee that this is a one-off driver with a single hardware implementation and a single userspace. If you need the flexibility later to do more things, the risk is that you end up duplicating a lot of functionality that already exists in IIO. This would of course mean using the interfaces provided by the IIO core, with the addition of a tdd device type rather than just having a standalone driver with just the sysfs interface you have here. Arnd
