On Sat, Mar 24, 2018 at 05:21:40PM +0000, Jonathan Cameron wrote: >On Fri, 9 Mar 2018 13:43:19 -0500 >William Breathitt Gray <[email protected]> wrote: > >> This patch adds standard documentation for the Generic Counter interface >> userspace sysfs attributes of the 104-QUAD-8 driver. >> >> Signed-off-by: William Breathitt Gray <[email protected]> >A few minor comments inline... >Some of this seems generic and common enough you should just put it in the >main docs straight away rather that waiting for more devices to use it. > >Jonathan
That sounds reasonable so I'll move some of these into the main Generic Counter sysfs documentation file. Some comments inline follow. William Breathitt Gray > >> --- >> .../ABI/testing/sysfs-bus-counter-104-quad-8 | 115 >> +++++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 116 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 >> b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 >> new file mode 100644 >> index 000000000000..4269b438185a >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 >> @@ -0,0 +1,115 @@ >> +What: /sys/bus/counter/devices/counterX/countY_count_mode >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Count mode for channel Y. The preset value for channel Y is used >> + by the count mode where required. The following count modes are >> + available: >> + >> + Normal: >> + Counting is continuous in either direction. >> + >> + Range Limit: >> + An upper or lower limit is set, mimicking limit switches >> + in the mechanical counterpart. The upper limit is set to >> + the preset value, while the lower limit is set to 0. The >> + counter freezes at count = preset when counting up, and >> + at count = 0 when counting down. At either of these >> + limits, the counting is resumed only when the count >> + direction is reversed. >> + >> + Non-recycle: >> + Counter is disabled whenever a 24-bit count overflow or >> + underflow takes place. The counter is re-enabled when a >> + new count value is loaded to the counter via a preset >> + operation or write to raw. >> + >> + Modulo-N: >> + A count boundary is set between 0 and the preset value. >> + The counter is reset to 0 at count = preset when >> + counting up, while the counter is set to the preset >> + value at count = 0 when counting down; the counter does >> + not freeze at the bundary points, but counts >> + continuously throughout. > >This worries me a little in that you will end up with all sorts of subtle >variations around these concepts and hence end up with an impossible to >generalize userspace interface... There is a potention for a deal of variations due to the diverse range of counter devices out there -- in particular, I worry about the confusion of similar functionality using slightly different names (e.g. "Normal" may be named "Continuous" in another driver) and vice versa. Perhaps I should standardize these count modes in the main Generic Counter sysfs documentation file. A difficulty with standardizing these count modes is how to handle description cases such as "Non-recycle" mode whose limit range is that of the count register size (24-bit). This limit range would be different in a device of a different count register size; for example, the LS7366R is a 32-bit version of this counter device with the same interface, but which has a 32-bit limit in "Non-recycle" mode. However, this may not be such a problem since the datasheet for these devices does use a more generic description of this mode which I can utilize: "counter disabled with carry or borrow, re-enabled with reset or load." The count_mode attribute is core enough to Generic Counter paradigm that I expect it to appear in many Counter drivers, so I'll move this to the main Generic Counter sysfs documentation file for better standardization of its modes. > >> + >> +What: >> /sys/bus/counter/devices/counterX/countY_count_mode_available >> +What: >> /sys/bus/counter/devices/counterX/countY_noise_error_available >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Discrete set of available values for the respective Count Y >> + configuration are listed in this file. >> + >> +What: /sys/bus/counter/devices/counterX/countY_direction >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Read-only attribute that indicates the count direction of >> + Count Y. Two count directions are available: Forward and >> + Backward. >Is this telling us which way it is currently counting? I would imagine >it's generic inversion control, but this description doesn't make that clear. The nature of quadrature encoding allows direction of movement to be determined -- in terms of count data this direction represents whether the count value is increasing or decreasing. For the 104-QUAD-8 device, this "direction" is a read-only value provided by the hardware evaluation of the A and B input lines. However, I can imagine some simple counter devices permitting write operations to configure a direction as a form of inversion control for the counter. This attribute is generic enough to include in the main Generic Counter sysfs documentation file, so I'll move it there and add a more detailed explanation of its read and write functionality. > >> + >> +What: /sys/bus/counter/devices/counterX/countY_enable >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Whether channel Y inputs A and B are enabled. Valid attribute >> + values are boolean. >Why would you disable them? I'm unclear on what userspace would do with this. Truthfully, I haven't found much use for this functionality in my applications, but several devices provide it so I have exposed it here. I believe the intention is to allow users to pause a counter temporarily (i.e. by ignoring changes on the A and B inputs) and then pick up where they left off. I think a possible real life use case would look like this: a conveyor system tracks total movement, reaches the end of a production run and pauses the counter, rehomes the conveyor belt, then restarts the counter and continues counting where it left off. This count enable functionality is generic enough that I can also move it to the main Generic Counter sysfs documentation file, but I'll give it a more generic description without referencing the A and B input lines. > >> + >> +What: /sys/bus/counter/devices/counterX/countY_noise_error >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Read-only attribute that indicates whether excessive noise is >> + present at the channel Y count inputs in quadrature clock mode; >> + irrelevant in non-quadrature (Pulse-Direction) clock mode. >If you are going to report errors like this I would suggest trying to have >a generic form that is easy for userspace to match. >countY_error_noise would allow easy presentation of all errors of the >form >countY_error_<error type> as a list based on <error type> > >> + >> +What: /sys/bus/counter/devices/counterX/countY_preset >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + If the counter device supports preset registers, the preset >> + count for channel Y is provided by this attribute. >> + >> +What: /sys/bus/counter/devices/counterX/countY_preset_enable >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Whether to set channel Y counter with channel Y preset value >> + when channel Y index input is active, or continuously count. >> + Valid attribute values are boolean. >> + >> +What: /sys/bus/counter/devices/counterX/signalY_index_polarity >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Active level of channel Y-16 index input; irrelevant in >> + non-synchronous load mode. >This seems like a generic control that should be in the main docs? >don't use Y-16 to identify the channel. Use "the associated channel to signal >Y". > >> + >> +What: >> /sys/bus/counter/devices/counterX/signalY_index_polarity_available >> +What: >> /sys/bus/counter/devices/counterX/signalY_synchronous_mode_available >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Discrete set of available values for the respective Signal Y >> + configuration are listed in this file. >> + >> +What: >> /sys/bus/counter/devices/counterX/signalY_synchronous_mode >> +KernelVersion: 4.17 >> +Contact: [email protected] >> +Description: >> + Configure channel Y-16 counter for non-synchronous or >> + synchronous load mode. Synchronous load mode cannot be selected >> + in non-quadrature (Pulse-Direction) clock mode. >> + >> + Non-synchronous: >> + A logic low level is the active level at this index >> + input. The index function (as enabled via preset_enable) >> + is performed directly on the active level of the index >> + input. >> + >> + Synchronous: >> + Intended for interfacing with encoder Index output in >> + quadrature clock mode. The active level is configured >> + via index_polarity. The index function (as enabled via >> + preset_enable) is performed synchronously with the >> + quadrature clock on the active level of the index input. >> diff --git a/MAINTAINERS b/MAINTAINERS >> index febe27a9962e..8134050d175a 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -270,6 +270,7 @@ ACCES 104-QUAD-8 DRIVER >> M: William Breathitt Gray <[email protected]> >> L: [email protected] >> S: Maintained >> +F: Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 >> F: Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8 >> F: drivers/counter/104-quad-8.c >> >
