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

[Benjamin Gaignard]

2018-01-15 10:02 GMT+01:00 Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>:
> 2018-01-01 14:04 GMT+01:00 Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx>:
>> 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.
>>
>
> Sorry for the long delay before answering to thread.
> I have succesfully implement and test a quadrature encoder driver
> on stm32 timer part. Some clean up are need but the basic functions
> like setting the two supported modes (quadX2 or quadX4) supported by
> my hardware, counting, preset and direction are functional.
>
> I have used the "simplified interface" so my driver is quite simple with
> only few functions to implement (~300 lines of code).
> When this series will be upstream we can convert stm32 drivers to use it.
>
> Thanks a lot for this work.
> Benjamin

Any news about those patches ?

Regards,
Benjamin

>
>> 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
>>