Re: [PATCH v6 2/3] counter: add GPIO-based counter driver

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

On Sun, Jun 28, 2026 at 11:02:40PM +0200, Wadim Mueller wrote:
> Add a platform driver that turns plain GPIOs into a counter device.
> Edge interrupts on signal-a, signal-b (and optional index) are
> decoded in software. Quadrature steps are validated with a 2-bit
> Gray-code parity check; direction comes from the same identity.
> 
> The driver exposes two Counts on the same wires:
> 
>   "Count 1" - Synapses on A, B and optional Index 1. Supports
>               QUADRATURE_X1_{A,B} / X2_{A,B} / X4, PULSE_DIRECTION,
>               INCREASE and DECREASE.
>   "Count 2" - Synapse on B and optional Index 2. Supports INCREASE
>               and DECREASE only.
> 
> Per-Count state (value, ceiling, preset, enable, function, direction)
> lives in struct gpio_counter_count_priv so new per-Count features
> (e.g. floor) can be added without touching parallel arrays.
> 
> Sleepable GPIO providers (I2C/SPI expanders) are rejected at probe
> since the ISRs run in hardirq context.
> 
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>

Hi Wadim,

I thought some more about the "enabled" race condition we discussed
previously [^1]. I realized it's not a problem afterall because "enable"
refers to the Count and not inherently the interrupt. In other words, an
interrupt firing during a "disabled" state is okay as long as the Count
is not affected. Because gpio_counter_a_isr and gpio_counter_b_isr only
update the Counts when they are enabled, we should be fine.

It looks like we're almost there with this driver, so hopefully v7 will
be our final revision. Some comments follow below.

> +enum gpio_counter_signal_id {
> +	GPIO_COUNTER_SIGNAL_A = 0,
> +	GPIO_COUNTER_SIGNAL_B,
> +	GPIO_COUNTER_SIGNAL_INDEX1,
> +	GPIO_COUNTER_SIGNAL_INDEX2,
> +	GPIO_COUNTER_NUM_SIGNALS_MAX,
> +};
> +
> +enum gpio_counter_count_id {
> +	GPIO_COUNTER_COUNT_1 = 0,
> +	GPIO_COUNTER_COUNT_2,
> +	GPIO_COUNTER_NUM_COUNTS,
> +};

I'd rename this to GPIO_COUNTER_NUM_COUNTS_MAX for consistency with
GPIO_COUNTER_NUM_SIGNALS_MAX. Albeit, this is nitpick so I don't mind
keeping the name if you prefer it that way.

> +
> +#define GPIO_COUNTER_COUNT1_NUM_SYNAPSES	3	/* A, B, Index 1 */
> +#define GPIO_COUNTER_COUNT2_NUM_SYNAPSES	2	/* B, Index 2 */
> +#define GPIO_COUNTER_MAX_INDEX			2

The theoretical index maximum would be the number of Counts, so you can
set it to GPIO_COUNTER_NUM_COUNTS so it always matches in case we add
new Counts in the future.

> +struct gpio_counter_priv {
> +	struct gpio_desc *gpio_a;
> +	struct gpio_desc *gpio_b;
> +	struct gpio_desc *gpio_idx[GPIO_COUNTER_MAX_INDEX];
> +
> +	int irq_a;
> +	int irq_b;
> +	int irq_idx[GPIO_COUNTER_MAX_INDEX];
> +
> +	int n_idx;
> +
> +	/* protects count state, prev_a, prev_b */
> +	spinlock_t lock;
> +
> +	/* serialises enable_write() callers and b_irq_users */
> +	struct mutex enable_lock;
> +
> +	/* irq_b is shared by Count 1 and Count 2 (under enable_lock) */
> +	int b_irq_users;
> +
> +	int prev_a;
> +	int prev_b;
> +
> +	struct gpio_counter_count_priv count_priv[GPIO_COUNTER_NUM_COUNTS];
> +
> +	struct counter_signal  signals[GPIO_COUNTER_NUM_SIGNALS_MAX];
> +	struct counter_synapse synapses_count1[GPIO_COUNTER_COUNT1_NUM_SYNAPSES];
> +	struct counter_synapse synapses_count2[GPIO_COUNTER_COUNT2_NUM_SYNAPSES];

A future goal is to support pulse-direction mode for Count 2 which will
require two GPIO lines similar to Count 1. So instead of synapses_count1
and synapses_count2, I suggest a GPIO_COUNTER_COUNT_NUM_SYNAPSES set to 3
and defining a single multi-dimensional array called
synapses[GPIO_COUNTER_NUM_COUNTS][GPIO_COUNTER_COUNT_NUM_SYNAPSES].

> +static irqreturn_t gpio_counter_a_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_1];
> +	unsigned long flags;
> +	bool enabled;
> +	int a, b;
> +
> +	/* !! drops negative gpiod_get_value() errors. */
> +	a = !!gpiod_get_value(priv->gpio_a);
> +	b = !!gpiod_get_value(priv->gpio_b);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +
> +	enabled = cp->enabled;
> +	if (!enabled)
> +		goto out;

Shouldn't the enable state check occur before we get the current values
of A and B? If we are in a disabled state, we exit early and thus have
no need for the A and B values.

> +static irqreturn_t gpio_counter_b_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *c1 =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_1];
> +	struct gpio_counter_count_priv *c2 =
> +		&priv->count_priv[GPIO_COUNTER_COUNT_2];
> +	bool c1_enabled, c2_enabled;
> +	unsigned long flags;
> +	int a, b;
> +
> +	a = !!gpiod_get_value(priv->gpio_a);
> +	b = !!gpiod_get_value(priv->gpio_b);

Same comment as in gpio_counter_a_isr().

> +static void gpio_counter_index_pulse(struct counter_device *counter,
> +				     enum gpio_counter_count_id count_id)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count_id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (cp->preset_enabled) {
> +		cp->value = cp->preset;
> +		if (cp->value > cp->ceiling)
> +			cp->value = cp->ceiling;

This is just an assignment so a ternary may communicate the intent more
succinctly:

	if (cp->preset_enabled)
		cp->value = (cp->preset > cp->ceiling) ? cp->ceiling : cp->preset;

> +static int gpio_counter_count_read(struct counter_device *counter,
> +				   struct counter_count *count, u64 *val)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*val = cp->value;
> +	spin_unlock_irqrestore(&priv->lock, flags);

This is a single read operation so the lock is unnecessary here.

> +static int gpio_counter_function_read(struct counter_device *counter,
> +				      struct counter_count *count,
> +				      enum counter_function *function)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*function = cp->function;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +static int gpio_counter_ceiling_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *val)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*val = cp->ceiling;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +static int gpio_counter_enable_read(struct counter_device *counter,
> +				    struct counter_count *count, u8 *enable)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*enable = cp->enabled;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +/* enable_lock held */
> +static void gpio_counter_enable_b(struct gpio_counter_priv *priv)
> +{
> +	if (priv->b_irq_users++ == 0)
> +		enable_irq(priv->irq_b);
> +}
> +
> +/* enable_lock held */
> +static void gpio_counter_disable_b(struct gpio_counter_priv *priv)
> +{
> +	if (--priv->b_irq_users == 0)
> +		disable_irq(priv->irq_b);
> +}

Conceptually, updating b_irq_users and checking for zero are two
separate tasks, so dedicate each task to its own line.

> +static int gpio_counter_enable_write(struct counter_device *counter,
> +				     struct counter_count *count, u8 enable)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +	bool current_state;
> +
> +	guard(mutex)(&priv->enable_lock);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	current_state = cp->enabled;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +
> +	if (current_state == enable)

The current_state variable is only used here so it can go away entirely
if you replace it with cp->enabled directly.

> +		return 0;
> +
> +	/* disable_irq() may sleep, so it can not run under the spinlock. */
> +	if (enable) {
> +		spin_lock_irqsave(&priv->lock, flags);
> +		priv->prev_a = !!gpiod_get_value(priv->gpio_a);
> +		priv->prev_b = !!gpiod_get_value(priv->gpio_b);
> +		cp->enabled = true;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		gpio_counter_enable_irqs(priv, count->id);

To ensures everything is ready to go before we start updating the
Counts, should the IRQs be enabled first before setting cp->enabled to
true?

> +	} else {
> +		spin_lock_irqsave(&priv->lock, flags);
> +		cp->enabled = false;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		gpio_counter_disable_irqs(priv, count->id);
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpio_counter_direction_read(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       u32 *direction)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*direction = cp->direction;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +static int gpio_counter_preset_read(struct counter_device *counter,
> +				    struct counter_count *count, u64 *val)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*val = cp->preset;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +static int gpio_counter_preset_enable_read(struct counter_device *counter,
> +					   struct counter_count *count,
> +					   u8 *val)
> +{
> +	struct gpio_counter_priv *priv = counter_priv(counter);
> +	struct gpio_counter_count_priv *cp = &priv->count_priv[count->id];
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*val = cp->preset_enabled;
> +	spin_unlock_irqrestore(&priv->lock, flags);

Same comment as in the count_read() callback.

> +static int gpio_counter_get_index_gpios(struct device *dev,
> +					struct gpio_counter_priv *priv)
> +{
> +	struct gpio_descs *descs;
> +	int i;
> +
> +	descs = devm_gpiod_get_array_optional(dev, "index", GPIOD_IN);
> +	if (IS_ERR(descs))
> +		return dev_err_probe(dev, PTR_ERR(descs),
> +				     "failed to get index GPIOs\n");
> +	if (!descs) {
> +		priv->n_idx = 0;
> +		return 0;
> +	}
> +
> +	if (descs->ndescs < 1 || descs->ndescs > GPIO_COUNTER_MAX_INDEX)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "index-gpios: expected 1..%d entries, got %u\n",
> +				     GPIO_COUNTER_MAX_INDEX, descs->ndescs);
> +
> +	for (i = 0; i < descs->ndescs; i++)
> +		priv->gpio_idx[i] = descs->desc[i];
> +	priv->n_idx = descs->ndescs;
> +
> +	return 0;
> +}

Initially, I thought it would be fine to let the index-gpios DT entries
map sequentially to the Counts, but now I noticed two problems with this
approach.

First, the code around n_idx is much harder to grok. For example, in
gpio_counter_enable_irqs() the n_idx value is used as an indirect way of
determining whether an index IRQ should be enabled for a Count. The more
natural approach would be a dedicated index variable on each Count, that
way each Count knows its own index IRQ if it has one.

The second problem is that we now require Count 1 to have an index line
even if we only care about Count 2. The would be a problem if the user
has only three GPIO lines available.

As such, we need a different approach in the DT entries that allows
users to directly specify which index lines are for which Counts. That
way, you can store this information in gpio_counter_count_priv and know
easily whether a particular Count has an index IRQ and what it is.

William Breathitt Gray

[^1]: https://lore.kernel.org/all/20260623053800.120909-1-wbg@kernel.org/