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 <[email protected]>
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
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 <[email protected]>
>
> 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/[email protected]/
- Nuno Sá
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Thursday, September 28, 2023 13:07
> To: Balas, Eliza <[email protected]>
> Cc: Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Conor Dooley
> <[email protected]>; [email protected]; [email protected]; Greg Kroah-Hartman <[email protected]>;
> [email protected]; [email protected]
> 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 <[email protected]>
>
> 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
Hello Arnd,
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.
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?
Thank you,
Eliza
On Thu, Sep 28, 2023, at 06:54, Balas, Eliza wrote:
>> <[email protected]>; [email protected]; [email protected]; Greg Kroah-Hartman <[email protected]>;
>> [email protected]; [email protected]
>> 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 <[email protected]>
>>
>> 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
On Thu, 2023-09-28 at 10:54 +0000, Balas, Eliza wrote:
>
>
> > -----Original Message-----
> > From: Arnd Bergmann <[email protected]>
> > Sent: Thursday, September 28, 2023 13:07
> > To: Balas, Eliza <[email protected]>
> > Cc: Rob Herring <[email protected]>; Krzysztof Kozlowski
> > <[email protected]>; Conor Dooley
> > <[email protected]>; [email protected]; [email protected]; Greg Kroah-
> > Hartman <[email protected]>;
> > [email protected]; [email protected]
> > 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 <[email protected]>
> >
> > 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
>
> Hello Arnd,
>
> 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.
>
Ehehehh, iio-fake device is an hack we have to make use of libiio capability to
access IIO devices trough USB, network and serial. I might be wrong, but I think
that's pretty much the reason for that driver.
In the past we used to treat non IIO devices as IIO solely for that reason so it's
better do it just once (with that fake device)
- Nuno Sá
On Thu, 2023-09-28 at 10:19 -0400, Arnd Bergmann wrote:
> On Thu, Sep 28, 2023, at 06:54, Balas, Eliza wrote:
> > > <[email protected]>; [email protected]; [email protected]; Greg
> > > Kroah-Hartman <[email protected]>;
> > > [email protected]; [email protected]
> > > 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 <[email protected]>
> > >
> > > 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?
>
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 that 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 reason 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 (or 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 drivers in
their right places. Not saying this justifies this fakedev but it is what it 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.
>
- Nuno Sá
> -----Original Message-----
> From: Nuno Sá <[email protected]>
> Sent: Thursday, September 28, 2023 19:35
> To: Arnd Bergmann <[email protected]>; Balas, Eliza <[email protected]>
> Cc: Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Conor Dooley
> <[email protected]>; [email protected]; [email protected]; Greg Kroah-Hartman <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
>
> [External]
>
> On Thu, 2023-09-28 at 10:19 -0400, Arnd Bergmann wrote:
> > On Thu, Sep 28, 2023, at 06:54, Balas, Eliza wrote:
> > > > <[email protected]>; [email protected]; [email protected]; Greg
> > > > Kroah-Hartman <[email protected]>;
> > > > [email protected]; [email protected]
> > > > 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 <[email protected]>
> > > >
> > > > 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://urldefense.com/v3/__https://github.com/analogdevicesinc/linux/blob/master/Documentation/devicetree/bindings/iio/jesd2
> 04/adi*2Ciio-fakedev.yaml__;JQ!!A3Ni8CS0y2Y!8Km0TXYcgKkusJTPoYw6V09_KZFbj09ZfbrUpnuVRZoKKOECMJQ-
> 8tGXAYWsjrQ50IbGnsYXzqx8-GjR0BkqRQ$
> > >
> > > 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?
> >
>
> 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 that 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 reason 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 (or 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 drivers in
> their right places. Not saying this justifies this fakedev but it is what it 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.
> >
>
> - Nuno Sá
As Nuno explained in the previous email, we are using the iio-fake device as a workaround for using the driver with the libiio API to interface it with some user-space client applications.
The device itself contains attributes which are not part of the IIO. We do not want to confuse this device with an IIO device, so I believe that the driver should reside in the misc subsystem.
This driver is created for a specific FPGA Core and the intent was to have a low-level interface so that the user can access the registers directly, in an easy way, without using a complex interface.
If there will be other future revisions of the TDD FPGA Core, we will keep the register space compatible, so we don't break the current functionality of the driver.
I will make the changes related to the dual-license and change it to single license for the driver. If that is okay with you, I will keep the driver in the misc subsystem.
I thank you all for taking the time to review this driver and for helping me figure out where this driver fits best.
Eliza