Re: [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder

[William Breathitt Gray]

On Fri, Sep 22, 2023 at 04:39:19PM +0200, Fabrice Gasnier wrote:
> This is a precursor patch to support capture channels on all possible
> channels and stm32 timer types. Original driver was intended to be used
> only as quadrature encoder and simple counter on internal clock.
> 
> So, add ch3 and ch4 definition. Also add a check on encoder capability,
> so the driver may be probed for timer instances without encoder feature.
> This way, all timers may be used as simple counter on internal clock,
> starting from here.

Hi Fabrice,

Let's split the encoder capability probing code, detect number of
channels code, and channel introduction code to their own patches in
order to simplify things.

> Encoder capability is retrieved by using the timer index (originally in
> stm32-timer-trigger driver and dt-bindings). The need to keep backward
> compatibility with existing device tree lead to parse aside trigger node.
> Add diversity as STM32 timers with capture feature may have either 4, 2,
> 1 or no cc (capture/compare) channels.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>

I think this patch is more complicated than it needs to be.

> @@ -400,13 +558,47 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>  	priv->clk = ddata->clk;
>  	priv->max_arr = ddata->max_arr;
>  
> +	ret = stm32_timer_cnt_probe_encoder(pdev, priv);
> +	if (ret)
> +		return ret;
> +
> +	stm32_timer_cnt_detect_channels(pdev, priv);
> +
>  	counter->name = dev_name(dev);
>  	counter->parent = dev;
>  	counter->ops = &stm32_timer_cnt_ops;
> -	counter->counts = &stm32_counts;
>  	counter->num_counts = 1;
> -	counter->signals = stm32_signals;
> -	counter->num_signals = ARRAY_SIZE(stm32_signals);

Keep this the same.

> +
> +	/*
> +	 * Handle diversity for stm32 timers features. For now encoder is found with
> +	 * advanced timers or gp timers with 4 channels. Timers with less channels
> +	 * doesn't support encoder.
> +	 */
> +	switch (priv->nchannels) {
> +	case 4:
> +		if (priv->has_encoder)
> +			counter->counts = &stm32_counts_enc_4ch;
> +		else
> +			counter->counts = &stm32_counts_4ch;
> +		counter->signals = stm32_signals;
> +		counter->num_signals = ARRAY_SIZE(stm32_signals);
> +		break;
> +	case 2:
> +		counter->counts = &stm32_counts_2ch;
> +		counter->signals = stm32_signals;
> +		counter->num_signals = 3; /* clock, ch1 and ch2 */
> +		break;
> +	case 1:
> +		counter->counts = &stm32_counts_1ch;
> +		counter->signals = stm32_signals;
> +		counter->num_signals = 2; /* clock, ch1 */
> +		break;
> +	default:
> +		counter->counts = &stm32_counts;
> +		counter->signals = stm32_signals;
> +		counter->num_signals = 1; /* clock */
> +		break;
> +	}

Rather than adjusting the number of counts and signals, keep the
configuration static and use a single stm32_counts array. The reason is
that in the Counter subsystem paradigm Signals do not necessary
correlate to specific hardware signals but are rather an abstract
representation of the device behavior at a high level. In other words, a
Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be
viewed as representing a Signal that does not affect the Count (i.e. in
this case equivalent to an unconnected line).

What you'll need to do instead is check priv->nchannels during
stm32_action_read and stm32_count_function_read calls in order to return
the correct synapse action and count function for the particular
channels configuration you have. In stm32_count_function_write you would
return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels
configuration does not support a particular count function.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature