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

[Julien Panis]

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