DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B31D621A1C
Date: Sun, 20 Aug 2017 13:00:16 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net,
        benjamin.gaignard@linaro.org, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org
Subject: Re: [PATCH 0/3] iio: Introduce the generic counter interface
Message-ID: <20170820130016.38e896c4@archlinux>
In-Reply-To: <cover.1501516678.git.vilhelm.gray@gmail.com>
References: <cover.1501516678.git.vilhelm.gray@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 13333
Lines: 282

On Mon, 31 Jul 2017 12:02:45 -0400
William Breathitt Gray <vilhelm.gray@gmail.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.
> 
> 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!
> 
> 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.

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


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

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
>