Re: [PATCH] counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED

From: William Breathitt Gray <vilhelm.gray@gmail.com>
To: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: jic23@kernel.org, Alexandre Torgue <alexandre.torgue@st.com>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
Date: Thu, 25 Feb 2021 22:22:00 +0900	[thread overview]
Message-ID: <YDekeKfygdUht3HL@shinobu> (raw)
In-Reply-To: <288929fc-6984-072b-359a-10e163056bad@foss.st.com>

[-- Attachment #1: Type: text/plain, Size: 9250 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]