[PATCH v6 0/5] Introduce the Counter character device interface

From: William Breathitt Gray
Date: Sun Nov 22 2020 - 15:30:33 EST


Changes in v6:
- Consolidated the value member of struct counter_event down to a
single u64; u64 should be capable of representing all component
values
- Removed extension width sysfs attributes; no longer needed when value
is always u64
- Implemented COUNTER_COMPONENT_DUMMY to allow timestamp grabs without
component data reads
- Implemented events_config() callback; called during
COUNTER_CLEAR_WATCHES_IOCTL and COUNTER_LOAD_WATCHES_IOCTL in order
to allow devices a chance to adjust (enable/disable IRQ, etc.) for
the new events configuration requested by the user
- Simplified example code in Documentation by removing confusing use of
poll() call
- Removed redundant ida_simple_remove() from counter_register()
- Renamed devm_counter_unreg() to devm_counter_unregister()
- Renamed functions in counter-sysfs.c to be clearer
- Fixed miscellaneous typos throughout files
- Added more kernel doc comments; I've left some defines without
comments if they seemed obvious -- but please let me know if further
documentation is needed
- Refactored quad8_irq_handler() to use WARN_ONCE() instead of
returning on error; this should prevent interrupts from entering an
endless loop
- General refactoring and additional comments for clarity
- Returns EOPNOTSUPP instead of EFAULT now if a Counter watch is added
for unsupported component
- Renamed COUNTER_SET_WATCH_IOCTL TO COUNTER_ADD_WATCH_IOCTL to make
the use clear
- Reimplemented the parent and id members of struct counter_component
as __u8 instead of __u64; it's unlikely we'll ever have a device that
supports more than 255 components
- Reimplement __u64 variables in include/uapi/linux/counter.h as
__aligned_u64 to prevent 32-bit vs 64-bit alignment issues
- Fixed return value bug in counter_comp_u8_store(); enums set to a
value with index > 0 should now work correctly
- Fixed spectre issues in counter-chrdev.c
- Removed redundant get_device() call from counter_register()
- Moved put_device() to after the events_list is freed lest we leak
memory

I'm skipping the introduction blurb because it was just a rehashing of
information included in the documentation patches within this patchset.
Instead I will focus this cover letter on discussions about this
patchset and the userspace interface implications.

1. Should standard Counter component data types be defined as u8 or u32?

Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
Counter subsystem code as u8 data types.

If u32 is used for these values instead, C enum structures could be
used by driver authors to implicitly cast these values via the driver
callback parameters.

This question is primarily addressed to David Lechner. I'm somewhat
confused about how this setup would look in device drivers. I've gone
ahead and refactored the code to support u32 enums, and pushed it to
a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum

Please check it out and let me know what you think. Is this the
support you had in mind? I'm curious to see an example of how would
your driver callback functions would look in this case. Is everything
works out fine, then I'll submit this branch as v7 of this patchset.

2. How should we handle "raw" timestamps?

Ahmad Fatoum brought up the possibility of returning "raw" timestamps
similar to what the network stack offers (see the network stack
SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).

I'm not very familiar with the networking stack code, but if I
understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
values returned from the device. If so, I suspect we would be able to
support these "raw" timestamps by defining them as Counter Extensions
and returning them in struct counter_event elements similar to the
other Extension values.

William Breathitt Gray (5):
counter: Internalize sysfs interface code
docs: counter: Update to reflect sysfs internalization
counter: Add character device interface
docs: counter: Document character device interface
counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8

Documentation/ABI/testing/sysfs-bus-counter | 18 +-
.../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 +
Documentation/driver-api/generic-counter.rst | 411 ++++-
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 2 +-
drivers/counter/104-quad-8.c | 778 +++++----
drivers/counter/Kconfig | 6 +-
drivers/counter/Makefile | 1 +
drivers/counter/counter-chrdev.c | 476 ++++++
drivers/counter/counter-chrdev.h | 16 +
drivers/counter/counter-core.c | 183 ++
drivers/counter/counter-sysfs.c | 806 +++++++++
drivers/counter/counter-sysfs.h | 13 +
drivers/counter/counter.c | 1496 -----------------
drivers/counter/ftm-quaddec.c | 60 +-
drivers/counter/microchip-tcb-capture.c | 114 +-
drivers/counter/stm32-lptimer-cnt.c | 175 +-
drivers/counter/stm32-timer-cnt.c | 145 +-
drivers/counter/ti-eqep.c | 224 +--
include/linux/counter.h | 676 ++++----
include/linux/counter_enum.h | 45 -
include/uapi/linux/counter.h | 105 ++
22 files changed, 3094 insertions(+), 2689 deletions(-)
create mode 100644 drivers/counter/counter-chrdev.c
create mode 100644 drivers/counter/counter-chrdev.h
create mode 100644 drivers/counter/counter-core.c
create mode 100644 drivers/counter/counter-sysfs.c
create mode 100644 drivers/counter/counter-sysfs.h
delete mode 100644 drivers/counter/counter.c
delete mode 100644 include/linux/counter_enum.h
create mode 100644 include/uapi/linux/counter.h

--
2.29.2