Re: [PATCH v3 2/2] counter: microchip-tcb-capture: Add capture extensions for registers RA-RC

[<Dharma.B@microchip.com>]

Hi Bence,

On 05/02/25 4:19 pm, Bence Csókás wrote:
> TCB hardware is capable of capturing the timer value to registers RA and
> RB. On top, it is capable of triggering on compare against a third
> register, RC. Add these registers as extensions.
> 
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> ---
> 
> Notes:
>      Changes in v2:
>      * Add IRQs
>      Changes in v3:
>      * Move IRQs to previous patch
> 
>   drivers/counter/microchip-tcb-capture.c | 58 +++++++++++++++++++++++++
>   1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index fef4bb69b486..1445ac512c52 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -253,6 +253,62 @@ static int mchp_tc_count_read(struct counter_device *counter,
>   	return 0;
>   }
>   
> +static int mchp_tc_count_cap_read(struct counter_device *counter,
> +				  struct counter_count *count, size_t idx, u64 *val)

The registers RA/B/C are 32 bit registers, hence use of u64 is unnecessary.

> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	u32 cnt;
> +
> +	switch (idx) {
> +	case 0:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
> +		break;
> +	case 1:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
> +		break;
> +	case 2:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), &cnt);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

The regmap_read() returns an error code, which is currently ignored. If 
regmap_read() fails, cnt remains uninitialized, potentially returning 
garbage data.

> +	*val = cnt;
> +
> +	return 0;
> +}
> +
> +static int mchp_tc_count_cap_write(struct counter_device *counter,
> +				   struct counter_count *count, size_t idx, u64 val)

ditto

> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +
> +	if (val > U32_MAX)
> +		return -ERANGE;
> +
> +	switch (idx) {
> +	case 0:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), val);

ditto

> +		break;
> +	case 1:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), val);
> +		break;
> +	case 2:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), val);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_COUNTER_ARRAY_CAPTURE(mchp_tc_cnt_cap_array, 3);
> +
> +static struct counter_comp mchp_tc_count_ext[] = {
> +	COUNTER_COMP_ARRAY_CAPTURE(mchp_tc_count_cap_read, mchp_tc_count_cap_write,
> +				   mchp_tc_cnt_cap_array),
> +};
> +
>   static struct counter_count mchp_tc_counts[] = {
>   	{
>   		.id = 0,
> @@ -261,6 +317,8 @@ static struct counter_count mchp_tc_counts[] = {
>   		.num_functions = ARRAY_SIZE(mchp_tc_count_functions),
>   		.synapses = mchp_tc_count_synapses,
>   		.num_synapses = ARRAY_SIZE(mchp_tc_count_synapses),
> +		.ext = mchp_tc_count_ext,
> +		.num_ext = ARRAY_SIZE(mchp_tc_count_ext),
>   	},
>   };
>   


-- 
With Best Regards,
Dharma B.