Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] counter: stm32-timer-cnt: Report count function when
 SLAVE_MODE_DISABLED
To:     William Breathitt Gray <vilhelm.gray@gmail.com>, <jic23@kernel.org>
CC:     Alexandre Torgue <alexandre.torgue@st.com>,
        <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Fabrice Gasnier <fabrice.gasnier@foss.st.com>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>
References: <20210219095906.220382-1-vilhelm.gray@gmail.com>
From:   Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Message-ID: <288929fc-6984-072b-359a-10e163056bad@foss.st.com>
Date:   Thu, 25 Feb 2021 12:19:18 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210219095906.220382-1-vilhelm.gray@gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 = {
>