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
