| Message ID | 20210219095906.220382-1-vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED | expand |
On 2/19/21 10:59 AM, William Breathitt Gray wrote: > 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. Hi William, Thanks for the patch, that's something I also noticed earlier. Please find few comment below. > > Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- > drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > index ef2a974a2f10..ec6d9e89c028 100644 > --- a/drivers/counter/stm32-timer-cnt.c > +++ b/drivers/counter/stm32-timer-cnt.c > @@ -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: 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. > + *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 ? > } > > 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; This makes sense, in case the error reporting is kept above. Otherwise, it always returns 0. > > 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; More a question here... > + 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 > + 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 = { >
On Thu, Feb 25, 2021 at 12:19:18PM +0100, Fabrice Gasnier wrote: > On 2/19/21 10:59 AM, William Breathitt Gray wrote: > > 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. > > Hi William, > > Thanks for the patch, that's something I also noticed earlier. > Please find few comment below. > > > > > Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") > > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@st.com> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > --- > > drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ > > 1 file changed, 19 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > > index ef2a974a2f10..ec6d9e89c028 100644 > > --- a/drivers/counter/stm32-timer-cnt.c > > +++ b/drivers/counter/stm32-timer-cnt.c > > @@ -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: > > 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. > > + *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 ? That should be fine; we'll fill in the additional cases as the functionalities are introduced to this driver in the future. > > } > > > > 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; > > 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. > > > > 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; > > More a question here... > > > + 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. > 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 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
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index ef2a974a2f10..ec6d9e89c028 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -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 = {
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. Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") Cc: Fabrice Gasnier <fabrice.gasnier@st.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Alexandre Torgue <alexandre.torgue@st.com> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- drivers/counter/stm32-timer-cnt.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)