Re: [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal

[William Breathitt Gray]

On Fri, Sep 22, 2023 at 04:39:18PM +0200, Fabrice Gasnier wrote:
> Introduce the internal clock signal, used to count when in simple rising
> function. Define signal ids, to improve readability. Also add the
> "frequency" attribute for the clock signal, and "prescaler" for the
> counter.

Hi Fabrice,

Split the addition of "frequency" and "prescaler" extensions each to
their own respective patches so we can keep the clock signal
introduction code separate (useful in case we need to git bisect an
issue in the future).

> 
> Whit this patch, signal action reports consistent state when "increase"

Looks like a typo there for the first word.

> function is used, and the counting frequency:
> $ echo increase > function
> $ grep -H "" signal*_action
> signal0_action:rising edge
> signal1_action:none
> signal2_action:none
> $ echo 1 > enable
> $ cat count
> 25425
> $ cat count
> 44439
> $ cat ../signal0/frequency
> 208877930

Since you're fixing this description anyway, indent the shell example by
four spaces to make it stand-out and look nice.

> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>
> ---
>  drivers/counter/stm32-timer-cnt.c | 84 ++++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index 668e9d1061d3..11c66876b213 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -21,6 +21,10 @@
>  #define TIM_CCER_MASK	(TIM_CCER_CC1P | TIM_CCER_CC1NP | \
>  			 TIM_CCER_CC2P | TIM_CCER_CC2NP)
>  
> +#define STM32_CLOCK_SIG		0
> +#define STM32_CH1_SIG		1
> +#define STM32_CH2_SIG		2
> +
>  struct stm32_timer_regs {
>  	u32 cr1;
>  	u32 cnt;
> @@ -216,11 +220,44 @@ static int stm32_count_enable_write(struct counter_device *counter,
>  	return 0;
>  }
>  
> +static int stm32_count_prescaler_read(struct counter_device *counter,
> +				      struct counter_count *count, u64 *prescaler)
> +{
> +	struct stm32_timer_cnt *const priv = counter_priv(counter);
> +	u32 psc;
> +
> +	regmap_read(priv->regmap, TIM_PSC, &psc);
> +
> +	*prescaler = psc + 1;
> +
> +	return 0;
> +}
> +
> +static int stm32_count_prescaler_write(struct counter_device *counter,
> +				       struct counter_count *count, u64 prescaler)
> +{
> +	struct stm32_timer_cnt *const priv = counter_priv(counter);
> +	u32 psc;
> +
> +	if (!prescaler || prescaler > MAX_TIM_PSC + 1)
> +		return -ERANGE;
> +
> +	psc = prescaler - 1;
> +
> +	return regmap_write(priv->regmap, TIM_PSC, psc);
> +}
> +
>  static struct counter_comp stm32_count_ext[] = {
>  	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
>  	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
>  	COUNTER_COMP_CEILING(stm32_count_ceiling_read,
>  			     stm32_count_ceiling_write),
> +	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
> +			       stm32_count_prescaler_write),
> +};
> +
> +static const enum counter_synapse_action stm32_clock_synapse_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
>  };
>  
>  static const enum counter_synapse_action stm32_synapse_actions[] = {
> @@ -243,25 +280,31 @@ static int stm32_action_read(struct counter_device *counter,
>  	switch (function) {
>  	case COUNTER_FUNCTION_INCREASE:
>  		/* counts on internal clock when CEN=1 */
> -		*action = COUNTER_SYNAPSE_ACTION_NONE;
> +		if (synapse->signal->id == STM32_CLOCK_SIG)
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_NONE;
>  		return 0;
>  	case COUNTER_FUNCTION_QUADRATURE_X2_A:
>  		/* counts up/down on TI1FP1 edge depending on TI2FP2 level */
> -		if (synapse->signal->id == count->synapses[0].signal->id)
> +		if (synapse->signal->id == STM32_CH1_SIG)
>  			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>  		else
>  			*action = COUNTER_SYNAPSE_ACTION_NONE;
>  		return 0;
>  	case COUNTER_FUNCTION_QUADRATURE_X2_B:
>  		/* counts up/down on TI2FP2 edge depending on TI1FP1 level */
> -		if (synapse->signal->id == count->synapses[1].signal->id)
> +		if (synapse->signal->id == STM32_CH2_SIG)
>  			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>  		else
>  			*action = COUNTER_SYNAPSE_ACTION_NONE;
>  		return 0;
>  	case COUNTER_FUNCTION_QUADRATURE_X4:
>  		/* counts up/down on both TI1FP1 and TI2FP2 edges */
> -		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		if (synapse->signal->id == STM32_CH1_SIG || synapse->signal->id == STM32_CH2_SIG)
> +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_NONE;
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -276,27 +319,52 @@ static const struct counter_ops stm32_timer_cnt_ops = {
>  	.action_read = stm32_action_read,
>  };
>  
> +static int stm32_count_clk_get_freq(struct counter_device *counter,
> +				    struct counter_signal *signal, u64 *freq)
> +{
> +	struct stm32_timer_cnt *const priv = counter_priv(counter);
> +
> +	*freq = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static struct counter_comp stm32_count_clock_ext[] = {
> +	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
> +};
> +
>  static struct counter_signal stm32_signals[] = {
>  	{
> -		.id = 0,
> +		.id = STM32_CLOCK_SIG,

This will break userspace programs that expect signal0 to represent the
"Channel 1" Signal. Instead, add the clock Signal to the end of the
stm32_signals array so that the existing Signals are not reordered.
Although the clock signal may be represented by an id of 0 on the
device, the Counter API Signal id is a more abstract concept so it does
not necessarily need to match the device's numbering scheme.

Side note: you can keep the "id" member value the same if you want. The
Counter subsystem uses the array position to index the Signals; the "id"
value is ignored by the subsystem in that regard, and is rather provided
for the driver's internal use so it can differentiate between the
Signals.

> +		.name = "Clock Signal",
> +		.ext = stm32_count_clock_ext,
> +		.num_ext = ARRAY_SIZE(stm32_count_clock_ext),
> +	},
> +	{
> +		.id = STM32_CH1_SIG,
>  		.name = "Channel 1"
>  	},
>  	{
> -		.id = 1,
> +		.id = STM32_CH2_SIG,
>  		.name = "Channel 2"
>  	}
>  };
>  
>  static struct counter_synapse stm32_count_synapses[] = {
> +	{
> +		.actions_list = stm32_clock_synapse_actions,
> +		.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
> +		.signal = &stm32_signals[STM32_CLOCK_SIG]
> +	},

Same reordering issue here as the previous comment.

William Breathitt Gray

>  	{
>  		.actions_list = stm32_synapse_actions,
>  		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
> -		.signal = &stm32_signals[0]
> +		.signal = &stm32_signals[STM32_CH1_SIG]
>  	},
>  	{
>  		.actions_list = stm32_synapse_actions,
>  		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
> -		.signal = &stm32_signals[1]
> +		.signal = &stm32_signals[STM32_CH2_SIG]
>  	}
>  };
>  
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature