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@xxxxxxxxx/
Attachment:
signature.asc
Description: PGP signature
