Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 01/08/2022 16:08, Julien Panis wrote:


On 31/07/2022 17:41, Jonathan Cameron wrote:
On Thu, 28 Jul 2022 19:51:24 +0200
Julien Panis <jpanis@xxxxxxxxxxxx> wrote:

ECAP hardware on AM62x SoC supports capture feature. It can be used
to timestamp events (falling/rising edges) detected on signal input pin.

This commit adds capture driver support for ECAP hardware on AM62x SoC.

In the ECAP hardware, capture pin can also be configured to be in
PWM mode. Current implementation only supports capture operating mode.
Hardware also supports timebase sync between multiple instances, but
this driver supports simple independent capture functionality.

Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx>
Hi Julien,

So this isn't the first ecap driver we've had proposed, but the previous
one was a few years ago and never reached v2.
https://lore.kernel.org/all/20200818153614.6438-1-dts86@xxxxxxxxxxx/

Honestly I can't remember much about it, but maybe the discussion around
that will be worth a reread.

Hi Jonathan, thank you for your review.

I read the discussion about previous attempt, before submitting this patch. There were
interesting comments indeed.

But in this previous attempt, only one-shot pulses were handled (moreover, global IRQ flag was not cleared, so I'm not sure that IRQ could be raised more than once).

However, ECAP can be used to make time measurements for any type of "square waveform". That's why I tried to make this event mode configurable. Besides, using a continuous mode allows
handling much more signal types (not only single pulses).


The use of ABI here is unusual. So I'd definitely like to see some documentation probably as a file in the main kernel documentation to explain what the interface
is an how that relates to what is being captured.

OK, I will add some userspace documentation.


First thing to note here is the channel type of IIO_INDEX is now not actually used any more because we moved all the relevant drivers over to the counter
subsystem (and we failed to mark it deprecated).

I evaluated this counter subsystem before starting development. Counting events is not "a priori"
the goal when using ECAP.

Nevertheless, maybe "counter_push_event" function could do the job. If I use counter API : # Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the same counter... but there are not channels, there are just sequential timestamps actually. So I'm afraid this leads
to misunderstanding for the user.
Moreover, the user will have to read several entries (counts 1/2/3/4) to gather timestamps from
the same input signal, which is not very convenient.
# Option 2 : Either CAP 1/2/3/4 events could be gathered in a single channel...but then it will not be possible to configure their polarity (rising/falling edge) individually (unless I did
not understand well counter framework documentation).

So, even with counter framework, it will lead to some diverted use of the framwork, since ECAP
is a very specific hardware that do not fit 100% counter philosophy.

I admit that ECAP do not fit 100% IIO philosophy either.

Maybe misc API would be more relevant actually. Any opinion about it will be welcome. :-)

[Answering my own mail]

I got a closer look at counter framework. It is not suitable at all for ECAP. Initially, I thought that "counter_push_event" function could be used, but the only timestamp handled by this function is a software timestamp. I strongly doubt that counter framework maintainer would accept some modification here to support hardware timestamp : a patch rejection would be legitimate, since a counter is dedicated to "event counting". Whereas ECAP is dedicated to
"event timestamping".

Beside, ECAP has 4 timestamp registers but they are used to capture timestamps for a single input pin (only 1 channel). In ECAP context, 'index X" is used to identify CAP X
(used to capture event X detected on a single pin, with X = 0/1/2/3/0...).
In counter framework, "index X" is used to identify channel X (among several pins). So, the word "index" has not the same meaning in counter framework than in ECAP device. Somehow, this ECAP index (0/1/2/3 for CAP1/2/3/4 registers) must be logged with timestamp because it is an important part of signal info for the user (raw consecutive timestamps
are not enough).

So, here is my proposal for my next version :
(1) Replace IIO_INDEX by IIO_COUNT channel (already used in "stm32-timer-trigger.c" driver) # In ECAP documentation, the word "index" is not used. The word used to speak about this
0->1->2->3->0 sequenced counter is "Mod4 counter".
(2) Configure event mode with 4 sysfs entries (to remove the mix of buffers and events interfaces) # User will see 4 files (1 file for each CAP timestamp) named "falling_edge_active_0/1/2/3".
Writing 1 will select falling edge/ Writing 0 will select rising edge.

Would it be an acceptable alternative for you, Jonathan ? Would either (1) and/or (2) be a "no-go" ?



Anyhow, I've reviewed below, but need docs to discuss this in depth.  In particular the mix of buffers and events interfaces is unlikely to be an acceptable path
forwards.

OK, I will consider alternatives.


Jonathan




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux