Re: [PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[William Breathitt Gray <wbg@kernel.org>]

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

On Wed, Mar 05, 2025 at 07:01:19PM +0900, William Breathitt Gray wrote:
> Hardware initialize of the timer counter channel does not occur on probe
> thus leaving the Count in an undefined state until the first
> function_write() callback is executed. Fix this by performing the proper
> hardware initialization during probe.
> 
> Fixes: 106b104137fd ("counter: Add microchip TCB capture counter")
> Reported-by: Csókás Bence <csokas.bence@prolan.hu>
> Closes: https://lore.kernel.org/all/bfa70e78-3cc3-4295-820b-3925c26135cb@prolan.hu/
> Signed-off-by: William Breathitt Gray <wbg@kernel.org>
> ---
> This should fix the issue where a user needs to set the count function
> before they can use the counter. I don't have this hardware in person,
> so please test this patch and let me know whether it works for you.

While developing this bug fix, I noticed the following code in the
mchp_tc_count_function_write() function:

	if (!priv->tc_cfg->has_gclk)
		cmr |= ATMEL_TC_TIMER_CLOCK2;
	else
		cmr |= ATMEL_TC_TIMER_CLOCK1;
	/* Setup the period capture mode */
	cmr |=  ATMEL_TC_CMR_MASK;
	cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0);

It looks like it's trying to choose the TCCLKS value by evaluating
has_gclk. However, a couple lines later the cmr value is masked by
ATMEL_TC_XC0 which will clobber the previous choice by resetting bit 0.

Is this a bug, or am I misunderstanding how the TCCLKS value is set by
these defines?

William Breathitt Gray

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