Hi, William Breathitt Gray <vilhelm.gray@xxxxxxxxx> writes: > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote: >> Add support for Intel PSE Quadrature Encoder >> >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> >> --- >> >> Changes since v1: >> - Many more private sysfs files converted over to counter interface >> >> >> How do you want me to model this device's Capture Compare Mode (see >> below)? > > Hi Felipe, > > I'm CCing Fabien and David as they may be interested in the timestamps > discussion. See below for some ideas I have on implementing this. > >> For the few features which I couldn't find a matching property in >> counter framework, I still leave them as private sysfs files so we can >> discuss how to model them in the framework. >> >> Do you want Capture Compare to be a new function mode? >> >> BTW, I know I'm missing a Documentation file documenting sysfs files >> introduced by this driver, I'll do that once we agree how to move all >> other sysfs files to the framework. No worries. >> >> >> Details about the controller (do you want this in commit log?): >> >> >> Controller has 2 modes of operation: QEP and Capture. Both modes are >> mutually exclusive. We also have a set of maskable interrupts. Further >> details about each mode below. > > I noticed your interrupt handler takes care of a number of different > scenarios. Would you be able to summarize a bit further here the > conditions for this device that cause an interrupt to be fired (watchdog > timeout, FIFO updates, etc.)? > >> Quadrature Encoder Mode >> ======================= >> >> Used to measure rotational speed, direction and angle of rotation of a >> motor shaft. Feature list: >> >> - Quadrature decoder providing counter pulses with x4 count >> resolution and count direction >> >> - 32-bit up/down Position Counter for position measurement >> >> - Two modes of position counter reset: >> > Maximum Count (ceiling) to reset the position counter >> > Index pulse to reset the position counter >> >> - Watchdog timer functionality for detecting ‘stall’ events >> >> Capture Compare Mode >> ==================== >> >> Used to measure phase input signal Period or Duty Cycle. Feature List: >> >> - Capture function operates either on phase A or phase B input >> and can be configured to operate on lo/hi or hi/lo or both hi >> and lo transitions. >> >> - Free-running 32-bit counter to be configured to run at greater >> than or equal to 4x input signal frequency > > So in "Capture Compare" mode, the counter value is just increasing when > a state condition transition occurs. In that case we won't need a new > function mode to represent this behavior since one already exists: > "increase". > > You can add it to your intel_qep_count_functions array like so: > > [INTEL_QEP_ENCODER_MODE_CAPTURE] = > COUNTER_COUNT_FUNCTION_INCREASE, > > The various configurations for this mode are just Synapse action modes. > If you want only Phase A, you would set the action mode for Phase A > ("rising edge", "falling edge", or "both edges") and change the action > mode for Phase B to "none"; vice-versa configuration for Phase B instead > of Phase A. > > One thing to keep in mind is that action_set will need to maintain valid > configurations -- so if the user tries to set the action mode for Phase > A to something other than "none", you need to automatically set Phase > B's action mode to "none" (and vice-versa). interesting, thanks >> - Clock post-scaler to derive the counter clock source from the >> peripheral clock > > I see you already have a "prescaler" extension in your code. Is this > different from the "post-scaler" you mentioned here? This was probably a brain-fart on my side. It should be post-scaler, but that's only valid for capture compare mode. >> - 32B wide FIFO to capture 32-bit timestamps of up to 8 >> transition events > > You can implement this as a Count extension called "timestamps" or > similar. What we can do is have a read on this attribute return the > entire FIFO data buffer as unsigned integers, where each timestamp is > deliminated by a space. > > In addition, it may be useful to have another extension called > "timestamps_layout", or something along those lines, that will report > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.). > > Would this design work for your needs? Perhaps it would be best to have a pollable binary sysfs file (meaning, we need to be able to call sysfs_notify() at will) and userspace just receives POLLIN whenever there's data read. Then a read returns an array of e.g. struct counter_event where struct counter_event could be defined as: struct counter_event { uint32_t event_type; struct timespec64 timestamp; uint8_t reserved[32]; }; Userspace would do something along the lines of: fd = open("/sys/bus/counter/foo/capture/timestamps",...); pollfd[0].fd = fd; pollfd[0].events = POLLIN; poll(pollfd, 1, -1); if (pollfd[0].revents & POLLIN) { ret = read(fd, events, sizeof(struct counter_event) * 8); for (i = 0; i < ret / sizeof(struct counter_event); i++) process_event(events[i]); } Or something like that. I could, also, remove this part from the driver for now, so we can discuss how the capture-compare buffer read would look like. At least we could get QDP feature merged while we come to a consensus about capture compare. What do you think? -- balbi
Attachment:
signature.asc
Description: PGP signature