Re: [PATCH] counter: microchip-tcb-capture: Add watch validation support

[<Dharma.B@microchip.com>]

On 18/05/25 1:11 pm, William Breathitt Gray wrote:
> On Thu, May 15, 2025 at 10:28:25AM +0530, Dharma Balasubiramani wrote:
>> Introduce a watch validation callback to restrict supported event and
>> channel combinations. This allows userspace to receive notifications only
>> for valid event types and sources. Specifically, enable the following
>> supported events on channels RA, RB, and RC:
>>
>>    - COUNTER_EVENT_CAPTURE
>>    - COUNTER_EVENT_CHANGE_OF_STATE
>>    - COUNTER_EVENT_OVERFLOW
>>    - COUNTER_EVENT_THRESHOLD
>>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
>>   drivers/counter/microchip-tcb-capture.c | 28 +++++++++++++++++++++++++---
>>   1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
>> index 1de3c50b9804..179ff5595143 100644
>> --- a/drivers/counter/microchip-tcb-capture.c
>> +++ b/drivers/counter/microchip-tcb-capture.c
>> @@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
>>   	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
>>   };
>>
>> +static int mchp_tc_watch_validate(struct counter_device *counter,
>> +				  const struct counter_watch *watch)
>> +{
>> +	switch (watch->channel) {
>> +	case COUNTER_MCHP_EVCHN_RA:
>> +	case COUNTER_MCHP_EVCHN_RB:
>> +	case COUNTER_MCHP_EVCHN_RC:
> 
Hi William,

> Hello Dharma,
> 
> Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness. I
> know COUNTER_MCHP_EVCHN_CV and COUNTER_MCHP_EVCHN_RA have the same
> underlying channel id, but we're abstracting this fact so it's good to
> maintain the consistency of the abstraction across all callbacks.

To avoid the compiler error due to COUNTER_MCHP_EVCHN_CV and 
COUNTER_MCHP_EVCHN_RA sharing the same underlying value, would it be 
sufficient to include a comment indicating that both represent the same 
channel ID? Or would you prefer that I duplicate the logic explicitly 
for the sake of abstraction consistency, despite the shared value?

> 
>> +		switch (watch->event) {
>> +		case COUNTER_EVENT_CAPTURE:
>> +		case COUNTER_EVENT_CHANGE_OF_STATE:
>> +		case COUNTER_EVENT_OVERFLOW:
>> +		case COUNTER_EVENT_THRESHOLD:
>> +			return 0;
> 
> The watch_validate callback is used to ensure that the requested watch
> configuration is valid: i.e. the watch event is appropriate for the
> watch channel.
> 
> Looking at include/uapi/linux/counter/microchip-tcb-capture.h:
> 
>       * Channel 0:
>       * - CV register changed
>       * - CV overflowed
>       * - RA captured
>       * Channel 1:
>       * - RB captured
>       * Channel 2:
>       * - RC compare triggered
> 
> If I'm understanding correctly, channel 0 supports only the
> CHANGE_OF_STATE, OVERFLOW, and CAPTURE events; channel 1 supports only
> CAPTURE events; and channel 2 supports only THRESHOLD events.

Shouldn't it be

/*
  * Channel 0 (EVCHN_CV):
  *   - CV register changed             → COUNTER_EVENT_CHANGE_OF_STATE
  *   - CV overflowed                   → COUNTER_EVENT_OVERFLOW
  *
  * Channel 1 (EVCHN_RA):
  *   - RA captured                     → COUNTER_EVENT_CAPTURE
  *
  * Channel 2 (EVCHN_RB):
  *   - RB captured                     → COUNTER_EVENT_CAPTURE
  *
  * Channel 3 (EVCHN_RC):
  *   - RC compare threshold reached    → COUNTER_EVENT_THRESHOLD
  */

Could you please help me understand whether these are logical channels 
or hardware channels related to the reg?

> 
> Adjust the code to ensure those limitations.
> 
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>   static struct counter_count mchp_tc_counts[] = {
>>   	{
>>   		.id = 0,
>> @@ -351,12 +372,13 @@ static struct counter_count mchp_tc_counts[] = {
>>   };
>>
>>   static const struct counter_ops mchp_tc_ops = {
>> -	.signal_read    = mchp_tc_count_signal_read,
>> +	.action_read    = mchp_tc_count_action_read,
>> +	.action_write   = mchp_tc_count_action_write,
>>   	.count_read     = mchp_tc_count_read,
>>   	.function_read  = mchp_tc_count_function_read,
>>   	.function_write = mchp_tc_count_function_write,
>> -	.action_read    = mchp_tc_count_action_read,
>> -	.action_write   = mchp_tc_count_action_write
>> +	.signal_read    = mchp_tc_count_signal_read,
> 
> It's nice to alphabetize the counter_ops callbacks, but it's also
> unrelated to the watch_validate implementation. Move the alphabetization
> cleanup to a separate patch so that this patch remains dedicated to
> just watch_validate changes.

Sure, I will drop sorting in this patch.

> 
> Thanks,
> 
> William Breathitt Gray


-- 
With Best Regards,
Dharma B.