Simplify count_read/count_write/signal_read

PATCH In review William Breathitt Gray 2 participants 7 messages Oct 6, 2019 – Oct 17, 2019

Subsystems (8)

Documentation IIO Arm STM32 Counter +4 more subsystems

lore

Share & tools

lore

William Breathitt Gray

author

2019-10-06 20:03 UTC #0 lore raw

v5 cover Simplify count_read/count_write/signal_read

Changes in v5:
- Add changes and additions to generic-counter.rst to clarify theory and use of the Generic Counter interface
- Fix typo in counter.h action_get description comment

The changes in this patchset will not affect the userspace interface. Rather, these changes are intended to simplify the kernelspace Counter callbacks for counter device driver authors.

The following main changes are proposed:

* Retire the opaque counter_count_read_value/counter_count_write_value structures and simply represent count data as an unsigned integer.

* Retire the opaque counter_signal_read_value structure and represent Signal data as a counter_signal_value enum.

These changes should reduce some complexity and code in the use and implementation of the count_read, count_write, and signal_read callbacks.

The opaque structures for Count data and Signal data were introduced originally in anticipation of supporting various representations of counter data (e.g. arbitrary-precision tallies, floating-point spherical coordinate positions, etc). However, with the counter device drivers that have appeared, it's become apparent that utilizing opaque structures in kernelspace is not the best approach to take.

I believe it is best to let userspace applications decide how to interpret the count data they receive. There are a couple of reasons why it would be good to do so:

* Users use their devices in unexpected ways.

For example, a quadrature encoder counter device is typically used to keep track of the position of a motor, but a user could set the device in a pulse-direction mode and instead use it to count sporadic rising edges from an arbitrary signal line unrelated to positioning. Users should have the freedom to decide what their data represents.

* Most counter devices represent data as unsigned integers anyway.

For example, whether the device is a tally counter or position counter, the count data is represented to the user as an unsigned integer value. So specifying that one device is representing tallies while the other specifies positions does not provide much utility from an interface perspective.

For these reasons, the count_read and count_write callbacks have been redefined to pass count data directly as unsigned long instead of passed via opaque structures:

count_read(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
count_write(struct counter_device *counter,
struct counter_count *count, unsigned long val);

Similarly, the signal_read is redefined to pass Signal data directly as a counter_signal_value enum instead of via an opaque structure:

signal_read(struct counter_device *counter,
struct counter_signal *signal, enum counter_signal_value *val);

The counter_signal_value enum is simply the counter_signal_level enum redefined to remove the references to the Signal data "level" data type.

William Breathitt Gray (3):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
counter: Fix typo in action_get description

 Documentation/driver-api/generic-counter.rst | 162 +++++++++++--------
 drivers/counter/104-quad-8.c                 |  33 ++--
 drivers/counter/counter.c                    | 101 ++----------
 drivers/counter/ftm-quaddec.c                |  14 +-
 drivers/counter/stm32-lptimer-cnt.c          |   5 +-
 drivers/counter/stm32-timer-cnt.c            |  17 +-
 drivers/counter/ti-eqep.c                    |  19 +--
 include/linux/counter.h                      |  76 ++-------
 8 files changed, 144 insertions(+), 283 deletions(-)

base-commit: 0c3aa63a842d84990bd02622f2fa50d2bd33c652
prerequisite-patch-id: ebe284609b3db8d4130ea2915f7f7b185c743a70
prerequisite-patch-id: cbe857759f10d875690df125d18bc04f585ac7c9
prerequisite-patch-id: 21f2660dc88627387ee4666d08044c63dd961dae

Signature · 1 line

2.23.0

William Breathitt Gray

author

2019-10-06 20:03 UTC #1 lore raw

PATCH v5 1/3 counter: Simplify the count_read and count_write callbacks

The count_read and count_write callbacks are simplified to pass val as unsigned long rather than as an opaque data structure. The opaque counter_count_read_value and counter_count_write_value structures, counter_count_value_type enum, and relevant counter_count_read_value_set and counter_count_write_value_get functions, are removed as they are no longer used.

Cc Patrick Havelange <[email protected]>

Acked-by Fabrice Gasnier <[email protected]>

Acked-by David Lechner <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c        |  33 +++------
 drivers/counter/counter.c           | 101 +++++-----------------------
 drivers/counter/ftm-quaddec.c       |  14 ++--
 drivers/counter/stm32-lptimer-cnt.c |   5 +-
 drivers/counter/stm32-timer-cnt.c   |  17 ++---
 drivers/counter/ti-eqep.c           |  19 ++----
 include/linux/counter.h             |  74 +++-----------------
 7 files changed, 51 insertions(+), 212 deletions(-)

drivers/counter/104-quad-8.c +10 ~~-23~~

index 00b113f4b958..17e67a84777d 100644
@@ -562,11 +562,10 @@ static const struct iio_chan_spec quad8_channels[] = {
 };
 
 static int quad8_signal_read(struct counter_device *counter,
-	struct counter_signal *signal, struct counter_signal_read_value *val)
+	struct counter_signal *signal, enum counter_signal_value *val)
 {
 	const struct quad8_iio *const priv = counter->priv;
 	unsigned int state;
-	enum counter_signal_level level;
 
 	/* Only Index signal levels can be read */
 	if (signal->id < 16)

@@ -575,22 +574,19 @@ static int quad8_signal_read(struct counter_device *counter,
 	state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
 		& BIT(signal->id - 16);
 
-	level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
-
-	counter_signal_read_value_set(val, COUNTER_SIGNAL_LEVEL, &level);
+	*val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
 
 	return 0;
 }
 
 static int quad8_count_read(struct counter_device *counter,
-	struct counter_count *count, struct counter_count_read_value *val)
+	struct counter_count *count, unsigned long *val)
 {
 	const struct quad8_iio *const priv = counter->priv;
 	const int base_offset = priv->base + 2 * count->id;
 	unsigned int flags;
 	unsigned int borrow;
 	unsigned int carry;
-	unsigned long position;
 	int i;
 
 	flags = inb(base_offset + 1);

@@ -598,36 +594,27 @@ static int quad8_count_read(struct counter_device *counter,
 	carry = !!(flags & QUAD8_FLAG_CT);
 
 	/* Borrow XOR Carry effectively doubles count range */
-	position = (unsigned long)(borrow ^ carry) << 24;
+	*val = (unsigned long)(borrow ^ carry) << 24;
 
 	/* Reset Byte Pointer; transfer Counter to Output Latch */
 	outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
 	     base_offset + 1);
 
 	for (i = 0; i < 3; i++)
-		position |= (unsigned long)inb(base_offset) << (8 * i);
-
-	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &position);
+		*val |= (unsigned long)inb(base_offset) << (8 * i);
 
 	return 0;
 }
 
 static int quad8_count_write(struct counter_device *counter,
-	struct counter_count *count, struct counter_count_write_value *val)
+	struct counter_count *count, unsigned long val)
 {
 	const struct quad8_iio *const priv = counter->priv;
 	const int base_offset = priv->base + 2 * count->id;
-	int err;
-	unsigned long position;
 	int i;
 
-	err = counter_count_write_value_get(&position, COUNTER_COUNT_POSITION,
-					    val);
-	if (err)
-		return err;
-
 	/* Only 24-bit values are supported */
-	if (position > 0xFFFFFF)
+	if (val > 0xFFFFFF)
 		return -EINVAL;
 
 	/* Reset Byte Pointer */

@@ -635,7 +622,7 @@ static int quad8_count_write(struct counter_device *counter,
 
 	/* Counter can only be set via Preset Register */
 	for (i = 0; i < 3; i++)
-		outb(position >> (8 * i), base_offset);
+		outb(val >> (8 * i), base_offset);
 
 	/* Transfer Preset Register to Counter */
 	outb(QUAD8_CTR_RLD | QUAD8_RLD_PRESET_CNTR, base_offset + 1);

@@ -644,9 +631,9 @@ static int quad8_count_write(struct counter_device *counter,
 	outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
 
 	/* Set Preset Register back to original value */
-	position = priv->preset[count->id];
+	val = priv->preset[count->id];
 	for (i = 0; i < 3; i++)
-		outb(position >> (8 * i), base_offset);
+		outb(val >> (8 * i), base_offset);
 
 	/* Reset Borrow, Carry, Compare, and Sign flags */
 	outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_FLAGS, base_offset + 1);

drivers/counter/counter.c +15 ~~-86~~

142 lines not on this page · raw message

drivers/counter/ftm-quaddec.c +4 ~~-10~~

index 4046aa9f9234..c2b3fdfd8b77 100644
@@ -178,31 +178,25 @@ static const enum counter_count_function ftm_quaddec_count_functions[] = {
 
 static int ftm_quaddec_count_read(struct counter_device *counter,
 				  struct counter_count *count,
-				  struct counter_count_read_value *val)
+				  unsigned long *val)
 {
 	struct ftm_quaddec *const ftm = counter->priv;
 	uint32_t cntval;
 
 	ftm_read(ftm, FTM_CNT, &cntval);
 
-	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
+	*val = cntval;
 
 	return 0;
 }
 
 static int ftm_quaddec_count_write(struct counter_device *counter,
 				   struct counter_count *count,
-				   struct counter_count_write_value *val)
+				   const unsigned long val)
 {
 	struct ftm_quaddec *const ftm = counter->priv;
-	u32 cnt;
-	int err;
 
-	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
-	if (err)
-		return err;
-
-	if (cnt != 0) {
+	if (val != 0) {
 		dev_warn(&ftm->pdev->dev, "Can only accept '0' as new counter value\n");
 		return -EINVAL;
 	}

drivers/counter/stm32-lptimer-cnt.c +2 -3

index 28b63645c411..8e276eb655f5 100644
@@ -377,8 +377,7 @@ static enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = {
 };
 
 static int stm32_lptim_cnt_read(struct counter_device *counter,
-				struct counter_count *count,
-				struct counter_count_read_value *val)
+				struct counter_count *count, unsigned long *val)
 {
 	struct stm32_lptim_cnt *const priv = counter->priv;
 	u32 cnt;

@@ -388,7 +387,7 @@ static int stm32_lptim_cnt_read(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
+	*val = cnt;
 
 	return 0;
 }

drivers/counter/stm32-timer-cnt.c +5 ~~-12~~

index b61135b63ee8..3eafccec3beb 100644
@@ -48,34 +48,27 @@ static enum counter_count_function stm32_count_functions[] = {
 };
 
 static int stm32_count_read(struct counter_device *counter,
-			    struct counter_count *count,
-			    struct counter_count_read_value *val)
+			    struct counter_count *count, unsigned long *val)
 {
 	struct stm32_timer_cnt *const priv = counter->priv;
 	u32 cnt;
 
 	regmap_read(priv->regmap, TIM_CNT, &cnt);
-	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
+	*val = cnt;
 
 	return 0;
 }
 
 static int stm32_count_write(struct counter_device *counter,
 			     struct counter_count *count,
-			     struct counter_count_write_value *val)
+			     const unsigned long val)
 {
 	struct stm32_timer_cnt *const priv = counter->priv;
-	u32 cnt;
-	int err;
-
-	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
-	if (err)
-		return err;
 
-	if (cnt > priv->ceiling)
+	if (val > priv->ceiling)
 		return -EINVAL;
 
-	return regmap_write(priv->regmap, TIM_CNT, cnt);
+	return regmap_write(priv->regmap, TIM_CNT, val);
 }
 
 static int stm32_count_function_get(struct counter_device *counter,

drivers/counter/ti-eqep.c +6 ~~-13~~

index 4b3ef2449c06..1ff07faef27f 100644
@@ -93,35 +93,28 @@ struct ti_eqep_cnt {
 };
 
 static int ti_eqep_count_read(struct counter_device *counter,
-			      struct counter_count *count,
-			      struct counter_count_read_value *val)
+			      struct counter_count *count, unsigned long *val)
 {
 	struct ti_eqep_cnt *priv = counter->priv;
 	u32 cnt;
 
 	regmap_read(priv->regmap32, QPOSCNT, &cnt);
-	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
+	*val = cnt;
 
 	return 0;
 }
 
 static int ti_eqep_count_write(struct counter_device *counter,
-			       struct counter_count *count,
-			       struct counter_count_write_value *val)
+			       struct counter_count *count, unsigned long val)
 {
 	struct ti_eqep_cnt *priv = counter->priv;
-	u32 cnt, max;
-	int err;
-
-	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
-	if (err)
-		return err;
+	u32 max;
 
 	regmap_read(priv->regmap32, QPOSMAX, &max);
-	if (cnt > max)
+	if (val > max)
 		return -EINVAL;
 
-	return regmap_write(priv->regmap32, QPOSCNT, cnt);
+	return regmap_write(priv->regmap32, QPOSCNT, val);
 }
 
 static int ti_eqep_function_get(struct counter_device *counter,

include/linux/counter.h +9 ~~-65~~

104 lines not on this page · raw message

Signature · 1 line

2.23.0

William Breathitt Gray

author

2019-10-06 20:03 UTC #2 lore raw

PATCH v5 2/3 docs: driver-api: generic-counter: Update Count and Signal data types

In reply to William Breathitt Gray

Count data is now always represented as an unsigned integer, while Signal data is either SIGNAL_LOW or SIGNAL_HIGH. In addition, clarification changes and additions are made to better explain the theory of the Generic Counter interface and its use.

Signed-off-by William Breathitt Gray <[email protected]>

---
 Documentation/driver-api/generic-counter.rst | 162 +++++++++++--------
 1 file changed, 92 insertions(+), 70 deletions(-)

Documentation/driver-api/generic-counter.rst +92 ~~-70~~

index 8382f01a53e3..e622f8f6e56a 100644
@@ -7,7 +7,7 @@ Generic Counter Interface
 Introduction
 ============
 
-Counter devices are prevalent within a diverse spectrum of industries.
+Counter devices are prevalent among a diverse spectrum of industries.
 The ubiquitous presence of these devices necessitates a common interface
 and standard of interaction and exposure. This driver API attempts to
 resolve the issue of duplicate code found among existing counter device

@@ -26,23 +26,72 @@ the Generic Counter interface.
 
 There are three core components to a counter:
 
-* Count:
-  Count data for a set of Signals.
-
 * Signal:
-  Input data that is evaluated by the counter to determine the count
-  data.
+  Stream of data to be evaluated by the counter.
 
 * Synapse:
-  The association of a Signal with a respective Count.
+  Association of a Signal, and evaluation trigger, with a Count.
+
+* Count:
+  Accumulation of the effects of connected Synapses.
+
+SIGNAL
+------
+A Signal represents a stream of data. This is the input data that is
+evaluated by the counter to determine the count data; e.g. a quadrature
+signal output line of a rotary encoder. Not all counter devices provide
+user access to the Signal data, so exposure is optional for drivers.
+
+When the Signal data is available for user access, the Generic Counter
+interface provides the following available signal values:
+
+* SIGNAL_LOW:
+  Signal line is in a low state.
+
+* SIGNAL_HIGH:
+  Signal line is in a high state.
+
+A Signal may be associated with one or more Counts.
+
+SYNAPSE
+-------
+A Synapse represents the association of a Signal with a Count. Signal
+data affects respective Count data, and the Synapse represents this
+relationship.
+
+The Synapse action mode specifies the Signal data condition that
+triggers the respective Count's count function evaluation to update the
+count data. The Generic Counter interface provides the following
+available action modes:
+
+* None:
+  Signal does not trigger the count function. In Pulse-Direction count
+  function mode, this Signal is evaluated as Direction.
+
+* Rising Edge:
+  Low state transitions to high state.
+
+* Falling Edge:
+  High state transitions to low state.
+
+* Both Edges:
+  Any state transition.
+
+A counter is defined as a set of input signals associated with count
+data that are generated by the evaluation of the state of the associated
+input signals as defined by the respective count functions. Within the
+context of the Generic Counter interface, a counter consists of Counts
+each associated with a set of Signals, whose respective Synapse
+instances represent the count function update conditions for the
+associated Counts.
+
+A Synapse associates one Signal with one Count.
 
 COUNT
 -----
-A Count represents the count data for a set of Signals. The Generic
-Counter interface provides the following available count data types:
-
-* COUNT_POSITION:
-  Unsigned integer value representing position.
+A Count represents the accumulation of the effects of connected
+Synapses; i.e. the count data for a set of Signals. The Generic
+Counter interface represents the count data as a natural number.
 
 A Count has a count function mode which represents the update behavior
 for the count data. The Generic Counter interface provides the following

@@ -86,60 +135,7 @@ available count function modes:
     Any state transition on either quadrature pair signals updates the
     respective count. Quadrature encoding determines the direction.
 
-A Count has a set of one or more associated Signals.
-
-SIGNAL
-------
-A Signal represents a counter input data; this is the input data that is
-evaluated by the counter to determine the count data; e.g. a quadrature
-signal output line of a rotary encoder. Not all counter devices provide
-user access to the Signal data.
-
-The Generic Counter interface provides the following available signal
-data types for when the Signal data is available for user access:
-
-* SIGNAL_LEVEL:
-  Signal line state level. The following states are possible:
-
-  - SIGNAL_LEVEL_LOW:
-    Signal line is in a low state.
-
-  - SIGNAL_LEVEL_HIGH:
-    Signal line is in a high state.
-
-A Signal may be associated with one or more Counts.
-
-SYNAPSE
--------
-A Synapse represents the association of a Signal with a respective
-Count. Signal data affects respective Count data, and the Synapse
-represents this relationship.
-
-The Synapse action mode specifies the Signal data condition which
-triggers the respective Count's count function evaluation to update the
-count data. The Generic Counter interface provides the following
-available action modes:
-
-* None:
-  Signal does not trigger the count function. In Pulse-Direction count
-  function mode, this Signal is evaluated as Direction.
-
-* Rising Edge:
-  Low state transitions to high state.
-
-* Falling Edge:
-  High state transitions to low state.
-
-* Both Edges:
-  Any state transition.
-
-A counter is defined as a set of input signals associated with count
-data that are generated by the evaluation of the state of the associated
-input signals as defined by the respective count functions. Within the
-context of the Generic Counter interface, a counter consists of Counts
-each associated with a set of Signals, whose respective Synapse
-instances represent the count function update conditions for the
-associated Counts.
+A Count has a set of one or more associated Synapses.
 
 Paradigm
 ========

@@ -286,10 +282,36 @@ 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 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.
+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.
+
+Determining the type of extension to create is a matter of scope.
+
+* Signal extensions are attributes that expose information/control
+  specific to a Signal. These types of attributes will exist under a
+  Signal's directory in sysfs.
+
+  For example, if you have an invert feature for a Signal, you can have
+  a Signal extension called "invert" that toggles that feature:
+  /sys/bus/counter/devices/counterX/signalY/invert
+
+* Count extensions are attributes that expose information/control
+  specific to a Count. These type of attributes will exist under a
+  Count's directory in sysfs.
+
+  For example, if you want to pause/unpause a Count from updating, you
+  can have a Count extension called "enable" that toggles such:
+  /sys/bus/counter/devices/counterX/countY/enable
+
+* Device extensions are attributes that expose information/control
+  non-specific to a particular Count or Signal. This is where you would
+  put your global features or other miscellanous functionality.
+
+  For example, if your device has an overtemp sensor, you can report the
+  chip overheated via a device extension called "error_overtemp":
+  /sys/bus/counter/devices/counterX/error_overtemp
 
 Architecture
 ============

Signature · 1 line

2.23.0

William Breathitt Gray

author

2019-10-06 20:03 UTC #3 lore raw

PATCH v5 3/3 counter: Fix typo in action_get description

In reply to William Breathitt Gray

The action_get callback returns a Synapse's action mode.

Signed-off-by William Breathitt Gray <[email protected]>

---
 include/linux/counter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/counter.h +1 -1

index 32fb4d8cc3fd..9dbd5df4cd34 100644
@@ -315,7 +315,7 @@ enum counter_signal_value {
  *			Count's functions_list array.
  * @action_get:		function to get the current action mode. Returns 0 on
  *			success and negative error code on error. The index of
- *			the respective Signal's returned action mode should be
+ *			the respective Synapse's returned action mode should be
  *			passed back via the action parameter.
  * @action_set:		function to set the action mode. action is the index of
  *			the requested action mode from the respective Synapse's

Signature · 1 line

2.23.0

Jonathan Cameron

maintainer

2019-10-12 14:00 UTC #4 lore raw

In reply to William Breathitt Gray

Hi William

What's the status on these? If you are happy that reviews and testing is complete enough, do you want me to take them after I pick up the eqep driver (hopefully shortly dependent on the pull request Greg has from me being fine).

Thanks,

Jonathan

Quoted 87 lines · On Sun, 6 Oct 2019 16:03:08 -0400 William Breathitt Gray…

Changes in v5:
- Add changes and additions to generic-counter.rst to clarify theory and use of the Generic Counter interface
- Fix typo in counter.h action_get description comment
The changes in this patchset will not affect the userspace interface. Rather, these changes are intended to simplify the kernelspace Counter callbacks for counter device driver authors.
The following main changes are proposed:
* Retire the opaque counter_count_read_value/counter_count_write_value structures and simply represent count data as an unsigned integer.
* Retire the opaque counter_signal_read_value structure and represent Signal data as a counter_signal_value enum.
These changes should reduce some complexity and code in the use and implementation of the count_read, count_write, and signal_read callbacks.
The opaque structures for Count data and Signal data were introduced originally in anticipation of supporting various representations of counter data (e.g. arbitrary-precision tallies, floating-point spherical coordinate positions, etc). However, with the counter device drivers that have appeared, it's become apparent that utilizing opaque structures in kernelspace is not the best approach to take.
I believe it is best to let userspace applications decide how to interpret the count data they receive. There are a couple of reasons why it would be good to do so:
* Users use their devices in unexpected ways.
For example, a quadrature encoder counter device is typically used to keep track of the position of a motor, but a user could set the device in a pulse-direction mode and instead use it to count sporadic rising edges from an arbitrary signal line unrelated to positioning. Users should have the freedom to decide what their data represents.
* Most counter devices represent data as unsigned integers anyway.
For example, whether the device is a tally counter or position counter, the count data is represented to the user as an unsigned integer value. So specifying that one device is representing tallies while the other specifies positions does not provide much utility from an interface perspective.
For these reasons, the count_read and count_write callbacks have been redefined to pass count data directly as unsigned long instead of passed via opaque structures:
count_read(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
count_write(struct counter_device *counter,
struct counter_count *count, unsigned long val);
Similarly, the signal_read is redefined to pass Signal data directly as a counter_signal_value enum instead of via an opaque structure:
signal_read(struct counter_device *counter,
struct counter_signal *signal, enum counter_signal_value *val);
The counter_signal_value enum is simply the counter_signal_level enum redefined to remove the references to the Signal data "level" data type.
William Breathitt Gray (3):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
counter: Fix typo in action_get description
 Documentation/driver-api/generic-counter.rst | 162 +++++++++++--------
 drivers/counter/104-quad-8.c                 |  33 ++--
 drivers/counter/counter.c                    | 101 ++----------
 drivers/counter/ftm-quaddec.c                |  14 +-
 drivers/counter/stm32-lptimer-cnt.c          |   5 +-
 drivers/counter/stm32-timer-cnt.c            |  17 +-
 drivers/counter/ti-eqep.c                    |  19 +--
 include/linux/counter.h                      |  76 ++-------
 8 files changed, 144 insertions(+), 283 deletions(-)
base-commit: 0c3aa63a842d84990bd02622f2fa50d2bd33c652
prerequisite-patch-id: ebe284609b3db8d4130ea2915f7f7b185c743a70
prerequisite-patch-id: cbe857759f10d875690df125d18bc04f585ac7c9
prerequisite-patch-id: 21f2660dc88627387ee4666d08044c63dd961dae

William Breathitt Gray

author

2019-10-12 14:51 UTC #5 lore raw

Quoted 10 lines · Jonathan Cameron

Hi William
What's the status on these? If you are happy that reviews and testing is complete enough, do you want me to take them after I pick up the eqep driver (hopefully shortly dependent on the pull request Greg has from me being fine).
Thanks,
Jonathan

Yes, this is ready for you to take. So after the eqep driver is picked up you can apply this patchset.

Thanks,

William Breathitt Gray

Quoted 92 lines

Quoted 87 lines · On Sun, 6 Oct 2019 16:03:08 -0400 William Breathitt Gray…
Changes in v5:
- Add changes and additions to generic-counter.rst to clarify theory and use of the Generic Counter interface
- Fix typo in counter.h action_get description comment
The changes in this patchset will not affect the userspace interface. Rather, these changes are intended to simplify the kernelspace Counter callbacks for counter device driver authors.
The following main changes are proposed:
* Retire the opaque counter_count_read_value/counter_count_write_value structures and simply represent count data as an unsigned integer.
* Retire the opaque counter_signal_read_value structure and represent Signal data as a counter_signal_value enum.
These changes should reduce some complexity and code in the use and implementation of the count_read, count_write, and signal_read callbacks.
The opaque structures for Count data and Signal data were introduced originally in anticipation of supporting various representations of counter data (e.g. arbitrary-precision tallies, floating-point spherical coordinate positions, etc). However, with the counter device drivers that have appeared, it's become apparent that utilizing opaque structures in kernelspace is not the best approach to take.
I believe it is best to let userspace applications decide how to interpret the count data they receive. There are a couple of reasons why it would be good to do so:
* Users use their devices in unexpected ways.
For example, a quadrature encoder counter device is typically used to keep track of the position of a motor, but a user could set the device in a pulse-direction mode and instead use it to count sporadic rising edges from an arbitrary signal line unrelated to positioning. Users should have the freedom to decide what their data represents.
* Most counter devices represent data as unsigned integers anyway.
For example, whether the device is a tally counter or position counter, the count data is represented to the user as an unsigned integer value. So specifying that one device is representing tallies while the other specifies positions does not provide much utility from an interface perspective.
For these reasons, the count_read and count_write callbacks have been redefined to pass count data directly as unsigned long instead of passed via opaque structures:
count_read(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
count_write(struct counter_device *counter,
struct counter_count *count, unsigned long val);
Similarly, the signal_read is redefined to pass Signal data directly as a counter_signal_value enum instead of via an opaque structure:
signal_read(struct counter_device *counter,
struct counter_signal *signal, enum counter_signal_value *val);
The counter_signal_value enum is simply the counter_signal_level enum redefined to remove the references to the Signal data "level" data type.
William Breathitt Gray (3):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
counter: Fix typo in action_get description
 Documentation/driver-api/generic-counter.rst | 162 +++++++++++--------
 drivers/counter/104-quad-8.c                 |  33 ++--
 drivers/counter/counter.c                    | 101 ++----------
 drivers/counter/ftm-quaddec.c                |  14 +-
 drivers/counter/stm32-lptimer-cnt.c          |   5 +-
 drivers/counter/stm32-timer-cnt.c            |  17 +-
 drivers/counter/ti-eqep.c                    |  19 +--
 include/linux/counter.h                      |  76 ++-------
 8 files changed, 144 insertions(+), 283 deletions(-)
base-commit: 0c3aa63a842d84990bd02622f2fa50d2bd33c652
prerequisite-patch-id: ebe284609b3db8d4130ea2915f7f7b185c743a70
prerequisite-patch-id: cbe857759f10d875690df125d18bc04f585ac7c9
prerequisite-patch-id: 21f2660dc88627387ee4666d08044c63dd961dae

Jonathan Cameron

maintainer

2019-10-17 21:41 UTC #6 lore raw

Quoted 8 lines · On Sat, 12 Oct 2019 10:51:19 -0400 William Breathitt Gray…

Quoted 7 lines · Jonathan Cameron
Hi William
What's the status on these? If you are happy that reviews and testing is complete enough, do you want me to take them after I pick up the eqep driver (hopefully shortly dependent on the pull request Greg has from me being fine).

Quoted 6 lines

Quoted 3 lines
Thanks,
Jonathan
Yes, this is ready for you to take. So after the eqep driver is picked up you can apply this patchset.

Series applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 97 lines

Thanks,
William Breathitt Gray
Quoted 92 lines
Quoted 87 lines · On Sun, 6 Oct 2019 16:03:08 -0400 William Breathitt Gray…
Changes in v5:
- Add changes and additions to generic-counter.rst to clarify theory and use of the Generic Counter interface
- Fix typo in counter.h action_get description comment
The changes in this patchset will not affect the userspace interface. Rather, these changes are intended to simplify the kernelspace Counter callbacks for counter device driver authors.
The following main changes are proposed:
* Retire the opaque counter_count_read_value/counter_count_write_value structures and simply represent count data as an unsigned integer.
* Retire the opaque counter_signal_read_value structure and represent Signal data as a counter_signal_value enum.
These changes should reduce some complexity and code in the use and implementation of the count_read, count_write, and signal_read callbacks.
The opaque structures for Count data and Signal data were introduced originally in anticipation of supporting various representations of counter data (e.g. arbitrary-precision tallies, floating-point spherical coordinate positions, etc). However, with the counter device drivers that have appeared, it's become apparent that utilizing opaque structures in kernelspace is not the best approach to take.
I believe it is best to let userspace applications decide how to interpret the count data they receive. There are a couple of reasons why it would be good to do so:
* Users use their devices in unexpected ways.
For example, a quadrature encoder counter device is typically used to keep track of the position of a motor, but a user could set the device in a pulse-direction mode and instead use it to count sporadic rising edges from an arbitrary signal line unrelated to positioning. Users should have the freedom to decide what their data represents.
* Most counter devices represent data as unsigned integers anyway.
For example, whether the device is a tally counter or position counter, the count data is represented to the user as an unsigned integer value. So specifying that one device is representing tallies while the other specifies positions does not provide much utility from an interface perspective.
For these reasons, the count_read and count_write callbacks have been redefined to pass count data directly as unsigned long instead of passed via opaque structures:
count_read(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
count_write(struct counter_device *counter,
struct counter_count *count, unsigned long val);
Similarly, the signal_read is redefined to pass Signal data directly as a counter_signal_value enum instead of via an opaque structure:
signal_read(struct counter_device *counter,
struct counter_signal *signal, enum counter_signal_value *val);
The counter_signal_value enum is simply the counter_signal_level enum redefined to remove the references to the Signal data "level" data type.
William Breathitt Gray (3):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
counter: Fix typo in action_get description
 Documentation/driver-api/generic-counter.rst | 162 +++++++++++--------
 drivers/counter/104-quad-8.c                 |  33 ++--
 drivers/counter/counter.c                    | 101 ++----------
 drivers/counter/ftm-quaddec.c                |  14 +-
 drivers/counter/stm32-lptimer-cnt.c          |   5 +-
 drivers/counter/stm32-timer-cnt.c            |  17 +-
 drivers/counter/ti-eqep.c                    |  19 +--
 include/linux/counter.h                      |  76 ++-------
 8 files changed, 144 insertions(+), 283 deletions(-)
base-commit: 0c3aa63a842d84990bd02622f2fa50d2bd33c652
prerequisite-patch-id: ebe284609b3db8d4130ea2915f7f7b185c743a70
prerequisite-patch-id: cbe857759f10d875690df125d18bc04f585ac7c9
prerequisite-patch-id: 21f2660dc88627387ee4666d08044c63dd961dae