Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux