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

[<Dharma.B@microchip.com>]

On 20/05/25 5:24 pm, William Breathitt Gray wrote:
> On Tue, May 20, 2025 at 09:36:42AM +0530, Dharma Balasubiramani wrote:
>> The Timer Counter Block (TCB) exposes several kinds of events to the
>> Counter framework, but not every event is meaningful on every hardware
>> channel. Add a `watch_validate()` callback so userspace may register only
>> the combinations actually supported:
>>
>> * Channel 0 (COUNTER_MCHP_EVCHN_CV, COUNTER_MCHP_EVCHN_RA)
>>     - COUNTER_EVENT_CAPTURE
>>     - COUNTER_EVENT_CHANGE_OF_STATE
>>     - COUNTER_EVENT_OVERFLOW
>>
>> * Channel 1 (COUNTER_MCHP_EVCHN_RB)
>>     - COUNTER_EVENT_CAPTURE
>>
>> * Channel 2 (COUNTER_MCHP_EVCHN_RC)
>>     - COUNTER_EVENT_THRESHOLD
>>
>> Any other request is rejected with `-EINVAL`, preventing undefined
>> behaviour in userspace.
> 

Hi William,

> Hi Dharma
> 
> The requesting an invalid watch configuration wouldn't necessarily lead
> to undefined beaviour in userspace -- at least as far as the Counter
> character device interface is concerned. What would happen is that the
> requested event is never pushed to that particular channel, so userspace
> is left watching for an event that never arrives for that particular
> watch: not an ideal situation, but not undefined.

Got it, Thanks. (I'll rephrase the commit message in the next revision)

> 
>>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
>> Tested the code on target (sam9x60_curiosity) using the following commands
>>
>> valid ones:
>> ./counter_watch_events -d -wevt_change_of_state,chan=0
>> ./counter_watch_events -d -wevt_ovf,chan=0
>> ./counter_watch_events -d -wevt_capture,chan=0
>> ./counter_watch_events -d -wevt_capture,chan=1
>> ./counter_watch_events -d -wevt_threshold,chan=2
>>
>> invalid ones:
>> ./counter_watch_events -d -wevt_threshold,chan=0
>> ./counter_watch_events -d -wevt_threshold,chan=1
>> Error adding watches[0]: Invalid argument
>> ---
>> Changes in v2:
>> - Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness.
>> - Adjust the code to ensure channel limitations.
>> - Drop sorting in this patch, will be taken care seperately.
>> - Link to v1: https://lore.kernel.org/r/20250515-counter-tcb-v1-1-e547061ed80f@microchip.com
> 
> Thank you for the changes. I have a minor adjustment suggestion below
> that I believe makes the code look a little nicer.
> 
>> ---
>>   drivers/counter/microchip-tcb-capture.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
>> index 1de3c50b9804..fe817f4f1edc 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)
>> +{
>> +	if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA) {
>> +		switch (watch->event) {
>> +		case COUNTER_EVENT_CHANGE_OF_STATE:
>> +		case COUNTER_EVENT_OVERFLOW:
>> +		case COUNTER_EVENT_CAPTURE:
>> +			return 0;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RB) {
>> +		return (watch->event == COUNTER_EVENT_CAPTURE) ? 0 : -EINVAL;
>> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RC) {
>> +		return (watch->event == COUNTER_EVENT_THRESHOLD) ? 0 : -EINVAL;
>> +	} else {
>> +		return -EINVAL;
>> +	}
> 
> You can use the early returns to avoid the else statements, and some
> other additional cleanups can be done as well:
> 
>      if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA)
>      	switch (watch->event) {
>      	case COUNTER_EVENT_CHANGE_OF_STATE:
>      	case COUNTER_EVENT_OVERFLOW:
>      	case COUNTER_EVENT_CAPTURE:
>      		return 0;
>      	default:
>      		return -EINVAL;
>      	}
>      
>      if (watch->channel == COUNTER_MCHP_EVCHN_RB && watch->event == COUNTER_EVENT_CAPTURE)
>      	return 0;
>      
>      if (watch->channel == COUNTER_MCHP_EVCHN_RC && watch->event == COUNTER_EVENT_THRESHOLD)
>      	return 0;
>      
>      return -EINVAL;
> 
> I think something like that looks a bit closer to the Linux kernel style
> present in the other drivers, so that we're all consistent.

Thanks, I will stick to it.

> 
> William Breathitt Gray


-- 
With Best Regards,
Dharma B.