Re: [PATCH v4 00/11] Introduce the Counter subsystem

[Jonathan Cameron]

On Mon, 1 Jan 2018 11:16:30 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

Sorry to top post but I just want to add some general comments across
the whole series.

1) Basics look good to me.  It all fits together well.
2) I'm concerned about the two 'simplified interfaces' for a couple of reasons:
a) They add a lot of code and I'm not convinced the simplifications justify that.
b) They aren't as generically applicable as we might want. For example, it's
common for SoC Encoder blocks to operative in both the simple counter mode and
the quadrature counter mode.  To support that we have (I think) to go back to
basics and do it ourselves from the generic counter interface.  The TI eQEP
IP does these modes for example.

So these simplifications serve two purposes (I think)
1) To enforce interface.  This is nice (and I did some similar stuff in IIO
   for the same reason) but there is so much flexibility in the rest of the
   interface (from a code point of view) that I'm unsure this is significant.
2) To save on boiler plate.  I'm not sure we save that much and that it couldn't
   mostly be achieved by providing some useful utility functions and
   standard enums / string arrays for the common cases.

We have to justify a couple of thousand lines of core code.  To do that we
need to be saving a reasonably multiple more than that in driver code.

The full setup for a generic_counter is not so complex that we need this
stuff.  Your examples make it all pretty clear what is going on and a
couple of clean well commented drivers to act as a baseline for new
implementations would get us much of the rest of the way.

So going well, but this aspect needs some more consideration.

I also think we need at least rough outlines of a few more drivers
in here to convince people that there aren't any problems that this
is too inflexible to cover.  Hopefully an ST one will be forthcoming.
If not we can do the exercise off datasheets.

Jonathan

> On Thu, 14 Dec 2017 15:50:29 -0500
> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
> 
> > Introduction
> > ============
> > 
> > Apologies for going silent these past couple months just to return with
> > a patchset over 3000 lines larger than the last -- I should have been
> > releasing intermediate versions along the way so shame on me!   
> 
> :) Sometimes it's better to wait until you are moderately happy with it
> yourself!
> 
> > The
> > Counter system has effectively been rewritten anew, so I believe very
> > little of the code in the previous versions of this patchset remain.
> > However, the Generic Counter paradigm has pretty much remained the same
> > so the theory should be familar. Regardless, I realize I'm dropping off
> > this patchset near the winter holidays so I don't expect a review until
> > well into January -- I'm just releasing this now before I myself head
> > off on an end of year sabbatical.  
> 
> It's at least a few hours into January so here goes before life gets
> properly busy again.
> 
> > 
> > The most significant difference between this version and the previous,
> > as well as part of the reason for the implementation code changes, is
> > the complete separation of the Generic Counter system from IIO. I
> > decided it was improper to build the Generic Counter system on top of
> > IIO core: it was leading to ugly code, convulted hacks, and forced
> > restrictions on the Generic Counter interface in order to appease the
> > architecture of the IIO system. Most importantly, the IIO core code that
> > was leveraged by the Generic Counter was so minor (essentially just the
> > sysfs attribute support) that it did not justify the extensive
> > hoop-jumping performed to make the code work.
> > 
> > So this patchset introduces the Generic Counter interface without the
> > dependence on IIO code. This now gives the Generic Counter system the
> > freedom to aptly represent counter devices without implementation
> > compatibility concerns regarding other high-level subsystems.
> > 
> > This also makes sense ontologically I believe because whereas the IIO
> > system appears more focused on representing the industrial I/O of a
> > device and their configuration directly, the Generic Counter system is
> > more concerned with the abstract representation of that counter device
> > and the relationships and configurations within which define its
> > operation at a high-level; a counter driver could in theory separately
> > support both the high-level Generic Counter representation of the device
> > as a whole (what are we counting conceptually, how much are we counting,
> > etc.), as well as the low-level IIO representation of the individual
> > inputs and outputs on that device (are the signals differential, do
> > certain signals have current requirements, etc.).  
> 
> I think there are concepts that over time may blur the lines more
> but agree with the basic point. I'm just planning to nick all your
> good ideas if they will improve IIO in turn.
> 
> > 
> > Overview
> > ========
> > 
> > This patchset may be divided into three main groups:
> > 
> >         * Generic Counter
> >         * Simple Counter
> >         * Quadrature Counter
> > 
> > Each group begins with a patch introducing the implementation of the
> > interface system, followed afterwards by documentation patches. I
> > recommend reading through the documentation patches first to familiarize
> > your with the interface itself before jumping into the source code for
> > the implementation.
> > 
> > The Simple Counter and Quadrature Counter groups also have example
> > driver code in the dummy-counter and 104-quad-8 patches respectively.
> > The Simple Counter and Quadrature Counter systems themselves being
> > subclasses of the Generic Counter may serve as example driver code for
> > the Generic Counter interface -- though I may end up adding an explicit
> > Generic Counter example in a later patch to the dummy-counter for easier
> > reference.
> > 
> > Since the Generic Counter system no longer depends on IIO, I moved all
> > Counter related source code to the drivers/iio/counter/ directory to
> > keep everything contained together. In addition, with the IIO Kconfig
> > dependency removed, the COUNTER menu appear now appears at the same
> > level as the IIO menu:
> >   
> >         -> Device drivers
> >           -> Counter Support (COUNTER [=m])    
> > 
> > I'm not sure if I should move driver/iio/counter/ to driver/counter/ in
> > order to match the Kconfig heirarchy or to keep it where it is to match
> > the legacy IIO counter location established when we first added the
> > 104-QUAD-8 driver.  
> 
> I would move it out entirely - otherwise things are just confusing.
> You 'could' sit it in IIO (as in put it under the top level menu option)
> if you would prefer but I don't thing that really makes sense.
> 
> > 
> > Paradigm updates
> > ================
> > 
> > The Generic Counter paradigm has essentially remained the same from the
> > previous patch, but I have made some minor updates. In particular, I've
> > finally settled on a naming convention for the core components of a
> > Counter:
> > 
> >         COUNT
> >         -----
> >         A Count represents the count data for a set of Signals. A Count
> >         has a count function mode (e.g. "increase" or "quadrature x4")
> >         which represents the update behavior for the count data. A Count
> > 	also has a set of one or more associated Signals.
> > 
> > This component was called "Value" in the previous patches. I believe
> > "Count" is a more direct name for this data, and it also matches how
> > datasheets and people commonly refer to this information in
> > documentation.  
> 
> Agreed - better name.
> 
> > 
> >         SIGNAL
> >         ------
> >         A Signal represents a counter input data; this is the data that
> >         is typically analyzed by the counter to determine the count
> >         data. A Signal may be associated to one or more Counts.
> > 
> > The naming for this component has not changed since the previous
> > patches. I believe "Signal" is a fitting enough name for the input
> > data, as well as matching the common nomenclature for existing counter
> > devices.
> > 
> >         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 (e.g. "rising edge" or "double pulse") specifies the Signal
> >         data condition which triggers the respective Count's count
> >         function evaluation to update the count data. It is possible for
> >         the Synapse action mode to be "none" if a Signal is associated
> >         with a Count but does not trigger the count function (e.g. the
> >         direction signal line for a Pulse-Direction encoding counter).
> > 
> > This component was called "Trigger" in the previous patches. I do not
> > believe "Trigger" was a good name for two main reasons: it could easily
> > be confused for the existing IIO trigger concept, and most importantly
> > it does not convey the connection association aspect of the
> > Count-Signal relationship.  
> 
> An alternative here would be to use MAP as a number of similar
> 'connection' type arrangements in the kernel do. It doesn't really
> imply the 'how' element though so perhaps a new term is indeed better.
> 
> 
> > 
> > I settled on the "Synapse" name both due to etymology -- from Greek
> > _sunapsis_ meaning "conjunction" -- as well as a biological analogy:
> > just as neurons connect and fire communication over synapses, so does a
> > Counter Signal connect and fire communication to a Counter Count over a
> > Counter Synapse.
> > 
> > Following the same naming convention and analogy, what was previously
> > called trigger_mode is now known as action_mode, named in reference to
> > action potential -- the condition in a neuron which triggers a fire
> > communication over a synapse, just as a Counter Signal condition
> > specified in the action_mode of a Counter Synapse triggers the count
> > function evaluation for a Counter Count.
> > 
> > Counter classes descriptions
> > ============================
> > 
> > The Generic Counter interface is the most general interface for
> > supporting counter devices; if it qualifies as a Counter, then it can be
> > represented by the Generic Counter interface. Unfortunately, the
> > flexibility of the interface does result in a more cumbersome
> > integration for driver authors: much of the components must be manually
> > configured by the author, which can be a tedious task for large and
> > complex counter devices.
> > 
> > To this end, two subclasses of the Generic Counter interface as
> > introduced in this patchset: the Simple Counter interface, and the
> > Quadrature Counter interface. Both of these interfaces inherit the
> > Generic Counter paradigm, and may be seen as extensions to the interface
> > which restrict the components to a respective specific class of counter
> > devices in order to provide a more apt interface for such devices.
> > 
> >         Simple Counter
> >         --------------
> >         Simple Counters are devices that count edge pulses on an input
> >         line (e.g. tally counters).
> >         
> >         Since the relationship between Signals and Counts is known to be
> >         one-to-one, a simple_counter_count structure already contains
> >         the associated Signal member for the respective Count. A driver
> >         author no longer needs to worry about allocating a separate
> >         Signal and Synapse, nor about configuring the association
> >         between the respective Count and Signal; the Simple Counter
> >         interface abstracts away such details.
> > 
> >         Furthermore, since the device type is known, component
> >         properties may be further defined and restricted: Count data is
> >         a signed integer, Signal data "low" and "high" state is set via
> >         enumeration constants, and so are count function and action mode
> >         restricted to well-defined "increase"/"decrease" and
> >         "none"/"rising edge"/"falling edge"/"both edges" enumeration
> >         constants respectively.  
> 
> I do wonder a little on whether this is too restrictive to actually
> represent many devices.
> > 
> >         Quadrature Counter
> >         ------------------
> >         Quadrature Counters are devices that track position based on
> >         quadrature pair signals (e.g. rotary encoder counters).
> > 
> >         Since the relationship between Signals and Counts is known to be
> >         a quadrature pair of Signals to each Count, a quad_counter_count
> >         structure already contains the associated Signal members for the
> >         respective Count. A driver author no longer needs to worry about
> >         allocating separate Signals and Synapses for each quadrature
> >         pair, nor about configuring the association between the
> >         respective Count and Signals; the Quadrature Counter interface
> >         abstracts away such details.
> > 
> >         Furthermore, since the device type is known, component
> >         properties may be further defined and restricted: Count data is
> >         a signed integer, Signal data "low" and "high" state is set via
> >         enumeration constants, and so is count function mode restricted
> >         to well-defined enumeration constants to represent modes such as
> >         "pulse-direction" and "quadrature x4" for example.  
> 
> Pulse direction is definitely not a quadrature counter...  Maybe this needs
> a rename to dual-signal-counter or similar?
> 
> Another classic case here would be increment / decrement counters where
> a signal is used for each operation (counting items between two light gates
> - used a lot in tracking products in the production industry).
> 
> > 
> > Note how driver authors no longer need to interact with Synapses
> > directly when utilizing the Simple Counter and Quadrature Counter
> > interfaces. This should make it easier too for authors to add support
> > since they don't need to fully understand the underlying Counter
> > paradigm in order to take advantage of the interfaces -- just define the
> > Counts and Signals, and they're ready to go.
> > 
> > Even more so, the Quadrature Counter interface takes it a step further
> > and doesn't require action_modes to be explicitly set -- rather they are
> > implicitly determined internally by the system based on the direction
> > and function mode. Abstractions like these should make the Counter
> > interface system as a whole robust enough to handle the diverse classes
> > of counter devices out in the real world.
> > 
> > Compilation warnings
> > ====================
> > 
> > There are three main compilation warnings which pop for this patchset.
> > I've inspected these warnings and none are errors, however they do
> > require some explanation.
> > 
> >         * 104-quad-8: warning: enumeration value
> >                 âQUAD_COUNTER_FUNCTION_PULSE_DIRECTIONâ not handled in
> >                 switch
> > 
> > The first warning is rather simple to explain: the
> > QUAD_COUNTER_FUNCTION_PULSE_DIRECTION state is handled by the parent if
> > statement's else condition, so an explicit case condition is not
> > necessary. I can add a default case line to pacify the compiler, but
> > since it would be empty the effort seems frivolous.   
> 
> Do it anyway and put a comment of /* Should not get here */
> 
> Suppressing false warnings is useful from a code maintenance point of view.
> 
> > In some sense as
> > well, a default case may make the switch logic less clear by implying
> > the possibility of additional cases which are not possible in the
> > context of that code path.
> > 
> >         * simple-counter: warning: assignment discards âconstâ qualifier
> >                 from pointer target type
> >         * quad-counter: warning: assignment discards âconstâ qualifier
> >                 from pointer target type
> > 
> > The second warning comes from the mapping of
> > simple_counter_device_ext/quad_counter_device_ext,
> > simple_counter_count_ext/quad_counter_count_ext, and
> > simple_counter_signal_ext/quad_counter_signal_ext to the internal
> > Counter counter_device_ext, counter_count_ext, and counter_signal_ext
> > structures respectively.
> > 
> > The priv member of the counter_device_ext, counter_count_ext, or
> > counter_signal_ext is leveraged to pass the respective
> > simple_counter_device_ext/quad_counter_device_ext,
> > simple_counter_count_ext/quad_counter_count_ext, or
> > simple_counter_signal_ext/quad_counter_signal_ext structure to their
> > respective read/write callback. The priv member is generic on purpose to
> > allow any desired data to be passed; the supplied read/write callbacks
> > should know the datatype of the passed-in priv argument so they cast it
> > appropriately to access their expected data.
> > 
> > As such, the 'const' qualifier of the structures are thus discarded but
> > subsequently cast back when the respective registered callback functions
> > are called. Since this is the intended use case of the priv member -- to
> > generically pass driver data for later recast -- I don't believe this
> > warning needs to be rectified.  
> 
> All warnings need to be rectified.  Sorry but this noise will do two things:
> 1) Get you a patch every few weeks from someone fixing it.
> 2) Potentially make real warnings harder to see.
> 
> Sometimes we have to play games to work around them, but such is life.
> 
> > 
> >         * generic-counter: warning: passing argument 5 of
> >                 âcounter_attribute_createâ discards âconstâ qualifier
> >                 from pointer target type
> >         * generic-counter: warning: passing argument 6 of
> >                 âcounter_attribute_createâ discards âconstâ qualifier
> >                 from pointer target type
> > 
> > The third warnings comes from a similar situation to the second warning:
> > a 'const' argument is passed generically via 'void *' for later recast.
> > In this cast, I decided to create a generic function called
> > counter_attribute_create in order to simplify the sysfs attribute
> > registration code in the generic-counter.c file.
> > 
> > The counter_attribute_create function takes in read and write callbacks,
> > as well as two optional generic data arguments to be stored as 'void *'
> > (the component and component_data parameters). Using this setup allows
> > the counter_attribute_create function to be the sole function necessary
> > to create a desired Generic Counter sysfs attribute: read and write
> > callbacks are passed along with relevant Counter component and data
> > generically, which can be cast back later inside those read and write
> > functions to match the expected datatype.
> > 
> > Using a generic counter_attribute_create function reduces duplicate
> > code, but it does result in many superfluous compilation warnings. I can
> > define new attribute_create functions specific to each type of sysfs
> > attribute in order to pacify the warnings, but that seems to be such a
> > waste to increase the amount of code with duplications that are
> > unnecessary. What would you recommend; should I attempt to pacify these
> > warnings or leave them be?  
> 
> You must fix them I'm afraid.
> 
> > 
> > Known TODO items
> > ================
> > 
> > Although I've added the interface documentation files with rst file
> > extensions, I still need to familiarize myself with Sphinx markup
> > constructs to take advantage of the language. For example, I've copied
> > verbatim several structure definitions into the documentation directly,
> > but I believe this would be better left dynamically generated by using
> > the relevant markup syntax. I'll try to clean up the documentation then
> > once I've brushed up on Sphinx.
> > 
> > As noted in a previous patchset version, the signal_write callback
> > should be removed from the interface; there are few if any cases where
> > it makese sense to have a signal_write callback since Signals are
> > always considered inputs in the context of the Counter paradigm.
> > 
> > I've retained the signal_write callback in this version since I'm unsure
> > how to implement the dummy-counter Signal source. Benjamin Gaignard
> > suggested implementing dummy-counter as a gpio-counter which could use
> > gpio to provide a software quadratic counter. Is this the path I should
> > take?  
> 
> It would certainly work well and be simple enough for easy understanding.
> Also, it might be a useful driver in it's own right.
> 
> > 
> > Furthermore, the dummy-counter driver defines its own single
> > platform_device which restricts it to loading only a single instance.
> > I can fix this to allow multiple instances in the next patchset version
> > -- as suggested, I'll check out industrialio-sw-device.c for reference.
> > 
> > Right now the dummy-counter driver only has example code for the Simple
> > Counter interface. It may be prudent to add example code for the Generic
> > Counter and Quadrature Counter interfaces too. I think dummy-counter
> > should serve as the reference driver implementation for all the Counter
> > interfaces, so that driver authors have an example of how to integrate
> > the particular interface they desire.  
> 
> Such a driver is useful, but it doesn't add much if you have another,
> only slightly more complex real driver that also does the job.
> Perhaps do them all as gpio based drivers for example?
> > 
> > Finally, I only added very basic support for the Quadrature Counter
> > interface in the 104-QUAD-8 driver. It's possible to support all
> > existing IIO Counter sysfs attributes in the 104-QUAD-8 driver via
> > corresponding quad_counter_device_ext, quad_counter_count_ext, and
> > quad_counter_signal_ext structures, such that only the
> > /sys/bus/counter/devices/counterX/ directory needs to be accessed to
> > interact with the 104-QUAD-8 device. I'll try to add support for those
> > remaining sysfs attributes in the next patchset version.
> > 
> > If I missed anything from the last patchset version review just remind
> > me again and I'll add it to my TODO list. ;)  
> 
> You are seriously optimistic if you think we can remember!
> 
> Jonathan
> 
> > 
> > William Breathitt Gray (11):
> >   iio: Introduce the Generic Counter interface
> >   counter: Documentation: Add Generic Counter sysfs documentation
> >   docs: Add Generic Counter interface documentation
> >   counter: Introduce the Simple Counter interface
> >   counter: Documentation: Add Simple Counter sysfs documentation
> >   docs: Add Simple Counter interface documentation
> >   counter: Add dummy counter driver
> >   counter: Introduce the Quadrature Counter interface
> >   counter: Documentation: Add Quadrature Counter sysfs documentation
> >   docs: Add Quadrature Counter interface documentation
> >   counter: 104-quad-8: Add Quadrature Counter interface support
> > 
> >  .../ABI/testing/sysfs-bus-counter-generic-sysfs    |  73 ++
> >  .../ABI/testing/sysfs-bus-counter-quadrature-sysfs |  76 ++
> >  .../ABI/testing/sysfs-bus-counter-simple-sysfs     |  61 ++
> >  Documentation/driver-api/iio/generic-counter.rst   | 434 +++++++++
> >  Documentation/driver-api/iio/index.rst             |   3 +
> >  Documentation/driver-api/iio/quad-counter.rst      | 444 +++++++++
> >  Documentation/driver-api/iio/simple-counter.rst    | 393 ++++++++
> >  MAINTAINERS                                        |   9 +
> >  drivers/iio/Kconfig                                |   3 +-
> >  drivers/iio/counter/104-quad-8.c                   | 257 +++++-
> >  drivers/iio/counter/Kconfig                        |  35 +-
> >  drivers/iio/counter/Makefile                       |   6 +
> >  drivers/iio/counter/dummy-counter.c                | 308 +++++++
> >  drivers/iio/counter/generic-counter.c              | 992 +++++++++++++++++++++
> >  drivers/iio/counter/quad-counter.c                 | 774 ++++++++++++++++
> >  drivers/iio/counter/simple-counter.c               | 734 +++++++++++++++
> >  include/linux/iio/counter.h                        | 629 +++++++++++++
> >  17 files changed, 5216 insertions(+), 15 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-generic-sysfs
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-quadrature-sysfs
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-simple-sysfs
> >  create mode 100644 Documentation/driver-api/iio/generic-counter.rst
> >  create mode 100644 Documentation/driver-api/iio/quad-counter.rst
> >  create mode 100644 Documentation/driver-api/iio/simple-counter.rst
> >  create mode 100644 drivers/iio/counter/dummy-counter.c
> >  create mode 100644 drivers/iio/counter/generic-counter.c
> >  create mode 100644 drivers/iio/counter/quad-counter.c
> >  create mode 100644 drivers/iio/counter/simple-counter.c
> >  create mode 100644 include/linux/iio/counter.h
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html