Messages in this thread | | | Date | Wed, 17 Aug 2022 15:48:58 +0200 | Subject | Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC | From | 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
| |