Re: [PATCH v4 2/3] counter: add GPIO-based quadrature encoder driver

[Wadim Mueller]

On 2026-05-20 13:45, William Breathitt Gray wrote:
> On Fri, May 15, 2026 at 05:36:15PM +0200, Wadim Mueller wrote:
> > Add a platform driver that turns ordinary GPIOs into a quadrature
> > encoder counter device.  The driver requests edge-triggered interrupts
> > on the A and B (and optional Index) GPIOs and decodes the quadrature
> > signal in software using a classic state-table approach.
> > 
> > Supported counting modes:
> >   - Quadrature X1 (count on A rising edge only)
> >   - Quadrature X2 (count on both A edges)
> >   - Quadrature X4 (count on every A and B edge)
> >   - Pulse-direction (A = pulse, B = direction)
> > 
> > An optional index signal resets the count to zero on its rising edge
> > when enabled through sysfs.  A configurable ceiling clamps the count
> > to [0, ceiling].
> > 
> > Signed-off-by: Wadim Mueller <wafgo01@xxxxxxxxx>
> 
> Hi Wadim,
> 
> This driver supports non-quadrature modes such as pulse-direction and
> increase/decrease modes. Perhaps it's best to rename this driver to
> gpio-counter so that the name indicates the more general capabilities we
> have now. What do you think?

Agreed -- renamed accordingly in v5 (source file, Kconfig symbol,
compatible string, binding file, DT properties and MAINTAINERS).
Conor's v4 Acked-by on the binding is dropped because of this.

> 
> > +/*
> > + * Encode the four quadrature transitions in a single 4-bit state:
> > + *   bit3 = prev_a, bit2 = prev_b, bit1 = curr_a, bit0 = curr_b.
> > + *
> > + * Indexing the table with this value yields the signed delta for an
> > + * X4 decoder.  Illegal transitions (both inputs toggled at once)
> > + * remain 0 so the count is unchanged.
> > + */
> > +#define CREATE_QE_STATE(prev_a, prev_b, curr_a, curr_b) \
> > +	(((prev_a) << 3) | ((prev_b) << 2) | ((curr_a) << 1) | (curr_b))
> > +
> > +static const s8 gpio_qenc_quad_x4_table[16] = {
> > +	[CREATE_QE_STATE(0, 0, 0, 0)] =  0,
> > +	[CREATE_QE_STATE(0, 0, 0, 1)] = -1,
> > +	[CREATE_QE_STATE(0, 0, 1, 0)] =  1,
> > +	[CREATE_QE_STATE(0, 0, 1, 1)] =  0,
> > +	[CREATE_QE_STATE(0, 1, 0, 0)] =  1,
> > +	[CREATE_QE_STATE(0, 1, 0, 1)] =  0,
> > +	[CREATE_QE_STATE(0, 1, 1, 0)] =  0,
> > +	[CREATE_QE_STATE(0, 1, 1, 1)] = -1,
> > +	[CREATE_QE_STATE(1, 0, 0, 0)] = -1,
> > +	[CREATE_QE_STATE(1, 0, 0, 1)] =  0,
> > +	[CREATE_QE_STATE(1, 0, 1, 0)] =  0,
> > +	[CREATE_QE_STATE(1, 0, 1, 1)] =  1,
> > +	[CREATE_QE_STATE(1, 1, 0, 0)] =  0,
> > +	[CREATE_QE_STATE(1, 1, 0, 1)] =  1,
> > +	[CREATE_QE_STATE(1, 1, 1, 0)] = -1,
> > +	[CREATE_QE_STATE(1, 1, 1, 1)] =  0,
> > +};
> 
> I believe we can avoid the lookup table entirely by utilizing some
> simple bitwise operations.
> 
> Let's start with the "0" delta states:
> 
>     (PA, PB, CA, CB) = delta
>     ------------------------
>     (0, 0, 0, 0) =  0
>     (0, 1, 0, 1) =  0
>     (1, 0, 1, 0) =  0
>     (1, 1, 1, 1) =  0
>     (0, 0, 1, 1) =  0
>     (0, 1, 1, 0) =  0
>     (1, 0, 0, 1) =  0
>     (1, 1, 0, 0) =  0
> 
> A "0" delta is possible in two scenarios: either both signal levels
> remain the same, or both signal levels change. We can test for a nonzero
> delta using the following bitwise expression:
> 
>     HAS_NONZERO_DELTA = (PA ^ CA) ^ (PB ^ CB) = PA ^ PB ^ CA ^ CB
> 
> After that test, the remaining states are changes in a single Signal at
> a time:
> 
>     (PA, PB, CA, CB) = delta
>     ------------------------
>     (0, 0, 1, 0) =  1
>     (1, 0, 1, 1) =  1
>     (1, 1, 0, 1) =  1
>     (0, 1, 0, 0) =  1
>     (0, 0, 0, 1) = -1
>     (0, 1, 1, 1) = -1
>     (1, 1, 1, 0) = -1
>     (1, 0, 0, 0) = -1
> 
>     00 -> 10 -> 11 -> 01 -> 00 = FORWARD
>     00 -> 01 -> 11 -> 10 -> 00 = BACKWARD
> 
> The state change between previous and current is essentially a 2-bit
> Gray code. Gray code is designed such that successive values differ in
> only one bit. That means we can determine direction with just the
> previous state of Signal B (PB) and the current state of Signal A (CA):
> FORWARD when those two states differ, and BACKWARD when those two states
> are the same. A simple XOR operation computes such:
> 
>     GET_DIRECTION = PB ^ CA
> 
> So we can now reimplement the table as two macros:
> 
> 	#define GPIO_COUNTER_STATE_CHANGED(pa, pb, ca, cb) (pa ^ pb ^ ca ^ cb)
> 	#define GPIO_COUNTER_GET_DIRECTION(pb, ca) \
> 		((pb ^ ca) ? COUNTER_COUNT_DIRECTION_FORWARD : COUNTER_COUNT_DIRECTION_BACKWARD)
> 

Done in v5, exactly like proposed.  The two macros plus the >= / > 0
guards replace the 16-entry table; a short comment points at the
Gray-code reasoning.

> With that, gpio_qenc_quad_x4_table[state] can be replaced with something
> like the following:
> 
> 	if (!GPIO_COUNTER_STATE_CHANGED(prev_a, prev_b, a, b))
> 		return;
> 
> 	direction = GPIO_COUNTER_GET_DIRECTION(prev_b, a);
> 
> 	if (direction == COUNTER_COUNT_DIRECTION_FORWARD)
> 		if (count < ceiling)
> 			count++;
> 	else
> 		if (count > 0)
> 			count--;
> 
> I believe the intent of the code becomes clearer to read this way, but
> I'd like to hear what others think of this approach.
> 
> > +static int gpio_qenc_a_delta(struct gpio_qenc_priv *priv, int a, int b,
> > +			     int prev_a, int prev_b)
> > +{
> > +	int state = CREATE_QE_STATE(prev_a, prev_b, a, b);
> > +
> > +	switch (priv->function) {
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		return gpio_qenc_quad_x4_table[state];
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_A:
> > +		/* Both edges of A; sign comes from current A vs B. */
> > +		return (a == b) ? -1 : 1;
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X1_A:
> > +		/* Rising edge of A only. */
> > +		if (!prev_a && a)
> > +			return b ? -1 : 1;
> > +		return 0;
> 
> Quadrature X1 count modes trigger on the falling edge when the direction
> is backward. This isn't simply a requirement by definition, but
> necessary for the proper interpretation of the quadrature encoding.
> 
> Let's evaluate an incremental encoder used in a positioning application
> as typical use case.[^1] These are commonly implemented using a rotating
> shaft with a quadrature-offset pattern; aligned sensors detect the
> physical A/B pattern as the shaft rotates.[^2] As the shaft rotates a
> quadrature encoding emerges whose A-B phase difference allows us to
> determine direction: forward when rising edge of signal A leads B, and
> backward when it trails.[^3]
> 
> Now consider what happens to the signals when the rotation changes
> direction: there is a phase change between Signals A and B.[^4] The A/B
> pattern on the shaft is physically present so it has not changed; rather
> the pattern is now fed backwards to the sensors due to the direction
> reversal. The key point is the physical boundaries of the pattern are
> located in the same shaft positions they have always been, yet the
> signal edges representing those boundaries have flipped as a result of
> the direction change: positions marked by rising edges now appear as
> falling edges.
> 
> In Quadrature X4 and X2, the pattern reversal doesn't affect positioning
> because we count on both edges, so swapping rising and falling edges
> nets the same position count. Quadrature X1 presents a problem because
> we count on a single edge type, so a phase-difference in the encoding
> results in a physical shift in real-life position. The way to account
> for that phase shift is to swap counting to the other edge type when the
> direction changes. That's how dedicated quadrature encoder devices solve
> this problem.
> 
> I'm not sure of the best way to solve the Quadrature X1 problem in this
> driver. Right now we fire off interrupts on both edges, so perhaps
> there's a way for us to determine whether we're firing on a rising edge
> or falling edge and evaluate accordingly. Does the GPIO subsystem
> provide an indication for which edge triggered the interrupt? Or would
> it make sense to provide two interrupt service routines (one on rising
> edge and one on falling edge) and handle it that way?
> 
> > +static int gpio_qenc_function_write(struct counter_device *counter,
> > +				    struct counter_count *count,
> > +				    enum counter_function function)
> > +{
> > +	struct gpio_qenc_priv *priv = counter_priv(counter);
> > +	unsigned long flags;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(gpio_qenc_functions); i++)
> > +		if (gpio_qenc_functions[i] == function)
> > +			break;
> > +	if (i == ARRAY_SIZE(gpio_qenc_functions))
> > +		return -EINVAL;
> 
> The Counter subsystem ensures the function argument passed in to the
> function_write() callback exists within the count's functions_list. You
> can remove the gpio_qenc_functions array check entirely.
> 
> > +static int gpio_qenc_action_read(struct counter_device *counter,
> > +				 struct counter_count *count,
> > +				 struct counter_synapse *synapse,
> > +				 enum counter_synapse_action *action)
> > +{
> > +	struct gpio_qenc_priv *priv = counter_priv(counter);
> > +	enum gpio_qenc_signal_id signal_id = synapse->signal->id;
> > +
> > +	/* Index synapse always observes rising edges, regardless of mode. */
> > +	if (signal_id == GPIO_QENC_SIGNAL_INDEX) {
> > +		*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +		return 0;
> > +	}
> > +
> > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> > +
> > +	switch (priv->function) {
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_A:
> > +		if (signal_id == GPIO_QENC_SIGNAL_A)
> > +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		if (signal_id == GPIO_QENC_SIGNAL_B)
> > +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X1_A:
> > +		if (signal_id == GPIO_QENC_SIGNAL_A) {
> > +			if (priv->direction == COUNTER_COUNT_DIRECTION_FORWARD)
> > +				*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +			else
> > +				*action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE;
> > +		}
> > +		break;
> > +
> > +	case COUNTER_FUNCTION_QUADRATURE_X1_B:
> > +		if (signal_id == GPIO_QENC_SIGNAL_B) {
> > +			if (priv->direction == COUNTER_COUNT_DIRECTION_FORWARD)
> > +				*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +			else
> > +				*action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE;
> > +		}
> > +		break;
> > +
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +	case COUNTER_FUNCTION_INCREASE:
> > +	case COUNTER_FUNCTION_DECREASE:
> > +		if (signal_id == GPIO_QENC_SIGNAL_A)
> > +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> Instead of break statements, exit early by returning 0;

All removed/applied.  action_read() now also holds priv->lock while
reading function and direction (overlaps with one of the Sashiko
findings).

> 
> > +static int gpio_qenc_ceiling_write(struct counter_device *counter,
> > +				   struct counter_count *count, u64 val)
> > +{
> > +	struct gpio_qenc_priv *priv = counter_priv(counter);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	priv->ceiling = val;
> > +	if (priv->count > val)
> > +		priv->count = val;
> 
> Although having a count value above the ceiling isn't a particularly
> useful configuration, I wonder if it's unexpected for the the count
> value to be modified by an update to the ceiling value. It could be
> considered a loss of data in a sense because the user might reasonably
> expect the count value to hold the current position of whatever
> application is running.
> 
> How do other quadrature encoder devices handle this situation? Do they
> also adjust the count value immediately to ceiling, leave the count
> static entirely, or merely prevent the count from increasing further yet
> allow it to decrease gradually below the ceiling?
>

intel-qep, ti-eqep and stm32-timer-cnt all leave the count alone;
ftm-quaddec has no ceiling_write at all.  v5 follows that: ceiling_write
no longer touches priv->count, the update path uses a >= guard so an
out-of-range count can't grow, and the index ISR clamps after loading
the preset.

In addition, Sashiko AI[1] flagged seven issues on v4.  I went through
each against the code -- all real, no hallucinations.  v5 addresses
them as follow:

  1. !! normalisation of GPIO reads so negative errors don't index
     the state tables.
  2. priv->enabled under priv->lock; enable_write idempotent.
  3. preset/ceiling TOCTOU closed (check inside lock + >= guard +
     post-preset clamp).
  4. probe rejects sleepable GPIOs via gpiod_cansleep().
  5. action_read reports RISING/FALLING by current direction to match
     the ISR (overlaps with your X1 fix).
  6. action_read takes priv->lock.
  7. IRQF_NO_AUTOEN passed to devm_request_irq() instead of the global
     irq_set_status_flags() call.

v5 follows in a separate sub-thread under the v4 cover.

Thanks again for the patient review -- the Gray-code rewrite and the
X1 edge fix genuinly improved the driver.

[1] https://sashiko.dev/#/patchset/20260515153616.157605-1-wafgo01@xxxxxxxxx?part=2

Wadim

> > +static int gpio_qenc_preset_enable_write(struct counter_device *counter,
> > +					 struct counter_count *count, u8 val)
> > +{
> > +	struct gpio_qenc_priv *priv = counter_priv(counter);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	priv->preset_enabled = !!val;
> 
> All the COUNTER_COMP_*_ENABLE() components are ensured by the Counter
> subsystem to have boolean values, so no need for the double negation.
> 
> The reason the u8 type appears in the callbacks is to have a
> well-defined data width for the chrdev interface; the actual values
> passed to the callback will always be boolean.
> 
> William Breathitt Gray
> 
> [^1] https://en.wikipedia.org/wiki/Incremental_encoder#Quadrature_outputs
> [^2] https://upload.wikimedia.org/wikipedia/commons/1/1e/Incremental_directional_encoder.gif
> [^3] https://upload.wikimedia.org/wikipedia/commons/thumb/6/68/Quadrature_Diagram.svg/3840px-Quadrature_Diagram.svg.png
> [^4] https://upload.wikimedia.org/wikipedia/commons/thumb/1/10/QuadratureOscillatingShaft.png/500px-QuadratureOscillatingShaft.png