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

[William Breathitt Gray <wbg@kernel.org>]

On Fri, May 01, 2026 at 10:07:48PM +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)

Hi Wadim,

The Documentation/ABI/testing/sysfs-bus-counter file documents the Count
function modes. Because we're interpreting GPIO lines and can define our
own encoding states, it would be prudent to support all these Count
function modes (i.e. increase, decrease, x1 b, and x2 b modes).

> 
> 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@gmail.com>

I recommend adding support at some point for a compare
(COUNTER_COMP_COMPARE) component and floor component
(COUNTER_COMP_FLOOR) which are common functionalities in quadrature
devices. It'd be nice as well to have events pushed for
COUNTER_EVENT_OVERFLOW, COUNTER_EVENT_UNDERFLOW,
COUNTER_EVENT_OVERFLOW_UNDERFLOW, COUNTER_EVENT_THRESHOLD, and
COUNTER_EVENT_DIRECTION_CHANGE.

> +enum gpio_qenc_function {
> +	GPIO_QENC_FUNC_QUAD_X1 = 0,
> +	GPIO_QENC_FUNC_QUAD_X2,
> +	GPIO_QENC_FUNC_QUAD_X4,
> +	GPIO_QENC_FUNC_PULSE_DIR,
> +};

We use device specific enums to map device hardware register values to
the Counter subsystem function enum constants. However, because this
driver is not tied to any specific device hardware, you don't need to
define a new enum. In other words, just use the COUNTER_FUNCTION_*
constants directly in your code.

> +struct gpio_qenc_priv {
> +	struct gpio_desc *gpio_a;
> +	struct gpio_desc *gpio_b;
> +	struct gpio_desc *gpio_index;
> +
> +	int irq_a;
> +	int irq_b;
> +	int irq_index;
> +
> +	spinlock_t lock;
> +
> +	s64 count;

The count_read()/count_write() callbacks pass a u64 count value, so I
suspect this private count value store can be u64 as well.

> +	u64 ceiling;
> +	bool enabled;
> +	enum counter_count_direction direction;
> +	enum gpio_qenc_function function;
> +
> +	int prev_a;
> +	int prev_b;
> +
> +	bool index_enabled;
> +
> +	struct counter_signal signals[3];
> +	struct counter_synapse synapses[3];
> +	struct counter_count cnts;

I know you only have a single count, but for consistency with the rest
of the code and Counter subsystem, I recommend declaring this as a
single element array:

	struct counter_count cnts[1];

> +};
> +
> +/*
> + * Quadrature state table for X4 decoding.
> + * Rows = previous state (A<<1 | B), Columns = new state (A<<1 | B).
> + * Values: 0 = no change, +1 = forward, -1 = backward, 2 = error (skip).
> + */
> +static const int quad_table[4][4] = {
> +	/*          00  01  10  11  <- new */
> +	/* 00 */ {  0, -1,  1,  2 },
> +	/* 01 */ {  1,  0,  2, -1 },
> +	/* 10 */ { -1,  2,  0,  1 },
> +	/* 11 */ {  2,  1, -1,  0 },
> +};
> +
> +static void gpio_qenc_update_count(struct gpio_qenc_priv *priv, int delta)
> +{
> +	s64 new_count;
> +
> +	if (!delta)
> +		return;
> +
> +	new_count = priv->count + delta;
> +
> +	if (priv->ceiling) {
> +		if (new_count < 0)
> +			new_count = 0;
> +		else if (new_count > (s64)priv->ceiling)
> +			new_count = priv->ceiling;
> +	}
> +
> +	priv->count = new_count;
> +	priv->direction = (delta > 0) ? COUNTER_COUNT_DIRECTION_FORWARD
> +				      : COUNTER_COUNT_DIRECTION_BACKWARD;

Quadrature encoding represents changes of a single unit value at a time.
Because we never increase nor decrease more than a value of 1 at a time,
I think the code would read clearer if we determine the direction first,
then make the adjustments to the count value thereafter. Maybe something
like this (assuming priv->count is u64):

	if (!delta)
		return;
	
	priv->direction = (delta > 0) ? COUNTER_COUNT_DIRECTION_FORWARD
				      : COUNTER_COUNT_DIRECTION_BACKWARD;
	
	if (priv->direction == COUNTER_COUNT_DIRECTION_FORWARD)
		priv->count = (priv->count == priv->ceiling) ? priv->ceiling
							     : priv->count + 1;
	else
		priv->count = (priv->count == 0) ? priv->ceiling
						 : priv->count - 1;

> +}
> +
> +static irqreturn_t gpio_qenc_a_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +	int a, b, prev_state, new_state, delta;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (!priv->enabled)
> +		goto out;

Do we need to check the enabled state? If the IRQ is disabled, we don't
reach this path, or do we?

> +
> +	a = gpiod_get_value(priv->gpio_a);
> +	b = gpiod_get_value(priv->gpio_b);
> +
> +	prev_state = (priv->prev_a << 1) | priv->prev_b;
> +	new_state = (a << 1) | b;

I might make sense to wrap the state operation into a macro to make the
intention of these bitwise operations more intuitive. For example:

	prev_state = CREATE_QE_STATE(priv->prev_a, priv->prev_b);
	new_state = CREATE_QE_STATE(a, b);

Or something like that makes it more obvious that these two lines are
just building the indices for the state array rather than any inherent
meaning in the binary representation of the variables.

> +
> +	switch (priv->function) {
> +	case GPIO_QENC_FUNC_QUAD_X4:
> +		delta = quad_table[prev_state][new_state];
> +		if (delta == 2)
> +			delta = 0;
> +		gpio_qenc_update_count(priv, delta);
> +		break;
> +
> +	case GPIO_QENC_FUNC_QUAD_X2:
> +		delta = quad_table[prev_state][new_state];
> +		if (delta == 2)
> +			delta = 0;
> +		gpio_qenc_update_count(priv, delta);
> +		break;

After determining the delta value we call the gpio_qenc_update_count()
function immedidately. We can save on a function call and remove the
!delta check in gpio_qenc_update_count() if you make the call only when
delta is known to update the count; for example:

	if (!delta && delta != 2)
		gpio_qenc_update_count(priv, delta);

In fact, you may consider just removing the "2" states from the
quad_table and leaving it as just "0" because the effect is the same,
right?

> +static irqreturn_t gpio_qenc_b_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +	int a, b, prev_state, new_state, delta;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (!priv->enabled)
> +		goto out;
> +
> +	a = gpiod_get_value(priv->gpio_a);
> +	b = gpiod_get_value(priv->gpio_b);
> +
> +	prev_state = (priv->prev_a << 1) | priv->prev_b;
> +	new_state = (a << 1) | b;
> +
> +	switch (priv->function) {
> +	case GPIO_QENC_FUNC_QUAD_X4:
> +		delta = quad_table[prev_state][new_state];
> +		if (delta == 2)
> +			delta = 0;
> +		gpio_qenc_update_count(priv, delta);
> +		break;
> +
> +	case GPIO_QENC_FUNC_QUAD_X2:
> +		/* X2: only A-channel edges update count */
> +		break;
> +
> +	case GPIO_QENC_FUNC_QUAD_X1:
> +	case GPIO_QENC_FUNC_PULSE_DIR:
> +		break;

I guess you added these two cases to pacify the compiler warning for
missing switch cases. You can provide a default case instead so we don't
need to list out every case that is ignored.

> +static irqreturn_t gpio_qenc_index_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (priv->enabled && priv->index_enabled)
> +		priv->count = 0;
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	counter_push_event(counter, COUNTER_EVENT_INDEX, 0);
> +
> +	return IRQ_HANDLED;
> +}

In many quadrature encoder devices I've seen, Index functions can be
chosen which determine what happens to the count when an Index signal
occurs. Typically, one of those functions is to reset the count, but
that is not necessary the only function nor is it unconditionally
enabled.

Existing drivers such as 104-quad-8 use COUNTER_COMP_PRESET to define a
preset component that holds a value to preset the counter on an Index
signal; COUNTER_COMP_PRESET_ENABLE is used to enable/disable such
functionality. You may want to implement something similar for
gpio-quadrature-encoder, and consider other such Index functionality
that may be desirable to provide to users.

> +static int gpio_qenc_count_write(struct counter_device *counter,
> +				 struct counter_count *count, const u64 val)
> +{
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (priv->ceiling && val > priv->ceiling) {
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		return -EINVAL;
> +	}

So the ceiling component represents the highest value the count can
reach. I would argue a ceiling value of 0 does not indicate a disabled
ceiling but rather a Count which is limited to the value of 0; n.b. this
technically is not a disabled Count because COUNTER_EVENT_OVERFLOW could
still be pushed in this scenario, albeit a rather dubious configuration
for the Count in real life applications.

To "disable" a ceiling, the user would set the ceiling value to the
maximum value supported by the device hardware (or U64_MAX in our case
if priv->count is u64). In fact, you should initialize priv->ceiling to
this maximum value in your gpio_qenc_probe() callback so that we don't
limit.

> +static int gpio_qenc_function_read(struct counter_device *counter,
> +				   struct counter_count *count,
> +				   enum counter_function *function)
> +{
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	switch (priv->function) {
> +	case GPIO_QENC_FUNC_QUAD_X1:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X1_A;
> +		break;
> +	case GPIO_QENC_FUNC_QUAD_X2:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X2_A;
> +		break;
> +	case GPIO_QENC_FUNC_QUAD_X4:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> +		break;
> +	case GPIO_QENC_FUNC_PULSE_DIR:
> +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> +		break;
> +	}

If you use declare priv->function as enum counter_function, you can
replace the entire switch as a simple set operation:

	*function = priv->function;

> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +	return 0;
> +}
> +
> +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;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_QUADRATURE_X1_A:
> +		priv->function = GPIO_QENC_FUNC_QUAD_X1;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_A:
> +		priv->function = GPIO_QENC_FUNC_QUAD_X2;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		priv->function = GPIO_QENC_FUNC_QUAD_X4;
> +		break;
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		priv->function = GPIO_QENC_FUNC_PULSE_DIR;
> +		break;
> +	default:
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		return -EINVAL;
> +	}

Same suggestion as for gpio_qenc_function_read():

	priv->function = function;

> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +	return 0;
> +}
> +
> +static const enum counter_synapse_action gpio_qenc_synapse_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +};

If I'm not mistaken, you can fire interrupts on the falling edge of a
GPIO signal. It'd be good to support such a configuration as it's
necessary for Quadrature X1 A mode.

> +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;

You can eliminate a lot of the else statements the switch block by
handling a couple default cases. For example, you can define a default
action mode of "none" and have the rest of the code only set an action
if meets the right criteria.

Furthermore, the Index synapse should be treated as independent of
the quadrature mode; the Signal is not evaluated in determining the
encoding state, and is commonly used on physical quadrature encoder
devices as a count reset trigger in non-quadrature modes. So I recommend
handling it early here as well:

	/* Default action mode */
	*action = COUNTER_SYNAPSE_ACTION_NONE;

	/* Handle Index Signal */
	if (priv->index_enabled && signal_id == GPIO_QENC_SIGNAL_INDEX) {
		*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
		return 0;
	}

> +
> +	switch (priv->function) {
> +	case GPIO_QENC_FUNC_QUAD_X4:
> +		if (signal_id == GPIO_QENC_SIGNAL_A ||
> +		    signal_id == GPIO_QENC_SIGNAL_B)
> +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		return 0;

With the default cases suggested earlier, the if statement and else go
away completely and you can directly set the action to "both edges":

	case COUNTER_FUNCTION_QUADRATURE_X4:
		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
		return 0;

> +
> +	case GPIO_QENC_FUNC_QUAD_X2:
> +		if (signal_id == GPIO_QENC_SIGNAL_A)
> +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		else if (signal_id == GPIO_QENC_SIGNAL_B)
> +			*action = COUNTER_SYNAPSE_ACTION_NONE;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		return 0;

Ditto:

	case COUNTER_FUNCTION_QUADRATURE_X2_A:
		if (signal_id == GPIO_QENC_SIGNAL_A)
			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
		return 0;

> +
> +	case GPIO_QENC_FUNC_QUAD_X1:
> +		if (signal_id == GPIO_QENC_SIGNAL_A)
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		else if (signal_id == GPIO_QENC_SIGNAL_B)
> +			*action = COUNTER_SYNAPSE_ACTION_NONE;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		return 0;

In the commit description you mention that this mode counts only rising
edges, but you matched this to COUNTER_FUNCTION_QUADRATURE_X1_A which
depends on the current direction to determine which edge is active. I'll
assume that was your intention (otherwise this becomes a duplicate of
pulse-direction mode with Index handled independent of quadrature mode):

	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;
		}
		return 0;

> +
> +	case GPIO_QENC_FUNC_PULSE_DIR:
> +		if (signal_id == GPIO_QENC_SIGNAL_A)
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		else
> +			*action = COUNTER_SYNAPSE_ACTION_NONE;
> +		return 0;

This cas becomes simple too:

	case COUNTER_FUNCTION_PULSE_DIRECTION:
		if (signal_id == GPIO_QENC_SIGNAL_A)
			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
		return 0;

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int gpio_qenc_signal_read(struct counter_device *counter,
> +				 struct counter_signal *signal,
> +				 enum counter_signal_level *level)
> +{
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	struct gpio_desc *gpio;
> +	int ret;
> +
> +	switch (signal->id) {
> +	case GPIO_QENC_SIGNAL_A:
> +		gpio = priv->gpio_a;
> +		break;
> +	case GPIO_QENC_SIGNAL_B:
> +		gpio = priv->gpio_b;
> +		break;
> +	case GPIO_QENC_SIGNAL_INDEX:
> +		gpio = priv->gpio_index;
> +		break;
> +	default:
> +		return -EINVAL;

The default case is unneeded because all possible enum constants are
handled by the switch block.

> +	}
> +
> +	if (!gpio)
> +		return -EINVAL;

Why is it possible to get back a zero value? Is this to handle the case
where a GPIO line does not exist? Would it be possible avoid adding
Counter Signals for missing GPIO during the gpio_qenc_probe() call so we
don't encounter such zombie Counter Signals?

> +
> +	ret = gpiod_get_value(gpio);
> +	if (ret < 0)
> +		return ret;
> +
> +	*level = ret ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
> +	return 0;
> +}
> +
> +static int gpio_qenc_events_configure(struct counter_device *counter)
> +{
> +	return 0;
> +}

The events_configure() callback is option, so you don't need to define
it if it doesn't do anything. Its intention is to enable/disable device
interrupts based on the Counter watches requested by users; that doesn't
apply for this driver because our encoding state is virtual and we push
Counter events as needed when we update the virtual state.

> +static int gpio_qenc_enable_write(struct counter_device *counter,
> +				  struct counter_count *count, u8 enable)
> +{
> +	struct gpio_qenc_priv *priv = counter_priv(counter);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	if (priv->enabled == !!enable) {
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		return 0;
> +	}
> +
> +	if (enable) {
> +		priv->enabled = true;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		enable_irq(priv->irq_a);
> +		enable_irq(priv->irq_b);
> +		if (priv->irq_index)
> +			enable_irq(priv->irq_index);
> +	} else {
> +		priv->enabled = false;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		disable_irq(priv->irq_a);
> +		disable_irq(priv->irq_b);
> +		if (priv->irq_index)
> +			disable_irq(priv->irq_index);
> +	}
> +
> +	return 0;
> +}

The value of enable is ensured by the Counter subsystem to be a bool
when passed to the enable_write() callback, so there's no need for the
double negation; you can compare against enable directly. Also you can
simplify the if block by setting priv->enabled unconditionally and
adding a return 0 to the if path:

	spin_lock_irqsave(&priv->lock, flags);

	if (priv->enabled == enable) {
		spin_unlock_irqrestore(&priv->lock, flags);
		return 0;
	}
	priv->enabled = enable;

	spin_unlock_irqrestore(&priv->lock, flags);

	if (enable) {
		enable_irq(priv->irq_a);
		enable_irq(priv->irq_b);
		if (priv->irq_index)
			enable_irq(priv->irq_index);
		return 0;
	}

	disable_irq(priv->irq_a);
	disable_irq(priv->irq_b);
	if (priv->irq_index)
		disable_irq(priv->irq_index);

	return 0;

> +static struct counter_comp gpio_qenc_count_ext[] = {
> +	COUNTER_COMP_CEILING(gpio_qenc_ceiling_read, gpio_qenc_ceiling_write),
> +	COUNTER_COMP_ENABLE(gpio_qenc_enable_read, gpio_qenc_enable_write),
> +	COUNTER_COMP_DIRECTION(gpio_qenc_direction_read),
> +	COUNTER_COMP_COUNT_BOOL("index_enabled",
> +				gpio_qenc_index_enable_read,
> +				gpio_qenc_index_enable_write),

The Index function in this driver is used to preset (reset) the Count so
rather than define your own "index_enabled" component, the idiomatic way
to handle this behavior in the Counter subsystem is to set
priv->index_enabled via COUNTER_COMP_PRESET_ENABLE().

> +	priv->synapses[0].actions_list = gpio_qenc_synapse_actions;
> +	priv->synapses[0].num_actions = ARRAY_SIZE(gpio_qenc_synapse_actions);
> +	priv->synapses[0].signal = &priv->signals[GPIO_QENC_SIGNAL_A];
> +
> +	priv->synapses[1].actions_list = gpio_qenc_synapse_actions;
> +	priv->synapses[1].num_actions = ARRAY_SIZE(gpio_qenc_synapse_actions);
> +	priv->synapses[1].signal = &priv->signals[GPIO_QENC_SIGNAL_B];
> +
> +	if (has_index) {
> +		priv->synapses[2].actions_list = gpio_qenc_synapse_actions;

The Index Signal only has two possible Synapse actions "rising edge" and
"none". Because that differs from the Quadrature A and B Signals (which
can also have "both edges" for example), you need to define a separate
static const enum counter_synapse_action array for just the Index
Signal which has just those two actions.

> +	priv->cnts.id = 0;
> +	priv->cnts.name = "Position";

Name this something more generic such as "Count" because positioning is
only one of the possible applications for a quadrature encoder not its
sole use case.

> +	counter->name = dev_name(dev);
> +	counter->parent = dev;
> +	counter->ops = &gpio_qenc_ops;
> +	counter->signals = priv->signals;
> +	counter->num_signals = num_signals;
> +	counter->counts = &priv->cnts;
> +	counter->num_counts = 1;

Use ARRAY_SIZE() for num_counts for the sake of consistency (and in case
we ever support more than one Count in future updates).

William Breathitt Gray