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

Julien Panis
2022-08-16T10:22:37Z

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().

Somehow, this "index" parameter would make things much easier for driver
author.
Besides, defining all these similar functions (cap1_read, ...,
cap4_read) does not really
make sense for ECAP driver.
So, accessing this index parameter through callback and having the
possibility to use
the same function (cap_read) for several attributes would be great for
my driver,
and probably for future ones.

>
> 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