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

Dharma.B
2025-05-19T10:47:54Z

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