* Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
2023-10-23 14:36 ` Nuno Sá
@ 2023-10-28 16:29 ` Jonathan Cameron
0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2023-10-28 16:29 UTC (permalink / raw)
To: Nuno Sá
Cc: Arnd Bergmann, Eliza Balas, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
derek.kiernan@amd.com, dragan.cvetic@amd.com, Lars-Peter Clausen,
linux-iio, Fabrice Gasnier, Olivier Moysan
On Mon, 23 Oct 2023 16:36:57 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Mon, 2023-10-23 at 16:19 +0200, Arnd Bergmann wrote:
> > On Mon, Oct 23, 2023, at 15:30, Balas, Eliza wrote:
> > > > -----Original Message-----
> > > > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > > > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> >
> > > > > > > Since the device is not an iio device, using an iio function would
> > > > > > > be confusing.
> > > > > >
> > > > > > Why isn't this an iio device?
> > > > >
> > > > > The device is not registered into the IIO device tree,
> > > > > and does not rely on IIO kernel APIs.
> > > > > Even though there are a few attributes that resemble the
> > > > > ones from iio, and the sysfs structure is similar,
> > > > > this is not an IIO device.
> > > > > In the previous patch versions 1 and 2 we concluded
> > > > > that this device fits better in the misc subsystem.
> > > >
> > > > Ok, can you point to that in the changelog where the IIO maintainer
> > > > agreed that this doesn't fit into that subsystem?
> > > >
> > > This was one of the discussions from previous v2 :
> > > https://lore.kernel.org/all/5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com/
> > >
> > > I will add it to the changelog the next time I submit the patches.
> >
> > It sounds like Jonathan wasn't quite sure either here, and I would
> > still argue (as I did in that thread), that drivers/iio is probably
> > a better option than drivers/misc.
> >
>
> Well, if Jonathan agrees to have this in IIO, it would actually be better for
> us... The below hack would not be needed at all and IIO is very familiar.
I'm flexible on including this in IIO - we have a few weird and wonderful
devices already that need heavily custom userspace. This looks like it might
be another one of those - though I'm not sure it's all that unusual if you
consider the DDS devices we have drivers for (still in staging after a lot
of years).
A few things in the ABI would need to fit a little better such as matching
units were appropriate and not providing bother msec and raw control of things.
Mapping it as closely as possible onto IIO concepts might be tricky.
Sync is a bit trigger like, but we aren't doing output buffers so it doesn't
fit that well - also the IIO repeat infrastructure is simple and not much used
in IIO so I doubt it's effective enough for this. Maybe we can expand
the buffer control concepts to include 'hardware generated sequences'.
Magic might map over to label.
Scratch register says useful for debug on the webpage. Don't expose it then,
or if you do only in debugfs. Overall enable would be controlled by starting
the buffer.
I'm not against having more sophisticate output channel definitions to allow
for delays from trigger etc. We've talked about doing that for input channels
in the past to be able to identify the timing more accurately in devices with
a sequencer unit. So anything we define should be useful for that case as well.
So brief brainstorming for how it might be sort of aligned with an IIO ABI.
Abusing slightly the voltage channels - make it a buffer (messing heavily with
the definition and adding a 'type' field.
buffer0/type - userspace, pattern - for all existing devices it's userspace.
For each channel
buffer0/repeats //can't use the scan element spec as it's not writeable.
//I'd love to fix that but it's horribly invasive to do.
buffer0/repeat_delay //frame length? Or could map to sampling_frequency :)
buffer0/out_voltageX_en
buffer0/out_voltageX_polarity
buffer0/out_voltageX_index
buffer0/out_voltageX_delay //in seconds I think to match with timeout ABI
//not ondelay because I want to reuse this for
//the sequencer case mentioned above.
buffer0/out/voltageX_offdelay // also mapped from start of frame.
For the delays, interesting to think about how they map to more general DDS
devices.
startup delay could be considered a trigger characteristic or hammered in here
as a parameter applying to all channels.
Sync timing is a trigger parameter. Expose separate triggers for internal,
external and maybe soft - that one is up to you.
It's not that horrible, so if you want to give this a go - perhaps
start with a full formal ABI proposal and let's see if the fit is reasonable?
Feel free to take the above suggestions as the brief one person brainstorm that
they are and propose something much more sensible / coherent!
One thing that concerns me a little is mapping it to more similar
devices. For instances, treating the signals as voltages is misleading.
Maybe we should treat them as code words, thus supporting more general time
division multiplexers. We walked through some of the symbol type interfaces
a long time ago for PSK / DDS devices though I think they are still in staging
(feel free to deal with that whilst you are here :)
It's always hard to define ABI with one device of a new class...
The st micro folk have some complex ADC setups that aren't entirely dissimilar.
(we have chained triggers to deal with a similar 'repeat' on trigger situation
that looks to me similar to your burst + frame).
Maybe they have considered doing this for DACs or other usecase that overlap
with yours? +CC Olivier and Fabrice for input.
If there is some common ground here it would be useful to get inputs on the
ABI even if there is no plan to add equivalent support in the near future.
Jonathan
>
> > In particular, you mention that you actually make this device
> > appear as an IIO device to user space using the "iio-fake" hack.
> >
>
> I want to emphasize that is just our hack to make use of libiio RPC so that we
> can remotely access this device.
>
> - Nuno Sá
>
^ permalink raw reply [flat|nested] 3+ messages in thread