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

[<Dharma.B@microchip.com>]

On 19/05/25 4:17 pm, Dharma B wrote:
> 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?

I just replaced switch statements with if condition to resolve the 
compiler error and added CV, RA for completeness.

> 
>>
>>> +        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.