Hi
On 5/27/21 5:16 AM, William Breathitt Gray wrote:
+What: /sys/bus/counter/devices/counterX/signalY/invert
+KernelVersion: 5.14
+Contact: linux-iio@xxxxxxxxxxxxxxx
+Description:
+ Whether signal Y inversion is enabled. Valid attribute values
+ are boolean.
+
+ For counter devices that have feature to control inversion of
+ signal Y.
I want to understand this functionality a bit better. So, this device
has two quadrature encoder signals coming in (Phase A and Phase B) and
this "invert" option allows the user to configure the device to invert
these signals in hardware before before they are evaluated by the
quadrature encoding counter. Users are able to invert each signal
independent of the other; e.g. Phase A can be inverted, but Phase B can
be left alone. Is my understanding correct, or is the inversion applied
across all signals rather than just one independently?
What is the purpose of this functionality? Is this to allow users to
adjust the direction of the count without having to physically reorient
the encoding device (e.g. tracking counter-clockwise versus clockwise
movement)?
Yes, it's independent for each signal. Here Phase A, B and Index.
According to specification idea is to remove need for board specific
inversion logic. Which makes me thinking this kind configuration should
come from firmware. As well as inputs swapped function. Which is for
correcting possible miswiring of Phase A and B signals on the board.
I'm now puzzled is there even need to have userspace visibility and
control for these signal inversions and input swapping? Of course yes
with my hacker hat on but for an ordinary Linux distribution point of
view those inversions and input swapping should be set by the kernel
automatically IMHO.
What do you think? Should I keep these sysfs attributes in the next
version or remove them? Though I don't have plans to add firmware data
this time. It's nice to save room for future contributions if there is a
real need for these features :-)
+ return sysfs_emit(buf, "%u\n", !(reg & (uintptr_t)signal->priv));
It's easy to forget what signal->priv represents. I recommend using a
variable to hold this value so that the logic is a bit easier to
understand for future reviewers.
Ok.
+static struct counter_signal intel_qep_signals[] = {
+ INTEL_QEP_SIGNAL(0, "Phase A", INTEL_QEPCON_EDGE_A),
+ INTEL_QEP_SIGNAL(1, "Phase B", INTEL_QEPCON_EDGE_A),
+ INTEL_QEP_SIGNAL(2, "Index", INTEL_QEPCON_EDGE_A),
Is INTEL_QEPCON_EDGE_A three times here correct, or did mean to use
INTEL_QEPCON_EDGE_B and INTEL_QEPCON_EDGE_INDX as well?
What a facepalm... last minute "lets have a nice macro here and blind
copy-pasting" just before sending this out.
+ * Spike filter length is (MAX_COUNT + 2) clock periods. Disable
+ * filter when user space supplies shorter than 2 clock periods and
+ * otherwise enable and set MAX_COUNT = clock periods - 2.
+ */
+ length /= INTEL_QEP_CLK_PERIOD_NS;
+ if (length < 2) {
If userspace supplies a value that the filter cannot support, I think it
makes more sense to return an -EINVAL here. Otherwise, the user may
believe they have enabled the filter when it is in fact now disabled.
Fair point. Will change.
Jarkko
