Simplify count_read/count_write/signal_read
Subsystems (8)
v4 cover Simplify count_read/count_write/signal_read
Changes in v4:
- Rebase ontop of ti-eqep introduction patches and update ti-eqep
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 (2):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
Documentation/driver-api/generic-counter.rst | 22 ++-- 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 ++------------ 8 files changed, 59 insertions(+), 226 deletions(-)
base-commit: 0c3aa63a842d84990bd02622f2fa50d2bd33c652
prerequisite-patch-id: ebe284609b3db8d4130ea2915f7f7b185c743a70
prerequisite-patch-id: cbe857759f10d875690df125d18bc04f585ac7c9
prerequisite-patch-id: 21f2660dc88627387ee4666d08044c63dd961dae
Signature · 1 line
2.23.0
PATCH v4 1/2 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.
--- 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Re: [PATCH v4 1/2] counter: Simplify the count_read and count_write callbacks
Quoted 7 lines · William Breathitt Gray
The
count_readandcount_writecallbacks are simplified to pass val as unsigned long rather than as an opaque data structure. The opaquecounter_count_read_valueandcounter_count_write_valuestructures,counter_count_value_typeenum, and relevantcounter_count_read_value_setandcounter_count_write_value_getfunctions, are removed as they are no longer used.
Quoted 5 lines
Cc David Lechner <[email protected]>Cc Patrick Havelange <[email protected]>Acked-by Fabrice Gasnier <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>---
PATCH v4 2/2 docs: driver-api: generic-counter: Update Count and Signal data types
In reply to William Breathitt GrayCount data is now always represented as an unsigned integer, while Signal data is either SIGNAL_LOW or SIGNAL_HIGH.
--- Documentation/driver-api/generic-counter.rst | 22 +++++++------------- 1 file changed, 8 insertions(+), 14 deletions(-)
Documentation/driver-api/generic-counter.rst +8 -14
@@ -39,10 +39,7 @@ There are three core components to a counter: 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. +Counter interface represents the count data as an unsigned integer. A Count has a count function mode which represents the update behavior for the count data. The Generic Counter interface provides the following
@@ -93,19 +90,16 @@ 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: +user access to the Signal data, so exposure is optional for drivers. -* SIGNAL_LEVEL: - Signal line state level. The following states are possible: +When the Signal data is available for user access, the Generic Counter +interface provides the following available signal values: - - SIGNAL_LEVEL_LOW: - Signal line is in a low state. +* SIGNAL_LOW: + Signal line is in a low state. - - SIGNAL_LEVEL_HIGH: - Signal line is in a high state. +* SIGNAL_HIGH: + Signal line is in a high state. A Signal may be associated with one or more Counts.
Signature · 1 line
2.23.0
David Lechner
reviewerRe: [PATCH v4 2/2] docs: driver-api: generic-counter: Update Count and Signal data types
Quoted 13 lines · William Breathitt Gray
Count data is now always represented as an unsigned integer, while Signal data is either
SIGNAL_LOWorSIGNAL_HIGH.Signed-off-by William Breathitt Gray <[email protected]>--- Documentation/driver-api/generic-counter.rst | 22 +++++++------------- 1 file changed, 8 insertions(+), 14 deletions(-)Documentation/driver-api/generic-counter.rst +0
-0
Documentation/driver-api/generic-counter.rst There are three core components to a counter: · line 42
Quoted 8 lines
Documentation/driver-api/generic-counter.rst +1
-4@@ -39,10 +39,7 @@ There are three core components to a counter: 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. +Counter interface represents the count data as an unsigned integer.
The previous patch looks like it is using unsigned long instead of unsigned integer.
Re: [PATCH v4 2/2] docs: driver-api: generic-counter: Update Count and Signal data types
Quoted 9 lines · David Lechner
Quoted 8 lines · William Breathitt Gray
Count data is now always represented as an unsigned integer, while Signal data is either
SIGNAL_LOWorSIGNAL_HIGH.Signed-off-by William Breathitt Gray <[email protected]>--- Documentation/driver-api/generic-counter.rst | 22 +++++++------------- 1 file changed, 8 insertions(+), 14 deletions(-)
Quoted 16 lines
Documentation/driver-api/generic-counter.rst There are three core components to a counter: · line 42
Quoted 13 lines
Documentation/driver-api/generic-counter.rst +1
-4@@ -39,10 +39,7 @@ There are three core components to a counter: 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. +Counter interface represents the count data as an unsigned integer.The previous patch looks like it is using unsigned long instead of unsigned integer.
Thank you for the heads-up. Actually, this section of the documentation describes the theoretical representation of counter devices, as opposed to the in-kernel implementation of the Generic Counter interface. As such, the phrase "unsigned integer" here means a natural number and not the C language "unsigned int" data type.
I'm going to update this line to state that clearer since it can be easily confused in the context of the rest of the document.
William Breathitt Gray