Message-ID: <20170828141633.GA16755@sophia>
Date:   Mon, 28 Aug 2017 10:16:33 -0400
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        benjamin.gaignard@...aro.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 0/3] iio: Introduce the generic counter interface

On Sun, Aug 20, 2017 at 01:00:16PM +0100, Jonathan Cameron wrote:
>On Mon, 31 Jul 2017 12:02:45 -0400
>William Breathitt Gray <vilhelm.gray@...il.com> wrote:
>
>> Summary
>> =======
>
>I'd like to see some of this brought out as proper documentation files
>in future sets.  In particular the sysfs interface needs full docs.

I'll write up some proper documentation for the interface and
implementation for version 2 of this patchset; it should help elucidate
some of the concepts here a bit better.

>> 
>> 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 patchset 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 pedometers, 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:
>> 
>>         VALUE
>>         -----
>>         A Value represents the count data for a set of Signals. A Value
>>         has a count function mode (e.g. "increment" or "quadrature x4")
>>         which respresents the update behavior for the count data. A
>>         Value also has a set of one or more associated Signals.
>> 
>>         SIGNAL
>>         ------
>>         A Signal represents a count input line. A Signal may be
>> 	associated to one or more Values.
>> 
>>         TRIGGER
>> 	-------
>>         A Trigger represents a Value's count function trigger condition
>>         mode (e.g. "rising edge" or "double pulse") for an associated
>>         Signal.	If a Signal is associated with a Value, a respective
>>         Trigger	instance for that association exists -- albeit perhaps
>>         with a trigger condition mode of "none."
>> 
>> A counter is defined as a set of input signals associated to 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 Values
>> each associated to a set of Signals, whose respective Trigger instances
>> represent the count function update conditions for the associated
>> Values.
>> 
>> Implementation
>> ==============
>> 
>> The IIO generic counter interface piggybacks off of the IIO core. This
>> is primarily used to leverage the existing sysfs setup: the IIO_COUNT
>> channel attributes represent the "counter value," while the IIO_SIGNAL
>> channel attributes represent the "counter signal;" auxilary IIO_COUNT
>> attributes represent the "counter signal" connections and their
>> respective state change configurations which trigger an associated
>> "counter function" evaluation.
>> 
>> The iio_counter_ops structure serves as a container for driver callbacks
>> to communicate with the device; function callbacks are provided to read
>> and write various "counter signals" and "counter values," and set and
>> get the "trigger mode" and "function mode" for various "counter signals"
>> and "counter values" respectively.
>> 
>> To support a counter device, a driver must first allocate the available
>> "counter signals" via iio_counter_signal structures. These "counter
>> signals" should be stored as an array and set to the init_signals member
>> of an allocated iio_counter structure before the counter is registered.
>> 
>> "Counter values" may be allocated via iio_counter_value structures, and
>> respective "counter signal" associations made via iio_counter_trigger
>> structures. Initial associated iio_counter_trigger structures may be
>> stored as an array and set to the the init_triggers member of the
>> respective iio_counter_value structure. These iio_counter_value
>> structures may be set to the init_values member of an allocated
>> iio_counter structure before the counter is registered if so desired.
>> 
>> A counter device is registered to the system by passing the respective
>> initialized iio_counter structure to the iio_counter_register function;
>> similarly, the iio_counter_unregister function unregisters the
>> respective counter.
>> 
>> Userspace Interface
>> ===================
>> 
>> Several sysfs attributes are generated by the generic counter interface,
>> and reside under the /sys/bus/iio/devices/iio:deviceX directory.
>> 
>> Each counter has a respective set of countX-Y and signalX-Y prefixed
>> attributes, where X is the id set in the counter structure, and Y is the
>> id of the respective Value or Signal.
>> 
>> The generic counter interface sysfs attributes are as follows:
>> 
>>         countX-Y_function: count function mode
>>         countX-Y_function_available: available count function modes
>>         countX-Y_name: Value name
>>         countX-Y_raw: Value data
>>         countX-Y_triggers: Value's associated Signals
>>         countX-Y_trigger_signalX-Z: Value Y trigger mode for Signal Z
>>         countX-Y_trigger_signalX-Z_available: available Value Y trigger
>>                                               modes for Signal Z
>>         signalX-Y_name: Signal name
>>         signalX-Y_raw: Signal data
>> 
>> Future Development
>> ==================
>> 
>> This patchset is focused on providing the core functionality required to
>> introduce a usable generic counter interface. Unfortunately, that means
>> nice features were left out for the sake of simplicity. It's probably
>> useful to include support for common counter functionality and
>> configurations such as preset values, min/max boundaries, and such in
>> future patches.
>> 
>> Take into consideration however that it is my intention for this generic
>> counter interface to serve as a base level of code to support counter
>> devices; that is to say: a feature should only be added to the generic
>> counter interface is it is pervasive to counter devices in general. If a
>> feature is specific to a subset of counter devices, then that feature
>> should be added to the respective subset counter interface; for example,
>> a "quadrature pair" atribute should be introduced to the quadrature
>> counter interface instead of the generic counter interface.
>> 
>> Managed memory functions such as devm_iio_counter_register should be
>> introduced in a future patch to provide support that aligns with the
>> current design of code in the kernel. This should help prevent memory
>> leaks and reduce a lot of boiler plate code in drivers.
>> 
>> I would like to see a character device node interface similar to the one
>> provided by the GPIO subsystem. I designed the generic counter interface
>> to allow for dynamic reconfiguration of the Value-Signal associations,
>> where a user may decide to attached or remove Signals to/from Values at
>> will; I don't believe a sysfs attribute system lends itself well to
>> these kinds of interactions.
>
>I am unconvinced by this at the moment.  A character device
>for anything beyond trivial cases results in a nasty mess where you have
>to have a supporting userspace library.  It becomes impossible to do
>things with simple scripts.  We aren't talking about cases where
>we need the ioctls to ensure complex thing happen simultaneously.
>It may be that there is an argument here for it but as I say I'm
>not yet convinced.
>
>It may be that configfs is actually a better fit for this sort
>of dynamic reconfiguration.   A big question to my mind is whether
>there is actually hardware out there that really supports the level
>of flexibility you are describing.  There may well be!

I believe you are correct and I am likely jumping the gun somewhat on
this idea. Unwarranted complexity often leads to inefficencies and
errors, so I would like to avoid feature creep when possible. Focusing
on the sysfs interface should be a good plan at least while we see how
real hardware ends up using this interface.

>> 
>> Current Patchset Concerns
>> =========================
>> 
>> I'm piggybacking off the IIO core with this patchset, but I'm not sure
>> if its appropriate for the longterm. A lot of IIO core is ignored, and
>> I feel that I'm essentially just leveraging it for its sysfs code.
>> However, utilizing iio_device_register within iio_counter_register does
>> allow driver authors to pass in their own IIO channels to support
>> attributes and features not provide by the IIO generic counter interface
>> code -- so I'm a bit undecided whether to part completely either.
>
>This is a big question for me.  Do we often have these counter interfaces
>within devices supporting other types of IIO channel and if so is there
>a need to synchronize the two.  I.e. will we end up using triggered buffered
>capture with these devices.

What brought on this worry for me is how this generic counter interface
adds a certain level of abstraction from the hardware itself. For
example, a generic counter "Signal" does not necessarily represent a
physical pin in hardware; e.g. a single "Signal" could be a pair of
physical differential lines. Since the generic counter interface by
itself does not describe that level of hardware, leveraging IIO core
does enable us to expose physical hardware without having to duplicate
code specifically for the generic counter interface.

As you pointed out, this all depends on the extent to which real
hardware shares these other types with a counter (if the overlap in
functionality is minimum, then it may be better to keep the generic
counter interface separate afterall). I'll have to mull over this and
consider how real hardware is used.

>
>> 
>> For what its worth, I'd like the generic counter interface to stay
>> piggybacked at least until the quadrature counter interface is release;
>> that should allow the driver authors to start making use of the
>> quadrature counter interface while the generic counter interface is
>> improved under the hood.
>> 
>
>The nasty bit is that you are committing to a userspace ABI that puts
>these under the relevant IIO namings.  Once you've done that you are
>stuck and will have to support that going forward.
>
>So this decision needs to be made asap.
>
>Now I have no problem with having a new top level type alongside
>triggers and devices that support counters.  That may give you the
>flexibility you want. It would be grouped under IIO but not obliged to
>use any of the infrastructure (except the trivial registration bits
>and pieces to put it in the right directory).

Polluting userspace with a new ABI that is immediatedly deprecated on
the subsequent kernel release is exactly what I wish to avoid. You make
a excellent point that a hasty introduction would result in an ABI we
must thus forward support. I want to be confident that the path we
commit is prudent, so I will put some thought into this for the next
version.

>
>
>> You may have noticed that Signals do not have associated
>> iio_counter_signal_register/iio_counter_signal_unregister functions.
>> This is because I encountered a deadlock scenario where unregistering a
>> Signal dynamically requires a search of all Values' Triggers to
>> determine is the Signal is associated to any Values and subsequently
>> remove that association; a lock of the counter structure
>> signal_list_lock is required at the iio_counter_signal_unregister call,
>> and then once more when performing the search through the Triggers
>> (in order to prevent a trigger structure signal member pointer being
>> dereferenced after the respective Signal is freed).
>> 
>> Since Signals are likely to be static for the lifetime of a driver
>> (since Signals typically represent physical lines on the device), I
>> decided to prevent drivers from dynamically registering and
>> unregistering Signals for the time being. I'm not sure whether this
>> design should stay however; I don't want to restrict a clever driver
>> or future interface that wants to dynamically register/unregister
>> Signals.
>> 
>
>I think this is a sensible design decision.  Might change eventually
>but for now it makes sense.
>
>
>> I put a dependency on CONFIG_IIO_COUNTER for the Kconfig IIO Counter
>> menu. Is this appropriate; should this menu simply select IIO_COUNTER
>> instead of a hard depends; or should drivers individually depend on
>> CONFIG_IIO_COUNTER?
>
>This is fine.
>
>> 
>> This is more of a style nitpick: should I prefix static symbols with __?
>> I took on this coding convention when writing the generic counter
>> interface code, but I'm not sure if it is appropriate in this manner.
>
>I wouldn't bother.  I know I did it in various places in IIO, but I wasn't
>all that consistent with it.  The usual reason is to indicate that a lock
>must be held rather than that they are static.
>
>> 
>> Finally, I'd like to actively encourage symbol renaming suggestions. I'm
>> afraid names such as "Value" and "Trigger" may be too easily confused
>> for other unrelated subsystem conventions (e.g. IIO Triggers). I mainly
>> stuck with the names and symbols used in this patchset so that I could
>> focus on developing the interface code. However, if the chance of
>> confusion is small, we can stick with the existing naming convention as
>> well.
>> 
>
>This is certainly an interesting direction.   I'm not really clear
>enough in my head yet on exactly what the interface looks like and also
>the question of whether you have actually designed in too much flexibility
>at the cost of complexity internally.   There are a lot of list lookups
>which could get very expensive on devices with a lot of channels. It
>feels like at least some of those could be avoided if the flexibility
>was reduced slightly or at the least the burden of time could be moved
>(so vectors rather than lists perhaps).

I believe the flexibility in this case is warranted, though I realize
that the documentation for my rationale is lacking. Regardless, I agree
that the implementation itself suffers from complexity which can be
improved (vectors are a good idea which I overlooked). I'll try to
simplify the code a bit for version 2 and hopefully it'll make it both
easier to follow and less expensive.

>
>I'd like to get feedback from others on this.  In particular I think you
>need to work on the documentation of the various interfaces and how they
>fit together.  The basics are here in this cover letter, but we it's the
>details we'll probably trip up on.

By far I agree that proper documentation is a necessity. I intend to
write up more in-depth documentation about both the userspace interface
as well as the implementation and rationale. These documentation files
should also serve as decent entry points for discussions of the
interface itself. I would like to give as much opportunity for
individuals to criticize the architecture and theory of the generic
counter interface before actual code is committed.

I'm going to research some more counter devices to get a better idea of
the kind of hardware functionality commonly exposed together with these
counters, and have that aid in the decision of how to integrate with IIO
core. I suspect it'll be couple weeks however before I submit version 2
with the more in-depth documentation.

Sincerely,

William Breathitt Gray

>
>Thanks,
>
>Jonathan
>
>> William Breathitt Gray (3):
>>   iio: Implement counter channel specification and IIO_SIGNAL constant
>>   iio: Introduce the generic counter interface
>>   iio: 104-quad-8: Add IIO generic counter interface support
>> 
>>  MAINTAINERS                        |    7 +
>>  drivers/iio/Kconfig                |    8 +
>>  drivers/iio/Makefile               |    1 +
>>  drivers/iio/counter/104-quad-8.c   |  306 +++++++++-
>>  drivers/iio/counter/Kconfig        |    1 +
>>  drivers/iio/industrialio-core.c    |   14 +-
>>  drivers/iio/industrialio-counter.c | 1157 ++++++++++++++++++++++++++++++++++++
>>  include/linux/iio/counter.h        |  221 +++++++
>>  include/linux/iio/iio.h            |    2 +
>>  include/uapi/linux/iio/types.h     |    1 +
>>  10 files changed, 1700 insertions(+), 18 deletions(-)
>>  create mode 100644 drivers/iio/industrialio-counter.c
>>  create mode 100644 include/linux/iio/counter.h
>> 
>

Powered by blists - more mailing lists