Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray <wbg@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4581 bytes --]

On Tue, Feb 11, 2025 at 04:19:11PM +0100, Bence Csókás wrote:
> The TCB has three R/W-able "general purpose" hardware registers:
> RA, RB and RC. The hardware is capable of:
> * sampling Counter Value Register (CV) to RA/RB on a trigger edge
> * sending an interrupt of this change
> * sending an interrupt on CV change due to trigger
> * triggering an interrupt on CV compare to RC
> * stop counting after sampling to RB
> 
> To enable using these features in user-space, an interrupt handler
> was added, generating the necessary counter events. On top, RA/B/C
> registers are added as Count Extensions. To aid interoperation, a
> uapi header was also added, containing the various numeral IDs of
> the Extensions, Event channels etc.
> 
> Bence Csókás (2):
>   counter: microchip-tcb-capture: Add IRQ handling
>   counter: microchip-tcb-capture: Add capture extensions for registers
>     RA-RC
> 
>  MAINTAINERS                                   |   1 +
>  drivers/counter/microchip-tcb-capture.c       | 137 ++++++++++++++++++
>  .../linux/counter/microchip-tcb-capture.h     |  49 +++++++
>  3 files changed, 187 insertions(+)
>  create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

Hi Bence,

I had some time to read over your description of the three hardware
registers (RA, RB, and RC)[^1] and I have some suggestions.

First, register RC seems to serve only as a threshold value for a
compare operation. So it shouldn't be exposed as "capture2", but rather
as its own dedicated threshold component. I think the 104-quad-8 module
is the only other driver supporting THRESHOLD events; it exposes the
threshold value configuration via the "preset" component, but perhaps we
should introduce a proper "threshold" component instead so counter
drivers have a standard way to expose this functionality. What do you
think?

Regarding registers RA and RB, these do hold historic captures of count
data so it does seem appropriate to expose these as "capture0" and
"capture1". However, I'm still somewhat confused about why there are two
registers holding the same sampled CV (or do RA and RB hold different
values from each other?). Does a single external line trigger the sample
of CV to both RA and RB, or are there two separate external lines
triggering the samples independently; or is this a situation where it's
a single external line where rising edge triggers a sample to RA while
falling edge triggers a sample to RB?

Next, the driver right now has three separate event channels, but I
believe you only need two. The purpose of counter event channels is to
provide a way for users to differentiate between the same type of event
being issued from different sources. An example might clarify what I
mean.

Suppose a hypothetical counter device has two independent timers. When
either timer overflows, the device fires off a single interrupt. We can
consider this interrupt a counter OVERFLOW event. As users we can watch
for COUNTER_EVENT_OVERFLOW to collect these events. However, a problem
arises: we know an OVERFLOW event occurred, but we don't know which
particular timer is the source of the overflow. The solution is to
dedicate each source to its own event channel so that users can
differentiate between them (e.g. channel 0 are events sourced from the
first timer whereas channel 1 are events sourced from the second timer).

Going back to your driver, there seems to be no ambiguity about the
source of the CHANGE-OF-STATE, THRESHOLD, and OVERFLOW events, so these
events can all coexist in the same event channel. The only possible
ambiguity I see is regarding the CAPTURE events, which could be sourced
by either a sample to RA or RB. Given this, I believe all your events
could go in channel 0, with channel 1 serving to simply differentiate
CAPTURE events that are sourced by samples to RB.

Finally, you mentioned that this device supports a PWM mode that also
makes use of the RA, RB, and RC registers. To prevent globbering the
registers when in the wrong mode, you should verify the device is in the
counter capture mode before handling the capture components (or return
an appropriate "Operation not support" error code when in PWM mode). You
don't need to worry about adding these checks for now because it looks
like this driver does not support PWM mode yet, but it's something to
keep in mind if you do add support for it in the future.

William Breathitt Gray

[^1] https://lore.kernel.org/all/7b581014-a351-4077-8832-d3d347b4fdb5@prolan.hu/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]