* [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
@ 2020-12-26 0:15 ` William Breathitt Gray
2020-12-30 14:41 ` Jonathan Cameron
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: William Breathitt Gray @ 2020-12-26 0:15 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
The Counter subsystem architecture and driver implementations have
changed in order to handle Counter sysfs interactions in a more
consistent way. This patch updates the Generic Counter interface
documentation to reflect the changes.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-counter | 9 +-
Documentation/driver-api/generic-counter.rst | 242 ++++++++++++++-----
2 files changed, 184 insertions(+), 67 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 566bd99fe0a5..1820ce2f9183 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -219,7 +219,14 @@ What: /sys/bus/counter/devices/counterX/signalY/signal
KernelVersion: 5.2
Contact: linux-iio@vger.kernel.org
Description:
- Signal data of Signal Y represented as a string.
+ Signal level state of Signal Y. The following signal level
+ states are available:
+
+ low:
+ Low level state.
+
+ high:
+ High level state.
What: /sys/bus/counter/devices/counterX/signalY/name
KernelVersion: 5.2
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index b02c52cd69d6..b842ddbbd8a0 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -250,8 +250,8 @@ for defining a counter device.
.. kernel-doc:: drivers/counter/counter.c
:export:
-Implementation
-==============
+Driver Implementation
+=====================
To support a counter device, a driver must first allocate the available
Counter Signals via counter_signal structures. These Signals should
@@ -267,25 +267,59 @@ 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.
+Driver callbacks must be provided to the counter_device 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/miscellaneous exposure
-and 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.
+devm_counter_register function may be used if device memory-managed
+registration is desired.
+
+The struct counter_comp structure is used to define counter extensions
+for Signals, Synapses, and Counts.
+
+The "type" member specifies the type of high-level data (e.g. BOOL,
+COUNT_DIRECTION, etc.) handled by this extension. The "`*_read`" and
+"`*_write`" members can then be set by the counter device driver with
+callbacks to handle that data using native C data types (i.e. u8, u64,
+etc.).
+
+Convenience macros such as `COUNTER_COMP_COUNT_U64` are provided for use
+by driver authors. In particular, driver authors are expected to use
+the provided macros for standard Counter subsystem attributes in order
+to maintain a consistent interface for userspace. For example, a counter
+device driver may define several standard attributes like so::
+
+ struct counter_comp count_ext[] = {
+ COUNTER_COMP_DIRECTION(count_direction_read),
+ COUNTER_COMP_ENABLE(count_enable_read, count_enable_write),
+ COUNTER_COMP_CEILING(count_ceiling_read, count_ceiling_write),
+ };
+
+This makes it simple to see, add, and modify the attributes that are
+supported by this driver ("direction", "enable", and "ceiling") and to
+maintain this code without getting lost in a web of struct braces.
+
+Callbacks must match the function type expected for the respective
+component or extension. These function types are defined in the struct
+counter_comp structure as the "`*_read`" and "`*_write`" union members.
+
+The corresponding callback prototypes for the extensions mentioned in
+the previous example above would be::
+
+ int count_direction_read(struct counter_device *counter,
+ struct counter_count *count, u8 *direction);
+ int count_enable_read(struct counter_device *counter,
+ struct counter_count *count, u8 *enable);
+ int count_enable_write(struct counter_device *counter,
+ struct counter_count *count, u8 enable);
+ int count_ceiling_read(struct counter_device *counter,
+ struct counter_count *count, u64 *ceiling);
+ int count_ceiling_write(struct counter_device *counter,
+ struct counter_count *count, u64 ceiling);
Determining the type of extension to create is a matter of scope.
@@ -313,52 +347,128 @@ Determining the type of extension to create is a matter of scope.
chip overheated via a device extension called "error_overtemp":
/sys/bus/counter/devices/counterX/error_overtemp
-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/sysfs-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.
+Subsystem Architecture
+======================
+
+Counter drivers pass and take data natively (i.e. `u8`, `u64`, etc.) and
+the shared counter module handles the translation between the sysfs
+interface. This gurantees a standard userspace interface for all counter
+drivers, and helps generalize the Generic Counter driver ABI in order to
+support the Generic Counter chrdev interface without significant changes
+to the existing counter drivers.
+
+A high-level view of how a count value is passed down from a counter
+driver is exemplified by the following. The driver callbacks are first
+registered to the Counter core component for use by the Counter
+userspace interface components::
+
+ Driver callbacks registration:
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ +----------------------------+
+ | Counter device driver |
+ +----------------------------+
+ | Processes data from device |
+ +----------------------------+
+ |
+ -------------------
+ / driver callbacks /
+ -------------------
+ |
+ V
+ +----------------------+
+ | Counter core |
+ +----------------------+
+ | Routes device driver |
+ | callbacks to the |
+ | userspace interfaces |
+ +----------------------+
+ |
+ -------------------
+ / driver callbacks /
+ -------------------
+ |
+ +---------------+
+ |
+ V
+ +--------------------+
+ | Counter sysfs |
+ +--------------------+
+ | Translates to the |
+ | standard Counter |
+ | sysfs output |
+ +--------------------+
+
+Thereafter, data can be transferred directly between the Counter device
+driver and Counter userspace interface::
+
+ Count data request:
+ ~~~~~~~~~~~~~~~~~~~
+ ----------------------
+ / Counter device \
+ +----------------------+
+ | Count register: 0x28 |
+ +----------------------+
+ |
+ -----------------
+ / raw count data /
+ -----------------
+ |
+ V
+ +----------------------------+
+ | Counter device driver |
+ +----------------------------+
+ | Processes data from device |
+ |----------------------------|
+ | Type: u64 |
+ | Value: 42 |
+ +----------------------------+
+ |
+ ----------
+ / u64 /
+ ----------
+ |
+ +---------------+
+ |
+ V
+ +--------------------+
+ | Counter sysfs |
+ +--------------------+
+ | Translates to the |
+ | standard Counter |
+ | sysfs output |
+ |--------------------|
+ | Type: const char * |
+ | Value: "42" |
+ +--------------------+
+ |
+ ---------------
+ / const char * /
+ ---------------
+ |
+ V
+ +--------------------------------------------------+
+ | `/sys/bus/counter/devices/counterX/countY/count` |
+ +--------------------------------------------------+
+ \ Count: "42" /
+ --------------------------------------------------
+
+There are three primary components involved:
+
+Counter device driver
+---------------------
+Communicates with the hardware device to read/write data; e.g. counter
+drivers for quadrature encoders, timers, etc.
+
+Counter core
+------------
+Registers the counter device driver to the system so that the respective
+callbacks are called during userspace interaction.
+
+Counter sysfs
+-------------
+Translates counter data to the standard Counter sysfs interface format
+and vice versa.
+
+Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file
+for a detailed breakdown of the available Generic Counter interface
+sysfs attributes.
--
2.29.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization
2020-12-26 0:15 ` [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
@ 2020-12-30 14:41 ` Jonathan Cameron
0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2020-12-30 14:41 UTC (permalink / raw)
To: William Breathitt Gray
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue
On Fri, 25 Dec 2020 19:15:35 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> The Counter subsystem architecture and driver implementations have
> changed in order to handle Counter sysfs interactions in a more
> consistent way. This patch updates the Generic Counter interface
> documentation to reflect the changes.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
> Documentation/ABI/testing/sysfs-bus-counter | 9 +-
> Documentation/driver-api/generic-counter.rst | 242 ++++++++++++++-----
> 2 files changed, 184 insertions(+), 67 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 566bd99fe0a5..1820ce2f9183 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -219,7 +219,14 @@ What: /sys/bus/counter/devices/counterX/signalY/signal
> KernelVersion: 5.2
> Contact: linux-iio@vger.kernel.org
> Description:
> - Signal data of Signal Y represented as a string.
> + Signal level state of Signal Y. The following signal level
> + states are available:
> +
> + low:
> + Low level state.
> +
> + high:
> + High level state.
>
> What: /sys/bus/counter/devices/counterX/signalY/name
> KernelVersion: 5.2
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index b02c52cd69d6..b842ddbbd8a0 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -250,8 +250,8 @@ for defining a counter device.
> .. kernel-doc:: drivers/counter/counter.c
> :export:
>
> -Implementation
> -==============
> +Driver Implementation
> +=====================
>
> To support a counter device, a driver must first allocate the available
> Counter Signals via counter_signal structures. These Signals should
> @@ -267,25 +267,59 @@ 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.
> +Driver callbacks must be provided to the counter_device 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/miscellaneous exposure
> -and 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.
> +devm_counter_register function may be used if device memory-managed
> +registration is desired.
> +
> +The struct counter_comp structure is used to define counter extensions
> +for Signals, Synapses, and Counts.
> +
> +The "type" member specifies the type of high-level data (e.g. BOOL,
> +COUNT_DIRECTION, etc.) handled by this extension. The "`*_read`" and
> +"`*_write`" members can then be set by the counter device driver with
> +callbacks to handle that data using native C data types (i.e. u8, u64,
> +etc.).
> +
> +Convenience macros such as `COUNTER_COMP_COUNT_U64` are provided for use
> +by driver authors. In particular, driver authors are expected to use
> +the provided macros for standard Counter subsystem attributes in order
> +to maintain a consistent interface for userspace. For example, a counter
> +device driver may define several standard attributes like so::
> +
> + struct counter_comp count_ext[] = {
> + COUNTER_COMP_DIRECTION(count_direction_read),
> + COUNTER_COMP_ENABLE(count_enable_read, count_enable_write),
> + COUNTER_COMP_CEILING(count_ceiling_read, count_ceiling_write),
> + };
> +
> +This makes it simple to see, add, and modify the attributes that are
> +supported by this driver ("direction", "enable", and "ceiling") and to
> +maintain this code without getting lost in a web of struct braces.
> +
> +Callbacks must match the function type expected for the respective
> +component or extension. These function types are defined in the struct
> +counter_comp structure as the "`*_read`" and "`*_write`" union members.
> +
> +The corresponding callback prototypes for the extensions mentioned in
> +the previous example above would be::
> +
> + int count_direction_read(struct counter_device *counter,
> + struct counter_count *count, u8 *direction);
> + int count_enable_read(struct counter_device *counter,
> + struct counter_count *count, u8 *enable);
> + int count_enable_write(struct counter_device *counter,
> + struct counter_count *count, u8 enable);
> + int count_ceiling_read(struct counter_device *counter,
> + struct counter_count *count, u64 *ceiling);
> + int count_ceiling_write(struct counter_device *counter,
> + struct counter_count *count, u64 ceiling);
>
> Determining the type of extension to create is a matter of scope.
>
> @@ -313,52 +347,128 @@ Determining the type of extension to create is a matter of scope.
> chip overheated via a device extension called "error_overtemp":
> /sys/bus/counter/devices/counterX/error_overtemp
>
> -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/sysfs-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.
> +Subsystem Architecture
> +======================
> +
> +Counter drivers pass and take data natively (i.e. `u8`, `u64`, etc.) and
> +the shared counter module handles the translation between the sysfs
> +interface.
Same point as raised in previous patch description.
> This gurantees a standard userspace interface for all counter
Spell check this file. guarantees
> +drivers, and helps generalize the Generic Counter driver ABI in order to
> +support the Generic Counter chrdev interface without significant changes
> +to the existing counter drivers.
I would modify this to assume you've already done the chrdev interface.
"and enables a Generic Counter chrdev interface without..."
> +
> +A high-level view of how a count value is passed down from a counter
> +driver is exemplified by the following. The driver callbacks are first
> +registered to the Counter core component for use by the Counter
> +userspace interface components::
> +
> + Driver callbacks registration:
> + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + +----------------------------+
> + | Counter device driver |
That tab again ;)
> + +----------------------------+
> + | Processes data from device |
> + +----------------------------+
> + |
> + -------------------
> + / driver callbacks /
> + -------------------
> + |
> + V
> + +----------------------+
> + | Counter core |
> + +----------------------+
> + | Routes device driver |
> + | callbacks to the |
> + | userspace interfaces |
> + +----------------------+
> + |
> + -------------------
> + / driver callbacks /
> + -------------------
> + |
> + +---------------+
> + |
> + V
> + +--------------------+
> + | Counter sysfs |
> + +--------------------+
> + | Translates to the |
> + | standard Counter |
> + | sysfs output |
> + +--------------------+
> +
> +Thereafter, data can be transferred directly between the Counter device
> +driver and Counter userspace interface::
> +
> + Count data request:
> + ~~~~~~~~~~~~~~~~~~~
> + ----------------------
> + / Counter device \
> + +----------------------+
> + | Count register: 0x28 |
> + +----------------------+
> + |
> + -----------------
> + / raw count data /
> + -----------------
> + |
> + V
> + +----------------------------+
> + | Counter device driver |
> + +----------------------------+
> + | Processes data from device |
> + |----------------------------|
> + | Type: u64 |
> + | Value: 42 |
> + +----------------------------+
> + |
> + ----------
> + / u64 /
> + ----------
> + |
> + +---------------+
> + |
> + V
> + +--------------------+
> + | Counter sysfs |
> + +--------------------+
> + | Translates to the |
> + | standard Counter |
> + | sysfs output |
> + |--------------------|
> + | Type: const char * |
> + | Value: "42" |
> + +--------------------+
> + |
> + ---------------
> + / const char * /
> + ---------------
> + |
> + V
> + +--------------------------------------------------+
> + | `/sys/bus/counter/devices/counterX/countY/count` |
> + +--------------------------------------------------+
> + \ Count: "42" /
> + --------------------------------------------------
> +
> +There are three primary components involved:
> +
> +Counter device driver
> +---------------------
> +Communicates with the hardware device to read/write data; e.g. counter
> +drivers for quadrature encoders, timers, etc.
> +
> +Counter core
> +------------
> +Registers the counter device driver to the system so that the respective
> +callbacks are called during userspace interaction.
> +
> +Counter sysfs
> +-------------
> +Translates counter data to the standard Counter sysfs interface format
> +and vice versa.
> +
> +Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file
> +for a detailed breakdown of the available Generic Counter interface
> +sysfs attributes.
Otherwise LGTM.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 3/5] counter: Add character device interface
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-12-26 0:15 ` [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
@ 2020-12-26 0:15 ` William Breathitt Gray
2020-12-30 15:04 ` Jonathan Cameron
` (2 more replies)
2020-12-26 0:15 ` [PATCH v7 4/5] docs: counter: Document " William Breathitt Gray
` (3 subsequent siblings)
5 siblings, 3 replies; 23+ messages in thread
From: William Breathitt Gray @ 2020-12-26 0:15 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray,
Dan Carpenter
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
MAINTAINERS | 1 +
drivers/counter/Makefile | 2 +-
drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
drivers/counter/counter-chrdev.h | 16 +
drivers/counter/counter-core.c | 37 ++-
drivers/counter/counter-sysfs.c | 51 +++-
include/linux/counter.h | 87 +++---
include/uapi/linux/counter.h | 123 ++++++++
8 files changed, 751 insertions(+), 56 deletions(-)
create mode 100644 drivers/counter/counter-chrdev.c
create mode 100644 drivers/counter/counter-chrdev.h
create mode 100644 include/uapi/linux/counter.h
diff --git a/MAINTAINERS b/MAINTAINERS
index b64fa49d5796..3a240f70bdd4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4494,6 +4494,7 @@ F: Documentation/ABI/testing/sysfs-bus-counter*
F: Documentation/driver-api/generic-counter.rst
F: drivers/counter/
F: include/linux/counter.h
+F: include/uapi/linux/counter.h
CPMAC ETHERNET DRIVER
M: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index cbe1d06af6a9..c4870eb5b1dd 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -4,7 +4,7 @@
#
obj-$(CONFIG_COUNTER) += counter.o
-counter-y := counter-core.o counter-sysfs.o
+counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index 000000000000..61b11989546a
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+
+#include <linux/cdev.h>
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/kdev_t.h>
+#include <linux/kfifo.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/nospec.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timekeeping.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <linux/uaccess.h>
+
+#include "counter-chrdev.h"
+
+struct counter_comp_node {
+ struct list_head l;
+ struct counter_component component;
+ struct counter_comp comp;
+ void *parent;
+};
+
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(&counter->events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(&counter->events));
+ if (err < 0)
+ return err;
+ }
+
+ if (mutex_lock_interruptible(&counter->events_lock))
+ return -ERESTARTSYS;
+ err = kfifo_to_user(&counter->events, buf, len, &copied);
+ mutex_unlock(&counter->events_lock);
+ if (err < 0)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
+
+static __poll_t counter_chrdev_poll(struct file *filp,
+ struct poll_table_struct *pollt)
+{
+ struct counter_device *const counter = filp->private_data;
+ __poll_t events = 0;
+
+ poll_wait(filp, &counter->events_wait, pollt);
+
+ if (!kfifo_is_empty(&counter->events))
+ events = EPOLLIN | EPOLLRDNORM;
+
+ return events;
+}
+
+static void counter_events_list_free(struct list_head *const events_list)
+{
+ struct counter_event_node *p, *n;
+ struct counter_comp_node *q, *o;
+
+ list_for_each_entry_safe(p, n, events_list, l) {
+ /* Free associated component nodes */
+ list_for_each_entry_safe(q, o, &p->comp_list, l) {
+ list_del(&q->l);
+ kfree(q);
+ }
+
+ /* Free event node */
+ list_del(&p->l);
+ kfree(p);
+ }
+}
+
+static int counter_set_event_node(struct counter_device *const counter,
+ struct counter_watch *const watch,
+ const struct counter_comp_node *const cfg)
+{
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+
+ /* Search for event in the list */
+ list_for_each_entry(event_node, &counter->next_events_list, l)
+ if (event_node->event == watch->event &&
+ event_node->channel == watch->channel)
+ break;
+
+ /* If event is not already in the list */
+ if (&event_node->l == &counter->next_events_list) {
+ /* Allocate new event node */
+ event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+ if (!event_node)
+ return -ENOMEM;
+
+ /* Configure event node and add to the list */
+ event_node->event = watch->event;
+ event_node->channel = watch->channel;
+ INIT_LIST_HEAD(&event_node->comp_list);
+ list_add(&event_node->l, &counter->next_events_list);
+ }
+
+ /* Check if component watch has already been set before */
+ list_for_each_entry(comp_node, &event_node->comp_list, l)
+ if (comp_node->parent == cfg->parent &&
+ comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+ return -EINVAL;
+
+ /* Allocate component node */
+ comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+ if (!comp_node) {
+ /* Free event node if no one else is watching */
+ if (list_empty(&event_node->comp_list)) {
+ list_del(&event_node->l);
+ kfree(event_node);
+ }
+ return -ENOMEM;
+ }
+ *comp_node = *cfg;
+
+ /* Add component node to event node */
+ list_add_tail(&comp_node->l, &event_node->comp_list);
+
+ return 0;
+}
+
+static int counter_clear_watches(struct counter_device *const counter)
+{
+ unsigned long flags;
+ int err = 0;
+
+ raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+ counter_events_list_free(&counter->events_list);
+
+ if (counter->ops->events_configure)
+ err = counter->ops->events_configure(counter);
+
+ raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+ counter_events_list_free(&counter->next_events_list);
+
+ return err;
+}
+
+static int counter_add_watch(struct counter_device *const counter,
+ const unsigned long arg)
+{
+ void __user *const uwatch = (void __user *)arg;
+ struct counter_watch watch;
+ struct counter_comp_node comp_node = {0};
+ size_t parent, id;
+ struct counter_comp *ext;
+ size_t num_ext;
+ int err;
+
+ if (copy_from_user(&watch, uwatch, sizeof(watch)))
+ return -EFAULT;
+
+ if (watch.component.type == COUNTER_COMPONENT_NONE)
+ goto no_component;
+
+ parent = watch.component.parent;
+
+ /* Configure parent component info for comp node */
+ switch (watch.component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ext = counter->ext;
+ num_ext = counter->num_ext;
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ if (parent >= counter->num_signals)
+ return -EINVAL;
+ parent = array_index_nospec(parent, counter->num_signals);
+
+ comp_node.parent = counter->signals + parent;
+
+ ext = counter->signals[parent].ext;
+ num_ext = counter->signals[parent].num_ext;
+ break;
+ case COUNTER_SCOPE_COUNT:
+ if (parent >= counter->num_counts)
+ return -EINVAL;
+ parent = array_index_nospec(parent, counter->num_counts);
+
+ comp_node.parent = counter->counts + parent;
+
+ ext = counter->counts[parent].ext;
+ num_ext = counter->counts[parent].num_ext;
+ break;
+ }
+
+ id = watch.component.id;
+
+ /* Configure component info for comp node */
+ switch (watch.component.type) {
+ case COUNTER_COMPONENT_SIGNAL:
+ if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
+ comp_node.comp.signal_u32_read = counter->ops->signal_read;
+ break;
+ case COUNTER_COMPONENT_COUNT:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_U64;
+ comp_node.comp.count_u64_read = counter->ops->count_read;
+ break;
+ case COUNTER_COMPONENT_FUNCTION:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_FUNCTION;
+ comp_node.comp.count_u32_read = counter->ops->function_read;
+ break;
+ case COUNTER_COMPONENT_SYNAPSE_ACTION:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+ if (id >= counter->counts[parent].num_synapses)
+ return -EINVAL;
+ id = array_index_nospec(id, counter->counts[parent].num_synapses);
+
+ comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
+ comp_node.comp.action_read = counter->ops->action_read;
+ comp_node.comp.priv = counter->counts[parent].synapses + id;
+ break;
+ case COUNTER_COMPONENT_EXTENSION:
+ if (id >= num_ext)
+ return -EINVAL;
+ id = array_index_nospec(id, num_ext);
+
+ comp_node.comp = ext[id];
+ break;
+ default:
+ return -EINVAL;
+ }
+ /* Check if any read callback is set; this is part of a union */
+ if (!comp_node.comp.count_u8_read)
+ return -EOPNOTSUPP;
+
+no_component:
+ if (counter->ops->watch_validate) {
+ err = counter->ops->watch_validate(counter, &watch);
+ if (err < 0)
+ return err;
+ }
+
+ comp_node.component = watch.component;
+
+ return counter_set_event_node(counter, &watch, &comp_node);
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ unsigned long flags;
+ int err = 0;
+
+ switch (cmd) {
+ case COUNTER_CLEAR_WATCHES_IOCTL:
+ return counter_clear_watches(counter);
+ case COUNTER_ADD_WATCH_IOCTL:
+ return counter_add_watch(counter, arg);
+ case COUNTER_LOAD_WATCHES_IOCTL:
+ raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+ counter_events_list_free(&counter->events_list);
+ list_replace_init(&counter->next_events_list,
+ &counter->events_list);
+
+ if (counter->ops->events_configure)
+ err = counter->ops->events_configure(counter);
+
+ raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+ break;
+ default:
+ return -ENOIOCTLCMD;
+ }
+
+ return err;
+}
+
+static int counter_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = container_of(inode->i_cdev,
+ typeof(*counter),
+ chrdev);
+
+ get_device(&counter->dev);
+ filp->private_data = counter;
+
+ return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+
+ err = counter_clear_watches(counter);
+ if (err < 0)
+ return err;
+
+ put_device(&counter->dev);
+
+ return 0;
+}
+
+static const struct file_operations counter_fops = {
+ .llseek = no_llseek,
+ .read = counter_chrdev_read,
+ .poll = counter_chrdev_poll,
+ .unlocked_ioctl = counter_chrdev_ioctl,
+ .open = counter_chrdev_open,
+ .release = counter_chrdev_release,
+};
+
+int counter_chrdev_add(struct counter_device *const counter,
+ const dev_t counter_devt)
+{
+ struct device *const dev = &counter->dev;
+ struct cdev *const chrdev = &counter->chrdev;
+
+ /* Initialize Counter events lists */
+ INIT_LIST_HEAD(&counter->events_list);
+ INIT_LIST_HEAD(&counter->next_events_list);
+ raw_spin_lock_init(&counter->events_list_lock);
+
+ /* Initialize Counter events queue */
+ INIT_KFIFO(counter->events);
+ init_waitqueue_head(&counter->events_wait);
+ mutex_init(&counter->events_lock);
+
+ /* Initialize character device */
+ cdev_init(chrdev, &counter_fops);
+ dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
+ cdev_set_parent(chrdev, &dev->kobj);
+
+ return cdev_add(chrdev, dev->devt, 1);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+ cdev_del(&counter->chrdev);
+}
+
+static int counter_get_data(struct counter_device *const counter,
+ const struct counter_comp_node *const comp_node,
+ u64 *const value)
+{
+ const struct counter_comp *const comp = &comp_node->comp;
+ void *const parent = comp_node->parent;
+ int err = 0;
+ u8 value_u8 = 0;
+ u32 value_u32 = 0;
+
+ if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+ return 0;
+
+ switch (comp->type) {
+ case COUNTER_COMP_U8:
+ case COUNTER_COMP_BOOL:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ err = comp->device_u8_read(counter, &value_u8);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ err = comp->signal_u8_read(counter, parent, &value_u8);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ err = comp->count_u8_read(counter, parent, &value_u8);
+ break;
+ }
+ *value = value_u8;
+ break;
+ case COUNTER_COMP_SIGNAL_LEVEL:
+ case COUNTER_COMP_FUNCTION:
+ case COUNTER_COMP_ENUM:
+ case COUNTER_COMP_COUNT_DIRECTION:
+ case COUNTER_COMP_COUNT_MODE:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ err = comp->device_u32_read(counter, &value_u32);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ err = comp->signal_u32_read(counter, parent,
+ &value_u32);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ err = comp->count_u32_read(counter, parent, &value_u32);
+ break;
+ }
+ *value = value_u32;
+ break;
+ case COUNTER_COMP_U64:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ return comp->device_u64_read(counter, value);
+ case COUNTER_SCOPE_SIGNAL:
+ return comp->signal_u64_read(counter, parent, value);
+ case COUNTER_SCOPE_COUNT:
+ return comp->count_u64_read(counter, parent, value);
+ }
+ break;
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ err = comp->action_read(counter, parent, comp->priv,
+ &value_u32);
+ *value = value_u32;
+ break;
+ }
+
+ return err;
+}
+
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter: pointer to Counter structure
+ * @event: triggered event
+ * @channel: event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ */
+void counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev = {0};
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+ /* Search for event in the list */
+ list_for_each_entry(event_node, &counter->events_list, l)
+ if (event_node->event == event &&
+ event_node->channel == channel)
+ break;
+
+ /* If event is not in the list */
+ if (&event_node->l == &counter->events_list)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, &event_node->comp_list, l) {
+ ev.watch.component = comp_node->component;
+ ev.errno = -counter_get_data(counter, comp_node, &ev.value);
+
+ copied += kfifo_put(&counter->events, ev);
+ }
+
+ if (copied)
+ wake_up_poll(&counter->events_wait, EPOLLIN);
+
+exit_early:
+ raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
new file mode 100644
index 000000000000..cf5a318fe540
--- /dev/null
+++ b/drivers/counter/counter-chrdev.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _COUNTER_CHRDEV_H_
+#define _COUNTER_CHRDEV_H_
+
+#include <linux/counter.h>
+#include <linux/types.h>
+
+int counter_chrdev_add(struct counter_device *const counter,
+ const dev_t counter_devt);
+void counter_chrdev_remove(struct counter_device *const counter);
+
+#endif /* _COUNTER_CHRDEV_H_ */
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 78e07588717b..1334e2f4ce4e 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -5,12 +5,16 @@
*/
#include <linux/counter.h>
#include <linux/device.h>
+#include <linux/device/bus.h>
#include <linux/export.h>
+#include <linux/fs.h>
#include <linux/gfp.h>
#include <linux/idr.h>
#include <linux/init.h>
#include <linux/module.h>
+#include <linux/types.h>
+#include "counter-chrdev.h"
#include "counter-sysfs.h"
/* Provides a unique ID for each counter device */
@@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
.name = "counter"
};
+static dev_t counter_devt;
+
/**
* counter_register - register Counter to the system
* @counter: pointer to Counter to register
@@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
if (counter->id < 0)
return counter->id;
- /* Configure device structure for Counter */
dev->type = &counter_device_type;
dev->bus = &counter_bus_type;
if (counter->parent) {
@@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
device_initialize(dev);
dev_set_drvdata(dev, counter);
+ /* Add Counter character device */
+ err = counter_chrdev_add(counter, counter_devt);
+ if (err)
+ goto err_free_id;
+
/* Add Counter sysfs attributes */
err = counter_sysfs_add(counter);
if (err < 0)
- goto err_free_id;
+ goto err_remove_chrdev;
/* Add device to system */
err = device_add(dev);
if (err < 0)
- goto err_free_id;
+ goto err_remove_chrdev;
return 0;
+err_remove_chrdev:
+ counter_chrdev_remove(counter);
err_free_id:
put_device(dev);
return err;
@@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_counter_register);
+#define COUNTER_DEV_MAX 256
+
static int __init counter_init(void)
{
- return bus_register(&counter_bus_type);
+ int err;
+
+ err = bus_register(&counter_bus_type);
+ if (err < 0)
+ return err;
+
+ err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
+ if (err < 0)
+ goto err_unregister_bus;
+
+ return 0;
+
+err_unregister_bus:
+ bus_unregister(&counter_bus_type);
+ return err;
}
static void __exit counter_exit(void)
{
+ unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
bus_unregister(&counter_bus_type);
}
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 654afa91ae9f..4d97d6e50d48 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
static int counter_name_attr_create(struct device *const dev,
struct counter_attribute_group *const group,
+ const char *const attr_name,
const char *const name)
{
struct counter_attribute *counter_attr;
@@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
/* Configure device attribute */
sysfs_attr_init(&counter_attr->dev_attr.attr);
- counter_attr->dev_attr.attr.name = "name";
+ counter_attr->dev_attr.attr.name = attr_name;
counter_attr->dev_attr.attr.mode = 0444;
counter_attr->dev_attr.show = counter_comp_name_show;
@@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
return 0;
}
+static int counter_ext_name_attr_create(struct device *const dev,
+ struct counter_attribute_group *const group, const size_t i,
+ const char *const name)
+{
+ const char *attr_name;
+
+ attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
+ if (!attr_name)
+ return -ENOMEM;
+
+ return counter_name_attr_create(dev, group, attr_name, name);
+}
static struct counter_comp counter_signal_comp = {
.type = COUNTER_COMP_SIGNAL_LEVEL,
@@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
int err;
struct counter_comp comp;
size_t i;
+ struct counter_comp *ext;
/* Create main Signal attribute */
comp = counter_signal_comp;
@@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
return err;
/* Create Signal name attribute */
- err = counter_name_attr_create(dev, group, signal->name);
+ err = counter_name_attr_create(dev, group, "name", signal->name);
if (err < 0)
return err;
/* Create an attribute for each extension */
for (i = 0; i < signal->num_ext; i++) {
- err = counter_attr_create(dev, group, signal->ext + i, scope,
- signal);
+ ext = signal->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, signal);
+ if (err < 0)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err < 0)
return err;
}
@@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
int err;
struct counter_comp comp;
size_t i;
+ struct counter_comp *ext;
/* Create main Count attribute */
comp = counter_count_comp;
@@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
return err;
/* Create Count name attribute */
- err = counter_name_attr_create(dev, group, count->name);
+ err = counter_name_attr_create(dev, group, "name", count->name);
if (err < 0)
return err;
@@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
/* Create an attribute for each extension */
for (i = 0; i < count->num_ext; i++) {
- err = counter_attr_create(dev, group, count->ext + i, scope,
- count);
+ ext = count->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, count);
+ if (err < 0)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err < 0)
return err;
}
@@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
struct device *const dev = &counter->dev;
int err;
size_t i;
+ struct counter_comp *ext;
/* Add Signals sysfs attributes */
err = counter_sysfs_signals_add(counter, group);
@@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
group += counter->num_counts;
/* Create name attribute */
- err = counter_name_attr_create(dev, group, counter->name);
+ err = counter_name_attr_create(dev, group, "name", counter->name);
if (err < 0)
return err;
@@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
/* Create an attribute for each extension */
for (i = 0; i < counter->num_ext; i++) {
- err = counter_attr_create(dev, group, counter->ext + i, scope,
- NULL);
+ ext = counter->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, NULL);
+ if (err < 0)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err < 0)
return err;
}
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 2f01e1fec857..ce79e62a2e58 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -6,9 +6,15 @@
#ifndef _COUNTER_H_
#define _COUNTER_H_
+#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
#include <linux/types.h>
+#include <linux/wait.h>
+#include <uapi/linux/counter.h>
struct counter_device;
struct counter_count;
@@ -27,47 +33,6 @@ enum counter_comp_type {
COUNTER_COMP_COUNT_MODE,
};
-enum counter_scope {
- COUNTER_SCOPE_DEVICE,
- COUNTER_SCOPE_SIGNAL,
- COUNTER_SCOPE_COUNT,
-};
-
-enum counter_count_direction {
- COUNTER_COUNT_DIRECTION_FORWARD,
- COUNTER_COUNT_DIRECTION_BACKWARD,
-};
-
-enum counter_count_mode {
- COUNTER_COUNT_MODE_NORMAL,
- COUNTER_COUNT_MODE_RANGE_LIMIT,
- COUNTER_COUNT_MODE_NON_RECYCLE,
- COUNTER_COUNT_MODE_MODULO_N,
-};
-
-enum counter_function {
- COUNTER_FUNCTION_INCREASE,
- COUNTER_FUNCTION_DECREASE,
- COUNTER_FUNCTION_PULSE_DIRECTION,
- COUNTER_FUNCTION_QUADRATURE_X1_A,
- COUNTER_FUNCTION_QUADRATURE_X1_B,
- COUNTER_FUNCTION_QUADRATURE_X2_A,
- COUNTER_FUNCTION_QUADRATURE_X2_B,
- COUNTER_FUNCTION_QUADRATURE_X4,
-};
-
-enum counter_signal_level {
- COUNTER_SIGNAL_LEVEL_LOW,
- COUNTER_SIGNAL_LEVEL_HIGH,
-};
-
-enum counter_synapse_action {
- COUNTER_SYNAPSE_ACTION_NONE,
- COUNTER_SYNAPSE_ACTION_RISING_EDGE,
- COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
- COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
-};
-
/**
* struct counter_comp - Counter component node
* @type: Counter component data type
@@ -239,6 +204,20 @@ struct counter_count {
size_t num_ext;
};
+/**
+ * struct counter_event_node - Counter Event node
+ * @l: list of current watching Counter events
+ * @event: event that triggers
+ * @channel: event channel
+ * @comp_list: list of components to watch when event triggers
+ */
+struct counter_event_node {
+ struct list_head l;
+ u8 event;
+ u8 channel;
+ struct list_head comp_list;
+};
+
/**
* struct counter_ops - Callbacks from driver
* @signal_read: read callback for Signals. The read level of the
@@ -261,6 +240,13 @@ struct counter_count {
* @action_write: write callback for Synapse action modes. The action mode
* to write for the respective Synapse is passed in via the
* action parameter.
+ * @events_configure: write callback to configure events. The list of struct
+ * counter_event_node may be accessed via the events_list
+ * member of the counter parameter.
+ * @watch_validate: callback to validate a watch. The Counter component
+ * watch configuration is passed in via the watch
+ * parameter. A return value of 0 indicates a valid Counter
+ * component watch configuration.
*/
struct counter_ops {
int (*signal_read)(struct counter_device *counter,
@@ -284,6 +270,9 @@ struct counter_ops {
struct counter_count *count,
struct counter_synapse *synapse,
enum counter_synapse_action action);
+ int (*events_configure)(struct counter_device *counter);
+ int (*watch_validate)(struct counter_device *counter,
+ const struct counter_watch *watch);
};
/**
@@ -300,6 +289,13 @@ struct counter_ops {
* @priv: optional private data supplied by driver
* @id: unique ID used to identify the Counter
* @dev: internal device structure
+ * @chrdev: internal character device structure
+ * @events_list: list of current watching Counter events
+ * @events_list_lock: lock to protect Counter events list operations
+ * @next_events_list: list of next watching Counter events
+ * @events: queue of detected Counter events
+ * @events_wait: wait queue to allow blocking reads of Counter events
+ * @events_lock: lock to protect Counter events queue read operations
*/
struct counter_device {
const char *name;
@@ -319,12 +315,21 @@ struct counter_device {
int id;
struct device dev;
+ struct cdev chrdev;
+ struct list_head events_list;
+ raw_spinlock_t events_list_lock;
+ struct list_head next_events_list;
+ DECLARE_KFIFO(events, struct counter_event, 64);
+ wait_queue_head_t events_wait;
+ struct mutex events_lock;
};
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
int devm_counter_register(struct device *dev,
struct counter_device *const counter);
+void counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel);
#define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
{ \
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index 000000000000..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,123 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Userspace ABI for Counter character devices
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _UAPI_COUNTER_H_
+#define _UAPI_COUNTER_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/* Component type definitions */
+enum counter_component_type {
+ COUNTER_COMPONENT_NONE,
+ COUNTER_COMPONENT_SIGNAL,
+ COUNTER_COMPONENT_COUNT,
+ COUNTER_COMPONENT_FUNCTION,
+ COUNTER_COMPONENT_SYNAPSE_ACTION,
+ COUNTER_COMPONENT_EXTENSION,
+};
+
+/* Component scope definitions */
+enum counter_scope {
+ COUNTER_SCOPE_DEVICE,
+ COUNTER_SCOPE_SIGNAL,
+ COUNTER_SCOPE_COUNT,
+};
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
+ * @scope: component scope (Device, Count, or Signal)
+ * @parent: parent component identification number
+ * @id: component identification number
+ */
+struct counter_component {
+ __u8 type;
+ __u8 scope;
+ __u8 parent;
+ __u8 id;
+};
+
+/* Event type definitions */
+enum counter_event_type {
+ COUNTER_EVENT_OVERFLOW,
+ COUNTER_EVENT_UNDERFLOW,
+ COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+ COUNTER_EVENT_THRESHOLD,
+ COUNTER_EVENT_INDEX,
+};
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
+ * @channel: event channel
+ */
+struct counter_watch {
+ struct counter_component component;
+ __u8 event;
+ __u8 channel;
+};
+
+/* ioctl commands */
+#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
+#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
+
+/**
+ * struct counter_event - Counter event data
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ * @watch: component watch configuration
+ * @errno: system error number
+ */
+struct counter_event {
+ __aligned_u64 timestamp;
+ __aligned_u64 value;
+ struct counter_watch watch;
+ __u8 errno;
+};
+
+/* Count direction values */
+enum counter_count_direction {
+ COUNTER_COUNT_DIRECTION_FORWARD,
+ COUNTER_COUNT_DIRECTION_BACKWARD,
+};
+
+/* Count mode values */
+enum counter_count_mode {
+ COUNTER_COUNT_MODE_NORMAL,
+ COUNTER_COUNT_MODE_RANGE_LIMIT,
+ COUNTER_COUNT_MODE_NON_RECYCLE,
+ COUNTER_COUNT_MODE_MODULO_N,
+};
+
+/* Count function values */
+enum counter_function {
+ COUNTER_FUNCTION_INCREASE,
+ COUNTER_FUNCTION_DECREASE,
+ COUNTER_FUNCTION_PULSE_DIRECTION,
+ COUNTER_FUNCTION_QUADRATURE_X1_A,
+ COUNTER_FUNCTION_QUADRATURE_X1_B,
+ COUNTER_FUNCTION_QUADRATURE_X2_A,
+ COUNTER_FUNCTION_QUADRATURE_X2_B,
+ COUNTER_FUNCTION_QUADRATURE_X4,
+};
+
+/* Signal values */
+enum counter_signal_level {
+ COUNTER_SIGNAL_LEVEL_LOW,
+ COUNTER_SIGNAL_LEVEL_HIGH,
+};
+
+/* Action mode values */
+enum counter_synapse_action {
+ COUNTER_SYNAPSE_ACTION_NONE,
+ COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+ COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+ COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+#endif /* _UAPI_COUNTER_H_ */
--
2.29.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 3/5] counter: Add character device interface
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
@ 2020-12-30 15:04 ` Jonathan Cameron
2021-02-12 6:32 ` William Breathitt Gray
2020-12-30 21:36 ` David Lechner
2021-01-28 9:01 ` Oleksij Rempel
2 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2020-12-30 15:04 UTC (permalink / raw)
To: William Breathitt Gray
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, Dan Carpenter
On Fri, 25 Dec 2020 19:15:36 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
>
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
There are a few things in here that could profitably be pulled out as precursor
patches. I don't really understand the connection of extension_name to the
addition of a chardev for example. Might be needed to provide enough
info to actually use the chardev, but does it have meaning without that?
Either way, definitely feels like it can be done in a separate patch.
> ---
> MAINTAINERS | 1 +
> drivers/counter/Makefile | 2 +-
> drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
> drivers/counter/counter-chrdev.h | 16 +
> drivers/counter/counter-core.c | 37 ++-
> drivers/counter/counter-sysfs.c | 51 +++-
> include/linux/counter.h | 87 +++---
> include/uapi/linux/counter.h | 123 ++++++++
> 8 files changed, 751 insertions(+), 56 deletions(-)
> create mode 100644 drivers/counter/counter-chrdev.c
> create mode 100644 drivers/counter/counter-chrdev.h
> create mode 100644 include/uapi/linux/counter.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b64fa49d5796..3a240f70bdd4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4494,6 +4494,7 @@ F: Documentation/ABI/testing/sysfs-bus-counter*
> F: Documentation/driver-api/generic-counter.rst
> F: drivers/counter/
> F: include/linux/counter.h
> +F: include/uapi/linux/counter.h
>
> CPMAC ETHERNET DRIVER
> M: Florian Fainelli <f.fainelli@gmail.com>
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index cbe1d06af6a9..c4870eb5b1dd 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -4,7 +4,7 @@
> #
>
> obj-$(CONFIG_COUNTER) += counter.o
> -counter-y := counter-core.o counter-sysfs.o
> +counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
>
> obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
> obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> new file mode 100644
> index 000000000000..61b11989546a
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/counter.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kfifo.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/nospec.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/timekeeping.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +#include <linux/uaccess.h>
> +
> +#include "counter-chrdev.h"
> +
> +struct counter_comp_node {
> + struct list_head l;
> + struct counter_component component;
> + struct counter_comp comp;
> + void *parent;
> +};
> +
> +static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
> + size_t len, loff_t *f_ps)
> +{
> + struct counter_device *const counter = filp->private_data;
> + int err;
> + unsigned int copied;
> +
> + if (len < sizeof(struct counter_event))
> + return -EINVAL;
> +
> + do {
> + if (kfifo_is_empty(&counter->events)) {
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + err = wait_event_interruptible(counter->events_wait,
> + !kfifo_is_empty(&counter->events));
> + if (err < 0)
> + return err;
> + }
> +
> + if (mutex_lock_interruptible(&counter->events_lock))
> + return -ERESTARTSYS;
> + err = kfifo_to_user(&counter->events, buf, len, &copied);
> + mutex_unlock(&counter->events_lock);
> + if (err < 0)
> + return err;
> + } while (!copied);
> +
> + return copied;
> +}
> +
> +static __poll_t counter_chrdev_poll(struct file *filp,
> + struct poll_table_struct *pollt)
> +{
> + struct counter_device *const counter = filp->private_data;
> + __poll_t events = 0;
> +
> + poll_wait(filp, &counter->events_wait, pollt);
> +
> + if (!kfifo_is_empty(&counter->events))
> + events = EPOLLIN | EPOLLRDNORM;
> +
> + return events;
> +}
> +
> +static void counter_events_list_free(struct list_head *const events_list)
> +{
> + struct counter_event_node *p, *n;
> + struct counter_comp_node *q, *o;
> +
> + list_for_each_entry_safe(p, n, events_list, l) {
> + /* Free associated component nodes */
> + list_for_each_entry_safe(q, o, &p->comp_list, l) {
> + list_del(&q->l);
> + kfree(q);
> + }
> +
> + /* Free event node */
> + list_del(&p->l);
> + kfree(p);
> + }
> +}
> +
> +static int counter_set_event_node(struct counter_device *const counter,
> + struct counter_watch *const watch,
> + const struct counter_comp_node *const cfg)
> +{
> + struct counter_event_node *event_node;
> + struct counter_comp_node *comp_node;
> +
> + /* Search for event in the list */
> + list_for_each_entry(event_node, &counter->next_events_list, l)
> + if (event_node->event == watch->event &&
> + event_node->channel == watch->channel)
> + break;
> +
> + /* If event is not already in the list */
> + if (&event_node->l == &counter->next_events_list) {
> + /* Allocate new event node */
> + event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
> + if (!event_node)
> + return -ENOMEM;
> +
> + /* Configure event node and add to the list */
> + event_node->event = watch->event;
> + event_node->channel = watch->channel;
> + INIT_LIST_HEAD(&event_node->comp_list);
> + list_add(&event_node->l, &counter->next_events_list);
> + }
> +
> + /* Check if component watch has already been set before */
> + list_for_each_entry(comp_node, &event_node->comp_list, l)
> + if (comp_node->parent == cfg->parent &&
> + comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
> + return -EINVAL;
> +
> + /* Allocate component node */
> + comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
> + if (!comp_node) {
> + /* Free event node if no one else is watching */
> + if (list_empty(&event_node->comp_list)) {
> + list_del(&event_node->l);
> + kfree(event_node);
> + }
> + return -ENOMEM;
> + }
> + *comp_node = *cfg;
> +
> + /* Add component node to event node */
> + list_add_tail(&comp_node->l, &event_node->comp_list);
> +
> + return 0;
> +}
> +
> +static int counter_clear_watches(struct counter_device *const counter)
> +{
> + unsigned long flags;
> + int err = 0;
> +
> + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> + counter_events_list_free(&counter->events_list);
> +
> + if (counter->ops->events_configure)
> + err = counter->ops->events_configure(counter);
> +
> + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +
> + counter_events_list_free(&counter->next_events_list);
> +
> + return err;
> +}
> +
> +static int counter_add_watch(struct counter_device *const counter,
> + const unsigned long arg)
> +{
> + void __user *const uwatch = (void __user *)arg;
> + struct counter_watch watch;
> + struct counter_comp_node comp_node = {0};
> + size_t parent, id;
> + struct counter_comp *ext;
> + size_t num_ext;
> + int err;
> +
> + if (copy_from_user(&watch, uwatch, sizeof(watch)))
> + return -EFAULT;
> +
> + if (watch.component.type == COUNTER_COMPONENT_NONE)
> + goto no_component;
> +
> + parent = watch.component.parent;
> +
> + /* Configure parent component info for comp node */
> + switch (watch.component.scope) {
> + case COUNTER_SCOPE_DEVICE:
> + ext = counter->ext;
> + num_ext = counter->num_ext;
> + break;
> + case COUNTER_SCOPE_SIGNAL:
> + if (parent >= counter->num_signals)
> + return -EINVAL;
> + parent = array_index_nospec(parent, counter->num_signals);
> +
> + comp_node.parent = counter->signals + parent;
> +
> + ext = counter->signals[parent].ext;
> + num_ext = counter->signals[parent].num_ext;
> + break;
> + case COUNTER_SCOPE_COUNT:
> + if (parent >= counter->num_counts)
> + return -EINVAL;
> + parent = array_index_nospec(parent, counter->num_counts);
> +
> + comp_node.parent = counter->counts + parent;
> +
> + ext = counter->counts[parent].ext;
> + num_ext = counter->counts[parent].num_ext;
> + break;
> + }
> +
> + id = watch.component.id;
> +
> + /* Configure component info for comp node */
> + switch (watch.component.type) {
> + case COUNTER_COMPONENT_SIGNAL:
> + if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
> + return -EINVAL;
> +
> + comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
> + comp_node.comp.signal_u32_read = counter->ops->signal_read;
> + break;
> + case COUNTER_COMPONENT_COUNT:
> + if (watch.component.scope != COUNTER_SCOPE_COUNT)
> + return -EINVAL;
> +
> + comp_node.comp.type = COUNTER_COMP_U64;
> + comp_node.comp.count_u64_read = counter->ops->count_read;
> + break;
> + case COUNTER_COMPONENT_FUNCTION:
> + if (watch.component.scope != COUNTER_SCOPE_COUNT)
> + return -EINVAL;
> +
> + comp_node.comp.type = COUNTER_COMP_FUNCTION;
> + comp_node.comp.count_u32_read = counter->ops->function_read;
> + break;
> + case COUNTER_COMPONENT_SYNAPSE_ACTION:
> + if (watch.component.scope != COUNTER_SCOPE_COUNT)
> + return -EINVAL;
> + if (id >= counter->counts[parent].num_synapses)
> + return -EINVAL;
> + id = array_index_nospec(id, counter->counts[parent].num_synapses);
> +
> + comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
> + comp_node.comp.action_read = counter->ops->action_read;
> + comp_node.comp.priv = counter->counts[parent].synapses + id;
> + break;
> + case COUNTER_COMPONENT_EXTENSION:
> + if (id >= num_ext)
> + return -EINVAL;
> + id = array_index_nospec(id, num_ext);
> +
> + comp_node.comp = ext[id];
> + break;
> + default:
> + return -EINVAL;
> + }
> + /* Check if any read callback is set; this is part of a union */
> + if (!comp_node.comp.count_u8_read)
> + return -EOPNOTSUPP;
> +
> +no_component:
> + if (counter->ops->watch_validate) {
> + err = counter->ops->watch_validate(counter, &watch);
> + if (err < 0)
> + return err;
> + }
> +
> + comp_node.component = watch.component;
> +
> + return counter_set_event_node(counter, &watch, &comp_node);
> +}
> +
> +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct counter_device *const counter = filp->private_data;
> + unsigned long flags;
> + int err = 0;
> +
> + switch (cmd) {
> + case COUNTER_CLEAR_WATCHES_IOCTL:
> + return counter_clear_watches(counter);
> + case COUNTER_ADD_WATCH_IOCTL:
> + return counter_add_watch(counter, arg);
> + case COUNTER_LOAD_WATCHES_IOCTL:
> + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> + counter_events_list_free(&counter->events_list);
> + list_replace_init(&counter->next_events_list,
> + &counter->events_list);
> +
> + if (counter->ops->events_configure)
> + err = counter->ops->events_configure(counter);
> +
> + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> + break;
return here.
> + default:
> + return -ENOIOCTLCMD;
> + }
> +
> + return err;
> +}
> +
> +static int counter_chrdev_open(struct inode *inode, struct file *filp)
> +{
> + struct counter_device *const counter = container_of(inode->i_cdev,
> + typeof(*counter),
> + chrdev);
> +
> + get_device(&counter->dev);
> + filp->private_data = counter;
> +
> + return nonseekable_open(inode, filp);
> +}
> +
> +static int counter_chrdev_release(struct inode *inode, struct file *filp)
> +{
> + struct counter_device *const counter = filp->private_data;
> + int err;
> +
> + err = counter_clear_watches(counter);
> + if (err < 0)
> + return err;
> +
> + put_device(&counter->dev);
> +
> + return 0;
> +}
> +
> +static const struct file_operations counter_fops = {
> + .llseek = no_llseek,
> + .read = counter_chrdev_read,
> + .poll = counter_chrdev_poll,
> + .unlocked_ioctl = counter_chrdev_ioctl,
> + .open = counter_chrdev_open,
> + .release = counter_chrdev_release,
> +};
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> + const dev_t counter_devt)
> +{
> + struct device *const dev = &counter->dev;
> + struct cdev *const chrdev = &counter->chrdev;
> +
> + /* Initialize Counter events lists */
> + INIT_LIST_HEAD(&counter->events_list);
> + INIT_LIST_HEAD(&counter->next_events_list);
> + raw_spin_lock_init(&counter->events_list_lock);
> +
> + /* Initialize Counter events queue */
> + INIT_KFIFO(counter->events);
> + init_waitqueue_head(&counter->events_wait);
> + mutex_init(&counter->events_lock);
> +
> + /* Initialize character device */
> + cdev_init(chrdev, &counter_fops);
> + dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
> + cdev_set_parent(chrdev, &dev->kobj);
> +
> + return cdev_add(chrdev, dev->devt, 1);
> +}
> +
> +void counter_chrdev_remove(struct counter_device *const counter)
> +{
> + cdev_del(&counter->chrdev);
> +}
> +
> +static int counter_get_data(struct counter_device *const counter,
> + const struct counter_comp_node *const comp_node,
> + u64 *const value)
> +{
> + const struct counter_comp *const comp = &comp_node->comp;
> + void *const parent = comp_node->parent;
> + int err = 0;
> + u8 value_u8 = 0;
> + u32 value_u32 = 0;
> +
> + if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> + return 0;
> +
> + switch (comp->type) {
> + case COUNTER_COMP_U8:
> + case COUNTER_COMP_BOOL:
> + switch (comp_node->component.scope) {
> + case COUNTER_SCOPE_DEVICE:
> + err = comp->device_u8_read(counter, &value_u8);
> + break;
> + case COUNTER_SCOPE_SIGNAL:
> + err = comp->signal_u8_read(counter, parent, &value_u8);
> + break;
> + case COUNTER_SCOPE_COUNT:
> + err = comp->count_u8_read(counter, parent, &value_u8);
> + break;
> + }
> + *value = value_u8;
> + break;
> + case COUNTER_COMP_SIGNAL_LEVEL:
> + case COUNTER_COMP_FUNCTION:
> + case COUNTER_COMP_ENUM:
> + case COUNTER_COMP_COUNT_DIRECTION:
> + case COUNTER_COMP_COUNT_MODE:
> + switch (comp_node->component.scope) {
> + case COUNTER_SCOPE_DEVICE:
> + err = comp->device_u32_read(counter, &value_u32);
> + break;
> + case COUNTER_SCOPE_SIGNAL:
> + err = comp->signal_u32_read(counter, parent,
> + &value_u32);
> + break;
> + case COUNTER_SCOPE_COUNT:
> + err = comp->count_u32_read(counter, parent, &value_u32);
> + break;
> + }
> + *value = value_u32;
Seems like a return here would make more sense as no shared stuff to do at
end of the switch. Same in other similar cases.
> + break;
> + case COUNTER_COMP_U64:
> + switch (comp_node->component.scope) {
> + case COUNTER_SCOPE_DEVICE:
> + return comp->device_u64_read(counter, value);
> + case COUNTER_SCOPE_SIGNAL:
> + return comp->signal_u64_read(counter, parent, value);
> + case COUNTER_SCOPE_COUNT:
> + return comp->count_u64_read(counter, parent, value);
> + }
> + break;
> + case COUNTER_COMP_SYNAPSE_ACTION:
> + err = comp->action_read(counter, parent, comp->priv,
> + &value_u32);
> + *value = value_u32;
> + break;
> + }
> +
> + return err;
> +}
> +
> +/**
> + * counter_push_event - queue event for userspace reading
> + * @counter: pointer to Counter structure
> + * @event: triggered event
> + * @channel: event channel
> + *
> + * Note: If no one is watching for the respective event, it is silently
> + * discarded.
> + */
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> + const u8 channel)
> +{
> + struct counter_event ev = {0};
> + unsigned int copied = 0;
> + unsigned long flags;
> + struct counter_event_node *event_node;
> + struct counter_comp_node *comp_node;
> +
> + ev.timestamp = ktime_get_ns();
> + ev.watch.event = event;
> + ev.watch.channel = channel;
> +
> + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
For a raw spin lock, we definitely want to see comments on why it
is necessary.
> +
> + /* Search for event in the list */
> + list_for_each_entry(event_node, &counter->events_list, l)
> + if (event_node->event == event &&
> + event_node->channel == channel)
> + break;
> +
> + /* If event is not in the list */
> + if (&event_node->l == &counter->events_list)
> + goto exit_early;
> +
> + /* Read and queue relevant comp for userspace */
> + list_for_each_entry(comp_node, &event_node->comp_list, l) {
> + ev.watch.component = comp_node->component;
> + ev.errno = -counter_get_data(counter, comp_node, &ev.value);
> +
> + copied += kfifo_put(&counter->events, ev);
> + }
> +
> + if (copied)
> + wake_up_poll(&counter->events_wait, EPOLLIN);
> +
> +exit_early:
> + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(counter_push_event);
> diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
> new file mode 100644
> index 000000000000..cf5a318fe540
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _COUNTER_CHRDEV_H_
> +#define _COUNTER_CHRDEV_H_
> +
> +#include <linux/counter.h>
> +#include <linux/types.h>
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> + const dev_t counter_devt);
> +void counter_chrdev_remove(struct counter_device *const counter);
> +
> +#endif /* _COUNTER_CHRDEV_H_ */
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 78e07588717b..1334e2f4ce4e 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -5,12 +5,16 @@
> */
> #include <linux/counter.h>
> #include <linux/device.h>
> +#include <linux/device/bus.h>
> #include <linux/export.h>
> +#include <linux/fs.h>
> #include <linux/gfp.h>
> #include <linux/idr.h>
> #include <linux/init.h>
> #include <linux/module.h>
> +#include <linux/types.h>
>
> +#include "counter-chrdev.h"
> #include "counter-sysfs.h"
>
> /* Provides a unique ID for each counter device */
> @@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
> .name = "counter"
> };
>
> +static dev_t counter_devt;
> +
> /**
> * counter_register - register Counter to the system
> * @counter: pointer to Counter to register
> @@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
> if (counter->id < 0)
> return counter->id;
>
> - /* Configure device structure for Counter */
> dev->type = &counter_device_type;
> dev->bus = &counter_bus_type;
> if (counter->parent) {
> @@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
> device_initialize(dev);
> dev_set_drvdata(dev, counter);
>
> + /* Add Counter character device */
> + err = counter_chrdev_add(counter, counter_devt);
> + if (err)
> + goto err_free_id;
> +
> /* Add Counter sysfs attributes */
> err = counter_sysfs_add(counter);
> if (err < 0)
> - goto err_free_id;
> + goto err_remove_chrdev;
>
> /* Add device to system */
> err = device_add(dev);
> if (err < 0)
> - goto err_free_id;
> + goto err_remove_chrdev;
>
> return 0;
>
> +err_remove_chrdev:
> + counter_chrdev_remove(counter);
> err_free_id:
> put_device(dev);
> return err;
> @@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_counter_register);
>
> +#define COUNTER_DEV_MAX 256
> +
> static int __init counter_init(void)
> {
> - return bus_register(&counter_bus_type);
> + int err;
> +
> + err = bus_register(&counter_bus_type);
> + if (err < 0)
> + return err;
> +
> + err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
> + if (err < 0)
> + goto err_unregister_bus;
> +
> + return 0;
> +
> +err_unregister_bus:
> + bus_unregister(&counter_bus_type);
> + return err;
> }
>
> static void __exit counter_exit(void)
> {
> + unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
> bus_unregister(&counter_bus_type);
> }
>
> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> index 654afa91ae9f..4d97d6e50d48 100644
> --- a/drivers/counter/counter-sysfs.c
> +++ b/drivers/counter/counter-sysfs.c
> @@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
>
> static int counter_name_attr_create(struct device *const dev,
> struct counter_attribute_group *const group,
> + const char *const attr_name,
> const char *const name)
> {
> struct counter_attribute *counter_attr;
> @@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
>
> /* Configure device attribute */
> sysfs_attr_init(&counter_attr->dev_attr.attr);
> - counter_attr->dev_attr.attr.name = "name";
> + counter_attr->dev_attr.attr.name = attr_name;
> counter_attr->dev_attr.attr.mode = 0444;
> counter_attr->dev_attr.show = counter_comp_name_show;
>
> @@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
> return 0;
> }
>
> +static int counter_ext_name_attr_create(struct device *const dev,
> + struct counter_attribute_group *const group, const size_t i,
> + const char *const name)
> +{
> + const char *attr_name;
> +
> + attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
> + if (!attr_name)
> + return -ENOMEM;
> +
> + return counter_name_attr_create(dev, group, attr_name, name);
> +}
>
> static struct counter_comp counter_signal_comp = {
> .type = COUNTER_COMP_SIGNAL_LEVEL,
> @@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
> int err;
> struct counter_comp comp;
> size_t i;
> + struct counter_comp *ext;
>
> /* Create main Signal attribute */
> comp = counter_signal_comp;
> @@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
> return err;
>
> /* Create Signal name attribute */
> - err = counter_name_attr_create(dev, group, signal->name);
> + err = counter_name_attr_create(dev, group, "name", signal->name);
> if (err < 0)
> return err;
>
> /* Create an attribute for each extension */
> for (i = 0; i < signal->num_ext; i++) {
> - err = counter_attr_create(dev, group, signal->ext + i, scope,
> - signal);
> + ext = signal->ext + i;
> +
> + err = counter_attr_create(dev, group, ext, scope, signal);
> + if (err < 0)
> + return err;
> +
> + err = counter_ext_name_attr_create(dev, group, i, ext->name);
> if (err < 0)
> return err;
> }
> @@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> int err;
> struct counter_comp comp;
> size_t i;
> + struct counter_comp *ext;
>
> /* Create main Count attribute */
> comp = counter_count_comp;
> @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> return err;
>
> /* Create Count name attribute */
> - err = counter_name_attr_create(dev, group, count->name);
> + err = counter_name_attr_create(dev, group, "name", count->name);
This refactoring could also be pulled out to a precusor patch.
> if (err < 0)
> return err;
>
> @@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>
> /* Create an attribute for each extension */
> for (i = 0; i < count->num_ext; i++) {
> - err = counter_attr_create(dev, group, count->ext + i, scope,
> - count);
> + ext = count->ext + i;
> +
> + err = counter_attr_create(dev, group, ext, scope, count);
> + if (err < 0)
> + return err;
> +
> + err = counter_ext_name_attr_create(dev, group, i, ext->name);
> if (err < 0)
> return err;
> }
> @@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
> struct device *const dev = &counter->dev;
> int err;
> size_t i;
> + struct counter_comp *ext;
>
> /* Add Signals sysfs attributes */
> err = counter_sysfs_signals_add(counter, group);
> @@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
> group += counter->num_counts;
>
> /* Create name attribute */
> - err = counter_name_attr_create(dev, group, counter->name);
> + err = counter_name_attr_create(dev, group, "name", counter->name);
> if (err < 0)
> return err;
>
> @@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>
> /* Create an attribute for each extension */
> for (i = 0; i < counter->num_ext; i++) {
> - err = counter_attr_create(dev, group, counter->ext + i, scope,
> - NULL);
> + ext = counter->ext + i;
> +
> + err = counter_attr_create(dev, group, ext, scope, NULL);
> + if (err < 0)
> + return err;
> +
> + err = counter_ext_name_attr_create(dev, group, i, ext->name);
> if (err < 0)
> return err;
> }
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index 2f01e1fec857..ce79e62a2e58 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -6,9 +6,15 @@
> #ifndef _COUNTER_H_
> #define _COUNTER_H_
>
> +#include <linux/cdev.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock_types.h>
> #include <linux/types.h>
> +#include <linux/wait.h>
> +#include <uapi/linux/counter.h>
>
> struct counter_device;
> struct counter_count;
> @@ -27,47 +33,6 @@ enum counter_comp_type {
> COUNTER_COMP_COUNT_MODE,
> };
>
> -enum counter_scope {
> - COUNTER_SCOPE_DEVICE,
> - COUNTER_SCOPE_SIGNAL,
> - COUNTER_SCOPE_COUNT,
> -};
> -
> -enum counter_count_direction {
> - COUNTER_COUNT_DIRECTION_FORWARD,
> - COUNTER_COUNT_DIRECTION_BACKWARD,
> -};
> -
> -enum counter_count_mode {
> - COUNTER_COUNT_MODE_NORMAL,
> - COUNTER_COUNT_MODE_RANGE_LIMIT,
> - COUNTER_COUNT_MODE_NON_RECYCLE,
> - COUNTER_COUNT_MODE_MODULO_N,
> -};
> -
> -enum counter_function {
> - COUNTER_FUNCTION_INCREASE,
> - COUNTER_FUNCTION_DECREASE,
> - COUNTER_FUNCTION_PULSE_DIRECTION,
> - COUNTER_FUNCTION_QUADRATURE_X1_A,
> - COUNTER_FUNCTION_QUADRATURE_X1_B,
> - COUNTER_FUNCTION_QUADRATURE_X2_A,
> - COUNTER_FUNCTION_QUADRATURE_X2_B,
> - COUNTER_FUNCTION_QUADRATURE_X4,
> -};
> -
> -enum counter_signal_level {
> - COUNTER_SIGNAL_LEVEL_LOW,
> - COUNTER_SIGNAL_LEVEL_HIGH,
> -};
> -
> -enum counter_synapse_action {
> - COUNTER_SYNAPSE_ACTION_NONE,
> - COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> - COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> - COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> -};
> -
> /**
> * struct counter_comp - Counter component node
> * @type: Counter component data type
> @@ -239,6 +204,20 @@ struct counter_count {
> size_t num_ext;
> };
>
> +/**
> + * struct counter_event_node - Counter Event node
> + * @l: list of current watching Counter events
> + * @event: event that triggers
> + * @channel: event channel
> + * @comp_list: list of components to watch when event triggers
> + */
> +struct counter_event_node {
> + struct list_head l;
> + u8 event;
> + u8 channel;
> + struct list_head comp_list;
> +};
> +
> /**
> * struct counter_ops - Callbacks from driver
> * @signal_read: read callback for Signals. The read level of the
> @@ -261,6 +240,13 @@ struct counter_count {
> * @action_write: write callback for Synapse action modes. The action mode
> * to write for the respective Synapse is passed in via the
> * action parameter.
> + * @events_configure: write callback to configure events. The list of struct
> + * counter_event_node may be accessed via the events_list
> + * member of the counter parameter.
> + * @watch_validate: callback to validate a watch. The Counter component
> + * watch configuration is passed in via the watch
> + * parameter. A return value of 0 indicates a valid Counter
> + * component watch configuration.
> */
> struct counter_ops {
> int (*signal_read)(struct counter_device *counter,
> @@ -284,6 +270,9 @@ struct counter_ops {
> struct counter_count *count,
> struct counter_synapse *synapse,
> enum counter_synapse_action action);
> + int (*events_configure)(struct counter_device *counter);
> + int (*watch_validate)(struct counter_device *counter,
> + const struct counter_watch *watch);
> };
>
> /**
> @@ -300,6 +289,13 @@ struct counter_ops {
> * @priv: optional private data supplied by driver
> * @id: unique ID used to identify the Counter
> * @dev: internal device structure
> + * @chrdev: internal character device structure
> + * @events_list: list of current watching Counter events
> + * @events_list_lock: lock to protect Counter events list operations
> + * @next_events_list: list of next watching Counter events
> + * @events: queue of detected Counter events
> + * @events_wait: wait queue to allow blocking reads of Counter events
> + * @events_lock: lock to protect Counter events queue read operations
> */
> struct counter_device {
> const char *name;
> @@ -319,12 +315,21 @@ struct counter_device {
>
> int id;
> struct device dev;
> + struct cdev chrdev;
> + struct list_head events_list;
> + raw_spinlock_t events_list_lock;
> + struct list_head next_events_list;
> + DECLARE_KFIFO(events, struct counter_event, 64);
Why 64? Probably want that to be somewhat dynamic, even if only at build time.
> + wait_queue_head_t events_wait;
> + struct mutex events_lock;
> };
>
> int counter_register(struct counter_device *const counter);
> void counter_unregister(struct counter_device *const counter);
> int devm_counter_register(struct device *dev,
> struct counter_device *const counter);
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> + const u8 channel);
>
> #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
> { \
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
Small thing but I would have been tempted to do a precursor patch to the
main change simply putting in place the userspace header.
Classic Nop patch that makes it easier to focus on the real stuff in this
patch by getting that noise out of the way!
Jonathan
> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> + COUNTER_COMPONENT_NONE,
> + COUNTER_COMPONENT_SIGNAL,
> + COUNTER_COMPONENT_COUNT,
> + COUNTER_COMPONENT_FUNCTION,
> + COUNTER_COMPONENT_SYNAPSE_ACTION,
> + COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {
> + COUNTER_SCOPE_DEVICE,
> + COUNTER_SCOPE_SIGNAL,
> + COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)
> + * @scope: component scope (Device, Count, or Signal)
> + * @parent: parent component identification number
> + * @id: component identification number
> + */
> +struct counter_component {
> + __u8 type;
> + __u8 scope;
> + __u8 parent;
> + __u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> + COUNTER_EVENT_OVERFLOW,
> + COUNTER_EVENT_UNDERFLOW,
> + COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> + COUNTER_EVENT_THRESHOLD,
> + COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers
> + * @channel: event channel
> + */
> +struct counter_watch {
> + struct counter_component component;
> + __u8 event;
> + __u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> + __aligned_u64 timestamp;
> + __aligned_u64 value;
> + struct counter_watch watch;
> + __u8 errno;
> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> + COUNTER_COUNT_DIRECTION_FORWARD,
> + COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> + COUNTER_COUNT_MODE_NORMAL,
> + COUNTER_COUNT_MODE_RANGE_LIMIT,
> + COUNTER_COUNT_MODE_NON_RECYCLE,
> + COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> + COUNTER_FUNCTION_INCREASE,
> + COUNTER_FUNCTION_DECREASE,
> + COUNTER_FUNCTION_PULSE_DIRECTION,
> + COUNTER_FUNCTION_QUADRATURE_X1_A,
> + COUNTER_FUNCTION_QUADRATURE_X1_B,
> + COUNTER_FUNCTION_QUADRATURE_X2_A,
> + COUNTER_FUNCTION_QUADRATURE_X2_B,
> + COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> + COUNTER_SIGNAL_LEVEL_LOW,
> + COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> + COUNTER_SYNAPSE_ACTION_NONE,
> + COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> + COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> + COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 3/5] counter: Add character device interface
2020-12-30 15:04 ` Jonathan Cameron
@ 2021-02-12 6:32 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-02-12 6:32 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 7511 bytes --]
On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:36 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> >
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
> There are a few things in here that could profitably be pulled out as precursor
> patches. I don't really understand the connection of extension_name to the
> addition of a chardev for example. Might be needed to provide enough
> info to actually use the chardev, but does it have meaning without that?
> Either way, definitely feels like it can be done in a separate patch.
The extension_name attributes are needed so chrdev users have enough
info to identify which extension number corresponds to which extension.
I'll move this to change to a separate patch and provide an appropriate
explanation there to make things clearer.
> > +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct counter_device *const counter = filp->private_data;
> > + unsigned long flags;
> > + int err = 0;
> > +
> > + switch (cmd) {
> > + case COUNTER_CLEAR_WATCHES_IOCTL:
> > + return counter_clear_watches(counter);
> > + case COUNTER_ADD_WATCH_IOCTL:
> > + return counter_add_watch(counter, arg);
> > + case COUNTER_LOAD_WATCHES_IOCTL:
> > + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> > +
> > + counter_events_list_free(&counter->events_list);
> > + list_replace_init(&counter->next_events_list,
> > + &counter->events_list);
> > +
> > + if (counter->ops->events_configure)
> > + err = counter->ops->events_configure(counter);
> > +
> > + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> > + break;
>
> return here.
Ack.
> > +static int counter_get_data(struct counter_device *const counter,
> > + const struct counter_comp_node *const comp_node,
> > + u64 *const value)
> > +{
> > + const struct counter_comp *const comp = &comp_node->comp;
> > + void *const parent = comp_node->parent;
> > + int err = 0;
> > + u8 value_u8 = 0;
> > + u32 value_u32 = 0;
> > +
> > + if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> > + return 0;
> > +
> > + switch (comp->type) {
> > + case COUNTER_COMP_U8:
> > + case COUNTER_COMP_BOOL:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + err = comp->device_u8_read(counter, &value_u8);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u8_read(counter, parent, &value_u8);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u8_read(counter, parent, &value_u8);
> > + break;
> > + }
> > + *value = value_u8;
> > + break;
> > + case COUNTER_COMP_SIGNAL_LEVEL:
> > + case COUNTER_COMP_FUNCTION:
> > + case COUNTER_COMP_ENUM:
> > + case COUNTER_COMP_COUNT_DIRECTION:
> > + case COUNTER_COMP_COUNT_MODE:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + err = comp->device_u32_read(counter, &value_u32);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u32_read(counter, parent,
> > + &value_u32);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u32_read(counter, parent, &value_u32);
> > + break;
> > + }
> > + *value = value_u32;
>
> Seems like a return here would make more sense as no shared stuff to do at
> end of the switch. Same in other similar cases.
Ack.
> > + break;
> > + case COUNTER_COMP_U64:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + return comp->device_u64_read(counter, value);
> > + case COUNTER_SCOPE_SIGNAL:
> > + return comp->signal_u64_read(counter, parent, value);
> > + case COUNTER_SCOPE_COUNT:
> > + return comp->count_u64_read(counter, parent, value);
> > + }
> > + break;
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + err = comp->action_read(counter, parent, comp->priv,
> > + &value_u32);
> > + *value = value_u32;
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +/**
> > + * counter_push_event - queue event for userspace reading
> > + * @counter: pointer to Counter structure
> > + * @event: triggered event
> > + * @channel: event channel
> > + *
> > + * Note: If no one is watching for the respective event, it is silently
> > + * discarded.
> > + */
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > + const u8 channel)
> > +{
> > + struct counter_event ev = {0};
> > + unsigned int copied = 0;
> > + unsigned long flags;
> > + struct counter_event_node *event_node;
> > + struct counter_comp_node *comp_node;
> > +
> > + ev.timestamp = ktime_get_ns();
> > + ev.watch.event = event;
> > + ev.watch.channel = channel;
> > +
> > + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
>
> For a raw spin lock, we definitely want to see comments on why it
> is necessary.
Ack.
> > @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> > return err;
> >
> > /* Create Count name attribute */
> > - err = counter_name_attr_create(dev, group, count->name);
> > + err = counter_name_attr_create(dev, group, "name", count->name);
>
> This refactoring could also be pulled out to a precusor patch.
Ack. This will be part of the extension_name patch.
> > @@ -319,12 +315,21 @@ struct counter_device {
> >
> > int id;
> > struct device dev;
> > + struct cdev chrdev;
> > + struct list_head events_list;
> > + raw_spinlock_t events_list_lock;
> > + struct list_head next_events_list;
> > + DECLARE_KFIFO(events, struct counter_event, 64);
>
> Why 64? Probably want that to be somewhat dynamic, even if only at build time.
Ack. This will be dynamically configurable via sysfs attribute in v8.
> > + wait_queue_head_t events_wait;
> > + struct mutex events_lock;
> > };
> >
> > int counter_register(struct counter_device *const counter);
> > void counter_unregister(struct counter_device *const counter);
> > int devm_counter_register(struct device *dev,
> > struct counter_device *const counter);
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > + const u8 channel);
> >
> > #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
> > { \
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> Small thing but I would have been tempted to do a precursor patch to the
> main change simply putting in place the userspace header.
>
> Classic Nop patch that makes it easier to focus on the real stuff in this
> patch by getting that noise out of the way!
>
> Jonathan
Ack.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 3/5] counter: Add character device interface
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
2020-12-30 15:04 ` Jonathan Cameron
@ 2020-12-30 21:36 ` David Lechner
2021-01-30 4:59 ` William Breathitt Gray
2021-01-28 9:01 ` Oleksij Rempel
2 siblings, 1 reply; 23+ messages in thread
From: David Lechner @ 2020-12-30 21:36 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> + COUNTER_COMPONENT_NONE,
> + COUNTER_COMPONENT_SIGNAL,
> + COUNTER_COMPONENT_COUNT,
> + COUNTER_COMPONENT_FUNCTION,
> + COUNTER_COMPONENT_SYNAPSE_ACTION,
> + COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {
Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
> + COUNTER_SCOPE_DEVICE,
> + COUNTER_SCOPE_SIGNAL,
> + COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)
Instead of "Count, extension, etc.", it could be more helpful
to say one of enum counter_component_type.
> + * @scope: component scope (Device, Count, or Signal)
Same here. @scope must be one of enum counter_scope.
> + * @parent: parent component identification number
> + * @id: component identification number
It could be helpful to say that these id numbers match
the X/Y/Z in the sysfs paths as described in the sysfs
ABI.
> + */
> +struct counter_component {
> + __u8 type;
> + __u8 scope;
> + __u8 parent;
> + __u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> + COUNTER_EVENT_OVERFLOW,
> + COUNTER_EVENT_UNDERFLOW,
> + COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> + COUNTER_EVENT_THRESHOLD,
> + COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers
It would be helpful to say that @event must be one of
enum counter_event_type.
> + * @channel: event channel
It would be useful to say that @channel should be 0 unless
a component has more than one event of the same type.
> + */
> +struct counter_watch {
> + struct counter_component component;
> + __u8 event;
> + __u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> + __aligned_u64 timestamp;
> + __aligned_u64 value;
> + struct counter_watch watch;
> + __u8 errno;
There are error codes larger than 255. Probably better
make this __u32.
> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> + COUNTER_COUNT_DIRECTION_FORWARD,
> + COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> + COUNTER_COUNT_MODE_NORMAL,
> + COUNTER_COUNT_MODE_RANGE_LIMIT,
> + COUNTER_COUNT_MODE_NON_RECYCLE,
> + COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> + COUNTER_FUNCTION_INCREASE,
> + COUNTER_FUNCTION_DECREASE,
> + COUNTER_FUNCTION_PULSE_DIRECTION,
> + COUNTER_FUNCTION_QUADRATURE_X1_A,
> + COUNTER_FUNCTION_QUADRATURE_X1_B,
> + COUNTER_FUNCTION_QUADRATURE_X2_A,
> + COUNTER_FUNCTION_QUADRATURE_X2_B,
> + COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> + COUNTER_SIGNAL_LEVEL_LOW,
> + COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> + COUNTER_SYNAPSE_ACTION_NONE,
> + COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> + COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> + COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 3/5] counter: Add character device interface
2020-12-30 21:36 ` David Lechner
@ 2021-01-30 4:59 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-01-30 4:59 UTC (permalink / raw)
To: David Lechner
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 4024 bytes --]
On Wed, Dec 30, 2020 at 03:36:35PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Userspace ABI for Counter character devices
> > + * Copyright (C) 2020 William Breathitt Gray
> > + */
> > +#ifndef _UAPI_COUNTER_H_
> > +#define _UAPI_COUNTER_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +/* Component type definitions */
> > +enum counter_component_type {
> > + COUNTER_COMPONENT_NONE,
> > + COUNTER_COMPONENT_SIGNAL,
> > + COUNTER_COMPONENT_COUNT,
> > + COUNTER_COMPONENT_FUNCTION,
> > + COUNTER_COMPONENT_SYNAPSE_ACTION,
> > + COUNTER_COMPONENT_EXTENSION,
> > +};
> > +
> > +/* Component scope definitions */
> > +enum counter_scope {
>
> Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
COUNTER_COMPONENT_NONE alone should be fine because it already indicates
that the 'component' member of the struct counter_watch is to be ignored
(i.e. type, scope, etc. will not be evaluated and that section of code
is bypassed).
> > + COUNTER_SCOPE_DEVICE,
> > + COUNTER_SCOPE_SIGNAL,
> > + COUNTER_SCOPE_COUNT,
> > +};
> > +
> > +/**
> > + * struct counter_component - Counter component identification
> > + * @type: component type (Count, extension, etc.)
>
> Instead of "Count, extension, etc.", it could be more helpful
> to say one of enum counter_component_type.
Ack.
> > + * @scope: component scope (Device, Count, or Signal)
>
> Same here. @scope must be one of enum counter_scope.
Ack.
> > + * @parent: parent component identification number
> > + * @id: component identification number
>
> It could be helpful to say that these id numbers match
> the X/Y/Z in the sysfs paths as described in the sysfs
> ABI.
Ack.
> > + */
> > +struct counter_component {
> > + __u8 type;
> > + __u8 scope;
> > + __u8 parent;
> > + __u8 id;
> > +};
> > +
> > +/* Event type definitions */
> > +enum counter_event_type {
> > + COUNTER_EVENT_OVERFLOW,
> > + COUNTER_EVENT_UNDERFLOW,
> > + COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> > + COUNTER_EVENT_THRESHOLD,
> > + COUNTER_EVENT_INDEX,
> > +};
> > +
> > +/**
> > + * struct counter_watch - Counter component watch configuration
> > + * @component: component to watch when event triggers
> > + * @event: event that triggers
>
> It would be helpful to say that @event must be one of
> enum counter_event_type.
Ack.
> > + * @channel: event channel
>
> It would be useful to say that @channel should be 0 unless
> a component has more than one event of the same type.
I'll make this clearer.
> > + */
> > +struct counter_watch {
> > + struct counter_component component;
> > + __u8 event;
> > + __u8 channel;
> > +};
> > +
> > +/* ioctl commands */
> > +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> > +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> > +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> > +
> > +/**
> > + * struct counter_event - Counter event data
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @value: component value
> > + * @watch: component watch configuration
> > + * @errno: system error number
> > + */
> > +struct counter_event {
> > + __aligned_u64 timestamp;
> > + __aligned_u64 value;
> > + struct counter_watch watch;
> > + __u8 errno;
>
> There are error codes larger than 255. Probably better
> make this __u32.
Are error codes larger than 255 actually useful in this case? I noticed
the exit() function will only return the least significant byte of
status to the parent: https://man7.org/linux/man-pages/man3/exit.3.html
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 3/5] counter: Add character device interface
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
2020-12-30 15:04 ` Jonathan Cameron
2020-12-30 21:36 ` David Lechner
@ 2021-01-28 9:01 ` Oleksij Rempel
2021-01-30 5:15 ` William Breathitt Gray
2 siblings, 1 reply; 23+ messages in thread
From: Oleksij Rempel @ 2021-01-28 9:01 UTC (permalink / raw)
To: William Breathitt Gray
Cc: jic23, kamel.bouhara, gwendal, a.fatoum, david, linux-iio,
patrick.havelange, alexandre.belloni, mcoquelin.stm32,
linux-kernel, Dan Carpenter, kernel, fabrice.gasnier, syednwaris,
linux-stm32, linux-arm-kernel, alexandre.torgue
Hello William,
On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
>
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
...
> +struct counter_event {
> + __aligned_u64 timestamp;
> + __aligned_u64 value;
> + struct counter_watch watch;
> + __u8 errno;
This variable clashed in user space, as soon as you include errno.h,
with the libc's "magic" definition of errno. What about "err" instead.
I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 3/5] counter: Add character device interface
2021-01-28 9:01 ` Oleksij Rempel
@ 2021-01-30 5:15 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-01-30 5:15 UTC (permalink / raw)
To: Oleksij Rempel
Cc: jic23, kamel.bouhara, gwendal, a.fatoum, david, linux-iio,
patrick.havelange, alexandre.belloni, mcoquelin.stm32,
linux-kernel, Dan Carpenter, kernel, fabrice.gasnier, syednwaris,
linux-stm32, linux-arm-kernel, alexandre.torgue
[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]
On Thu, Jan 28, 2021 at 10:01:13AM +0100, Oleksij Rempel wrote:
> Hello William,
>
>
> On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> >
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> ...
> > +struct counter_event {
> > + __aligned_u64 timestamp;
> > + __aligned_u64 value;
> > + struct counter_watch watch;
> > + __u8 errno;
>
> This variable clashed in user space, as soon as you include errno.h,
> with the libc's "magic" definition of errno. What about "err" instead.
> I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
>
> Regards,
> Oleksij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Sure, I can rename this to avoid a possible clash with libc's errno.
Maybe "status" would be more apt to indicate that this is an exit status
for the event -- the code returned may simply be a warning and not
necessarily a critical error.
Regarding the datatype for this value, I've opened up the discussion in
my reply to David Lechner [1], so perhaps we can continue it there.
[1] https://lkml.org/lkml/2021/1/30/5
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 4/5] docs: counter: Document character device interface
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-12-26 0:15 ` [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
@ 2020-12-26 0:15 ` William Breathitt Gray
2020-12-30 14:47 ` Jonathan Cameron
2020-12-30 18:18 ` David Lechner
2020-12-26 0:15 ` [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
` (2 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: William Breathitt Gray @ 2020-12-26 0:15 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
This patch adds high-level documentation about the Counter subsystem
character device interface.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-counter | 9 +
Documentation/driver-api/generic-counter.rst | 236 +++++++++++++++---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
3 files changed, 205 insertions(+), 41 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 1820ce2f9183..8f6ea0a50b75 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -99,6 +99,15 @@ Description:
Read-only attribute that indicates whether excessive noise is
present at the channel Y counter inputs.
+What: /sys/bus/counter/devices/counterX/countY/extensionZ_name
+What: /sys/bus/counter/devices/counterX/extensionZ_name
+What: /sys/bus/counter/devices/counterX/signalY/extensionZ_name
+KernelVersion: 5.12
+Contact: linux-iio@vger.kernel.org
+Description:
+ Read-only attribute that indicates the component name of
+ Extension Z.
+
What: /sys/bus/counter/devices/counterX/countY/function
KernelVersion: 5.2
Contact: linux-iio@vger.kernel.org
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index b842ddbbd8a0..4775dcaff557 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -223,19 +223,6 @@ 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/sysfs-bus-counter 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
==========
@@ -387,16 +374,16 @@ userspace interface components::
/ driver callbacks /
-------------------
|
- +---------------+
- |
- V
- +--------------------+
- | Counter sysfs |
- +--------------------+
- | Translates to the |
- | standard Counter |
- | sysfs output |
- +--------------------+
+ +---------------+---------------+
+ | |
+ V V
+ +--------------------+ +---------------------+
+ | Counter sysfs | | Counter chrdev |
+ +--------------------+ +---------------------+
+ | Translates to the | | Translates to the |
+ | standard Counter | | standard Counter |
+ | sysfs output | | character device |
+ +--------------------+ +---------------------+
Thereafter, data can be transferred directly between the Counter device
driver and Counter userspace interface::
@@ -427,23 +414,30 @@ driver and Counter userspace interface::
/ u64 /
----------
|
- +---------------+
- |
- V
- +--------------------+
- | Counter sysfs |
- +--------------------+
- | Translates to the |
- | standard Counter |
- | sysfs output |
- |--------------------|
- | Type: const char * |
- | Value: "42" |
- +--------------------+
- |
- ---------------
- / const char * /
- ---------------
+ +---------------+---------------+
+ | |
+ V V
+ +--------------------+ +---------------------+
+ | Counter sysfs | | Counter chrdev |
+ +--------------------+ +---------------------+
+ | Translates to the | | Translates to the |
+ | standard Counter | | standard Counter |
+ | sysfs output | | character device |
+ |--------------------| |---------------------|
+ | Type: const char * | | Type: u64 |
+ | Value: "42" | | Value: 42 |
+ +--------------------+ +---------------------+
+ | |
+ --------------- -----------------------
+ / const char * / / struct counter_event /
+ --------------- -----------------------
+ | |
+ | V
+ | +-----------+
+ | | read |
+ | +-----------+
+ | \ Count: 42 /
+ | -----------
|
V
+--------------------------------------------------+
@@ -452,7 +446,7 @@ driver and Counter userspace interface::
\ Count: "42" /
--------------------------------------------------
-There are three primary components involved:
+There are four primary components involved:
Counter device driver
---------------------
@@ -472,3 +466,163 @@ and vice versa.
Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file
for a detailed breakdown of the available Generic Counter interface
sysfs attributes.
+
+Counter chrdev
+--------------
+Translates counter data to the standard Counter character device; data
+is transferred via standard character device read calls, while Counter
+events are configured via ioctl calls.
+
+Sysfs Interface
+===============
+
+Several sysfs attributes are generated by the Generic Counter interface,
+and reside under the `/sys/bus/counter/devices/counterX` directory,
+where `X` is to the respective counter device id. Please see
+`Documentation/ABI/testing/sysfs-bus-counter` 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.
+
+Counter Character Device
+========================
+
+Counter character device nodes are created under the `/dev` directory as
+`counterX`, where `X` is the respective counter device id. Defines for
+the standard Counter data types are exposed via the userspace
+`include/uapi/linux/counter.h` file.
+
+Counter events
+--------------
+Counter device drivers can support Counter events by utilizing the
+`counter_push_event` function::
+
+ int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel);
+
+The event id is specified by the `event` parameter; the event channel id
+is specified by the `channel` parameter. When this function is called,
+the Counter data associated with the respective event is gathered, and a
+`struct counter_event` is generated for each datum and pushed to
+userspace.
+
+Counter events can be configured by users to report various Counter
+data of interest. This can be conceptualized as a list of Counter
+component read calls to perform. For example::
+
+ +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
+ | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX |
+ +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
+ | Channel 0 | Channel 0 |
+ +------------------------+------------------------+
+ | * Count 0 | * Signal 0 |
+ | * Count 1 | * Signal 0 Extension 0 |
+ | * Signal 3 | * Extension 4 |
+ | * Count 4 Extension 2 +------------------------+
+ | * Signal 5 Extension 0 | Channel 1 |
+ | +------------------------+
+ | | * Signal 4 |
+ | | * Signal 4 Extension 0 |
+ | | * Count 7 |
+ +------------------------+------------------------+
+
+When `counter_push_event(counter, COUNTER_EVENT_INDEX, 1)` is called for
+example, it will go down the list for the `COUNTER_EVENT_INDEX` event
+channel 1 and execute the read callbacks for Signal 4, Signal 4
+Extension 0, and Count 4 -- the data returned for each is pushed to a
+kfifo as a `struct counter_event`, which userspace can retrieve via a
+standard read operation on the respective character device node.
+
+Userspace
+---------
+Userspace applications can configure Counter events via ioctl operations
+on the Counter character device node. There following ioctl codes are
+supported and provided by the `linux/counter.h` userspace header file:
+
+* COUNTER_CLEAR_WATCHES_IOCTL:
+ Clear all Counter watches from all events
+
+* COUNTER_ADD_WATCH_IOCTL:
+ Add a Counter watch for the specified event
+
+* COUNTER_LOAD_WATCHES_IOCTL:
+ Activates the Counter watches added earlier
+
+To configure events to gather Counter data, users first populate a
+`struct counter_watch` with the relevant event id, event channel id, and
+the information for the desired Counter component from which to read,
+and then pass it via the `COUNTER_ADD_WATCH_IOCTL` ioctl command.
+
+Note that an event can be watched without gathering Counter data by
+setting the `component.type` member equal to `COUNTER_COMPONENT_NONE`.
+With this configuration the Counter character device will simply
+populate the event timestamps for those respective
+`struct counter_event` elements and ignore the component value.
+
+The `COUNTER_ADD_WATCH_IOCTL` command will buffer these Counter watches.
+When ready, the `COUNTER_LOAD_WATCHES_IOCTL` ioctl command may be used
+to activate these Counter watches.
+
+Userspace applications can then execute a `read` operation (optionally
+calling `poll` first) on the Counter character device node to retrieve
+`struct counter_event` elements with the desired data.
+
+For example, the following userspace code opens `/dev/counter0`,
+configures the `COUNTER_EVENT_INDEX` event channel 0 to gather Count 0
+and Count 1, and prints out the data as it becomes available on the
+character device node::
+
+ #include <fcntl.h>
+ #include <linux/counter.h>
+ #include <stdio.h>
+ #include <string.h>
+ #include <sys/ioctl.h>
+ #include <unistd.h>
+
+ struct counter_watch watches[2] = {
+ {
+ .component.type = COUNTER_COMPONENT_COUNT,
+ .component.scope = COUNTER_SCOPE_COUNT,
+ .component.parent = 0,
+ .event = COUNTER_EVENT_INDEX,
+ .channel = 0,
+ },
+ {
+ .component.type = COUNTER_COMPONENT_COUNT,
+ .component.scope = COUNTER_SCOPE_COUNT,
+ .component.parent = 1,
+ .event = COUNTER_EVENT_INDEX,
+ .channel = 0,
+ },
+ };
+
+ int main(void)
+ {
+ int fd;
+ struct counter_event event_data[2];
+
+ fd = open("/dev/counter0", O_RDWR);
+
+ ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
+ ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
+ ioctl(fd, COUNTER_LOAD_WATCHES_IOCTL);
+
+ for (;;) {
+ read(fd, event_data, sizeof(event_data));
+
+ printf("Timestamp 0: %llu\tCount 0: %llu\n"
+ "Error Message 0: %s\n"
+ "Timestamp 1: %llu\tCount 1: %llu\n"
+ "Error Message 1: %s\n",
+ (unsigned long long)event_data[0].timestamp,
+ (unsigned long long)event_data[0].value,
+ strerror(event_data[0].errno),
+ (unsigned long long)event_data[1].timestamp,
+ (unsigned long long)event_data[1].value,
+ strerror(event_data[1].errno));
+ }
+
+ return 0;
+ }
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 55a2d9b2ce33..b7761ceb82a7 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -88,6 +88,7 @@ Code Seq# Include File Comments
<http://infiniband.sourceforge.net/>
0x20 all drivers/cdrom/cm206.h
0x22 all scsi/sg.h
+0x3E 00-0F linux/counter.h <mailto:linux-iio@vger.kernel.org>
'!' 00-1F uapi/linux/seccomp.h
'#' 00-3F IEEE 1394 Subsystem
Block for the entire subsystem
--
2.29.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 4/5] docs: counter: Document character device interface
2020-12-26 0:15 ` [PATCH v7 4/5] docs: counter: Document " William Breathitt Gray
@ 2020-12-30 14:47 ` Jonathan Cameron
2020-12-30 18:18 ` David Lechner
1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2020-12-30 14:47 UTC (permalink / raw)
To: William Breathitt Gray
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue
On Fri, 25 Dec 2020 19:15:37 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch adds high-level documentation about the Counter subsystem
> character device interface.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
> Documentation/ABI/testing/sysfs-bus-counter | 9 +
> Documentation/driver-api/generic-counter.rst | 236 +++++++++++++++---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> 3 files changed, 205 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 1820ce2f9183..8f6ea0a50b75 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -99,6 +99,15 @@ Description:
> Read-only attribute that indicates whether excessive noise is
> present at the channel Y counter inputs.
>
> +What: /sys/bus/counter/devices/counterX/countY/extensionZ_name
> +What: /sys/bus/counter/devices/counterX/extensionZ_name
> +What: /sys/bus/counter/devices/counterX/signalY/extensionZ_name
> +KernelVersion: 5.12
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the component name of
> + Extension Z.
Dumb question, but why is this only related to character device introduction?
> +
> What: /sys/bus/counter/devices/counterX/countY/function
> KernelVersion: 5.2
> Contact: linux-iio@vger.kernel.org
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index b842ddbbd8a0..4775dcaff557 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -223,19 +223,6 @@ 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/sysfs-bus-counter 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
> ==========
>
> @@ -387,16 +374,16 @@ userspace interface components::
> / driver callbacks /
> -------------------
> |
> - +---------------+
> - |
> - V
> - +--------------------+
> - | Counter sysfs |
> - +--------------------+
> - | Translates to the |
> - | standard Counter |
> - | sysfs output |
> - +--------------------+
> + +---------------+---------------+
> + | |
> + V V
> + +--------------------+ +---------------------+
> + | Counter sysfs | | Counter chrdev |
> + +--------------------+ +---------------------+
> + | Translates to the | | Translates to the |
> + | standard Counter | | standard Counter |
> + | sysfs output | | character device |
> + +--------------------+ +---------------------+
>
> Thereafter, data can be transferred directly between the Counter device
> driver and Counter userspace interface::
> @@ -427,23 +414,30 @@ driver and Counter userspace interface::
> / u64 /
> ----------
> |
> - +---------------+
> - |
> - V
> - +--------------------+
> - | Counter sysfs |
> - +--------------------+
> - | Translates to the |
> - | standard Counter |
> - | sysfs output |
> - |--------------------|
> - | Type: const char * |
> - | Value: "42" |
> - +--------------------+
> - |
> - ---------------
> - / const char * /
> - ---------------
> + +---------------+---------------+
> + | |
> + V V
> + +--------------------+ +---------------------+
> + | Counter sysfs | | Counter chrdev |
> + +--------------------+ +---------------------+
> + | Translates to the | | Translates to the |
> + | standard Counter | | standard Counter |
> + | sysfs output | | character device |
> + |--------------------| |---------------------|
> + | Type: const char * | | Type: u64 |
> + | Value: "42" | | Value: 42 |
> + +--------------------+ +---------------------+
> + | |
> + --------------- -----------------------
> + / const char * / / struct counter_event /
> + --------------- -----------------------
> + | |
> + | V
> + | +-----------+
> + | | read |
> + | +-----------+
> + | \ Count: 42 /
> + | -----------
> |
> V
> +--------------------------------------------------+
> @@ -452,7 +446,7 @@ driver and Counter userspace interface::
> \ Count: "42" /
> --------------------------------------------------
>
> -There are three primary components involved:
> +There are four primary components involved:
>
> Counter device driver
> ---------------------
> @@ -472,3 +466,163 @@ and vice versa.
> Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file
> for a detailed breakdown of the available Generic Counter interface
> sysfs attributes.
> +
> +Counter chrdev
> +--------------
> +Translates counter data to the standard Counter character device; data
> +is transferred via standard character device read calls, while Counter
> +events are configured via ioctl calls.
> +
> +Sysfs Interface
> +===============
> +
> +Several sysfs attributes are generated by the Generic Counter interface,
> +and reside under the `/sys/bus/counter/devices/counterX` directory,
> +where `X` is to the respective counter device id. Please see
> +`Documentation/ABI/testing/sysfs-bus-counter` 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.
> +
> +Counter Character Device
> +========================
> +
> +Counter character device nodes are created under the `/dev` directory as
> +`counterX`, where `X` is the respective counter device id. Defines for
> +the standard Counter data types are exposed via the userspace
> +`include/uapi/linux/counter.h` file.
> +
> +Counter events
> +--------------
> +Counter device drivers can support Counter events by utilizing the
> +`counter_push_event` function::
> +
> + int counter_push_event(struct counter_device *const counter, const u8 event,
> + const u8 channel);
> +
> +The event id is specified by the `event` parameter; the event channel id
> +is specified by the `channel` parameter. When this function is called,
> +the Counter data associated with the respective event is gathered, and a
> +`struct counter_event` is generated for each datum and pushed to
> +userspace.
> +
> +Counter events can be configured by users to report various Counter
> +data of interest. This can be conceptualized as a list of Counter
> +component read calls to perform. For example::
> +
> + +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
> + | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX |
> + +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
> + | Channel 0 | Channel 0 |
> + +------------------------+------------------------+
> + | * Count 0 | * Signal 0 |
> + | * Count 1 | * Signal 0 Extension 0 |
> + | * Signal 3 | * Extension 4 |
> + | * Count 4 Extension 2 +------------------------+
> + | * Signal 5 Extension 0 | Channel 1 |
> + | +------------------------+
> + | | * Signal 4 |
> + | | * Signal 4 Extension 0 |
> + | | * Count 7 |
> + +------------------------+------------------------+
> +
> +When `counter_push_event(counter, COUNTER_EVENT_INDEX, 1)` is called for
> +example, it will go down the list for the `COUNTER_EVENT_INDEX` event
> +channel 1 and execute the read callbacks for Signal 4, Signal 4
> +Extension 0, and Count 4 -- the data returned for each is pushed to a
> +kfifo as a `struct counter_event`, which userspace can retrieve via a
> +standard read operation on the respective character device node.
> +
> +Userspace
> +---------
> +Userspace applications can configure Counter events via ioctl operations
> +on the Counter character device node. There following ioctl codes are
> +supported and provided by the `linux/counter.h` userspace header file:
> +
> +* COUNTER_CLEAR_WATCHES_IOCTL:
> + Clear all Counter watches from all events
> +
> +* COUNTER_ADD_WATCH_IOCTL:
> + Add a Counter watch for the specified event
> +
> +* COUNTER_LOAD_WATCHES_IOCTL:
> + Activates the Counter watches added earlier
> +
> +To configure events to gather Counter data, users first populate a
> +`struct counter_watch` with the relevant event id, event channel id, and
> +the information for the desired Counter component from which to read,
> +and then pass it via the `COUNTER_ADD_WATCH_IOCTL` ioctl command.
> +
> +Note that an event can be watched without gathering Counter data by
> +setting the `component.type` member equal to `COUNTER_COMPONENT_NONE`.
> +With this configuration the Counter character device will simply
> +populate the event timestamps for those respective
> +`struct counter_event` elements and ignore the component value.
> +
> +The `COUNTER_ADD_WATCH_IOCTL` command will buffer these Counter watches.
> +When ready, the `COUNTER_LOAD_WATCHES_IOCTL` ioctl command may be used
> +to activate these Counter watches.
> +
> +Userspace applications can then execute a `read` operation (optionally
> +calling `poll` first) on the Counter character device node to retrieve
> +`struct counter_event` elements with the desired data.
> +
> +For example, the following userspace code opens `/dev/counter0`,
> +configures the `COUNTER_EVENT_INDEX` event channel 0 to gather Count 0
> +and Count 1, and prints out the data as it becomes available on the
> +character device node::
> +
> + #include <fcntl.h>
> + #include <linux/counter.h>
> + #include <stdio.h>
> + #include <string.h>
> + #include <sys/ioctl.h>
> + #include <unistd.h>
> +
> + struct counter_watch watches[2] = {
> + {
> + .component.type = COUNTER_COMPONENT_COUNT,
> + .component.scope = COUNTER_SCOPE_COUNT,
> + .component.parent = 0,
> + .event = COUNTER_EVENT_INDEX,
> + .channel = 0,
> + },
> + {
> + .component.type = COUNTER_COMPONENT_COUNT,
> + .component.scope = COUNTER_SCOPE_COUNT,
> + .component.parent = 1,
> + .event = COUNTER_EVENT_INDEX,
> + .channel = 0,
> + },
> + };
> +
> + int main(void)
> + {
> + int fd;
> + struct counter_event event_data[2];
> +
> + fd = open("/dev/counter0", O_RDWR);
> +
> + ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> + ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
> + ioctl(fd, COUNTER_LOAD_WATCHES_IOCTL);
> +
> + for (;;) {
> + read(fd, event_data, sizeof(event_data));
> +
> + printf("Timestamp 0: %llu\tCount 0: %llu\n"
> + "Error Message 0: %s\n"
> + "Timestamp 1: %llu\tCount 1: %llu\n"
> + "Error Message 1: %s\n",
> + (unsigned long long)event_data[0].timestamp,
> + (unsigned long long)event_data[0].value,
> + strerror(event_data[0].errno),
> + (unsigned long long)event_data[1].timestamp,
> + (unsigned long long)event_data[1].value,
> + strerror(event_data[1].errno));
> + }
> +
> + return 0;
> + }
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 55a2d9b2ce33..b7761ceb82a7 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -88,6 +88,7 @@ Code Seq# Include File Comments
> <http://infiniband.sourceforge.net/>
> 0x20 all drivers/cdrom/cm206.h
> 0x22 all scsi/sg.h
> +0x3E 00-0F linux/counter.h <mailto:linux-iio@vger.kernel.org>
> '!' 00-1F uapi/linux/seccomp.h
> '#' 00-3F IEEE 1394 Subsystem
> Block for the entire subsystem
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 4/5] docs: counter: Document character device interface
2020-12-26 0:15 ` [PATCH v7 4/5] docs: counter: Document " William Breathitt Gray
2020-12-30 14:47 ` Jonathan Cameron
@ 2020-12-30 18:18 ` David Lechner
1 sibling, 0 replies; 23+ messages in thread
From: David Lechner @ 2020-12-30 18:18 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> +Userspace
> +---------
> +Userspace applications can configure Counter events via ioctl operations
> +on the Counter character device node. There following ioctl codes are
> +supported and provided by the `linux/counter.h` userspace header file:
> +
> +* COUNTER_CLEAR_WATCHES_IOCTL:
> + Clear all Counter watches from all events
> +
> +* COUNTER_ADD_WATCH_IOCTL:
> + Add a Counter watch for the specified event
> +
> +* COUNTER_LOAD_WATCHES_IOCTL:
> + Activates the Counter watches added earlier
Would it make more sense to call the last and first ones
COUNTER_ENABLE_EVENTS_IOCTL and COUNTER_DISABLE_EVENTS_IOCTL?
In any case, more explanation would be helpful.
* COUNTER_ADD_WATCH_IOCTL:
Queues a Counter watch for the specified event. The queued watches
will not be applied until COUNTER_ENABLE_EVENTS_IOCTL is called.
* COUNTER_ENABLE_EVENTS_IOCTL:
Enables monitoring the events specified by the Counter watches that were
queued by COUNTER_ADD_WATCH_IOCTL. If events are already enabled, the new
set of watches replaces the old one. Calling this ioctl also has the
effect of clearing the queue of watches added by COUNTER_ADD_WATCH_IOCTL.
* COUNTER_DISABLE_EVENTS_IOCTL:
Stops monitoring the previously enabled events.
> +
> +To configure events to gather Counter data, users first populate a
> +`struct counter_watch` with the relevant event id, event channel id, and
> +the information for the desired Counter component from which to read,
> +and then pass it via the `COUNTER_ADD_WATCH_IOCTL` ioctl command.
for restructured text, two backticks are needed for ``code`` formatting
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
` (2 preceding siblings ...)
2020-12-26 0:15 ` [PATCH v7 4/5] docs: counter: Document " William Breathitt Gray
@ 2020-12-26 0:15 ` William Breathitt Gray
2020-12-30 15:31 ` Jonathan Cameron
2020-12-30 17:36 ` David Lechner
[not found] ` <fc40ab7f4a38e80d86715daa5eaf744dd645a75b.1608935587.git.vilhelm.gray@gmail.com>
2020-12-30 23:34 ` [PATCH v7 0/5] Introduce the Counter character device interface David Lechner
5 siblings, 2 replies; 23+ messages in thread
From: William Breathitt Gray @ 2020-12-26 0:15 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
The LSI/CSI LS7266R1 chip provides programmable output via the FLG pins.
When interrupts are enabled on the ACCES 104-QUAD-8, they occur whenever
FLG1 is active. Four functions are available for the FLG1 signal: Carry,
Compare, Carry-Borrow, and Index.
Carry:
Interrupt generated on active low Carry signal. Carry
signal toggles every time the respective channel's
counter overflows.
Compare:
Interrupt generated on active low Compare signal.
Compare signal toggles every time respective channel's
preset register is equal to the respective channel's
counter.
Carry-Borrow:
Interrupt generated on active low Carry signal and
active low Borrow signal. Carry signal toggles every
time the respective channel's counter overflows. Borrow
signal toggles every time the respective channel's
counter underflows.
Index:
Interrupt generated on active high Index signal.
The irq_trigger Count extension is introduced to allow the selection of
the desired IRQ trigger function per channel. Interrupts push Counter
events to event channel X, where 'X' is the respective channel whose
FLG1 activated.
This patch adds IRQ support for the ACCES 104-QUAD-8. The interrupt line
numbers for the devices may be configured via the irq array module
parameter.
Reviewed-by: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
.../ABI/testing/sysfs-bus-counter-104-quad-8 | 25 ++
drivers/counter/104-quad-8.c | 318 ++++++++++++++----
drivers/counter/Kconfig | 6 +-
3 files changed, 276 insertions(+), 73 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
index eac32180c40d..0ecba24d43aa 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
+++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
@@ -1,3 +1,28 @@
+What: /sys/bus/counter/devices/counterX/countY/irq_trigger
+KernelVersion: 5.12
+Contact: linux-iio@vger.kernel.org
+Description:
+ IRQ trigger function for channel Y. Four trigger functions are
+ available: carry, compare, carry-borrow, and index.
+
+ carry:
+ Interrupt generated on active low Carry signal. Carry
+ signal toggles every time channel Y counter overflows.
+
+ compare:
+ Interrupt generated on active low Compare signal.
+ Compare signal toggles every time channel Y preset
+ register is equal to channel Y counter.
+
+ carry-borrow:
+ Interrupt generated on active low Carry signal and
+ active low Borrow signal. Carry signal toggles every
+ time channel Y counter overflows. Borrow signal toggles
+ every time channel Y counter underflows.
+
+ index:
+ Interrupt generated on active high Index signal.
+
What: /sys/bus/counter/devices/counterX/signalY/cable_fault
KernelVersion: 5.7
Contact: linux-iio@vger.kernel.org
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index f4fb36b751c4..7537575568d0 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -13,23 +13,30 @@
#include <linux/iio/types.h>
#include <linux/io.h>
#include <linux/ioport.h>
+#include <linux/interrupt.h>
#include <linux/isa.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/types.h>
+#include <linux/spinlock.h>
#define QUAD8_EXTENT 32
static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
static unsigned int num_quad8;
-module_param_array(base, uint, &num_quad8, 0);
+module_param_hw_array(base, uint, ioport, &num_quad8, 0);
MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
+static unsigned int irq[max_num_isa_dev(QUAD8_EXTENT)];
+module_param_hw_array(irq, uint, irq, NULL, 0);
+MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers");
+
#define QUAD8_NUM_COUNTERS 8
/**
* struct quad8_iio - IIO device private data structure
+ * @lock: synchronization lock to prevent I/O race conditions
* @counter: instance of the counter_device
* @fck_prescaler: array of filter clock prescaler configurations
* @preset: array of preset values
@@ -38,13 +45,14 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
* @quadrature_scale: array of quadrature mode scale configurations
* @ab_enable: array of A and B inputs enable configurations
* @preset_enable: array of set_to_preset_on_index attribute configurations
+ * @irq_trigger: array of interrupt trigger function configurations
* @synchronous_mode: array of index function synchronous mode configurations
* @index_polarity: array of index function polarity configurations
* @cable_fault_enable: differential encoder cable status enable configurations
* @base: base port address of the IIO device
*/
struct quad8_iio {
- struct mutex lock;
+ raw_spinlock_t lock;
struct counter_device counter;
unsigned int fck_prescaler[QUAD8_NUM_COUNTERS];
unsigned int preset[QUAD8_NUM_COUNTERS];
@@ -53,13 +61,16 @@ struct quad8_iio {
unsigned int quadrature_scale[QUAD8_NUM_COUNTERS];
unsigned int ab_enable[QUAD8_NUM_COUNTERS];
unsigned int preset_enable[QUAD8_NUM_COUNTERS];
+ unsigned int irq_trigger[QUAD8_NUM_COUNTERS];
unsigned int synchronous_mode[QUAD8_NUM_COUNTERS];
unsigned int index_polarity[QUAD8_NUM_COUNTERS];
unsigned int cable_fault_enable;
unsigned int base;
};
+#define QUAD8_REG_INTERRUPT_STATUS 0x10
#define QUAD8_REG_CHAN_OP 0x11
+#define QUAD8_REG_INDEX_INTERRUPT 0x12
#define QUAD8_REG_INDEX_INPUT_LEVELS 0x16
#define QUAD8_DIFF_ENCODER_CABLE_STATUS 0x17
/* Borrow Toggle flip-flop */
@@ -92,8 +103,8 @@ struct quad8_iio {
#define QUAD8_RLD_CNTR_OUT 0x10
/* Transfer Preset Register LSB to FCK Prescaler */
#define QUAD8_RLD_PRESET_PSC 0x18
-#define QUAD8_CHAN_OP_ENABLE_COUNTERS 0x00
#define QUAD8_CHAN_OP_RESET_COUNTERS 0x01
+#define QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC 0x04
#define QUAD8_CMR_QUADRATURE_X1 0x08
#define QUAD8_CMR_QUADRATURE_X2 0x10
#define QUAD8_CMR_QUADRATURE_X4 0x18
@@ -107,6 +118,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
unsigned int flags;
unsigned int borrow;
unsigned int carry;
+ unsigned long irqflags;
int i;
switch (mask) {
@@ -124,7 +136,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
/* Borrow XOR Carry effectively doubles count range */
*val = (borrow ^ carry) << 24;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Reset Byte Pointer; transfer Counter to Output Latch */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
@@ -133,7 +145,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
for (i = 0; i < 3; i++)
*val |= (unsigned int)inb(base_offset) << (8 * i);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return IIO_VAL_INT;
case IIO_CHAN_INFO_ENABLE:
@@ -153,6 +165,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
{
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel;
+ unsigned long flags;
int i;
unsigned int ior_cfg;
@@ -165,7 +178,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
if ((unsigned int)val > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
/* Reset Byte Pointer */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
@@ -190,7 +203,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
/* Reset Error flag */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
case IIO_CHAN_INFO_ENABLE:
@@ -198,25 +211,26 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
if (val < 0 || val > 1)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
priv->ab_enable[chan->channel] = val;
- ior_cfg = val | priv->preset_enable[chan->channel] << 1;
+ ior_cfg = val | priv->preset_enable[chan->channel] << 1 |
+ priv->irq_trigger[chan->channel] << 3;
/* Load I/O control configuration */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
case IIO_CHAN_INFO_SCALE:
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
/* Quadrature scaling only available in quadrature mode */
if (!priv->quadrature_mode[chan->channel] &&
(val2 || val != 1)) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
@@ -232,15 +246,15 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
priv->quadrature_scale[chan->channel] = 2;
break;
default:
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
else {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
}
@@ -266,6 +280,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel;
unsigned int preset;
+ unsigned long irqflags;
int ret;
int i;
@@ -277,7 +292,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
if (preset > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->preset[chan->channel] = preset;
@@ -288,7 +303,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
for (i = 0; i < 3; i++)
outb(preset >> (8 * i), base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return len;
}
@@ -309,6 +324,7 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
bool preset_enable;
+ unsigned long irqflags;
int ret;
unsigned int ior_cfg;
@@ -319,17 +335,18 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
/* Preset enable is active low in Input/Output Control register */
preset_enable = !preset_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->preset_enable[chan->channel] = preset_enable;
ior_cfg = priv->ab_enable[chan->channel] |
- (unsigned int)preset_enable << 1;
+ (unsigned int)preset_enable << 1 |
+ priv->irq_trigger[chan->channel] << 3;
/* Load I/O control configuration to Input / Output Control Register */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return len;
}
@@ -387,8 +404,9 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
unsigned int mode_cfg = cnt_mode << 1;
const int base_offset = priv->base + 2 * chan->channel + 1;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->count_mode[chan->channel] = cnt_mode;
@@ -399,7 +417,7 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -430,14 +448,15 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
unsigned int idr_cfg = synchronous_mode;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->index_polarity[chan->channel] << 1;
/* Index function must be non-synchronous in non-quadrature mode */
if (synchronous_mode && !priv->quadrature_mode[chan->channel]) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -446,7 +465,7 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -476,9 +495,10 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
{
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
+ unsigned long irqflags;
unsigned int mode_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
mode_cfg = priv->count_mode[chan->channel] << 1;
@@ -498,7 +518,7 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -529,8 +549,9 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
unsigned int idr_cfg = index_polarity << 1;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->synchronous_mode[chan->channel];
@@ -539,7 +560,7 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -647,6 +668,7 @@ static int quad8_count_read(struct counter_device *counter,
unsigned int flags;
unsigned int borrow;
unsigned int carry;
+ unsigned long irqflags;
int i;
flags = inb(base_offset + 1);
@@ -656,7 +678,7 @@ static int quad8_count_read(struct counter_device *counter,
/* Borrow XOR Carry effectively doubles count range */
*val = (unsigned long)(borrow ^ carry) << 24;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Reset Byte Pointer; transfer Counter to Output Latch */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
@@ -665,7 +687,7 @@ static int quad8_count_read(struct counter_device *counter,
for (i = 0; i < 3; i++)
*val |= (unsigned long)inb(base_offset) << (8 * i);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -675,13 +697,14 @@ static int quad8_count_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
+ unsigned long irqflags;
int i;
/* Only 24-bit values are supported */
if (val > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Reset Byte Pointer */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
@@ -706,7 +729,7 @@ static int quad8_count_write(struct counter_device *counter,
/* Reset Error flag */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -725,8 +748,9 @@ static int quad8_function_read(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int id = count->id;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
if (priv->quadrature_mode[id])
switch (priv->quadrature_scale[id]) {
@@ -743,7 +767,7 @@ static int quad8_function_read(struct counter_device *counter,
else
*function = COUNTER_FUNCTION_PULSE_DIRECTION;
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -758,10 +782,11 @@ static int quad8_function_write(struct counter_device *counter,
unsigned int *const scale = priv->quadrature_scale + id;
unsigned int *const synchronous_mode = priv->synchronous_mode + id;
const int base_offset = priv->base + 2 * id + 1;
+ unsigned long irqflags;
unsigned int mode_cfg;
unsigned int idr_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
mode_cfg = priv->count_mode[id] << 1;
idr_cfg = priv->index_polarity[id] << 1;
@@ -795,7 +820,7 @@ static int quad8_function_write(struct counter_device *counter,
mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
break;
default:
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
}
@@ -803,7 +828,7 @@ static int quad8_function_write(struct counter_device *counter,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -898,13 +923,62 @@ static int quad8_action_read(struct counter_device *counter,
return 0;
}
+static int quad8_events_configure(struct counter_device *counter)
+{
+ struct quad8_iio *const priv = counter->priv;
+ unsigned long irq_enabled = 0;
+ struct counter_event_node *event_node;
+
+ /* Enable interrupts for the requested channels, disable for the rest */
+ list_for_each_entry(event_node, &counter->events_list, l)
+ irq_enabled |= BIT(event_node->channel);
+
+ outb(irq_enabled, priv->base + QUAD8_REG_INDEX_INTERRUPT);
+
+ return 0;
+}
+
+static int quad8_watch_validate(struct counter_device *counter,
+ const struct counter_watch *watch)
+{
+ struct quad8_iio *const priv = counter->priv;
+
+ if (watch->channel > QUAD8_NUM_COUNTERS - 1)
+ return -EINVAL;
+
+ switch (watch->event) {
+ case COUNTER_EVENT_OVERFLOW:
+ if (priv->irq_trigger[watch->channel] != 0)
+ return -EINVAL;
+ break;
+ case COUNTER_EVENT_THRESHOLD:
+ if (priv->irq_trigger[watch->channel] != 1)
+ return -EINVAL;
+ break;
+ case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
+ if (priv->irq_trigger[watch->channel] != 2)
+ return -EINVAL;
+ break;
+ case COUNTER_EVENT_INDEX:
+ if (priv->irq_trigger[watch->channel] != 3)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static const struct counter_ops quad8_ops = {
.signal_read = quad8_signal_read,
.count_read = quad8_count_read,
.count_write = quad8_count_write,
.function_read = quad8_function_read,
.function_write = quad8_function_write,
- .action_read = quad8_action_read
+ .action_read = quad8_action_read,
+ .events_configure = quad8_events_configure,
+ .watch_validate = quad8_watch_validate,
};
static int quad8_index_polarity_get(struct counter_device *counter,
@@ -926,9 +1000,10 @@ static int quad8_index_polarity_set(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id - 16;
const int base_offset = priv->base + 2 * channel_id + 1;
+ unsigned long irqflags;
unsigned int idr_cfg = index_polarity << 1;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->synchronous_mode[channel_id];
@@ -937,7 +1012,7 @@ static int quad8_index_polarity_set(struct counter_device *counter,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -961,15 +1036,16 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id - 16;
const int base_offset = priv->base + 2 * channel_id + 1;
+ unsigned long irqflags;
unsigned int idr_cfg = synchronous_mode;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->index_polarity[channel_id] << 1;
/* Index function must be non-synchronous in non-quadrature mode */
if (synchronous_mode && !priv->quadrature_mode[channel_id]) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -978,7 +1054,7 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1025,6 +1101,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
unsigned int count_mode;
unsigned int mode_cfg;
const int base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
/* Map Generic Counter count mode to 104-QUAD-8 count mode */
switch (cnt_mode) {
@@ -1044,7 +1121,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
return -EINVAL;
}
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->count_mode[count->id] = count_mode;
@@ -1058,7 +1135,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1078,18 +1155,20 @@ static int quad8_count_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
+ unsigned long irqflags;
unsigned int ior_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->ab_enable[count->id] = enable;
- ior_cfg = enable | priv->preset_enable[count->id] << 1;
+ ior_cfg = enable | priv->preset_enable[count->id] << 1 |
+ priv->irq_trigger[count->id] << 3;
/* Load I/O control configuration */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1135,16 +1214,17 @@ static int quad8_count_preset_write(struct counter_device *counter,
struct counter_count *count, u64 preset)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
/* Only 24-bit values are supported */
if (preset > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
quad8_preset_register_set(priv, count->id, preset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1153,8 +1233,9 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
struct counter_count *count, u64 *ceiling)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
@@ -1168,7 +1249,7 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
break;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1177,12 +1258,13 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
struct counter_count *count, u64 ceiling)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
/* Only 24-bit values are supported */
if (ceiling > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
@@ -1192,7 +1274,7 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
break;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -1214,21 +1296,56 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
unsigned int ior_cfg;
/* Preset enable is active low in Input/Output Control register */
preset_enable = !preset_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->preset_enable[count->id] = preset_enable;
- ior_cfg = priv->ab_enable[count->id] | preset_enable << 1;
+ ior_cfg = priv->ab_enable[count->id] | preset_enable << 1 |
+ priv->irq_trigger[count->id] << 3;
+
+ /* Load I/O control configuration to Input / Output Control Register */
+ outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
+
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
+
+ return 0;
+}
+
+static int quad8_irq_trigger_get(struct counter_device *counter,
+ struct counter_count *count, u32 *irq_trigger)
+{
+ const struct quad8_iio *const priv = counter->priv;
+
+ *irq_trigger = priv->irq_trigger[count->id];
+
+ return 0;
+}
+
+static int quad8_irq_trigger_set(struct counter_device *counter,
+ struct counter_count *count, u32 irq_trigger)
+{
+ struct quad8_iio *const priv = counter->priv;
+ const unsigned long base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
+ unsigned long ior_cfg;
+
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
+
+ priv->irq_trigger[count->id] = irq_trigger;
+
+ ior_cfg = priv->ab_enable[count->id] |
+ priv->preset_enable[count->id] << 1 | irq_trigger << 3;
/* Load I/O control configuration to Input / Output Control Register */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1239,22 +1356,23 @@ static int quad8_signal_cable_fault_read(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
+ unsigned long irqflags;
bool disabled;
unsigned int status;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
disabled = !(priv->cable_fault_enable & BIT(channel_id));
if (disabled) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
/* Logic 0 = cable fault */
status = inb(priv->base + QUAD8_DIFF_ENCODER_CABLE_STATUS);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
/* Mask respective channel and invert logic */
*cable_fault = !(status & BIT(channel_id));
@@ -1280,9 +1398,10 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
+ unsigned long irqflags;
unsigned int cable_fault_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
if (enable)
priv->cable_fault_enable |= BIT(channel_id);
@@ -1294,7 +1413,7 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,
outb(cable_fault_enable, priv->base + QUAD8_DIFF_ENCODER_CABLE_STATUS);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1317,8 +1436,9 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
const int base_offset = priv->base + 2 * channel_id;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->fck_prescaler[channel_id] = prescaler;
@@ -1330,7 +1450,7 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC,
base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1433,7 +1553,15 @@ static const enum counter_count_mode quad8_cnt_modes[] = {
static DEFINE_COUNTER_AVAILABLE(quad8_count_mode_available, quad8_cnt_modes);
+static const char *const quad8_irq_trigger_states[] = {
+ "carry",
+ "compare",
+ "carry-borrow",
+ "index",
+};
+
static DEFINE_COUNTER_ENUM(quad8_error_noise_enum, quad8_noise_error_states);
+static DEFINE_COUNTER_ENUM(quad8_irq_trigger_enum, quad8_irq_trigger_states);
static struct counter_comp quad8_count_ext[] = {
COUNTER_COMP_CEILING(quad8_count_ceiling_read,
@@ -1448,6 +1576,8 @@ static struct counter_comp quad8_count_ext[] = {
COUNTER_COMP_PRESET(quad8_count_preset_read, quad8_count_preset_write),
COUNTER_COMP_PRESET_ENABLE(quad8_count_preset_enable_read,
quad8_count_preset_enable_write),
+ COUNTER_COMP_COUNT_ENUM("irq_trigger", quad8_irq_trigger_get,
+ quad8_irq_trigger_set, quad8_irq_trigger_enum),
};
#define QUAD8_COUNT(_id, _cntname) { \
@@ -1472,6 +1602,48 @@ static struct counter_count quad8_counts[] = {
QUAD8_COUNT(7, "Channel 8 Count")
};
+static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
+{
+ struct quad8_iio *const priv = quad8iio;
+ const unsigned long base = priv->base;
+ unsigned long irq_status;
+ unsigned long channel;
+ u8 event;
+
+ irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
+ if (!irq_status)
+ return IRQ_NONE;
+
+ for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
+ switch (priv->irq_trigger[channel]) {
+ case 0:
+ event = COUNTER_EVENT_OVERFLOW;
+ break;
+ case 1:
+ event = COUNTER_EVENT_THRESHOLD;
+ break;
+ case 2:
+ event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
+ break;
+ case 3:
+ event = COUNTER_EVENT_INDEX;
+ break;
+ default:
+ /* should never reach this path */
+ WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
+ priv->irq_trigger[channel], channel);
+ continue;
+ }
+
+ counter_push_event(&priv->counter, event, channel);
+ }
+
+ /* Clear pending interrupts on device */
+ outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
+
+ return IRQ_HANDLED;
+}
+
static int quad8_probe(struct device *dev, unsigned int id)
{
struct iio_dev *indio_dev;
@@ -1510,9 +1682,10 @@ static int quad8_probe(struct device *dev, unsigned int id)
quad8iio->counter.priv = quad8iio;
quad8iio->base = base[id];
- /* Initialize mutex */
- mutex_init(&quad8iio->lock);
+ raw_spin_lock_init(&quad8iio->lock);
+ /* Reset Index/Interrupt Register */
+ outb(0x00, base[id] + QUAD8_REG_INDEX_INTERRUPT);
/* Reset all counters and disable interrupt function */
outb(QUAD8_CHAN_OP_RESET_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
/* Set initial configuration for all counters */
@@ -1542,8 +1715,8 @@ static int quad8_probe(struct device *dev, unsigned int id)
}
/* Disable Differential Encoder Cable Status for all channels */
outb(0xFF, base[id] + QUAD8_DIFF_ENCODER_CABLE_STATUS);
- /* Enable all counters */
- outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
+ /* Enable all counters and enable interrupt function */
+ outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP);
/* Register IIO device */
err = devm_iio_device_register(dev, indio_dev);
@@ -1551,7 +1724,12 @@ static int quad8_probe(struct device *dev, unsigned int id)
return err;
/* Register Counter device */
- return devm_counter_register(dev, &quad8iio->counter);
+ err = devm_counter_register(dev, &quad8iio->counter);
+ if (err)
+ return err;
+
+ return devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
+ quad8iio->counter.name, quad8iio);
}
static struct isa_driver quad8_driver = {
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2de53ab0dd25..bd42df98f522 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -23,11 +23,11 @@ config 104_QUAD_8
A counter's respective error flag may be cleared by performing a write
operation on the respective count value attribute. Although the
104-QUAD-8 counters have a 25-bit range, only the lower 24 bits may be
- set, either directly or via the counter's preset attribute. Interrupts
- are not supported by this driver.
+ set, either directly or via the counter's preset attribute.
The base port addresses for the devices may be configured via the base
- array module parameter.
+ array module parameter. The interrupt line numbers for the devices may
+ be configured via the irq array module parameter.
config STM32_TIMER_CNT
tristate "STM32 Timer encoder counter driver"
--
2.29.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-12-26 0:15 ` [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
@ 2020-12-30 15:31 ` Jonathan Cameron
2021-02-12 6:04 ` William Breathitt Gray
2020-12-30 17:36 ` David Lechner
1 sibling, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2020-12-30 15:31 UTC (permalink / raw)
To: William Breathitt Gray
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue
On Fri, 25 Dec 2020 19:15:38 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> The LSI/CSI LS7266R1 chip provides programmable output via the FLG pins.
> When interrupts are enabled on the ACCES 104-QUAD-8, they occur whenever
> FLG1 is active. Four functions are available for the FLG1 signal: Carry,
> Compare, Carry-Borrow, and Index.
>
> Carry:
> Interrupt generated on active low Carry signal. Carry
> signal toggles every time the respective channel's
> counter overflows.
>
> Compare:
> Interrupt generated on active low Compare signal.
> Compare signal toggles every time respective channel's
> preset register is equal to the respective channel's
> counter.
>
> Carry-Borrow:
> Interrupt generated on active low Carry signal and
> active low Borrow signal. Carry signal toggles every
> time the respective channel's counter overflows. Borrow
> signal toggles every time the respective channel's
> counter underflows.
>
> Index:
> Interrupt generated on active high Index signal.
>
> The irq_trigger Count extension is introduced to allow the selection of
> the desired IRQ trigger function per channel. Interrupts push Counter
> events to event channel X, where 'X' is the respective channel whose
> FLG1 activated.
>
> This patch adds IRQ support for the ACCES 104-QUAD-8. The interrupt line
> numbers for the devices may be configured via the irq array module
> parameter.
>
> Reviewed-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
Immediate thought on this is that you should pull the lock type change
out as a precursor patch that simply says we need it to be a spin_lock
for the following patch. That should be an obvious change to review
and then leave a much shorter patch to focus on here.
Jonathan
> ---
> .../ABI/testing/sysfs-bus-counter-104-quad-8 | 25 ++
> drivers/counter/104-quad-8.c | 318 ++++++++++++++----
> drivers/counter/Kconfig | 6 +-
> 3 files changed, 276 insertions(+), 73 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> index eac32180c40d..0ecba24d43aa 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> @@ -1,3 +1,28 @@
> +What: /sys/bus/counter/devices/counterX/countY/irq_trigger
A warning on this. The ABI docs are now built into the main kernel
html docs build. The snag is that it doesn't cope will with specialising
or different devices having the same named file with different allowed values.
You have to unify them all into one place. It may be worth just doing that
from the start.
> +KernelVersion: 5.12
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + IRQ trigger function for channel Y. Four trigger functions are
> + available: carry, compare, carry-borrow, and index.
> +
> + carry:
> + Interrupt generated on active low Carry signal. Carry
> + signal toggles every time channel Y counter overflows.
> +
> + compare:
> + Interrupt generated on active low Compare signal.
> + Compare signal toggles every time channel Y preset
> + register is equal to channel Y counter.
> +
> + carry-borrow:
> + Interrupt generated on active low Carry signal and
> + active low Borrow signal. Carry signal toggles every
> + time channel Y counter overflows. Borrow signal toggles
> + every time channel Y counter underflows.
> +
> + index:
> + Interrupt generated on active high Index signal.
> +
> What: /sys/bus/counter/devices/counterX/signalY/cable_fault
> KernelVersion: 5.7
> Contact: linux-iio@vger.kernel.org
> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> index f4fb36b751c4..7537575568d0 100644
> --- a/drivers/counter/104-quad-8.c
> +++ b/drivers/counter/104-quad-8.c
> @@ -13,23 +13,30 @@
> #include <linux/iio/types.h>
> #include <linux/io.h>
> #include <linux/ioport.h>
> +#include <linux/interrupt.h>
> #include <linux/isa.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/types.h>
> +#include <linux/spinlock.h>
>
> #define QUAD8_EXTENT 32
>
> static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
> static unsigned int num_quad8;
> -module_param_array(base, uint, &num_quad8, 0);
> +module_param_hw_array(base, uint, ioport, &num_quad8, 0);
Why this change? Perhaps a note in the patch description on why
this is needed?
> MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
>
> +static unsigned int irq[max_num_isa_dev(QUAD8_EXTENT)];
> +module_param_hw_array(irq, uint, irq, NULL, 0);
> +MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers");
> +
> #define QUAD8_NUM_COUNTERS 8
>
> /**
> * struct quad8_iio - IIO device private data structure
> + * @lock: synchronization lock to prevent I/O race conditions
Probably want to be more specific. RMW, indexed writes or what sort of race?
> * @counter: instance of the counter_device
> * @fck_prescaler: array of filter clock prescaler configurations
> * @preset: array of preset values
> @@ -38,13 +45,14 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
> * @quadrature_scale: array of quadrature mode scale configurations
> * @ab_enable: array of A and B inputs enable configurations
> * @preset_enable: array of set_to_preset_on_index attribute configurations
> + * @irq_trigger: array of interrupt trigger function configurations
> * @synchronous_mode: array of index function synchronous mode configurations
> * @index_polarity: array of index function polarity configurations
> * @cable_fault_enable: differential encoder cable status enable configurations
> * @base: base port address of the IIO device
> */
> struct quad8_iio {
> - struct mutex lock;
> + raw_spinlock_t lock;
So it was here before but not documented. You should fix that with a precusor patch.
Why raw_spinlock_t rather than spinlock_t?
> struct counter_device counter;
> unsigned int fck_prescaler[QUAD8_NUM_COUNTERS];
> unsigned int preset[QUAD8_NUM_COUNTERS];
> @@ -53,13 +61,16 @@ struct quad8_iio {
> unsigned int quadrature_scale[QUAD8_NUM_COUNTERS];
> unsigned int ab_enable[QUAD8_NUM_COUNTERS];
> unsigned int preset_enable[QUAD8_NUM_COUNTERS];
> + unsigned int irq_trigger[QUAD8_NUM_COUNTERS];
> unsigned int synchronous_mode[QUAD8_NUM_COUNTERS];
> unsigned int index_polarity[QUAD8_NUM_COUNTERS];
> unsigned int cable_fault_enable;
> unsigned int base;
> };
>
> +#define QUAD8_REG_INTERRUPT_STATUS 0x10
> #define QUAD8_REG_CHAN_OP 0x11
> +#define QUAD8_REG_INDEX_INTERRUPT 0x12
> #define QUAD8_REG_INDEX_INPUT_LEVELS 0x16
> #define QUAD8_DIFF_ENCODER_CABLE_STATUS 0x17
> /* Borrow Toggle flip-flop */
> @@ -92,8 +103,8 @@ struct quad8_iio {
> #define QUAD8_RLD_CNTR_OUT 0x10
> /* Transfer Preset Register LSB to FCK Prescaler */
> #define QUAD8_RLD_PRESET_PSC 0x18
> -#define QUAD8_CHAN_OP_ENABLE_COUNTERS 0x00
> #define QUAD8_CHAN_OP_RESET_COUNTERS 0x01
> +#define QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC 0x04
> #define QUAD8_CMR_QUADRATURE_X1 0x08
> #define QUAD8_CMR_QUADRATURE_X2 0x10
> #define QUAD8_CMR_QUADRATURE_X4 0x18
> @@ -107,6 +118,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
> unsigned int flags;
> unsigned int borrow;
> unsigned int carry;
> + unsigned long irqflags;
> int i;
>
> switch (mask) {
> @@ -124,7 +136,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
> /* Borrow XOR Carry effectively doubles count range */
> *val = (borrow ^ carry) << 24;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> /* Reset Byte Pointer; transfer Counter to Output Latch */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
> @@ -133,7 +145,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
> for (i = 0; i < 3; i++)
> *val |= (unsigned int)inb(base_offset) << (8 * i);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_ENABLE:
> @@ -153,6 +165,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
> {
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel;
> + unsigned long flags;
> int i;
> unsigned int ior_cfg;
>
> @@ -165,7 +178,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
> if ((unsigned int)val > 0xFFFFFF)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, flags);
>
> /* Reset Byte Pointer */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
> @@ -190,7 +203,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
> /* Reset Error flag */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, base_offset + 1);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
>
> return 0;
> case IIO_CHAN_INFO_ENABLE:
> @@ -198,25 +211,26 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
> if (val < 0 || val > 1)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, flags);
>
> priv->ab_enable[chan->channel] = val;
>
> - ior_cfg = val | priv->preset_enable[chan->channel] << 1;
> + ior_cfg = val | priv->preset_enable[chan->channel] << 1 |
> + priv->irq_trigger[chan->channel] << 3;
>
> /* Load I/O control configuration */
> outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
>
> return 0;
> case IIO_CHAN_INFO_SCALE:
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, flags);
>
> /* Quadrature scaling only available in quadrature mode */
> if (!priv->quadrature_mode[chan->channel] &&
> (val2 || val != 1)) {
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
> return -EINVAL;
> }
>
> @@ -232,15 +246,15 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
> priv->quadrature_scale[chan->channel] = 2;
> break;
> default:
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
> return -EINVAL;
> }
> else {
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
> return -EINVAL;
> }
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, flags);
> return 0;
> }
>
> @@ -266,6 +280,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel;
> unsigned int preset;
> + unsigned long irqflags;
> int ret;
> int i;
>
> @@ -277,7 +292,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
> if (preset > 0xFFFFFF)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->preset[chan->channel] = preset;
>
> @@ -288,7 +303,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
> for (i = 0; i < 3; i++)
> outb(preset >> (8 * i), base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return len;
> }
> @@ -309,6 +324,7 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel + 1;
> bool preset_enable;
> + unsigned long irqflags;
> int ret;
> unsigned int ior_cfg;
>
> @@ -319,17 +335,18 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
> /* Preset enable is active low in Input/Output Control register */
> preset_enable = !preset_enable;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->preset_enable[chan->channel] = preset_enable;
>
> ior_cfg = priv->ab_enable[chan->channel] |
> - (unsigned int)preset_enable << 1;
> + (unsigned int)preset_enable << 1 |
> + priv->irq_trigger[chan->channel] << 3;
>
> /* Load I/O control configuration to Input / Output Control Register */
> outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return len;
> }
> @@ -387,8 +404,9 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
> struct quad8_iio *const priv = iio_priv(indio_dev);
> unsigned int mode_cfg = cnt_mode << 1;
> const int base_offset = priv->base + 2 * chan->channel + 1;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->count_mode[chan->channel] = cnt_mode;
>
> @@ -399,7 +417,7 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
> /* Load mode configuration to Counter Mode Register */
> outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -430,14 +448,15 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel + 1;
> unsigned int idr_cfg = synchronous_mode;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> idr_cfg |= priv->index_polarity[chan->channel] << 1;
>
> /* Index function must be non-synchronous in non-quadrature mode */
> if (synchronous_mode && !priv->quadrature_mode[chan->channel]) {
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
> return -EINVAL;
> }
>
> @@ -446,7 +465,7 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
> /* Load Index Control configuration to Index Control Register */
> outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -476,9 +495,10 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
> {
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel + 1;
> + unsigned long irqflags;
> unsigned int mode_cfg;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> mode_cfg = priv->count_mode[chan->channel] << 1;
>
> @@ -498,7 +518,7 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
> /* Load mode configuration to Counter Mode Register */
> outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -529,8 +549,9 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
> struct quad8_iio *const priv = iio_priv(indio_dev);
> const int base_offset = priv->base + 2 * chan->channel + 1;
> unsigned int idr_cfg = index_polarity << 1;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> idr_cfg |= priv->synchronous_mode[chan->channel];
>
> @@ -539,7 +560,7 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
> /* Load Index Control configuration to Index Control Register */
> outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -647,6 +668,7 @@ static int quad8_count_read(struct counter_device *counter,
> unsigned int flags;
> unsigned int borrow;
> unsigned int carry;
> + unsigned long irqflags;
> int i;
>
> flags = inb(base_offset + 1);
> @@ -656,7 +678,7 @@ static int quad8_count_read(struct counter_device *counter,
> /* Borrow XOR Carry effectively doubles count range */
> *val = (unsigned long)(borrow ^ carry) << 24;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> /* Reset Byte Pointer; transfer Counter to Output Latch */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
> @@ -665,7 +687,7 @@ static int quad8_count_read(struct counter_device *counter,
> for (i = 0; i < 3; i++)
> *val |= (unsigned long)inb(base_offset) << (8 * i);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -675,13 +697,14 @@ static int quad8_count_write(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const int base_offset = priv->base + 2 * count->id;
> + unsigned long irqflags;
> int i;
>
> /* Only 24-bit values are supported */
> if (val > 0xFFFFFF)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> /* Reset Byte Pointer */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
> @@ -706,7 +729,7 @@ static int quad8_count_write(struct counter_device *counter,
> /* Reset Error flag */
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, base_offset + 1);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -725,8 +748,9 @@ static int quad8_function_read(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const int id = count->id;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> if (priv->quadrature_mode[id])
> switch (priv->quadrature_scale[id]) {
> @@ -743,7 +767,7 @@ static int quad8_function_read(struct counter_device *counter,
> else
> *function = COUNTER_FUNCTION_PULSE_DIRECTION;
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -758,10 +782,11 @@ static int quad8_function_write(struct counter_device *counter,
> unsigned int *const scale = priv->quadrature_scale + id;
> unsigned int *const synchronous_mode = priv->synchronous_mode + id;
> const int base_offset = priv->base + 2 * id + 1;
> + unsigned long irqflags;
> unsigned int mode_cfg;
> unsigned int idr_cfg;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> mode_cfg = priv->count_mode[id] << 1;
> idr_cfg = priv->index_polarity[id] << 1;
> @@ -795,7 +820,7 @@ static int quad8_function_write(struct counter_device *counter,
> mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
> break;
> default:
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
> return -EINVAL;
> }
> }
> @@ -803,7 +828,7 @@ static int quad8_function_write(struct counter_device *counter,
> /* Load mode configuration to Counter Mode Register */
> outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -898,13 +923,62 @@ static int quad8_action_read(struct counter_device *counter,
> return 0;
> }
>
> +static int quad8_events_configure(struct counter_device *counter)
> +{
> + struct quad8_iio *const priv = counter->priv;
> + unsigned long irq_enabled = 0;
> + struct counter_event_node *event_node;
> +
> + /* Enable interrupts for the requested channels, disable for the rest */
> + list_for_each_entry(event_node, &counter->events_list, l)
> + irq_enabled |= BIT(event_node->channel);
> +
> + outb(irq_enabled, priv->base + QUAD8_REG_INDEX_INTERRUPT);
> +
> + return 0;
> +}
> +
> +static int quad8_watch_validate(struct counter_device *counter,
> + const struct counter_watch *watch)
> +{
> + struct quad8_iio *const priv = counter->priv;
> +
> + if (watch->channel > QUAD8_NUM_COUNTERS - 1)
> + return -EINVAL;
> +
> + switch (watch->event) {
> + case COUNTER_EVENT_OVERFLOW:
> + if (priv->irq_trigger[watch->channel] != 0)
> + return -EINVAL;
> + break;
> + case COUNTER_EVENT_THRESHOLD:
> + if (priv->irq_trigger[watch->channel] != 1)
> + return -EINVAL;
> + break;
> + case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
> + if (priv->irq_trigger[watch->channel] != 2)
> + return -EINVAL;
> + break;
> + case COUNTER_EVENT_INDEX:
> + if (priv->irq_trigger[watch->channel] != 3)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
I'm a great fan of early returns if nothing to be done at
the end of a switch. Much cleaner than having to follow the
breaks and see where they end up (nothing in this case!)
> +}
> +
> static const struct counter_ops quad8_ops = {
> .signal_read = quad8_signal_read,
> .count_read = quad8_count_read,
> .count_write = quad8_count_write,
> .function_read = quad8_function_read,
> .function_write = quad8_function_write,
> - .action_read = quad8_action_read
> + .action_read = quad8_action_read,
> + .events_configure = quad8_events_configure,
> + .watch_validate = quad8_watch_validate,
> };
>
> static int quad8_index_polarity_get(struct counter_device *counter,
> @@ -926,9 +1000,10 @@ static int quad8_index_polarity_set(struct counter_device *counter,
> struct quad8_iio *const priv = counter->priv;
> const size_t channel_id = signal->id - 16;
> const int base_offset = priv->base + 2 * channel_id + 1;
> + unsigned long irqflags;
> unsigned int idr_cfg = index_polarity << 1;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> idr_cfg |= priv->synchronous_mode[channel_id];
>
> @@ -937,7 +1012,7 @@ static int quad8_index_polarity_set(struct counter_device *counter,
> /* Load Index Control configuration to Index Control Register */
> outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -961,15 +1036,16 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
> struct quad8_iio *const priv = counter->priv;
> const size_t channel_id = signal->id - 16;
> const int base_offset = priv->base + 2 * channel_id + 1;
> + unsigned long irqflags;
> unsigned int idr_cfg = synchronous_mode;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> idr_cfg |= priv->index_polarity[channel_id] << 1;
>
> /* Index function must be non-synchronous in non-quadrature mode */
> if (synchronous_mode && !priv->quadrature_mode[channel_id]) {
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
> return -EINVAL;
> }
>
> @@ -978,7 +1054,7 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
> /* Load Index Control configuration to Index Control Register */
> outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1025,6 +1101,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
> unsigned int count_mode;
> unsigned int mode_cfg;
> const int base_offset = priv->base + 2 * count->id + 1;
> + unsigned long irqflags;
>
> /* Map Generic Counter count mode to 104-QUAD-8 count mode */
> switch (cnt_mode) {
> @@ -1044,7 +1121,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
> return -EINVAL;
> }
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->count_mode[count->id] = count_mode;
>
> @@ -1058,7 +1135,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
> /* Load mode configuration to Counter Mode Register */
> outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1078,18 +1155,20 @@ static int quad8_count_enable_write(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const int base_offset = priv->base + 2 * count->id;
> + unsigned long irqflags;
> unsigned int ior_cfg;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->ab_enable[count->id] = enable;
>
> - ior_cfg = enable | priv->preset_enable[count->id] << 1;
> + ior_cfg = enable | priv->preset_enable[count->id] << 1 |
> + priv->irq_trigger[count->id] << 3;
>
> /* Load I/O control configuration */
> outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1135,16 +1214,17 @@ static int quad8_count_preset_write(struct counter_device *counter,
> struct counter_count *count, u64 preset)
> {
> struct quad8_iio *const priv = counter->priv;
> + unsigned long irqflags;
>
> /* Only 24-bit values are supported */
> if (preset > 0xFFFFFF)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> quad8_preset_register_set(priv, count->id, preset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1153,8 +1233,9 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
> struct counter_count *count, u64 *ceiling)
> {
> struct quad8_iio *const priv = counter->priv;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> /* Range Limit and Modulo-N count modes use preset value as ceiling */
> switch (priv->count_mode[count->id]) {
> @@ -1168,7 +1249,7 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
> break;
> }
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1177,12 +1258,13 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
> struct counter_count *count, u64 ceiling)
> {
> struct quad8_iio *const priv = counter->priv;
> + unsigned long irqflags;
>
> /* Only 24-bit values are supported */
> if (ceiling > 0xFFFFFF)
> return -EINVAL;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> /* Range Limit and Modulo-N count modes use preset value as ceiling */
> switch (priv->count_mode[count->id]) {
> @@ -1192,7 +1274,7 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
> break;
> }
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return -EINVAL;
> }
> @@ -1214,21 +1296,56 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const int base_offset = priv->base + 2 * count->id + 1;
> + unsigned long irqflags;
> unsigned int ior_cfg;
>
> /* Preset enable is active low in Input/Output Control register */
> preset_enable = !preset_enable;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->preset_enable[count->id] = preset_enable;
>
> - ior_cfg = priv->ab_enable[count->id] | preset_enable << 1;
> + ior_cfg = priv->ab_enable[count->id] | preset_enable << 1 |
> + priv->irq_trigger[count->id] << 3;
> +
> + /* Load I/O control configuration to Input / Output Control Register */
> + outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
> +
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
> +
> + return 0;
> +}
> +
> +static int quad8_irq_trigger_get(struct counter_device *counter,
> + struct counter_count *count, u32 *irq_trigger)
> +{
> + const struct quad8_iio *const priv = counter->priv;
> +
> + *irq_trigger = priv->irq_trigger[count->id];
> +
> + return 0;
> +}
> +
> +static int quad8_irq_trigger_set(struct counter_device *counter,
> + struct counter_count *count, u32 irq_trigger)
> +{
> + struct quad8_iio *const priv = counter->priv;
> + const unsigned long base_offset = priv->base + 2 * count->id + 1;
> + unsigned long irqflags;
> + unsigned long ior_cfg;
> +
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
> +
> + priv->irq_trigger[count->id] = irq_trigger;
> +
> + ior_cfg = priv->ab_enable[count->id] |
> + priv->preset_enable[count->id] << 1 | irq_trigger << 3;
>
> /* Load I/O control configuration to Input / Output Control Register */
> outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1239,22 +1356,23 @@ static int quad8_signal_cable_fault_read(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const size_t channel_id = signal->id / 2;
> + unsigned long irqflags;
> bool disabled;
> unsigned int status;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> disabled = !(priv->cable_fault_enable & BIT(channel_id));
>
> if (disabled) {
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
> return -EINVAL;
> }
>
> /* Logic 0 = cable fault */
> status = inb(priv->base + QUAD8_DIFF_ENCODER_CABLE_STATUS);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> /* Mask respective channel and invert logic */
> *cable_fault = !(status & BIT(channel_id));
> @@ -1280,9 +1398,10 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,
> {
> struct quad8_iio *const priv = counter->priv;
> const size_t channel_id = signal->id / 2;
> + unsigned long irqflags;
> unsigned int cable_fault_enable;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> if (enable)
> priv->cable_fault_enable |= BIT(channel_id);
> @@ -1294,7 +1413,7 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,
>
> outb(cable_fault_enable, priv->base + QUAD8_DIFF_ENCODER_CABLE_STATUS);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1317,8 +1436,9 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
> struct quad8_iio *const priv = counter->priv;
> const size_t channel_id = signal->id / 2;
> const int base_offset = priv->base + 2 * channel_id;
> + unsigned long irqflags;
>
> - mutex_lock(&priv->lock);
> + raw_spin_lock_irqsave(&priv->lock, irqflags);
>
> priv->fck_prescaler[channel_id] = prescaler;
>
> @@ -1330,7 +1450,7 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
> outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC,
> base_offset + 1);
>
> - mutex_unlock(&priv->lock);
> + raw_spin_unlock_irqrestore(&priv->lock, irqflags);
>
> return 0;
> }
> @@ -1433,7 +1553,15 @@ static const enum counter_count_mode quad8_cnt_modes[] = {
>
> static DEFINE_COUNTER_AVAILABLE(quad8_count_mode_available, quad8_cnt_modes);
>
> +static const char *const quad8_irq_trigger_states[] = {
> + "carry",
> + "compare",
> + "carry-borrow",
> + "index",
> +};
> +
> static DEFINE_COUNTER_ENUM(quad8_error_noise_enum, quad8_noise_error_states);
> +static DEFINE_COUNTER_ENUM(quad8_irq_trigger_enum, quad8_irq_trigger_states);
>
> static struct counter_comp quad8_count_ext[] = {
> COUNTER_COMP_CEILING(quad8_count_ceiling_read,
> @@ -1448,6 +1576,8 @@ static struct counter_comp quad8_count_ext[] = {
> COUNTER_COMP_PRESET(quad8_count_preset_read, quad8_count_preset_write),
> COUNTER_COMP_PRESET_ENABLE(quad8_count_preset_enable_read,
> quad8_count_preset_enable_write),
> + COUNTER_COMP_COUNT_ENUM("irq_trigger", quad8_irq_trigger_get,
> + quad8_irq_trigger_set, quad8_irq_trigger_enum),
> };
>
> #define QUAD8_COUNT(_id, _cntname) { \
> @@ -1472,6 +1602,48 @@ static struct counter_count quad8_counts[] = {
> QUAD8_COUNT(7, "Channel 8 Count")
> };
>
> +static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
> +{
> + struct quad8_iio *const priv = quad8iio;
> + const unsigned long base = priv->base;
> + unsigned long irq_status;
> + unsigned long channel;
> + u8 event;
> +
Take the spin lock?
> + irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
> + if (!irq_status)
> + return IRQ_NONE;
> +
> + for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
> + switch (priv->irq_trigger[channel]) {
> + case 0:
> + event = COUNTER_EVENT_OVERFLOW;
> + break;
> + case 1:
> + event = COUNTER_EVENT_THRESHOLD;
> + break;
> + case 2:
> + event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
> + break;
> + case 3:
> + event = COUNTER_EVENT_INDEX;
> + break;
> + default:
> + /* should never reach this path */
> + WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
> + priv->irq_trigger[channel], channel);
> + continue;
> + }
> +
> + counter_push_event(&priv->counter, event, channel);
> + }
> +
> + /* Clear pending interrupts on device */
> + outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int quad8_probe(struct device *dev, unsigned int id)
> {
> struct iio_dev *indio_dev;
> @@ -1510,9 +1682,10 @@ static int quad8_probe(struct device *dev, unsigned int id)
> quad8iio->counter.priv = quad8iio;
> quad8iio->base = base[id];
>
> - /* Initialize mutex */
> - mutex_init(&quad8iio->lock);
> + raw_spin_lock_init(&quad8iio->lock);
>
> + /* Reset Index/Interrupt Register */
> + outb(0x00, base[id] + QUAD8_REG_INDEX_INTERRUPT);
> /* Reset all counters and disable interrupt function */
> outb(QUAD8_CHAN_OP_RESET_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> /* Set initial configuration for all counters */
> @@ -1542,8 +1715,8 @@ static int quad8_probe(struct device *dev, unsigned int id)
> }
> /* Disable Differential Encoder Cable Status for all channels */
> outb(0xFF, base[id] + QUAD8_DIFF_ENCODER_CABLE_STATUS);
> - /* Enable all counters */
> - outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> + /* Enable all counters and enable interrupt function */
> + outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP);
>
> /* Register IIO device */
> err = devm_iio_device_register(dev, indio_dev);
> @@ -1551,7 +1724,12 @@ static int quad8_probe(struct device *dev, unsigned int id)
> return err;
>
> /* Register Counter device */
> - return devm_counter_register(dev, &quad8iio->counter);
> + err = devm_counter_register(dev, &quad8iio->counter);
> + if (err)
> + return err;
> +
> + return devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
> + quad8iio->counter.name, quad8iio);
You probably want to do that before the counter_register above
as I assume that's where the userspace interface gets exposed.
Otherwise there is probably a theoretical race.
If not then I'd expect a comment here to explain why it needs to be
in this unexpected order.
> }
>
> static struct isa_driver quad8_driver = {
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2de53ab0dd25..bd42df98f522 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -23,11 +23,11 @@ config 104_QUAD_8
> A counter's respective error flag may be cleared by performing a write
> operation on the respective count value attribute. Although the
> 104-QUAD-8 counters have a 25-bit range, only the lower 24 bits may be
> - set, either directly or via the counter's preset attribute. Interrupts
> - are not supported by this driver.
> + set, either directly or via the counter's preset attribute.
>
> The base port addresses for the devices may be configured via the base
> - array module parameter.
> + array module parameter. The interrupt line numbers for the devices may
> + be configured via the irq array module parameter.
>
> config STM32_TIMER_CNT
> tristate "STM32 Timer encoder counter driver"
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-12-30 15:31 ` Jonathan Cameron
@ 2021-02-12 6:04 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-02-12 6:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue
[-- Attachment #1: Type: text/plain, Size: 10405 bytes --]
On Wed, Dec 30, 2020 at 03:31:15PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:38 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
> > The LSI/CSI LS7266R1 chip provides programmable output via the FLG pins.
> > When interrupts are enabled on the ACCES 104-QUAD-8, they occur whenever
> > FLG1 is active. Four functions are available for the FLG1 signal: Carry,
> > Compare, Carry-Borrow, and Index.
> >
> > Carry:
> > Interrupt generated on active low Carry signal. Carry
> > signal toggles every time the respective channel's
> > counter overflows.
> >
> > Compare:
> > Interrupt generated on active low Compare signal.
> > Compare signal toggles every time respective channel's
> > preset register is equal to the respective channel's
> > counter.
> >
> > Carry-Borrow:
> > Interrupt generated on active low Carry signal and
> > active low Borrow signal. Carry signal toggles every
> > time the respective channel's counter overflows. Borrow
> > signal toggles every time the respective channel's
> > counter underflows.
> >
> > Index:
> > Interrupt generated on active high Index signal.
> >
> > The irq_trigger Count extension is introduced to allow the selection of
> > the desired IRQ trigger function per channel. Interrupts push Counter
> > events to event channel X, where 'X' is the respective channel whose
> > FLG1 activated.
> >
> > This patch adds IRQ support for the ACCES 104-QUAD-8. The interrupt line
> > numbers for the devices may be configured via the irq array module
> > parameter.
> >
> > Reviewed-by: Syed Nayyar Waris <syednwaris@gmail.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
> Immediate thought on this is that you should pull the lock type change
> out as a precursor patch that simply says we need it to be a spin_lock
> for the following patch. That should be an obvious change to review
> and then leave a much shorter patch to focus on here.
>
> Jonathan
Ack.
> > ---
> > .../ABI/testing/sysfs-bus-counter-104-quad-8 | 25 ++
> > drivers/counter/104-quad-8.c | 318 ++++++++++++++----
> > drivers/counter/Kconfig | 6 +-
> > 3 files changed, 276 insertions(+), 73 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > index eac32180c40d..0ecba24d43aa 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > @@ -1,3 +1,28 @@
> > +What: /sys/bus/counter/devices/counterX/countY/irq_trigger
>
> A warning on this. The ABI docs are now built into the main kernel
> html docs build. The snag is that it doesn't cope will with specialising
> or different devices having the same named file with different allowed values.
> You have to unify them all into one place. It may be worth just doing that
> from the start.
Ack.
> > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> > index f4fb36b751c4..7537575568d0 100644
> > --- a/drivers/counter/104-quad-8.c
> > +++ b/drivers/counter/104-quad-8.c
> > @@ -13,23 +13,30 @@
> > #include <linux/iio/types.h>
> > #include <linux/io.h>
> > #include <linux/ioport.h>
> > +#include <linux/interrupt.h>
> > #include <linux/isa.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/types.h>
> > +#include <linux/spinlock.h>
> >
> > #define QUAD8_EXTENT 32
> >
> > static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
> > static unsigned int num_quad8;
> > -module_param_array(base, uint, &num_quad8, 0);
> > +module_param_hw_array(base, uint, ioport, &num_quad8, 0);
>
> Why this change? Perhaps a note in the patch description on why
> this is needed?
This change is unrelated to this patch so I'll move it to a precursor
patch with an explanation of the reason for the change.
> > MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
> >
> > +static unsigned int irq[max_num_isa_dev(QUAD8_EXTENT)];
> > +module_param_hw_array(irq, uint, irq, NULL, 0);
> > +MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers");
> > +
> > #define QUAD8_NUM_COUNTERS 8
> >
> > /**
> > * struct quad8_iio - IIO device private data structure
> > + * @lock: synchronization lock to prevent I/O race conditions
>
> Probably want to be more specific. RMW, indexed writes or what sort of race?
Ack.
> > * @counter: instance of the counter_device
> > * @fck_prescaler: array of filter clock prescaler configurations
> > * @preset: array of preset values
> > @@ -38,13 +45,14 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
> > * @quadrature_scale: array of quadrature mode scale configurations
> > * @ab_enable: array of A and B inputs enable configurations
> > * @preset_enable: array of set_to_preset_on_index attribute configurations
> > + * @irq_trigger: array of interrupt trigger function configurations
> > * @synchronous_mode: array of index function synchronous mode configurations
> > * @index_polarity: array of index function polarity configurations
> > * @cable_fault_enable: differential encoder cable status enable configurations
> > * @base: base port address of the IIO device
> > */
> > struct quad8_iio {
> > - struct mutex lock;
> > + raw_spinlock_t lock;
>
> So it was here before but not documented. You should fix that with a precusor patch.
> Why raw_spinlock_t rather than spinlock_t?
Ack.
> > +static int quad8_watch_validate(struct counter_device *counter,
> > + const struct counter_watch *watch)
> > +{
> > + struct quad8_iio *const priv = counter->priv;
> > +
> > + if (watch->channel > QUAD8_NUM_COUNTERS - 1)
> > + return -EINVAL;
> > +
> > + switch (watch->event) {
> > + case COUNTER_EVENT_OVERFLOW:
> > + if (priv->irq_trigger[watch->channel] != 0)
> > + return -EINVAL;
> > + break;
> > + case COUNTER_EVENT_THRESHOLD:
> > + if (priv->irq_trigger[watch->channel] != 1)
> > + return -EINVAL;
> > + break;
> > + case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
> > + if (priv->irq_trigger[watch->channel] != 2)
> > + return -EINVAL;
> > + break;
> > + case COUNTER_EVENT_INDEX:
> > + if (priv->irq_trigger[watch->channel] != 3)
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> I'm a great fan of early returns if nothing to be done at
> the end of a switch. Much cleaner than having to follow the
> breaks and see where they end up (nothing in this case!)
Ack.
> > +static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
> > +{
> > + struct quad8_iio *const priv = quad8iio;
> > + const unsigned long base = priv->base;
> > + unsigned long irq_status;
> > + unsigned long channel;
> > + u8 event;
> > +
>
> Take the spin lock?
A lock isn't necessary for this particular function. This is the only place
where the device's pending interrupt state is read and reset, so there is no
risk of stale data nor of clobbering the device state because interrupts are
currently disabled.
> > + irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
> > + if (!irq_status)
> > + return IRQ_NONE;
> > +
> > + for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
> > + switch (priv->irq_trigger[channel]) {
> > + case 0:
> > + event = COUNTER_EVENT_OVERFLOW;
> > + break;
> > + case 1:
> > + event = COUNTER_EVENT_THRESHOLD;
> > + break;
> > + case 2:
> > + event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
> > + break;
> > + case 3:
> > + event = COUNTER_EVENT_INDEX;
> > + break;
> > + default:
> > + /* should never reach this path */
> > + WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
> > + priv->irq_trigger[channel], channel);
> > + continue;
> > + }
> > +
> > + counter_push_event(&priv->counter, event, channel);
> > + }
> > +
> > + /* Clear pending interrupts on device */
> > + outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static int quad8_probe(struct device *dev, unsigned int id)
> > {
> > struct iio_dev *indio_dev;
> > @@ -1510,9 +1682,10 @@ static int quad8_probe(struct device *dev, unsigned int id)
> > quad8iio->counter.priv = quad8iio;
> > quad8iio->base = base[id];
> >
> > - /* Initialize mutex */
> > - mutex_init(&quad8iio->lock);
> > + raw_spin_lock_init(&quad8iio->lock);
> >
> > + /* Reset Index/Interrupt Register */
> > + outb(0x00, base[id] + QUAD8_REG_INDEX_INTERRUPT);
> > /* Reset all counters and disable interrupt function */
> > outb(QUAD8_CHAN_OP_RESET_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> > /* Set initial configuration for all counters */
> > @@ -1542,8 +1715,8 @@ static int quad8_probe(struct device *dev, unsigned int id)
> > }
> > /* Disable Differential Encoder Cable Status for all channels */
> > outb(0xFF, base[id] + QUAD8_DIFF_ENCODER_CABLE_STATUS);
> > - /* Enable all counters */
> > - outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> > + /* Enable all counters and enable interrupt function */
> > + outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP);
> >
> > /* Register IIO device */
> > err = devm_iio_device_register(dev, indio_dev);
> > @@ -1551,7 +1724,12 @@ static int quad8_probe(struct device *dev, unsigned int id)
> > return err;
> >
> > /* Register Counter device */
> > - return devm_counter_register(dev, &quad8iio->counter);
> > + err = devm_counter_register(dev, &quad8iio->counter);
> > + if (err)
> > + return err;
> > +
> > + return devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
> > + quad8iio->counter.name, quad8iio);
> You probably want to do that before the counter_register above
> as I assume that's where the userspace interface gets exposed.
> Otherwise there is probably a theoretical race.
> If not then I'd expect a comment here to explain why it needs to be
> in this unexpected order.
Ack.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-12-26 0:15 ` [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
2020-12-30 15:31 ` Jonathan Cameron
@ 2020-12-30 17:36 ` David Lechner
2021-02-11 23:56 ` William Breathitt Gray
1 sibling, 1 reply; 23+ messages in thread
From: David Lechner @ 2020-12-30 17:36 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> index eac32180c40d..0ecba24d43aa 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> @@ -1,3 +1,28 @@
> +What: /sys/bus/counter/devices/counterX/countY/irq_trigger
Do we really need this sysfs attribute? Shouldn't interrupts be configured
_only_ by the chrdev interface?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-12-30 17:36 ` David Lechner
@ 2021-02-11 23:56 ` William Breathitt Gray
2021-02-12 1:10 ` David Lechner
0 siblings, 1 reply; 23+ messages in thread
From: William Breathitt Gray @ 2021-02-11 23:56 UTC (permalink / raw)
To: David Lechner
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]
On Wed, Dec 30, 2020 at 11:36:45AM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>
> > diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > index eac32180c40d..0ecba24d43aa 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > @@ -1,3 +1,28 @@
> > +What: /sys/bus/counter/devices/counterX/countY/irq_trigger
>
> Do we really need this sysfs attribute? Shouldn't interrupts be configured
> _only_ by the chrdev interface?
I think this attribute can go away because we can implicitly figure out
the correct IRQ configuration from the struct counter_watch data when a
user executes a COUNTER_ADD_WATCH_IOCTL ioctl command.
However, I need some help deciding on an appropriate behavior for
conflicting counter_watch configurations. Let me give some context
first.
The 104-QUAD-8 features 8 channels (essentially 8 independent physical
counters on the device). Each channel can independently issue an event,
but any particular channel can only be set to a single kind of event
(COUNTER_EVENT_INDEX, COUNTER_EVENT_OVERFLOW, etc.).
The purpose of the irq_trigger sysfs attribute I introduced in this
patch is to allow the user to select the event configuration they want
for a particular channel. We can theoretically figure this out
implicitly from the struct counter_watch request, so this sysfs
attribute may not be necessary.
However, how do we handle the case where a user executes two
COUNTER_ADD_WATCH_IOCTL ioctl commands for the same channel but with
different event selections? I'm considering three possible behaviors:
* Fail the second ioctl call; event selection of the first struct
counter_watch takes precedence and thus second is incompatible.
* Issue a dev_warn() indicating that the second struct counter_watch
event selection will now be the event configuration for that channel.
* Don't notify the user, just silently reconfigure for the second struct
counter_watch event selection.
I'm suspecting the first behavior I listed here (ioctl returning failed)
is the most appropriate as a user is explicitly made known of this
particular device's inability to support more than one type of event per
channel.
What do you think?
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2021-02-11 23:56 ` William Breathitt Gray
@ 2021-02-12 1:10 ` David Lechner
0 siblings, 0 replies; 23+ messages in thread
From: David Lechner @ 2021-02-12 1:10 UTC (permalink / raw)
To: William Breathitt Gray
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 2/11/21 5:56 PM, William Breathitt Gray wrote:
> On Wed, Dec 30, 2020 at 11:36:45AM -0600, David Lechner wrote:
>> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
>>> index eac32180c40d..0ecba24d43aa 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
>>> +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
>>> @@ -1,3 +1,28 @@
>>> +What: /sys/bus/counter/devices/counterX/countY/irq_trigger
>>
>> Do we really need this sysfs attribute? Shouldn't interrupts be configured
>> _only_ by the chrdev interface?
>
> I think this attribute can go away because we can implicitly figure out
> the correct IRQ configuration from the struct counter_watch data when a
> user executes a COUNTER_ADD_WATCH_IOCTL ioctl command.
>
> However, I need some help deciding on an appropriate behavior for
> conflicting counter_watch configurations. Let me give some context
> first.
>
> The 104-QUAD-8 features 8 channels (essentially 8 independent physical
> counters on the device). Each channel can independently issue an event,
> but any particular channel can only be set to a single kind of event
> (COUNTER_EVENT_INDEX, COUNTER_EVENT_OVERFLOW, etc.).
>
> The purpose of the irq_trigger sysfs attribute I introduced in this
> patch is to allow the user to select the event configuration they want
> for a particular channel. We can theoretically figure this out
> implicitly from the struct counter_watch request, so this sysfs
> attribute may not be necessary.
>
> However, how do we handle the case where a user executes two
> COUNTER_ADD_WATCH_IOCTL ioctl commands for the same channel but with
> different event selections? I'm considering three possible behaviors:
>
> * Fail the second ioctl call; event selection of the first struct
> counter_watch takes precedence and thus second is incompatible.
> * Issue a dev_warn() indicating that the second struct counter_watch
> event selection will now be the event configuration for that channel.
> * Don't notify the user, just silently reconfigure for the second struct
> counter_watch event selection.
>
> I'm suspecting the first behavior I listed here (ioctl returning failed)
> is the most appropriate as a user is explicitly made known of this
> particular device's inability to support more than one type of event per
> channel.
>
> What do you think?
>
I agree that it should return an error instead of adding the watch.
I'm pretty sure that is how I implemented the TI eQEP driver already.
^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <fc40ab7f4a38e80d86715daa5eaf744dd645a75b.1608935587.git.vilhelm.gray@gmail.com>]
* Re: [PATCH v7 1/5] counter: Internalize sysfs interface code
[not found] ` <fc40ab7f4a38e80d86715daa5eaf744dd645a75b.1608935587.git.vilhelm.gray@gmail.com>
@ 2020-12-30 23:24 ` David Lechner
2021-01-06 5:30 ` William Breathitt Gray
[not found] ` <20201230143719.28a90914@archlinux>
1 sibling, 1 reply; 23+ messages in thread
From: David Lechner @ 2020-12-30 23:24 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index a60aee1a1a29..6c058b93dc98 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> -static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
> - struct counter_count *count,
> - void *ext_priv, const char *buf,
> - size_t len)
> +static int ti_eqep_position_floor_write(struct counter_device *counter,
> + struct counter_count *count, u64 floor)
> {
> struct ti_eqep_cnt *priv = counter->priv;
> - int err;
> - u32 res;
>
> - err = kstrtouint(buf, 0, &res);
> - if (err < 0)
> - return err;
> + if (floor != (u32)floor)
> + return -ERANGE;
>
> - regmap_write(priv->regmap32, QPOSINIT, res);
> + regmap_write(priv->regmap32, QPOSINIT, floor);
>
> - return len;
> + return 0;
> }
This will conflict with 2ba7b50893de "counter:ti-eqep: remove floor"
(in Jonathan's fixes-togreg branch) which removes these functions.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 1/5] counter: Internalize sysfs interface code
2020-12-30 23:24 ` [PATCH v7 1/5] counter: Internalize sysfs interface code David Lechner
@ 2021-01-06 5:30 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-01-06 5:30 UTC (permalink / raw)
To: David Lechner
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]
On Wed, Dec 30, 2020 at 05:24:34PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>
> > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> > index a60aee1a1a29..6c058b93dc98 100644
> > --- a/drivers/counter/ti-eqep.c
> > +++ b/drivers/counter/ti-eqep.c
>
>
> > -static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
> > - struct counter_count *count,
> > - void *ext_priv, const char *buf,
> > - size_t len)
> > +static int ti_eqep_position_floor_write(struct counter_device *counter,
> > + struct counter_count *count, u64 floor)
> > {
> > struct ti_eqep_cnt *priv = counter->priv;
> > - int err;
> > - u32 res;
> >
> > - err = kstrtouint(buf, 0, &res);
> > - if (err < 0)
> > - return err;
> > + if (floor != (u32)floor)
> > + return -ERANGE;
> >
> > - regmap_write(priv->regmap32, QPOSINIT, res);
> > + regmap_write(priv->regmap32, QPOSINIT, floor);
> >
> > - return len;
> > + return 0;
> > }
>
> This will conflict with 2ba7b50893de "counter:ti-eqep: remove floor"
> (in Jonathan's fixes-togreg branch) which removes these functions.
Ack, I'll rebase and remove these changes.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <20201230143719.28a90914@archlinux>]
* Re: [PATCH v7 1/5] counter: Internalize sysfs interface code
[not found] ` <20201230143719.28a90914@archlinux>
@ 2021-01-06 5:29 ` William Breathitt Gray
0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2021-01-06 5:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 12192 bytes --]
On Wed, Dec 30, 2020 at 02:37:19PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:34 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
> > This is a reimplementation of the Generic Counter driver interface.
> > There are no modifications to the Counter subsystem userspace interface,
> > so existing userspace applications should continue to run seamlessly.
> Hi William
>
> Been a while since I looked at this series. Its rather big and I'm lazy
> (or busy depending on who I'm talking to :)
>
> Hmm. I'm a bit in two minds about how you should handle the huge amount of
> description here. Some of it clearly belongs in the kernel docs (and some
> is I think put there in a later patch). Other parts are specific to
> this series, but I'm not 100% sure this much detail is really useful in the
> git log. Note that we now have links to the threads for all patches applied
> using b4 (which this will be) so it's fine to have really detailed stuff
> in cover letters rather than the individual patches.
I'll simplify the description here to something more succinct.
> One thing that would be handy for review, might be if you put up a tree
> somewhere with this applied and included a link.
This is such a large set of changes that having a tree to checkout for
review would be convenient. I typically push to my personal tree, so you
can check out the changes there in the counter_chrdev_v* branches:
https://gitlab.com/vilhelmgray/iio
I'll include a link to it again in the cover letter for v8 when it's
ready.
> Mind you I don't feel that strongly about it if it you do want to maintain
> it in the individual patch descriptions.
>
> I've been a bit lazy and not cropped this down as much as I ideally should
> have done (to include only bits I'm commenting on).
>
> Anyhow, various minor things inline but this fundamentally looks fine to me.
>
> Jonathan
>
>
> >
> > Overview
> > ========
> >
> > The purpose of this patch is to internalize the sysfs interface code
> > among the various counter drivers into a shared module. Counter drivers
> > pass and take data natively (i.e. u8, u64, etc.) and the shared counter
> > module handles the translation between the sysfs interface.
>
> Confusing statement. Between the sysfs interface and what?
> Perhaps "handles the translation to/from the sysfs interface."
Looks like I cut that line short by accident; it should read: "between
the sysfs interface and the device drivers". I'll fix this up.
> > This
> > guarantees a standard userspace interface for all counter drivers, and
> > helps generalize the Generic Counter driver ABI in order to support the
> > Generic Counter chrdev interface (introduced in a subsequent patch)
> > without significant changes to the existing counter drivers.
> >
> > A high-level view of how a count value is passed down from a counter
> > driver is exemplified by the following. The driver callbacks are first
> > registered to the Counter core component for use by the Counter
> > userspace interface components:
> >
> > +----------------------------+
> > | Counter device driver |
>
> Looks like something snuck a tab in amongst your spaces.
Ack.
> > static int quad8_signal_read(struct counter_device *counter,
> > - struct counter_signal *signal, enum counter_signal_value *val)
> > + struct counter_signal *signal,
> > + enum counter_signal_level *level)
> > {
> > const struct quad8_iio *const priv = counter->priv;
> > unsigned int state;
> > @@ -633,13 +634,13 @@ static int quad8_signal_read(struct counter_device *counter,
> > state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
> > & BIT(signal->id - 16);
> >
> > - *val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
> > + *level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
>
> This bit of refactoring / renaming could have been split out as a precursor patch
> I think. There may be other opportunities.
Ack. I'll look around for additional changes I can pull out as precursor
patches too.
> >
> > return 0;
> > }
> >
> > static int quad8_count_read(struct counter_device *counter,
> > - struct counter_count *count, unsigned long *val)
> > + struct counter_count *count, u64 *val)
>
> Could the type change for val have been done as a precursor?
I don't think we can pull this one out as a precursor unfortunately.
Since unsigned long is passed in as pointer, we could get a type
mismatch if we're on a 32-bit system. For this to work, it requires the
u64 change in struct counter_ops and subsequent dependent code, so we'll
have to keep it as part of this patch for now.
> > @@ -785,18 +782,21 @@ static int quad8_function_set(struct counter_device *counter,
> > *quadrature_mode = 1;
> >
> > switch (function) {
> > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X1:
> > + case COUNTER_FUNCTION_QUADRATURE_X1_A:
> > *scale = 0;
> > mode_cfg |= QUAD8_CMR_QUADRATURE_X1;
> > break;
> > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X2:
> > + case COUNTER_FUNCTION_QUADRATURE_X2_A:
> > *scale = 1;
> > mode_cfg |= QUAD8_CMR_QUADRATURE_X2;
> > break;
> > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X4:
> > + case COUNTER_FUNCTION_QUADRATURE_X4:
> > *scale = 2;
> > mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
> > break;
> > + default:
> > + mutex_unlock(&priv->lock);
> > + return -EINVAL;
>
> This looks like a sensible precaution / cleanup but could have been
> done separately to the rest of the patch I think?
Ack.
> > @@ -1229,30 +1194,28 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
> >
> > mutex_unlock(&priv->lock);
> >
> > - return len;
> > + return -EINVAL;
>
> ? That looks like the good exit path to me.
You're right, this should be a return 0.
> > +/**
> > + * counter_register - register Counter to the system
> > + * @counter: pointer to Counter to register
> > + *
> > + * This function registers a Counter to the system. A sysfs "counter" directory
> > + * will be created and populated with sysfs attributes correlating with the
> > + * Counter Signals, Synapses, and Counts respectively.
>
> Where easy to do it's worth documenting return values.
Ack.
> > +static void devm_counter_unregister(struct device *dev, void *res)
> > +{
> > + counter_unregister(*(struct counter_device **)res);
>
> Rename this. It looks like it's a generic way of unwinding
> devm_counter_register which it is definitely not...
Ack.
> > +/**
> > + * struct counter_attribute - Counter sysfs attribute
> > + * @dev_attr: device attribute for sysfs
> > + * @l: node to add Counter attribute to attribute group list
> > + * @comp: Counter component callbacks and data
> > + * @scope: Counter scope of the attribute
> > + * @parent: pointer to the parent component
> > + */
> > +struct counter_attribute {
> > + struct device_attribute dev_attr;
> > + struct list_head l;
> > +
> > + struct counter_comp comp;
> > + __u8 scope;
>
> Why not an enum?
This should be enum; I missed it from the previous revision.
> > + switch (a->comp.type) {
> > + case COUNTER_COMP_FUNCTION:
> > + return sprintf(buf, "%s\n", counter_function_str[data]);
> > + case COUNTER_COMP_SIGNAL_LEVEL:
> > + return sprintf(buf, "%s\n", counter_signal_value_str[data]);
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + return sprintf(buf, "%s\n", counter_synapse_action_str[data]);
> > + case COUNTER_COMP_ENUM:
> > + return sprintf(buf, "%s\n", avail->strs[data]);
> > + case COUNTER_COMP_COUNT_DIRECTION:
> > + return sprintf(buf, "%s\n", counter_count_direction_str[data]);
> > + case COUNTER_COMP_COUNT_MODE:
> > + return sprintf(buf, "%s\n", counter_count_mode_str[data]);
> > + default:
>
> Perhaps move the below return sprintf() up here?
Ack.
> > + break;
> > + }
> > +
> > + return sprintf(buf, "%u\n", (unsigned int)data);
> > +}
> > +
> > +static int find_in_string_array(u32 *const enum_item, const u32 *const enums,
> > + const size_t num_enums, const char *const buf,
> > + const char *const string_array[])
>
> Please avoid defining such generically named functions. High chance of a clash
> with something that turns up in generic headers sometime in the future.
Ack.
> > +static ssize_t enums_available_show(const u32 *const enums,
> > + const size_t num_enums,
> > + const char *const strs[], char *buf)
> > +{
> > + size_t len = 0;
> > + size_t index;
> > +
> > + for (index = 0; index < num_enums; index++)
> > + len += sprintf(buf + len, "%s\n", strs[enums[index]]);
>
> Probably better to add protections on overrunning the buffer to this.
> Sure it won't actually happen but that may not be obvious to someone reading
> this code in future.
>
> Look at new sysfs_emit * family for this.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2efc459d06f1630001e3984854848a5647086232
Ack.
> > +static ssize_t counter_comp_available_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + const struct counter_attribute *const a = to_counter_attribute(attr);
> > + const struct counter_count *const count = a->parent;
> > + const struct counter_synapse *const synapse = a->comp.priv;
> > + const struct counter_available *const avail = a->comp.priv;
> > +
> > + switch (a->comp.type) {
> > + case COUNTER_COMP_FUNCTION:
> > + return enums_available_show(count->functions_list,
> > + count->num_functions,
> > + counter_function_str, buf);
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + return enums_available_show(synapse->actions_list,
> > + synapse->num_actions,
> > + counter_synapse_action_str, buf);
> > + case COUNTER_COMP_ENUM:
> > + return strs_available_show(avail, buf);
> > + case COUNTER_COMP_COUNT_MODE:
> > + return enums_available_show(avail->enums, avail->num_items,
> > + counter_count_mode_str, buf);
> > + default:
> > + break;
>
> Might as well return -EINVAL; here
Ack.
> > + /* Store list node */
> > + list_add(&counter_attr->l, &group->attr_list);
> > + group->num_attr++;
> > +
> > + switch (comp->type) {
> > + case COUNTER_COMP_FUNCTION:
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + case COUNTER_COMP_ENUM:
> > + case COUNTER_COMP_COUNT_MODE:
> > + return counter_avail_attr_create(dev, group, comp, parent);
> > + default:
> > + break;
>
> return 0 in here. Also add a comment on why it isn't an error.
Ack.
> > +static int counter_sysfs_synapses_add(struct counter_device *const counter,
> > + struct counter_attribute_group *const group,
> > + struct counter_count *const count)
> > +{
> > + const __u8 scope = COUNTER_SCOPE_COUNT;
> > + struct device *const dev = &counter->dev;
> > + size_t i;
> > + struct counter_synapse *synapse;
> > + size_t id;
> > + struct counter_comp comp;
> > + int err;
> > +
> > + /* Add each Synapse */
> > + for (i = 0; i < count->num_synapses; i++) {
> Could reduce scope and make code a bit more readable by
> pulling
>
> struct counter_synapse *synapse;
> struct counter_comp comp;
> size_t id;
>
> and maybe other things in here. Makes it clear their scope
> is just within this loop.
Ack.
> > /**
> > * struct counter_synapse - Counter Synapse node
> > - * @action: index of current action mode
> > * @actions_list: array of available action modes
> > * @num_actions: number of action modes specified in @actions_list
> > - * @signal: pointer to associated signal
> > + * @signal: pointer to the associated Signal
>
> Might have been nice to pull the cases that were purely capitalization out as
> a separate patch immediately following this one. There aren't
> a huge number, but from a review point of view it's a noop patch
> so doesn't need the reviewer to be awake :)
Ack.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 0/5] Introduce the Counter character device interface
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
` (4 preceding siblings ...)
[not found] ` <fc40ab7f4a38e80d86715daa5eaf744dd645a75b.1608935587.git.vilhelm.gray@gmail.com>
@ 2020-12-30 23:34 ` David Lechner
5 siblings, 0 replies; 23+ messages in thread
From: David Lechner @ 2020-12-30 23:34 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> Changes in v7:
> - Implemented u32 enums; enum types can now be used directly for
> callbacks and values
> - Fixed refcount underflow bug
> - Refactored all err check to check for err < 0; this should help
> prevent future oversights on valid positive return valids
> - Use mutex instead of raw_spin_lock in counter_chrdev_read();
> kifo_to_user() can now safely sleep
> - Renamed COUNTER_COMPONENT_DUMMY to COUNTER_COMPONENT_NONE to make
> purpose more obvious
> - Introduced a watch_validate() callback
> - Consolidated the functionality to clear the watches to the
> counter_clear_watches() function
> - Reimplemented counter_push_event() as a void function; on error,
> errno is returned via struct counter_event so that it can be handled
> by userspace (because interrupt handlers can't do much for this)
> - Renamed the events_config() callback to events_configure();
> "events_config" could be confused as a read callback when this is
> actually intended to configure the device for the requested events
> - Reimplemented 104-QUAD-8 driver to use events_configure() and
> watch_validate() callbacks; irq_trigger_enable sysfs attribute
> removed because events_configure() now serves this purpose
>
> The changes for this revision were much simpler compared to the previous
> revisions. I don't have any further questions for this patchset, so it's
> really just a search for possible bugs or regressions now. Please test
> and verify this patchset on your systems, and ACK appropriately.
>
I'll wait for the next round to give my Reviewed-By, Tested-By but I've
rebased my WIP TI eQEP changes[1] on this and I haven't ran into any
problems yet.
[1]: https://github.com/dlech/linux/commits/bone-counter
^ permalink raw reply [flat|nested] 23+ messages in thread