Message-ID: <47fdb798-b66e-3d30-dcbe-a3d15dc45619@baylibre.com>
Date:   Tue, 16 Aug 2022 10:26:53 +0200
From:   Julien Panis <jpanis@...libre.com>
To:     William Breathitt Gray <william.gray@...aro.org>
Cc:     robh+dt@...nel.org, jic23@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mranostay@...com
Subject: Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC



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

Powered by blists - more mailing lists