Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC

[Julien Panis]

On 14/08/2022 20:00, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:21PM +0200, Julien Panis wrote:
> > The Enhanced Capture (ECAP) module can be used to timestamp events
> > detected on signal input pin. It can be used for time measurements
> > of pulse train signals.
> >
> > ECAP module includes 4 timestamp capture registers. For all 4 sequenced
> > timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
> > edge) can be selected.
> >
> > This driver leverages counter subsystem to :
> > - select edge polarity for all 4 capture events (event mode)
> > - log timestamps for each capture event
> > Event polarity, and CAP1/2/3/4 timestamps give all the information
> > about the input pulse train. Further information can easily be computed :
> > period and/or duty cycle if frequency is constant, elapsed time between
> > pulses, etc...
> >
> > Modifications since v3:
> > 	- Migrate driver from IIO to Counter subsystem
> > 	- Minor modification in yaml ($id) to match Counter subsystem
> > 	- Add ABI documentation
> >
> > Userspace commands :
> > 	### SIGNAL ###
> > 	cd /sys/bus/counter/devices/counter0/signal0
> >
> > 	# Get available polarities for each capture event
> > 	cat polarity1_available
> > 	cat polarity2_available
> > 	cat polarity3_available
> > 	cat polarity4_available
> >
> > 	# Get polarity for each capture event
> > 	cat polarity1
> > 	cat polarity2
> > 	cat polarity3
> > 	cat polarity4
> >
> > 	# Set polarity for each capture event
> > 	echo rising > polarity1
> > 	echo falling > polarity2
> > 	echo rising > polarity3
> > 	echo falling > polarity4
> >
> > 	### COUNT ###
> > 	cd /sys/bus/counter/devices/counter0/count0
> >
> > 	# Run ECAP
> > 	echo 1 > enable
> >
> > 	# Get current timebase counter value
> > 	cat count
> >
> > 	# Get captured timestamps
> > 	cat capture1
> > 	cat capture2
> > 	cat capture3
> > 	cat capture4
> >
> > 	# Note that counter watches can also be used to get
> > 	# data from userspace application
> > 	# -> see tools/counter/counter_example.c
> >
> > 	# Stop ECAP
> > 	echo 0 > enable
> >
> > Julien Panis (3):
> >    dt-binding: counter: add ti,am62-ecap-capture.yaml
> >    Documentation: ABI: add sysfs-bus-counter-ecap
> >    counter: capture-tiecap: capture driver support for ECAP
> >
> >   .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
> >   .../counter/ti,am62-ecap-capture.yaml         |  61 ++
> >   drivers/counter/Kconfig                       |  14 +
> >   drivers/counter/Makefile                      |   1 +
> >   drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
> >   include/uapi/linux/counter.h                  |   2 +
> >   6 files changed, 776 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
> >   create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
> >   create mode 100644 drivers/counter/capture-tiecap.c
> >
> > --
> > 2.25.1
> Something that has become apparent to me is the code repetition in this
> driver in order to support the capture buffer. Buffers are common
> components in devices, so it'll be good for us to standardize some of
> what we're exploring here into an interface that other drivers can also
> use. We have two ABIs to consider: the driver interface and the sysfs
> interface.
>
> For the sysfs interface, I think we'll have to expose each element
> individually (e.g. capture1, capture2, etc.) because sysfs attributes
> are suppose to expose only a single datum for any given attribute.
>
> For the driver side, we might want to introduce a new Counter component
> type for buffers and respective macros to streamline some of the code
> for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum
> counter_comp_type constant could be introduced to represent a u64 buffer
> element; respective struct counter_comp read callbacks could be
> introduced::
>
>      int (*count_buffer_u64_read)(struct counter_device *counter,
>                                   struct counter_count *count,
> 				 size_t index, u64 *val);
>
> So a driver author can use the "index" parameter to locate the buffer
> element and pass back its value via the "val" parameter. To define the
> buffer, maybe helper macros like this could be introduced::
>
>      COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)
>
> This would define four u64 buffer elements each named prefixed with
> "capture" and with their read callbacks set to ecap_cnt_cap_read().
>
> One problem however is that I'm not sure if the C preprocessor would be
> able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of
> elements based on a macro parameter (maybe there is a GCC extension).
>
> I'm just throwing out ideas, so I'd like to hear some comments and
> suggestions from others about how we should add buffer support to the
> Counter subsystem.
>
> William Breathitt Gray

Hi William,

I am going to send a new version (PATCH v5) without buffers as described 
above
(I added macros for cap1/2/3/4 similar functions). I tried taking into 
account the
comments made by Jonathan and you.

Regarding buffers, I don't know how driver upstream process is done when 
'significant'
subsystem modifications are being considered. Is it a problem for you if 
we continue
driver upstream process until it's merged, and then modify counter 
subsystem (and
ECAP driver with it) to add some buffer functionalities ? Or do you 
prefer adding such
functionalities to counter subsystem while upstreaming ECAP driver, 
without any
intermediate ECAP driver version ?

Julien Panis