| Message ID | aa4d62315bb11ddc66d0e1a685c81b1721ffe624.1526487615.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi William, I love your patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.17-rc5 next-20180517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/William-Breathitt-Gray/Introduce-the-Counter-subsystem/20180519-151353 reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev' include/net/cfg80211.h:4216: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev' include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw' include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw' include/net/mac80211.h:955: warning: Function parameter or member 'control.rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info' net/mac80211/sta_info.h:586: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info' kernel/sched/fair.c:3719: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf' include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array' >> include/linux/counter.h:330: warning: Function parameter or member 'groups_list' not described in 'counter_device_state' >> include/linux/counter.h:330: warning: Function parameter or member 'num_groups' not described in 'counter_device_state' include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.sign' not described in 'iio_chan_spec' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.realbits' not described in 'iio_chan_spec' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.storagebits' not described in 'iio_chan_spec' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.shift' not described in 'iio_chan_spec' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.repeat' not described in 'iio_chan_spec' include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.endianness' not described in 'iio_chan_spec' include/linux/iio/hw-consumer.h:1: warning: no structured comments found include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/mtd/rawnand.h:752: warning: Function parameter or member 'timings.sdr' not described in 'nand_data_interface' include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf' not described in 'nand_op_data_instr' include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.in' not described in 'nand_op_data_instr' include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.out' not described in 'nand_op_data_instr' include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx' not described in 'nand_op_instr' include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.cmd' not described in 'nand_op_instr' include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_instr' include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.data' not described in 'nand_op_instr' include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.waitrdy' not described in 'nand_op_instr' include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx' not described in 'nand_op_parser_pattern_elem' include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_parser_pattern_elem' include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.data' not described in 'nand_op_parser_pattern_elem' include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.desc' not described in 'nand_chip' include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.priv' not described in 'nand_chip' include/linux/regulator/driver.h:222: warning: Function parameter or member 'resume_early' not described in 'regulator_ops' drivers/regulator/core.c:4306: warning: Excess function parameter 'state' description in 'regulator_suspend_late' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb' drivers/usb/typec/mux.c:186: warning: Function parameter or member 'mux' not described in 'typec_mux_unregister' drivers/usb/typec/mux.c:186: warning: Excess function parameter 'sw' description in 'typec_mux_unregister' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver' drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or member 'attach' not described in 'drm_gem_unmap_dma_buf' drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or member 'sgt' not described in 'drm_gem_unmap_dma_buf' drivers/gpu/drm/drm_prime.c:342: warning: Function parameter or member 'dir' not described in 'drm_gem_unmap_dma_buf' drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or member 'dma_buf' not described in 'drm_gem_dmabuf_kmap_atomic' drivers/gpu/drm/drm_prime.c:438: warning: Function parameter or member 'page_num' not described in 'drm_gem_dmabuf_kmap_atomic' drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or member 'dma_buf' not described in 'drm_gem_dmabuf_kunmap_atomic' drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or member 'page_num' not described in 'drm_gem_dmabuf_kunmap_atomic' drivers/gpu/drm/drm_prime.c:450: warning: Function parameter or member 'addr' not described in 'drm_gem_dmabuf_kunmap_atomic' drivers/gpu/drm/drm_prime.c:461: warning: Function parameter or member 'dma_buf' not described in 'drm_gem_dmabuf_kmap' drivers/gpu/drm/drm_prime.c:461: warning: Function parameter or member 'page_num' not described in 'drm_gem_dmabuf_kmap' drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or member 'dma_buf' not described in 'drm_gem_dmabuf_kunmap' drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or member 'page_num' not described in 'drm_gem_dmabuf_kunmap' drivers/gpu/drm/drm_prime.c:473: warning: Function parameter or member 'addr' not described in 'drm_gem_dmabuf_kunmap' include/media/v4l2-dev.h:42: warning: Enum value 'VFL_TYPE_MAX' not described in enum 'vfl_devnode_type' include/linux/skbuff.h:850: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'skb_mstamp' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member '__unused' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'pfmemalloc' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'csum_valid' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'csum_level' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff' include/linux/skbuff.h:850: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff' include/net/sock.h:234: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_portpair' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_cookie' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_listener' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common' include/net/sock.h:234: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common' include/net/sock.h:488: warning: Function parameter or member 'sk_backlog.rmem_alloc' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'sk_backlog.len' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'sk_backlog.head' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'sk_backlog.tail' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'sk_wq_raw' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock' include/net/sock.h:488: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock' include/linux/netdevice.h:1955: warning: Function parameter or member 'adj_list.upper' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'adj_list.lower' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'gso_partial_features' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'switchdev_ops' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'name_assign_type' not described in 'net_device' include/linux/netdevice.h:1955: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device' vim +330 include/linux/counter.h 624995ae William Breathitt Gray 2018-05-16 @330 :::::: The code at line 330 was first introduced by commit :::::: 624995aed3646d19eb4ce4cd7f527fe95a165a92 counter: Introduce the Generic Counter interface :::::: TO: William Breathitt Gray <vilhelm.gray@gmail.com> :::::: CC: 0day robot <lkp@intel.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 16 May 2018 13:51:06 -0400 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > This patch adds high-level documentation about the Generic Counter > interface. > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> Various comments inline. I've been doing a lot long reviews recently (outside of the kernel world) and keep discovering the old rule that everytime you read a document you'll find something else to improve :( Jonathan > --- > Documentation/driver-api/generic-counter.rst | 336 +++++++++++++++++++ > Documentation/driver-api/index.rst | 1 + > MAINTAINERS | 1 + > 3 files changed, 338 insertions(+) > create mode 100644 Documentation/driver-api/generic-counter.rst > > diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst > new file mode 100644 > index 000000000000..5c6b9c008c06 > --- /dev/null > +++ b/Documentation/driver-api/generic-counter.rst > @@ -0,0 +1,336 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +Generic Counter Interface > +========================= > + > +Introduction > +============ > + > +Counter devices are prevalent within a diverse spectrum of industries. > +The ubiquitous presence of these devices necessitates a common interface > +and standard of interaction and exposure. This driver API attempts to > +resolve the issue of duplicate code found among existing counter device > +drivers by introducing a generic counter interface for consumption. The > +Generic Counter interface enables drivers to support and expose a common > +set of components and functionality present in counter devices. > + > +Theory > +====== > + > +Counter devices can vary greatly in design, but regardless of whether > +some devices are quadrature encoder counters or tally counters, all > +counter devices consist of a core set of components. This core set of > +components, shared by all counter devices, is what forms the essence of > +the Generic Counter interface. > + > +There are three core components to a counter: Enumerate them here. If people are reading this as a paged document (pdf etc) then the list of 3 as titles of next few sections may not be clear. * Count * Signal * Synapse > + > +COUNT > +----- > +A Count represents the count data for a set of Signals. The Generic > +Counter interface provides the following available count data types: > + > +* COUNT_POSITION_UNSIGNED: > + Unsigned integer value representing position. > + > +* COUNT_POSITION_SIGNED: > + Signed integer value representing position. Just a thought: As the '0' position is effectively arbitrary is there any actual difference between signed and unsigned? If we defined all counters to be unsigned and just offset any signed ones so the range still fitted would we end up with a simpler interface - there would be no real loss of meaning that I can see.. I suppose it might not be what people expect though when they see their counters start at a large positive value... > + > +A Count has a count function mode which represents the update behavior > +for the count data. The Generic Counter interface provides the following > +available count function modes: > + > +* Increase: > + Accumulated count is incremented. > + > +* Decrease: > + Accumulated count is decremented. > + > +* Pulse-Direction: > + Rising edges on quadrature pair signal A updates the respective count. > + The input level of quadrature pair signal B determines direction. > + Perhaps group the quadrature modes for the point of view of this document? Might be slightly easier to read? * Quadrature Modes - x1 A: etc. > +* Quadrature x1 A: > + If direction is forward, rising edges on quadrature pair signal A > + updates the respective count; if the direction is backward, falling > + edges on quadrature pair signal A updates the respective count. > + Quadrature encoding determines the direction. > + > +* Quadrature x1 B: > + If direction is forward, rising edges on quadrature pair signal B > + updates the respective count; if the direction is backward, falling > + edges on quadrature pair signal B updates the respective count. > + Quadrature encoding determines the direction. > + > +* Quadrature x2 A: > + Any state transition on quadrature pair signal A updates the > + respective count. Quadrature encoding determines the direction. > + > +* Quadrature x2 B: > + Any state transition on quadrature pair signal B updates the > + respective count. Quadrature encoding determines the direction. > + > +* Quadrature x2 Rising: > + Rising edges on either quadrature pair signals updates the respective > + count. Quadrature encoding determines the direction. This one I've never met. Really? There are devices who do this form of crazy? It gives really uneven counting and I'm failing to see when it would ever make sense... References for these odd corner cases would be good. __|---|____|-----|____ ____|----|____|-----|____ 001122222223334444444 > + > +* Quadrature x2 Falling: > + Falling edges on either quadrature pair signals updates the respective > + count. Quadrature encoding determines the direction. > + > +* Quadrature x4: > + Any state transition on either quadrature pair signals updates the > + respective count. Quadrature encoding determines the direction. > + > +A Count has a set of one or more associated Signals. > + > +SIGNAL > +------ > +A Signal represents a counter input data; this is the input data that is > +analyzed by the counter to determine the count data; e.g. a quadrature > +signal output line of a rotary encoder. Not all counter devices provide > +user access to the Signal data. > + > +The Generic Counter interface provides the following available signal > +data types for when the Signal data is available for user access: > + > +* SIGNAL_LEVEL_LOW: > + Signal line is in a low state. > + > +* SIGNAL_LEVEL_HIGH: > + Signal line is in a high state. > + > +A Signal may be associated with one or more Counts. > + > +SYNAPSE > +------- > +A Synapse represents the association of a Signal with a respective > +Count. Signal data affects respective Count data, and the Synapse > +represents this relationship. > + > +The Synapse action mode specifies the Signal data condition which > +triggers the respective Count's count function evaluation to update the > +count data. The Generic Counter interface provides the following > +available action modes: > + > +* None: > + Signal does not trigger the count function. In Pulse-Direction count > + function mode, this Signal is evaluated as Direction. > + > +* Rising Edge: > + Low state transitions to high state. > + > +* Falling Edge: > + High state transitions to low state. > + > +* Both Edges: > + Any state transition. > + > +A counter is defined as a set of input signals associated with count > +data that are generated by the evaluation of the state of the associated > +input signals as defined by the respective count functions. Within the > +context of the Generic Counter interface, a counter consists of Counts > +each associated with a set of Signals, whose respective Synapse > +instances represent the count function update conditions for the > +associated Counts. > + > +Paradigm > +======== > + > +The most basic counter device may be expressed as a single Count > +associated with a single Signal via a single Synapse. Take for example > +a counter device which simply accumulates a count of rising edges on a > +source input line:: > + > + Count Synapse Signal > + ----- ------- ------ > + +---------------------+ > + | Data: Count | Rising Edge ________ > + | Function: Increase | <------------- / Source \ > + | | ____________ > + +---------------------+ > + > +In this example, the Signal is a source input line with a pulsing > +voltage, while the Count is a persistent count value which is repeatedly > +incremented. The Signal is associated with the respective Count via a > +Synapse. The increase function is triggered by the Signal data condition > +specified by the Synapse -- in this case a rising edge condition on the > +voltage input line. In summary, the counter device existence and > +behavior is aptly represented by respective Count, Signal, and Synapse > +components: a rising edge condition triggers an increase function on an > +accumulating count datum. > + > +A counter device is not limited to a single Signal; in fact, in theory > +many Signals may be associated with even a single Count. For example, a > +quadrature encoder counter device can keep track of position based on > +the states of two input lines:: > + > + Count Synapse Signal > + ----- ------- ------ > + +-------------------------+ > + | Data: Position | Both Edges ___ > + | Function: Quadrature x4 | <------------ / A \ > + | | _______ > + | | > + | | Both Edges ___ > + | | <------------ / B \ > + | | _______ > + +-------------------------+ > + > +In this example, two Signals (quadrature encoder lines A and B) are > +associated with a single Count: a rising or falling edge on either A or > +B triggers the "Quadrature x4" function which determines the direction > +of movement and updates the respective position data. The "Quadrature > +x4" function is likely implemented in the hardware of the quadrature > +encoder counter device; the Count, Signals, and Synapses simply > +represent this hardware behavior and functionality. > + > +Signals associated with the same Count can have differing Synapse action > +mode conditions. For example, a quadrature encoder counter device > +operating in a non-quadrature Pulse-Direction mode could have one input > +line dedicated for movement and a second input line dedicated for > +direction:: > + > + Count Synapse Signal > + ----- ------- ------ > + +---------------------------+ > + | Data: Position | Rising Edge ___ > + | Function: Pulse-Direction | <------------- / A \ (Movement) > + | | _______ > + | | > + | | None ___ > + | | <------------- / B \ (Direction) > + | | _______ > + +---------------------------+ > + > +Only Signal A triggers the "Pulse-Direction" update function, but the > +instantaneous state of Signal B is still required in order to know the > +direction so that the position data may be properly updated. Ultimately, > +both Signals are associated with the same Count via two respective > +Synapses, but only one Synapse has an active action mode condition which > +triggers the respective count function while the other is left with a > +"None" condition action mode to indicate its respective Signal's > +availability for state evaluation despite its non-triggering mode. > + > +Keep in mind that the Signal, Synapse, and Count are abstract > +representations which do not need to be closely married to their > +respective physical sources. This allows the user of a counter to > +divorce themselves from the nuances of physical components (such as > +whether an input line is differential or single-ended) and instead focus > +on the core idea of what the data and process represent (e.g. position > +as interpreted from quadrature encoding data). > + > +Userspace Interface > +=================== > + > +Several sysfs attributes are generated by the Generic Counter interface, > +and reside under the /sys/bus/counter/devices/counterX directory, where > +counterX refers to the respective counter device. Please see > +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed > +information on each Generic Counter interface sysfs attribute. > + > +Through these sysfs attributes, programs and scripts may interact with > +the Generic Counter paradigm Counts, Signals, and Synapses of respective > +counter devices. > + > +Driver API > +========== > + > +Driver authors may utilize the Generic Counter interface in their code > +by including the include/linux/iio/counter.h header file. This header Didn't this move? > +file provides several core data structures, function prototypes, and > +macros for defining a counter device. > + > +.. kernel-doc:: include/linux/counter.h > + :internal: > + > +.. kernel-doc:: drivers/counter/generic-counter.c > + :export: > + > +Implementation > +============== > + > +To support a counter device, a driver must first allocate the available > +Counter Signals via counter_signal structures. These Signals should > +be stored as an array and set to the signals array member of an > +allocated counter_device structure before the Counter is registered to > +the system. > + > +Counter Counts may be allocated via counter_count structures, and > +respective Counter Signal associations (Synapses) made via > +counter_synapse structures. Associated counter_synapse structures are > +stored as an array and set to the the synapses array member of the > +respective counter_count structure. These counter_count structures are > +set to the counts array member of an allocated counter_device structure > +before the Counter is registered to the system. > + > +Driver callbacks should be provided to the counter_device structure via > +a constant counter_ops structure in order to communicate with the > +device: to read and write various Signals and Counts, and to set and get > +the "action mode" and "function mode" for various Synapses and Counts > +respectively. > + > +A defined counter_device structure may be registered to the system by > +passing it to the counter_register function, and unregistered by passing > +it to the counter_unregister function. Similarly, the > +devm_counter_register and devm_counter_unregister functions may be used > +if device memory-managed registration is desired. > + > +Extension sysfs attributes can be created for auxiliary functionality > +and data by passing in defined counter_device_ext, counter_count_ext, > +and counter_signal_ext structures. In these cases, the > +counter_device_ext structure is used for global configuration of the > +respective Counter device, while the counter_count_ext and > +counter_signal_ext structures allow for auxiliary exposure and > +configuration of a specific Count or Signal respectively. > + > +Architecture > +============ > + > +When the Generic Counter interface counter module is loaded, the > +counter_init function is called which registers a bus_type named > +"counter" to the system. Subsequently, when the module is unloaded, the > +counter_exit function is called which unregisters the bus_type named > +"counter" from the system. > + > +Counter devices are registered to the system via the counter_register > +function, and later removed via the counter_unregister function. The > +counter_register function establishes a unique ID for the Counter > +device and creates a respective sysfs directory, where X is the > +mentioned unique ID: > + > + /sys/bus/counter/devices/counterX > + > +Sysfs attributes are created within the counterX directory to expose > +functionality, configurations, and data relating to the Counts, Signals, > +and Synapses of the Counter device, as well as options and information > +for the Counter device itself. > + > +Each Signal has a directory created to house its relevant sysfs > +attributes, where Y is the unique ID of the respective Signal: > + > + /sys/bus/counter/devices/counterX/signalY > + > +Similarly, each Count has a directory created to house its relevant > +sysfs attributes, where Y is the unique ID of the respective Count: > + > + /sys/bus/counter/devices/counterX/countY > + > +For a more detailed breakdown of the available Generic Counter interface > +sysfs attributes, please refer to the > +Documentation/ABI/testing/sys-bus-counter file. > + > +The Signals and Counts associated with the Counter device are registered > +to the system as well by the counter_register function. The > +signal_read/signal_write driver callbacks are associated with their > +respective Signal attributes, while the count_read/count_write and > +function_get/function_set driver callbacks are associated with their > +respective Count attributes; similarly, the same is true for the > +action_get/action_set driver callbacks and their respective Synapse > +attributes. If a driver callback is left undefined, then the respective > +read/write permission is left disabled for the relevant attributes. > + > +Similarly, extension sysfs attributes are created for the defined > +counter_device_ext, counter_count_ext, and counter_signal_ext > +structures that are passed in. > diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst > index 6d8352c0f354..5fd747c4f2ce 100644 > --- a/Documentation/driver-api/index.rst > +++ b/Documentation/driver-api/index.rst > @@ -25,6 +25,7 @@ available subsections can be seen below. > frame-buffer > regulator > iio/index > + generic-counter > input > usb/index > pci > diff --git a/MAINTAINERS b/MAINTAINERS > index 1413e3eb49e5..7a01aa63fb33 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3674,6 +3674,7 @@ M: William Breathitt Gray <vilhelm.gray@gmail.com> > L: linux-iio@vger.kernel.org > S: Maintained > F: Documentation/ABI/testing/sysfs-bus-counter* > +F: Documentation/driver-api/generic-counter.rst > F: drivers/counter/ > F: include/linux/counter.h > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 20, 2018 at 04:31:09PM +0100, Jonathan Cameron wrote: >On Wed, 16 May 2018 13:51:06 -0400 >William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > >> This patch adds high-level documentation about the Generic Counter >> interface. >> >> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > >Various comments inline. I've been doing a lot long reviews recently >(outside of the kernel world) and keep discovering the old rule that >everytime you read a document you'll find something else to >improve :( > >Jonathan But it is good too to have multiple eyes on this -- I have found as an author my brain tends to skip over minor errors while rereading passages, so having persons reading it for both the first and subsequent times helps catch those mistakes I may have overlooked in my mind. :) >> --- >> Documentation/driver-api/generic-counter.rst | 336 +++++++++++++++++++ >> Documentation/driver-api/index.rst | 1 + >> MAINTAINERS | 1 + >> 3 files changed, 338 insertions(+) >> create mode 100644 Documentation/driver-api/generic-counter.rst >> >> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst >> new file mode 100644 >> index 000000000000..5c6b9c008c06 >> --- /dev/null >> +++ b/Documentation/driver-api/generic-counter.rst >> @@ -0,0 +1,336 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> + >> +========================= >> +Generic Counter Interface >> +========================= >> + >> +Introduction >> +============ >> + >> +Counter devices are prevalent within a diverse spectrum of industries. >> +The ubiquitous presence of these devices necessitates a common interface >> +and standard of interaction and exposure. This driver API attempts to >> +resolve the issue of duplicate code found among existing counter device >> +drivers by introducing a generic counter interface for consumption. The >> +Generic Counter interface enables drivers to support and expose a common >> +set of components and functionality present in counter devices. >> + >> +Theory >> +====== >> + >> +Counter devices can vary greatly in design, but regardless of whether >> +some devices are quadrature encoder counters or tally counters, all >> +counter devices consist of a core set of components. This core set of >> +components, shared by all counter devices, is what forms the essence of >> +the Generic Counter interface. >> + >> +There are three core components to a counter: > >Enumerate them here. If people are reading this as a paged document (pdf etc) >then the list of 3 as titles of next few sections may not be clear. > >* Count > >* Signal > >* Synapse > >> + >> +COUNT >> +----- >> +A Count represents the count data for a set of Signals. The Generic >> +Counter interface provides the following available count data types: >> + >> +* COUNT_POSITION_UNSIGNED: >> + Unsigned integer value representing position. >> + >> +* COUNT_POSITION_SIGNED: >> + Signed integer value representing position. > >Just a thought: As the '0' position is effectively arbitrary is there any >actual difference between signed and unsigned? If we defined all counters >to be unsigned and just offset any signed ones so the range still fitted >would we end up with a simpler interface - there would be no real loss >of meaning that I can see.. I suppose it might not be what people expect >though when they see their counters start at a large positive value... This is something I've been on the fence about for a while. I would actually prefer the interface to have simply a COUNT_POSITION data type to represent position and leave it as unsigned; distinguishing between signed and unsigned position seems ultimately arbitrary given that it's mathematically just an offset as you have pointed out. If we were to go down this route, then we'd have a count value that may always be represented using an unsigned data type, with an offset value that may always be represented using a signed data type -- the relationship being such: position = count + offset. Is that correct? My reason for giving the option for either signed or unsigned position was to help minimize the work userspace would have to do in order to get the value in which they're actually interested. I suppose it was a question of how abstract I want to make the interface -- although, making it simpler for userspace put more of a burden on the kernel side. However, the "offset" value route may actually be more robust in the end because userspace would have to know whether they want a signed or unsigned position regardless in order to parse, so with count and offet defined we know they will always be unsigned and signed respectively. > > > > >> + >> +A Count has a count function mode which represents the update behavior >> +for the count data. The Generic Counter interface provides the following >> +available count function modes: >> + >> +* Increase: >> + Accumulated count is incremented. >> + >> +* Decrease: >> + Accumulated count is decremented. >> + >> +* Pulse-Direction: >> + Rising edges on quadrature pair signal A updates the respective count. >> + The input level of quadrature pair signal B determines direction. >> + >Perhaps group the quadrature modes for the point of view of this document? >Might be slightly easier to read? > >* Quadrature Modes > > - x1 A: etc. > >> +* Quadrature x1 A: >> + If direction is forward, rising edges on quadrature pair signal A >> + updates the respective count; if the direction is backward, falling >> + edges on quadrature pair signal A updates the respective count. >> + Quadrature encoding determines the direction. >> + >> +* Quadrature x1 B: >> + If direction is forward, rising edges on quadrature pair signal B >> + updates the respective count; if the direction is backward, falling >> + edges on quadrature pair signal B updates the respective count. >> + Quadrature encoding determines the direction. >> + >> +* Quadrature x2 A: >> + Any state transition on quadrature pair signal A updates the >> + respective count. Quadrature encoding determines the direction. >> + >> +* Quadrature x2 B: >> + Any state transition on quadrature pair signal B updates the >> + respective count. Quadrature encoding determines the direction. >> + >> +* Quadrature x2 Rising: >> + Rising edges on either quadrature pair signals updates the respective >> + count. Quadrature encoding determines the direction. > >This one I've never met. Really? There are devices who do this form >of crazy? It gives really uneven counting and I'm failing to see when >it would ever make sense... References for these odd corner cases >would be good. > > >__|---|____|-----|____ >____|----|____|-----|____ > >001122222223334444444 That's the same reaction I had when I discovered this -- in fact the STM32 LP Timer is the first time I've come across such a quadrature mode. I'm not sure of the use case for this mode, because positioning wouldn't be precise as you've pointed out. Perhaps Fabrice or Benjamin can probe the ST guys responsible for this design choice to figure out the rationale. I'm leaving in these modes for now, as they do exist in the STM32 LP Timer, but it does make me curious what the intentions for them were (perhaps use cases outside of traditional quadrature encoder positioning). > > >> + >> +* Quadrature x2 Falling: >> + Falling edges on either quadrature pair signals updates the respective >> + count. Quadrature encoding determines the direction. >> + >> +* Quadrature x4: >> + Any state transition on either quadrature pair signals updates the >> + respective count. Quadrature encoding determines the direction. >> + >> +A Count has a set of one or more associated Signals. >> + >> +SIGNAL >> +------ >> +A Signal represents a counter input data; this is the input data that is >> +analyzed by the counter to determine the count data; e.g. a quadrature >> +signal output line of a rotary encoder. Not all counter devices provide >> +user access to the Signal data. >> + >> +The Generic Counter interface provides the following available signal >> +data types for when the Signal data is available for user access: >> + >> +* SIGNAL_LEVEL_LOW: >> + Signal line is in a low state. >> + >> +* SIGNAL_LEVEL_HIGH: >> + Signal line is in a high state. >> + >> +A Signal may be associated with one or more Counts. >> + >> +SYNAPSE >> +------- >> +A Synapse represents the association of a Signal with a respective >> +Count. Signal data affects respective Count data, and the Synapse >> +represents this relationship. >> + >> +The Synapse action mode specifies the Signal data condition which >> +triggers the respective Count's count function evaluation to update the >> +count data. The Generic Counter interface provides the following >> +available action modes: >> + >> +* None: >> + Signal does not trigger the count function. In Pulse-Direction count >> + function mode, this Signal is evaluated as Direction. >> + >> +* Rising Edge: >> + Low state transitions to high state. >> + >> +* Falling Edge: >> + High state transitions to low state. >> + >> +* Both Edges: >> + Any state transition. >> + >> +A counter is defined as a set of input signals associated with count >> +data that are generated by the evaluation of the state of the associated >> +input signals as defined by the respective count functions. Within the >> +context of the Generic Counter interface, a counter consists of Counts >> +each associated with a set of Signals, whose respective Synapse >> +instances represent the count function update conditions for the >> +associated Counts. >> + >> +Paradigm >> +======== >> + >> +The most basic counter device may be expressed as a single Count >> +associated with a single Signal via a single Synapse. Take for example >> +a counter device which simply accumulates a count of rising edges on a >> +source input line:: >> + >> + Count Synapse Signal >> + ----- ------- ------ >> + +---------------------+ >> + | Data: Count | Rising Edge ________ >> + | Function: Increase | <------------- / Source \ >> + | | ____________ >> + +---------------------+ >> + >> +In this example, the Signal is a source input line with a pulsing >> +voltage, while the Count is a persistent count value which is repeatedly >> +incremented. The Signal is associated with the respective Count via a >> +Synapse. The increase function is triggered by the Signal data condition >> +specified by the Synapse -- in this case a rising edge condition on the >> +voltage input line. In summary, the counter device existence and >> +behavior is aptly represented by respective Count, Signal, and Synapse >> +components: a rising edge condition triggers an increase function on an >> +accumulating count datum. >> + >> +A counter device is not limited to a single Signal; in fact, in theory >> +many Signals may be associated with even a single Count. For example, a >> +quadrature encoder counter device can keep track of position based on >> +the states of two input lines:: >> + >> + Count Synapse Signal >> + ----- ------- ------ >> + +-------------------------+ >> + | Data: Position | Both Edges ___ >> + | Function: Quadrature x4 | <------------ / A \ >> + | | _______ >> + | | >> + | | Both Edges ___ >> + | | <------------ / B \ >> + | | _______ >> + +-------------------------+ >> + >> +In this example, two Signals (quadrature encoder lines A and B) are >> +associated with a single Count: a rising or falling edge on either A or >> +B triggers the "Quadrature x4" function which determines the direction >> +of movement and updates the respective position data. The "Quadrature >> +x4" function is likely implemented in the hardware of the quadrature >> +encoder counter device; the Count, Signals, and Synapses simply >> +represent this hardware behavior and functionality. >> + >> +Signals associated with the same Count can have differing Synapse action >> +mode conditions. For example, a quadrature encoder counter device >> +operating in a non-quadrature Pulse-Direction mode could have one input >> +line dedicated for movement and a second input line dedicated for >> +direction:: >> + >> + Count Synapse Signal >> + ----- ------- ------ >> + +---------------------------+ >> + | Data: Position | Rising Edge ___ >> + | Function: Pulse-Direction | <------------- / A \ (Movement) >> + | | _______ >> + | | >> + | | None ___ >> + | | <------------- / B \ (Direction) >> + | | _______ >> + +---------------------------+ >> + >> +Only Signal A triggers the "Pulse-Direction" update function, but the >> +instantaneous state of Signal B is still required in order to know the >> +direction so that the position data may be properly updated. Ultimately, >> +both Signals are associated with the same Count via two respective >> +Synapses, but only one Synapse has an active action mode condition which >> +triggers the respective count function while the other is left with a >> +"None" condition action mode to indicate its respective Signal's >> +availability for state evaluation despite its non-triggering mode. >> + >> +Keep in mind that the Signal, Synapse, and Count are abstract >> +representations which do not need to be closely married to their >> +respective physical sources. This allows the user of a counter to >> +divorce themselves from the nuances of physical components (such as >> +whether an input line is differential or single-ended) and instead focus >> +on the core idea of what the data and process represent (e.g. position >> +as interpreted from quadrature encoding data). >> + >> +Userspace Interface >> +=================== >> + >> +Several sysfs attributes are generated by the Generic Counter interface, >> +and reside under the /sys/bus/counter/devices/counterX directory, where >> +counterX refers to the respective counter device. Please see >> +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed >> +information on each Generic Counter interface sysfs attribute. >> + >> +Through these sysfs attributes, programs and scripts may interact with >> +the Generic Counter paradigm Counts, Signals, and Synapses of respective >> +counter devices. >> + >> +Driver API >> +========== >> + >> +Driver authors may utilize the Generic Counter interface in their code >> +by including the include/linux/iio/counter.h header file. This header > >Didn't this move? Yes you are correct, looks like an oversight I made. I'll cleanup this and the rest with the next revision then. William Breathitt Gray > >> +file provides several core data structures, function prototypes, and >> +macros for defining a counter device. >> + >> +.. kernel-doc:: include/linux/counter.h >> + :internal: >> + >> +.. kernel-doc:: drivers/counter/generic-counter.c >> + :export: >> + >> +Implementation >> +============== >> + >> +To support a counter device, a driver must first allocate the available >> +Counter Signals via counter_signal structures. These Signals should >> +be stored as an array and set to the signals array member of an >> +allocated counter_device structure before the Counter is registered to >> +the system. >> + >> +Counter Counts may be allocated via counter_count structures, and >> +respective Counter Signal associations (Synapses) made via >> +counter_synapse structures. Associated counter_synapse structures are >> +stored as an array and set to the the synapses array member of the >> +respective counter_count structure. These counter_count structures are >> +set to the counts array member of an allocated counter_device structure >> +before the Counter is registered to the system. >> + >> +Driver callbacks should be provided to the counter_device structure via >> +a constant counter_ops structure in order to communicate with the >> +device: to read and write various Signals and Counts, and to set and get >> +the "action mode" and "function mode" for various Synapses and Counts >> +respectively. >> + >> +A defined counter_device structure may be registered to the system by >> +passing it to the counter_register function, and unregistered by passing >> +it to the counter_unregister function. Similarly, the >> +devm_counter_register and devm_counter_unregister functions may be used >> +if device memory-managed registration is desired. >> + >> +Extension sysfs attributes can be created for auxiliary functionality >> +and data by passing in defined counter_device_ext, counter_count_ext, >> +and counter_signal_ext structures. In these cases, the >> +counter_device_ext structure is used for global configuration of the >> +respective Counter device, while the counter_count_ext and >> +counter_signal_ext structures allow for auxiliary exposure and >> +configuration of a specific Count or Signal respectively. >> + >> +Architecture >> +============ >> + >> +When the Generic Counter interface counter module is loaded, the >> +counter_init function is called which registers a bus_type named >> +"counter" to the system. Subsequently, when the module is unloaded, the >> +counter_exit function is called which unregisters the bus_type named >> +"counter" from the system. >> + >> +Counter devices are registered to the system via the counter_register >> +function, and later removed via the counter_unregister function. The >> +counter_register function establishes a unique ID for the Counter >> +device and creates a respective sysfs directory, where X is the >> +mentioned unique ID: >> + >> + /sys/bus/counter/devices/counterX >> + >> +Sysfs attributes are created within the counterX directory to expose >> +functionality, configurations, and data relating to the Counts, Signals, >> +and Synapses of the Counter device, as well as options and information >> +for the Counter device itself. >> + >> +Each Signal has a directory created to house its relevant sysfs >> +attributes, where Y is the unique ID of the respective Signal: >> + >> + /sys/bus/counter/devices/counterX/signalY >> + >> +Similarly, each Count has a directory created to house its relevant >> +sysfs attributes, where Y is the unique ID of the respective Count: >> + >> + /sys/bus/counter/devices/counterX/countY >> + >> +For a more detailed breakdown of the available Generic Counter interface >> +sysfs attributes, please refer to the >> +Documentation/ABI/testing/sys-bus-counter file. >> + >> +The Signals and Counts associated with the Counter device are registered >> +to the system as well by the counter_register function. The >> +signal_read/signal_write driver callbacks are associated with their >> +respective Signal attributes, while the count_read/count_write and >> +function_get/function_set driver callbacks are associated with their >> +respective Count attributes; similarly, the same is true for the >> +action_get/action_set driver callbacks and their respective Synapse >> +attributes. If a driver callback is left undefined, then the respective >> +read/write permission is left disabled for the relevant attributes. >> + >> +Similarly, extension sysfs attributes are created for the defined >> +counter_device_ext, counter_count_ext, and counter_signal_ext >> +structures that are passed in. >> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst >> index 6d8352c0f354..5fd747c4f2ce 100644 >> --- a/Documentation/driver-api/index.rst >> +++ b/Documentation/driver-api/index.rst >> @@ -25,6 +25,7 @@ available subsections can be seen below. >> frame-buffer >> regulator >> iio/index >> + generic-counter >> input >> usb/index >> pci >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 1413e3eb49e5..7a01aa63fb33 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3674,6 +3674,7 @@ M: William Breathitt Gray <vilhelm.gray@gmail.com> >> L: linux-iio@vger.kernel.org >> S: Maintained >> F: Documentation/ABI/testing/sysfs-bus-counter* >> +F: Documentation/driver-api/generic-counter.rst >> F: drivers/counter/ >> F: include/linux/counter.h >> > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
... > >> + > >> +COUNT > >> +----- > >> +A Count represents the count data for a set of Signals. The Generic > >> +Counter interface provides the following available count data types: > >> + > >> +* COUNT_POSITION_UNSIGNED: > >> + Unsigned integer value representing position. > >> + > >> +* COUNT_POSITION_SIGNED: > >> + Signed integer value representing position. > > > >Just a thought: As the '0' position is effectively arbitrary is there any > >actual difference between signed and unsigned? If we defined all counters > >to be unsigned and just offset any signed ones so the range still fitted > >would we end up with a simpler interface - there would be no real loss > >of meaning that I can see.. I suppose it might not be what people expect > >though when they see their counters start at a large positive value... > > This is something I've been on the fence about for a while. I would > actually prefer the interface to have simply a COUNT_POSITION data type > to represent position and leave it as unsigned; distinguishing between > signed and unsigned position seems ultimately arbitrary given that it's > mathematically just an offset as you have pointed out. > > If we were to go down this route, then we'd have a count value that may > always be represented using an unsigned data type, with an offset value > that may always be represented using a signed data type -- the > relationship being such: position = count + offset. Is that correct? Given the positions are all arbitrary (such as limits etc) there is no real need to have 0 as in anyway special. So you could just apply an offset to everything then don't make them visible to userspace at all. > > My reason for giving the option for either signed or unsigned position > was to help minimize the work userspace would have to do in order to get > the value in which they're actually interested. I suppose it was a > question of how abstract I want to make the interface -- although, > making it simpler for userspace put more of a burden on the kernel side. > > However, the "offset" value route may actually be more robust in the end > because userspace would have to know whether they want a signed or > unsigned position regardless in order to parse, so with count and offet > defined we know they will always be unsigned and signed respectively. Hmm. It's not obvious to me which is the best option. > > > > > > > > > > >> + > >> +A Count has a count function mode which represents the update behavior > >> +for the count data. The Generic Counter interface provides the following > >> +available count function modes: > >> + > >> +* Increase: > >> + Accumulated count is incremented. > >> + > >> +* Decrease: > >> + Accumulated count is decremented. > >> + > >> +* Pulse-Direction: > >> + Rising edges on quadrature pair signal A updates the respective count. > >> + The input level of quadrature pair signal B determines direction. > >> + > >Perhaps group the quadrature modes for the point of view of this document? > >Might be slightly easier to read? > > > >* Quadrature Modes > > > > - x1 A: etc. > > > >> +* Quadrature x1 A: > >> + If direction is forward, rising edges on quadrature pair signal A > >> + updates the respective count; if the direction is backward, falling > >> + edges on quadrature pair signal A updates the respective count. > >> + Quadrature encoding determines the direction. > >> + > >> +* Quadrature x1 B: > >> + If direction is forward, rising edges on quadrature pair signal B > >> + updates the respective count; if the direction is backward, falling > >> + edges on quadrature pair signal B updates the respective count. > >> + Quadrature encoding determines the direction. > >> + > >> +* Quadrature x2 A: > >> + Any state transition on quadrature pair signal A updates the > >> + respective count. Quadrature encoding determines the direction. > >> + > >> +* Quadrature x2 B: > >> + Any state transition on quadrature pair signal B updates the > >> + respective count. Quadrature encoding determines the direction. > >> + > >> +* Quadrature x2 Rising: > >> + Rising edges on either quadrature pair signals updates the respective > >> + count. Quadrature encoding determines the direction. > > > >This one I've never met. Really? There are devices who do this form > >of crazy? It gives really uneven counting and I'm failing to see when > >it would ever make sense... References for these odd corner cases > >would be good. > > > > > >__|---|____|-----|____ > >____|----|____|-----|____ > > > >001122222223334444444 > > That's the same reaction I had when I discovered this -- in fact the > STM32 LP Timer is the first time I've come across such a quadrature > mode. I'm not sure of the use case for this mode, because positioning > wouldn't be precise as you've pointed out. Perhaps Fabrice or Benjamin > can probe the ST guys responsible for this design choice to figure out > the rationale. Hmm. My inclination would be to not support it unless someone can up with a meaningful use. We are adding ABI (be it not much) for a case that to us makes no sense. Looks rather like the sort of thing that is a side effect of the implementation rather than deliberate. > > I'm leaving in these modes for now, as they do exist in the STM32 LP > Timer, but it does make me curious what the intentions for them were > (perhaps use cases outside of traditional quadrature encoder > positioning). > Sure if there is a usecase then fair enough. Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/22/2018 07:08 PM, Jonathan Cameron wrote: >>>> +* Quadrature x2 Rising: >>>> + Rising edges on either quadrature pair signals updates the respective >>>> + count. Quadrature encoding determines the direction. >>> This one I've never met. Really? There are devices who do this form >>> of crazy? It gives really uneven counting and I'm failing to see when >>> it would ever make sense... References for these odd corner cases >>> would be good. >>> >>> >>> __|---|____|-----|____ >>> ____|----|____|-----|____ >>> >>> 001122222223334444444 >> That's the same reaction I had when I discovered this -- in fact the >> STM32 LP Timer is the first time I've come across such a quadrature >> mode. I'm not sure of the use case for this mode, because positioning >> wouldn't be precise as you've pointed out. Perhaps Fabrice or Benjamin >> can probe the ST guys responsible for this design choice to figure out >> the rationale. > Hmm. My inclination would be to not support it unless someone can up > with a meaningful use. We are adding ABI (be it not much) for a case > that to us makes no sense. Hi Jonathan, William, Sorry for the late reply. To follow your advise, we can probably drop this for now. I think simple counter, or quadrature x4 will be mostly used for now. As you pointed out, there's not much ABI for x2 rising/falling cases. It will not be a big deal to add it later if needed. I can help to update (remove & test) this in LP-Timer counter driver if you wish. Please let me know, Thanks, Fabrice > > Looks rather like the sort of thing that is a side effect of the > implementation rather than deliberate. > >> I'm leaving in these modes for now, as they do exist in the STM32 LP >> Timer, but it does make me curious what the intentions for them were >> (perhaps use cases outside of traditional quadrature encoder >> positioning). >> > Sure if there is a usecase then fair enough. > > Jonathan > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 25, 2018 at 11:26:11AM +0200, Fabrice Gasnier wrote: >On 05/22/2018 07:08 PM, Jonathan Cameron wrote: >>>>> +* Quadrature x2 Rising: >>>>> + Rising edges on either quadrature pair signals updates the respective >>>>> + count. Quadrature encoding determines the direction. >>>> This one I've never met. Really? There are devices who do this form >>>> of crazy? It gives really uneven counting and I'm failing to see when >>>> it would ever make sense... References for these odd corner cases >>>> would be good. >>>> >>>> >>>> __|---|____|-----|____ >>>> ____|----|____|-----|____ >>>> >>>> 001122222223334444444 >>> That's the same reaction I had when I discovered this -- in fact the >>> STM32 LP Timer is the first time I've come across such a quadrature >>> mode. I'm not sure of the use case for this mode, because positioning >>> wouldn't be precise as you've pointed out. Perhaps Fabrice or Benjamin >>> can probe the ST guys responsible for this design choice to figure out >>> the rationale. >> Hmm. My inclination would be to not support it unless someone can up >> with a meaningful use. We are adding ABI (be it not much) for a case >> that to us makes no sense. > >Hi Jonathan, William, > >Sorry for the late reply. To follow your advise, we can probably drop >this for now. I think simple counter, or quadrature x4 will be mostly >used for now. As you pointed out, there's not much ABI for x2 >rising/falling cases. It will not be a big deal to add it later if needed. > >I can help to update (remove & test) this in LP-Timer counter driver if >you wish. > >Please let me know, > >Thanks, >Fabrice All right, let's postpone the COUNT_FUNCTION_QUADRATURE_X2_RISING and COUNT_FUNCTION_QUADRATURE_X2_FALLING modes for now. Fabrice, send me over an update patch removing these modes from the LP-Timer counter driver and I'll squash it in with the next patchset revision. I'll keep the rest of the quadrature modes the same as they are used in the other counter drivers as well (with the remaining "Quadrature x1 B" staying to complete the pattern) and I've seen real world use cases for each: * COUNT_FUNCTION_QUADRATURE_X1_A * COUNT_FUNCTION_QUADRATURE_X1_B * COUNT_FUNCTION_QUADRATURE_X2_A * COUNT_FUNCTION_QUADRATURE_X2_B Adding support in the future for "Quadrature x2 Rising" and "Quadrature x2 Falling" will be trivial, so really the main requirement in order to bring these modes back is to provide reasonable use cases for them. My suspicion is that there was some rationale for these Quadrature x2 modes in the STM32 LP-Timer -- afterall, why else would the engineers go through the trouble of designing and implementing it -- but until that use case is clear, it's best to wait on changing the Generic Counter ABI lest we end up with an interface that is never used in the real world. William Breathitt Gray > >> >> Looks rather like the sort of thing that is a side effect of the >> implementation rather than deliberate. >> >>> I'm leaving in these modes for now, as they do exist in the STM32 LP >>> Timer, but it does make me curious what the intentions for them were >>> (perhaps use cases outside of traditional quadrature encoder >>> positioning). >>> >> Sure if there is a usecase then fair enough. >> >> Jonathan >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst new file mode 100644 index 000000000000..5c6b9c008c06 --- /dev/null +++ b/Documentation/driver-api/generic-counter.rst @@ -0,0 +1,336 @@ +.. SPDX-License-Identifier: GPL-2.0 + +========================= +Generic Counter Interface +========================= + +Introduction +============ + +Counter devices are prevalent within a diverse spectrum of industries. +The ubiquitous presence of these devices necessitates a common interface +and standard of interaction and exposure. This driver API attempts to +resolve the issue of duplicate code found among existing counter device +drivers by introducing a generic counter interface for consumption. The +Generic Counter interface enables drivers to support and expose a common +set of components and functionality present in counter devices. + +Theory +====== + +Counter devices can vary greatly in design, but regardless of whether +some devices are quadrature encoder counters or tally counters, all +counter devices consist of a core set of components. This core set of +components, shared by all counter devices, is what forms the essence of +the Generic Counter interface. + +There are three core components to a counter: + +COUNT +----- +A Count represents the count data for a set of Signals. The Generic +Counter interface provides the following available count data types: + +* COUNT_POSITION_UNSIGNED: + Unsigned integer value representing position. + +* COUNT_POSITION_SIGNED: + Signed integer value representing position. + +A Count has a count function mode which represents the update behavior +for the count data. The Generic Counter interface provides the following +available count function modes: + +* Increase: + Accumulated count is incremented. + +* Decrease: + Accumulated count is decremented. + +* Pulse-Direction: + Rising edges on quadrature pair signal A updates the respective count. + The input level of quadrature pair signal B determines direction. + +* Quadrature x1 A: + If direction is forward, rising edges on quadrature pair signal A + updates the respective count; if the direction is backward, falling + edges on quadrature pair signal A updates the respective count. + Quadrature encoding determines the direction. + +* Quadrature x1 B: + If direction is forward, rising edges on quadrature pair signal B + updates the respective count; if the direction is backward, falling + edges on quadrature pair signal B updates the respective count. + Quadrature encoding determines the direction. + +* Quadrature x2 A: + Any state transition on quadrature pair signal A updates the + respective count. Quadrature encoding determines the direction. + +* Quadrature x2 B: + Any state transition on quadrature pair signal B updates the + respective count. Quadrature encoding determines the direction. + +* Quadrature x2 Rising: + Rising edges on either quadrature pair signals updates the respective + count. Quadrature encoding determines the direction. + +* Quadrature x2 Falling: + Falling edges on either quadrature pair signals updates the respective + count. Quadrature encoding determines the direction. + +* Quadrature x4: + Any state transition on either quadrature pair signals updates the + respective count. Quadrature encoding determines the direction. + +A Count has a set of one or more associated Signals. + +SIGNAL +------ +A Signal represents a counter input data; this is the input data that is +analyzed by the counter to determine the count data; e.g. a quadrature +signal output line of a rotary encoder. Not all counter devices provide +user access to the Signal data. + +The Generic Counter interface provides the following available signal +data types for when the Signal data is available for user access: + +* SIGNAL_LEVEL_LOW: + Signal line is in a low state. + +* SIGNAL_LEVEL_HIGH: + Signal line is in a high state. + +A Signal may be associated with one or more Counts. + +SYNAPSE +------- +A Synapse represents the association of a Signal with a respective +Count. Signal data affects respective Count data, and the Synapse +represents this relationship. + +The Synapse action mode specifies the Signal data condition which +triggers the respective Count's count function evaluation to update the +count data. The Generic Counter interface provides the following +available action modes: + +* None: + Signal does not trigger the count function. In Pulse-Direction count + function mode, this Signal is evaluated as Direction. + +* Rising Edge: + Low state transitions to high state. + +* Falling Edge: + High state transitions to low state. + +* Both Edges: + Any state transition. + +A counter is defined as a set of input signals associated with count +data that are generated by the evaluation of the state of the associated +input signals as defined by the respective count functions. Within the +context of the Generic Counter interface, a counter consists of Counts +each associated with a set of Signals, whose respective Synapse +instances represent the count function update conditions for the +associated Counts. + +Paradigm +======== + +The most basic counter device may be expressed as a single Count +associated with a single Signal via a single Synapse. Take for example +a counter device which simply accumulates a count of rising edges on a +source input line:: + + Count Synapse Signal + ----- ------- ------ + +---------------------+ + | Data: Count | Rising Edge ________ + | Function: Increase | <------------- / Source \ + | | ____________ + +---------------------+ + +In this example, the Signal is a source input line with a pulsing +voltage, while the Count is a persistent count value which is repeatedly +incremented. The Signal is associated with the respective Count via a +Synapse. The increase function is triggered by the Signal data condition +specified by the Synapse -- in this case a rising edge condition on the +voltage input line. In summary, the counter device existence and +behavior is aptly represented by respective Count, Signal, and Synapse +components: a rising edge condition triggers an increase function on an +accumulating count datum. + +A counter device is not limited to a single Signal; in fact, in theory +many Signals may be associated with even a single Count. For example, a +quadrature encoder counter device can keep track of position based on +the states of two input lines:: + + Count Synapse Signal + ----- ------- ------ + +-------------------------+ + | Data: Position | Both Edges ___ + | Function: Quadrature x4 | <------------ / A \ + | | _______ + | | + | | Both Edges ___ + | | <------------ / B \ + | | _______ + +-------------------------+ + +In this example, two Signals (quadrature encoder lines A and B) are +associated with a single Count: a rising or falling edge on either A or +B triggers the "Quadrature x4" function which determines the direction +of movement and updates the respective position data. The "Quadrature +x4" function is likely implemented in the hardware of the quadrature +encoder counter device; the Count, Signals, and Synapses simply +represent this hardware behavior and functionality. + +Signals associated with the same Count can have differing Synapse action +mode conditions. For example, a quadrature encoder counter device +operating in a non-quadrature Pulse-Direction mode could have one input +line dedicated for movement and a second input line dedicated for +direction:: + + Count Synapse Signal + ----- ------- ------ + +---------------------------+ + | Data: Position | Rising Edge ___ + | Function: Pulse-Direction | <------------- / A \ (Movement) + | | _______ + | | + | | None ___ + | | <------------- / B \ (Direction) + | | _______ + +---------------------------+ + +Only Signal A triggers the "Pulse-Direction" update function, but the +instantaneous state of Signal B is still required in order to know the +direction so that the position data may be properly updated. Ultimately, +both Signals are associated with the same Count via two respective +Synapses, but only one Synapse has an active action mode condition which +triggers the respective count function while the other is left with a +"None" condition action mode to indicate its respective Signal's +availability for state evaluation despite its non-triggering mode. + +Keep in mind that the Signal, Synapse, and Count are abstract +representations which do not need to be closely married to their +respective physical sources. This allows the user of a counter to +divorce themselves from the nuances of physical components (such as +whether an input line is differential or single-ended) and instead focus +on the core idea of what the data and process represent (e.g. position +as interpreted from quadrature encoding data). + +Userspace Interface +=================== + +Several sysfs attributes are generated by the Generic Counter interface, +and reside under the /sys/bus/counter/devices/counterX directory, where +counterX refers to the respective counter device. Please see +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed +information on each Generic Counter interface sysfs attribute. + +Through these sysfs attributes, programs and scripts may interact with +the Generic Counter paradigm Counts, Signals, and Synapses of respective +counter devices. + +Driver API +========== + +Driver authors may utilize the Generic Counter interface in their code +by including the include/linux/iio/counter.h header file. This header +file provides several core data structures, function prototypes, and +macros for defining a counter device. + +.. kernel-doc:: include/linux/counter.h + :internal: + +.. kernel-doc:: drivers/counter/generic-counter.c + :export: + +Implementation +============== + +To support a counter device, a driver must first allocate the available +Counter Signals via counter_signal structures. These Signals should +be stored as an array and set to the signals array member of an +allocated counter_device structure before the Counter is registered to +the system. + +Counter Counts may be allocated via counter_count structures, and +respective Counter Signal associations (Synapses) made via +counter_synapse structures. Associated counter_synapse structures are +stored as an array and set to the the synapses array member of the +respective counter_count structure. These counter_count structures are +set to the counts array member of an allocated counter_device structure +before the Counter is registered to the system. + +Driver callbacks should be provided to the counter_device structure via +a constant counter_ops structure in order to communicate with the +device: to read and write various Signals and Counts, and to set and get +the "action mode" and "function mode" for various Synapses and Counts +respectively. + +A defined counter_device structure may be registered to the system by +passing it to the counter_register function, and unregistered by passing +it to the counter_unregister function. Similarly, the +devm_counter_register and devm_counter_unregister functions may be used +if device memory-managed registration is desired. + +Extension sysfs attributes can be created for auxiliary functionality +and data by passing in defined counter_device_ext, counter_count_ext, +and counter_signal_ext structures. In these cases, the +counter_device_ext structure is used for global configuration of the +respective Counter device, while the counter_count_ext and +counter_signal_ext structures allow for auxiliary exposure and +configuration of a specific Count or Signal respectively. + +Architecture +============ + +When the Generic Counter interface counter module is loaded, the +counter_init function is called which registers a bus_type named +"counter" to the system. Subsequently, when the module is unloaded, the +counter_exit function is called which unregisters the bus_type named +"counter" from the system. + +Counter devices are registered to the system via the counter_register +function, and later removed via the counter_unregister function. The +counter_register function establishes a unique ID for the Counter +device and creates a respective sysfs directory, where X is the +mentioned unique ID: + + /sys/bus/counter/devices/counterX + +Sysfs attributes are created within the counterX directory to expose +functionality, configurations, and data relating to the Counts, Signals, +and Synapses of the Counter device, as well as options and information +for the Counter device itself. + +Each Signal has a directory created to house its relevant sysfs +attributes, where Y is the unique ID of the respective Signal: + + /sys/bus/counter/devices/counterX/signalY + +Similarly, each Count has a directory created to house its relevant +sysfs attributes, where Y is the unique ID of the respective Count: + + /sys/bus/counter/devices/counterX/countY + +For a more detailed breakdown of the available Generic Counter interface +sysfs attributes, please refer to the +Documentation/ABI/testing/sys-bus-counter file. + +The Signals and Counts associated with the Counter device are registered +to the system as well by the counter_register function. The +signal_read/signal_write driver callbacks are associated with their +respective Signal attributes, while the count_read/count_write and +function_get/function_set driver callbacks are associated with their +respective Count attributes; similarly, the same is true for the +action_get/action_set driver callbacks and their respective Synapse +attributes. If a driver callback is left undefined, then the respective +read/write permission is left disabled for the relevant attributes. + +Similarly, extension sysfs attributes are created for the defined +counter_device_ext, counter_count_ext, and counter_signal_ext +structures that are passed in. diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index 6d8352c0f354..5fd747c4f2ce 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -25,6 +25,7 @@ available subsections can be seen below. frame-buffer regulator iio/index + generic-counter input usb/index pci diff --git a/MAINTAINERS b/MAINTAINERS index 1413e3eb49e5..7a01aa63fb33 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3674,6 +3674,7 @@ M: William Breathitt Gray <vilhelm.gray@gmail.com> L: linux-iio@vger.kernel.org S: Maintained F: Documentation/ABI/testing/sysfs-bus-counter* +F: Documentation/driver-api/generic-counter.rst F: drivers/counter/ F: include/linux/counter.h
This patch adds high-level documentation about the Generic Counter interface. Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- Documentation/driver-api/generic-counter.rst | 336 +++++++++++++++++++ Documentation/driver-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 338 insertions(+) create mode 100644 Documentation/driver-api/generic-counter.rst