Hi
On 4/2/21 1:30 PM, William Breathitt Gray wrote:
On Thu, Apr 01, 2021 at 06:32:28PM +0300, Jarkko Nikula wrote:
Add support for Intel Quadrature Encoder Peripheral found on Intel
Elkhart Lake platform.
Initial implementation was done by Felipe Balbi while he was working at
Intel with later changes from Raymond Tan and me.
Co-developed-by: Felipe Balbi (Intel) <balbi@xxxxxxxxxx>
Signed-off-by: Felipe Balbi (Intel) <balbi@xxxxxxxxxx>
Co-developed-by: Raymond Tan <raymond.tan@xxxxxxxxx>
Signed-off-by: Raymond Tan <raymond.tan@xxxxxxxxx>
Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
Hello Jarkko,
Please see the questions and suggestions inline below.
Thanks for review! I'll address them for the next version. Some comments
and discussion below.
+static enum counter_synapse_action intel_qep_synapse_actions[] = {
This enum can be const too.
+ [INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
+ COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+
+ [INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
+ COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+};
Quadrature x4 mode is expected to evaluate on both edges on both phase
signals. Shouldn't this array have COUNTER_SYNAPSE_ACTION_BOTH_EDGES?
...
+static int intel_qep_action_get(struct counter_device *counter,
+ struct counter_count *count,
+ struct counter_synapse *synapse,
+ size_t *action)
+{
+ struct intel_qep *qep = counter_to_qep(counter);
+ u32 reg;
+
+ pm_runtime_get_sync(qep->dev);
+ reg = intel_qep_readl(qep, INTEL_QEPCON);
+ pm_runtime_put(qep->dev);
+
+ *action = (reg & synapse->signal->id) ?
+ INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
+ INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
I'm a bit confused about this quadrature encoding. Is this counting only
one edge on each phase signal?
...
Are you actually able to set the action mode for the phase signals? My
expectation is that the action mode for the phase signals will always be
"both edges" because the encoding is quadrature x4. What exactly is
happening for the device when you write INTEL_QEPCON?
You are right. I've overlooked this. Action is always both edges but
what HW actually does here is an inversion control for each input signal
so these should be then extensions I think.
From the specification "An individually configurable Edge Select block
allows control over rising or falling edge detection on each input pins,
removing the need for platform logic inversion.".
+static ssize_t enable_write(struct counter_device *counter,
+ struct counter_count *count,
+ void *priv, const char *buf, size_t len)
+{
+ struct intel_qep *qep = counter_to_qep(counter);
+ u32 reg;
+ u32 val;
+ int ret;
+
+ ret = kstrtou32(buf, 0, &val);
"enable" is expected to handle boolean values so use kstrtobool here.
+ if (ret < 0)
+ return ret;
+
+ mutex_lock(&qep->lock);
+ if (val && !qep->enabled) {
+ pm_runtime_get_sync(qep->dev);
+ reg = intel_qep_readl(qep, INTEL_QEPCON);
+ reg |= INTEL_QEPCON_EN;
+ intel_qep_writel(qep, INTEL_QEPCON, reg);
+ qep->enabled = true;
Are you missing pm_runtime_put() here?
+ } else if (!val && qep->enabled) {
Are you missing pm_runtime_get_sync() here?
Both are intented. Here pm_runtime_ calls are not only for accessing the
registers like in other functions but keep the HW block on and be able
to count when enabled. So after enabling the runtime PM usage count
stays at least 1 and goes to zero only after disabling.
If you have both val and qep->enabled as bool, you can make this logic
clearer by evaluating their xor to determine if there's a change. So
something like this:
bool changed = val ^ qep->enabled;
if (!changed)
return len;
Good point. Will check how does it look.
+static const struct counter_device_ext intel_qep_ext[] = {
+ INTEL_QEP_COUNTER_EXT_RW(noise),
+ INTEL_QEP_COUNTER_EXT_RW(preset_enable)
+};
"noise" is a new attribute so you'll need to provide an entry in
Documentation/ABI/testing/sysfs-bus-counter explaining it.
Should this noise actually be visible as seconds (ns, µs, ms) instead of
plain register value? From the spec "It selects the maximum glitch width
to remove in terms of peripheral clock cycles: PCLK_CYCLES = MAX_COUNT + 2".
I think it should since for a person configuring the system plain
register value is more or less guesswork.
Jarkko
