counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
Subsystems (4)
PATCH 1/1 counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
When in SLAVE_MODE_DISABLED mode, the count still increases if the counter is enabled because an internal clock is used. This patch fixes the stm32_count_function_get() function to properly report this behavior.
--- drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
drivers/counter/stm32-timer-cnt.c +19 -12
@@ -44,13 +44,14 @@ struct stm32_timer_cnt {* @STM32_COUNT_ENCODER_MODE_3: counts on both TI1FP1 and TI2FP2 edges */ enum stm32_count_function { -STM32_COUNT_SLAVE_MODE_DISABLED =-1 , +STM32_COUNT_SLAVE_MODE_DISABLED ,STM32_COUNT_ENCODER_MODE_1 ,STM32_COUNT_ENCODER_MODE_2 ,STM32_COUNT_ENCODER_MODE_3 , };static enum counter_count_function stm32_count_functions[] = { + [STM32_COUNT_SLAVE_MODE_DISABLED ] =COUNTER_COUNT_FUNCTION_INCREASE , [STM32_COUNT_ENCODER_MODE_1 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A , [STM32_COUNT_ENCODER_MODE_2 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B , [STM32_COUNT_ENCODER_MODE_3 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X4 ,
@@ -99,9 +100,10 @@ static int stm32_count_function_get(struct counter_device *counter,case 3 : *function =STM32_COUNT_ENCODER_MODE_3 ;return 0 ; +default : + *function =STM32_COUNT_SLAVE_MODE_DISABLED ; +return 0 ; } - -return -EINVAL ; } static int stm32_count_function_set(struct counter_device *counter,
@@ -274,31 +276,36 @@ static int stm32_action_get(struct counter_device *counter,size_t function;int err; -/* Default action mode (e.g. STM32_COUNT_SLAVE_MODE_DISABLED) */ - *action =STM32_SYNAPSE_ACTION_NONE ; - err =stm32_count_function_get (counter, count, &function);if (err) -return 0 ; +return err;switch (function) {case STM32_COUNT_ENCODER_MODE_1 :/* counts up/down on TI1FP1 edge depending on TI2FP2 level */ if (synapse->signal->id == count->synapses[0 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ; +return 0 ;case STM32_COUNT_ENCODER_MODE_2 :/* counts up/down on TI2FP2 edge depending on TI1FP1 level */ if (synapse->signal->id == count->synapses[1 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ; +return 0 ;case STM32_COUNT_ENCODER_MODE_3 :/* counts up/down on both TI1FP1 and TI2FP2 edges */ *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +return 0 ; +case STM32_COUNT_SLAVE_MODE_DISABLED : +/* counts on internal clock when CEN=1 */ + *action =STM32_SYNAPSE_ACTION_NONE ; +return 0 ; +default : +return -EINVAL ; } - -return 0 ; }static const struct counter_ops stm32_timer_cnt_ops = {
Signature · 1 line
2.30.1
Fabrice Gasnier
maintainerQuoted 4 lines · William Breathitt Gray
When in
SLAVE_MODE_DISABLEDmode, the count still increases if the counter is enabled because an internal clock is used. This patch fixes thestm32_count_function_get()function to properly report this behavior.
Hi William,
Thanks for the patch, that's something I also noticed earlier. Please find few comment below.
Quoted 31 lines
Fixes ad29937e206f ("counter: Add STM32 Timer quadrature encoder")Cc Fabrice Gasnier <[email protected]>Cc Maxime Coquelin <[email protected]>Cc Alexandre Torgue <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>--- drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)drivers/counter/stm32-timer-cnt.c +2
-1@@ -44,13 +44,14 @@ struct stm32_timer_cnt {* @STM32_COUNT_ENCODER_MODE_3: counts on both TI1FP1 and TI2FP2 edges */ enum stm32_count_function { -STM32_COUNT_SLAVE_MODE_DISABLED =-1 , +STM32_COUNT_SLAVE_MODE_DISABLED ,STM32_COUNT_ENCODER_MODE_1 ,STM32_COUNT_ENCODER_MODE_2 ,STM32_COUNT_ENCODER_MODE_3 , };static enum counter_count_function stm32_count_functions[] = { + [STM32_COUNT_SLAVE_MODE_DISABLED ] =COUNTER_COUNT_FUNCTION_INCREASE , [STM32_COUNT_ENCODER_MODE_1 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A , [STM32_COUNT_ENCODER_MODE_2 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B , [STM32_COUNT_ENCODER_MODE_3 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X4 ,
drivers/counter/stm32-timer-cnt.c static int stm32_count_function_get(struct counter_device *counter, · line 103
Quoted 4 lines
drivers/counter/stm32-timer-cnt.c +1
-0@@ -99,9 +100,10 @@ static int stm32_count_function_get(struct counter_device *counter,case 3 : *function =STM32_COUNT_ENCODER_MODE_3 ;return 0 ; +default :
I'd rather add a 'case 0', as that's the real value for slave mode disabled. For reference, here's what the STM32 timer spec says on slave mode selection:
0000: Slave mode disabled - if CEN = ‘1’ then the prescaler is clocked directly by the internal clock.
drivers/counter/stm32-timer-cnt.c static int stm32_count_function_get(struct counter_device *counter, · line 104
Quoted 5 lines
drivers/counter/stm32-timer-cnt.c +2
-2+ *function =STM32_COUNT_SLAVE_MODE_DISABLED ; +return 0 ; } - -return -EINVAL ;
Other slave modes could be added later (like counting on external
signals: channel A, B, ETR or other signals). But this isn't supported right now in the driver.
Then I suggest to keep the returning -EINVAL for the default case here. Do you agree with this approach ?
Quoted 4 lines
drivers/counter/stm32-timer-cnt.c +0
-0} static int stm32_count_function_set(struct counter_device *counter,
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 281
Quoted 10 lines
drivers/counter/stm32-timer-cnt.c +1
-4@@ -274,31 +276,36 @@ static int stm32_action_get(struct counter_device *counter,size_t function;int err; -/* Default action mode (e.g. STM32_COUNT_SLAVE_MODE_DISABLED) */ - *action =STM32_SYNAPSE_ACTION_NONE ; - err =stm32_count_function_get (counter, count, &function);if (err) -return 0 ; +return err;
This makes sense, in case the error reporting is kept above. Otherwise, it always returns 0.
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 289
Quoted 9 lines
drivers/counter/stm32-timer-cnt.c +2
-1switch (function) {case STM32_COUNT_ENCODER_MODE_1 :/* counts up/down on TI1FP1 edge depending on TI2FP2 level */ if (synapse->signal->id == count->synapses[0 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ;
More a question here...
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 296
Quoted 8 lines
drivers/counter/stm32-timer-cnt.c +3
-1+return 0 ;case STM32_COUNT_ENCODER_MODE_2 :/* counts up/down on TI2FP2 edge depending on TI1FP1 level */ if (synapse->signal->id == count->synapses[1 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ;
..., not related to your fix: In "quadrature x2 a" or "quadrature x2 b", the other signal determines the counting direction.
I feel like this isn't really represented with the "none" action.
I just realized if we want to extend this driver to add new signals (e.g. like counting on chA, chB or even by adding other signals like ETR on STM32 with the increase function), this may start to be confusing. Currently only the two signal names could give some hint (so it's rather obvious currently).
Maybe there could be some change later to indicate the other signal (channel A or channel B) participates in quadrature encoding ?
Thanks and best regards,
Fabrice
Quoted 19 lines
drivers/counter/stm32-timer-cnt.c +8
-3+return 0 ;case STM32_COUNT_ENCODER_MODE_3 :/* counts up/down on both TI1FP1 and TI2FP2 edges */ *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +return 0 ; +case STM32_COUNT_SLAVE_MODE_DISABLED : +/* counts on internal clock when CEN=1 */ + *action =STM32_SYNAPSE_ACTION_NONE ; +return 0 ; +default : +return -EINVAL ; } - -return 0 ; }static const struct counter_ops stm32_timer_cnt_ops = {
Quoted 36 lines · Fabrice Gasnier
Quoted 4 lines · William Breathitt Gray
When in
SLAVE_MODE_DISABLEDmode, the count still increases if the counter is enabled because an internal clock is used. This patch fixes thestm32_count_function_get()function to properly report this behavior.Hi William,
Thanks for the patch, that's something I also noticed earlier. Please find few comment below.
Quoted 25 lines
Fixes ad29937e206f ("counter: Add STM32 Timer quadrature encoder")Cc Fabrice Gasnier <[email protected]>Cc Maxime Coquelin <[email protected]>Cc Alexandre Torgue <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>--- drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)drivers/counter/stm32-timer-cnt.c +1
-1@@ -44,13 +44,14 @@ struct stm32_timer_cnt {* @STM32_COUNT_ENCODER_MODE_3: counts on both TI1FP1 and TI2FP2 edges */ enum stm32_count_function { -STM32_COUNT_SLAVE_MODE_DISABLED =-1 , +STM32_COUNT_SLAVE_MODE_DISABLED ,STM32_COUNT_ENCODER_MODE_1 ,STM32_COUNT_ENCODER_MODE_2 ,STM32_COUNT_ENCODER_MODE_3 , };
Quoted 16 lines
drivers/counter/stm32-timer-cnt.c static int stm32_count_function_get(struct counter_device *counter, · line 103
Quoted 10 lines
drivers/counter/stm32-timer-cnt.c +2
-0static enum counter_count_function stm32_count_functions[] = { + [STM32_COUNT_SLAVE_MODE_DISABLED ] =COUNTER_COUNT_FUNCTION_INCREASE , [STM32_COUNT_ENCODER_MODE_1 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A , [STM32_COUNT_ENCODER_MODE_2 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B , [STM32_COUNT_ENCODER_MODE_3 ] =COUNTER_COUNT_FUNCTION_QUADRATURE_X4 ,@@ -99,9 +100,10 @@ static int stm32_count_function_get(struct counter_device *counter,case 3 : *function =STM32_COUNT_ENCODER_MODE_3 ;return 0 ; +default :I'd rather add a 'case 0', as that's the real value for slave mode disabled. For reference, here's what the STM32 timer spec says on slave mode selection:
0000: Slave mode disabled - if CEN = ‘1’ then the prescaler is clocked directly by the internal clock.
Ack.
Quoted 11 lines
drivers/counter/stm32-timer-cnt.c static int stm32_count_function_get(struct counter_device *counter, · line 104
Quoted 5 lines
drivers/counter/stm32-timer-cnt.c +2
-2+ *function =STM32_COUNT_SLAVE_MODE_DISABLED ; +return 0 ; } - -return -EINVAL ;Other slave modes could be added later (like counting on external
signals: channel A, B, ETR or other signals). But this isn't supported right now in the driver.
Then I suggest to keep the returning-EINVALfor the default case here. Do you agree with this approach ?
That should be fine; we'll fill in the additional cases as the functionalities are introduced to this driver in the future.
Quoted 7 lines
Quoted 7 lines
drivers/counter/stm32-timer-cnt.c +0
-0} static int stm32_count_function_set(struct counter_device *counter,@@ -274,31 +276,36 @@ static int stm32_action_get(struct counter_device *counter,size_t function;int err;
Quoted 10 lines
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 281
Quoted 7 lines
drivers/counter/stm32-timer-cnt.c +1
-4-/* Default action mode (e.g. STM32_COUNT_SLAVE_MODE_DISABLED) */ - *action =STM32_SYNAPSE_ACTION_NONE ; - err =stm32_count_function_get (counter, count, &function);if (err) -return 0 ; +return err;This makes sense, in case the error reporting is kept above. Otherwise, it always returns 0.
Conceptually, a nonzero value from the function_get() callback should indicate an invalid function mode for a Counter driver. The changes in this patch should bring us to that behavior so fortunately we won't have to worry about remembering whether the stm32_count_function_get() return value is valid or not.
Quoted 12 lines
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 289
Quoted 9 lines
drivers/counter/stm32-timer-cnt.c +2
-1switch (function) {case STM32_COUNT_ENCODER_MODE_1 :/* counts up/down on TI1FP1 edge depending on TI2FP2 level */ if (synapse->signal->id == count->synapses[0 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ;More a question here...
Quoted 12 lines
drivers/counter/stm32-timer-cnt.c static int stm32_action_get(struct counter_device *counter, · line 296
Quoted 8 lines
drivers/counter/stm32-timer-cnt.c +3
-1+return 0 ;case STM32_COUNT_ENCODER_MODE_2 :/* counts up/down on TI2FP2 edge depending on TI1FP1 level */ if (synapse->signal->id == count->synapses[1 ].signal->id) *action =STM32_SYNAPSE_ACTION_BOTH_EDGES ; -break ; +else + *action =STM32_SYNAPSE_ACTION_NONE ;..., not related to your fix: In "quadrature x2 a" or "quadrature x2 b", the other signal determines the counting direction.
I feel like this isn't really represented with the "none" action.
Be careful not to confuse the Generic Counter paradigm with the hardware description of your device. Within the context of the Generic Counter paradigm, Synapse actions are the trigger conditions of a hypothetical counting function evaluating Signals for an idealized Counter. In other words, a Synapse action indicates whether a Signal triggers a change in the Count value, not whether the Signal value is evaluated by the counting function.
"Quadrature x2 A" and "Quadrature x2 B" are two different counting functions. Both happen to evaluate two Signals, but only one of those Signals is ever triggering the counting function evaluation to update the Count value. In other words, the Signal serving as a direction can change value as much as you like but it will never trigger a change in the respective Count's value; i.e. that Signal's Synapse action is "none" because it does not trigger the count function evaluation.
Quoted 6 lines
I just realized if we want to extend this driver to add new signals (e.g. like counting on chA, chB or even by adding other signals like ETR on STM32 with the increase function), this may start to be confusing. Currently only the two signal names could give some hint (so it's rather obvious currently).
Quoted 6 lines
Maybe there could be some change later to indicate the other signal (channel A or channel B) participates in quadrature encoding ?
Thanks and best regards,
Fabrice
Well, one thing could try is to introduce new count functions in the future if there is some configuration you want to support with a count function evaluation that doesn't really fit one of the ones currently available; we can create a new enum counter_count_function constant to represent it.
Remember that in the Generic Counter paradigm we are not necessarily matching hardware layout of your device, but rather abstracting its functionality. Because of that, you can associate multiple Signals to your Count component, even if your hardware device has only two physical lines.
For example, let's say a counter device has 3 modes: quadrature lines, external pulses, clock source. In quadrature lines mode, a "QUADA" and "QUADB" signal are evaluate as a quadrature x4 encoding; in external pulses mode, a "PULSE" signal is evaluated for both falling and rising edges; and in clock source mode, a "CLOCK" signal is evaluated for rising edges.
Using the Generic Counter paradigm, we would construct a Count with 4 Synapes associating the four Signals: QUADA, QUADB, PULSE, CLOCK. There would be 2 count functions: COUNTER_COUNT_FUNCTION_QUADRATURE_X4, COUNTER_COUNT_FUNCTION_INCREASE. The following 3 configurations would be possible:
* Count Function: COUNTER_COUNT_FUNCTION_QUADRATURE_X4 Synapse Actions: QUADA => COUNTER_SYNAPSE_ACTION_BOTH_EDGES QUADB => COUNTER_SYNAPSE_ACTION_BOTH_EDGES PULSE => COUNTER_SYNAPSE_ACTION_NONE CLOCK => COUNTER_SYNAPSE_ACTION_NONE
* Count Function: COUNTER_COUNT_FUNCTION_INCREASE Synapse Actions: QUADA => COUNTER_SYNAPSE_ACTION_NONE QUADB => COUNTER_SYNAPSE_ACTION_NONE PULSE => COUNTER_SYNAPSE_ACTION_BOTH_EDGES CLOCK => COUNTER_SYNAPSE_ACTION_NONE
* Count Function: COUNTER_COUNT_FUNCTION_INCREASE Synapse Actions: QUADA => COUNTER_SYNAPSE_ACTION_NONE QUADB => COUNTER_SYNAPSE_ACTION_NONE PULSE => COUNTER_SYNAPSE_ACTION_NONE CLOCK => COUNTER_SYNAPSE_ACTION_RISING_EDGE
So a Synapse action isn't where the differentiation occurs between which Signals are evaluated by a particular count function; the Synapse actions only indicate whether a change in the respective Signal value will trigger an update of the associated Count value.
However, I see what you mean that this does leave some ambiguity when we have multiple Signals associated to the same Count, yet various possible count functions that only evaluate a subsection of those Signals.
Perhaps one solution is to create a second Count component dedicated to just those Signals: so we impose a requirement that the only Signals associated with a particular Count component are Signals that are evaluated by all the specified count functions. Alternatively, perhaps we can expose a new attribute to communicate which Signals are evaluated.
We're starting to go off on a tangent here though, so lets postpone this discussion to a later time, perhaps when support for the ETR signal is proposed for this driver.
William Breathitt Gray