counter: add GPIO-based counter driver
Subsystems (3)
v6 cover counter: add GPIO-based counter driver
This series adds a counter subsystem driver that does quadrature encoder position tracking with plain GPIO pins and edge interrupts. Compared to interrupt-cnt.c (pulse-only) it provides full A/B/Index decoding and exposes the counter sysfs ABI. Target hardware is low- to medium-speed rotary encoders on SoCs without a free eQEP / FTM / etc. Benchmark rig: github.com/wafgo/qenc-bench.
Following William's v5 review [1][2] the driver now exposes two Counts on the same wires:
Count 1: Synapses on A and B (+ optional Index 1). Supports
QUADRATURE_X1_{A,B} / X2_{A,B} / X4, PULSE_DIRECTION, INCREASE and DECREASE.
Count 2: Synapse on B (+ optional Index 2). INCREASE / DECREASE.
== Changes in v6 ==
Driver:
- Two-Count layout with per-Count state struct (gpio_counter_count_priv) so future per-Count features can be added without parallel arrays.
- ISR rewrite: STATE_CHANGED check used in all quadrature modes (not only X4); the *_delta helpers are gone.
- INCREASE / DECREASE now count on both edges of their input (A for Count 1, B for Count 2).
- PULSE_DIRECTION: direction is sampled in the B-ISR, the A-ISR only steps the value.
- direction is initialised in function_write so the first read after a mode switch returns a defined value.
- enable_write: added enable_lock mutex and reordered the enable/disable sequence so the spinlock is dropped before disable_irq() (which can sleep). irq_b is shared by Count 1 and Count 2, so it is refcounted; the per-Count irqs (A, Index 1, Index 2) are toggled directly.
Binding:
- index-gpios takes 1..2 entries: Index for Count 1, optional Index for Count 2.
- Description and Counts wording updated for the two-Count layout.
- signal-b stays required. The single-line case is already covered by interrupt-cnt. Oleksij suggested [3] extending interrupt-cnt with a multi-line variant; I kept this as its own driver because the Generic Counter ABI (Counts / Synapses / Signals, ceiling, preset, action) does not fit interrupt-cnt's pulse hot path well.
- Conor's v5 Acked-by [4] is dropped because of the binding changes -- a fresh Ack would be appreciated.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/20260525-register-bogus-b198545b69a8@spud/
Wadim Mueller (3):
dt-bindings: counter: add gpio-counter binding
counter: add GPIO-based counter driver
MAINTAINERS: add entry for GPIO counter driver
.../bindings/counter/gpio-counter.yaml | 59 ++ MAINTAINERS | 7 + drivers/counter/Kconfig | 17 + drivers/counter/Makefile | 1 + drivers/counter/gpio-counter.c | 983 ++++++++++++++++++ 5 files changed, 1067 insertions(+) create mode 100644 Documentation/devicetree/bindings/counter/gpio-counter.yaml create mode 100644 drivers/counter/gpio-counter.c
base-commit: 3cd8b194bf3428dfa53120fee47e827a7c495815
Signature · 1 line
2.52.0
PATCH v6 1/3 dt-bindings: counter: add gpio-counter binding
Add a binding for a generic GPIO-based counter. Two GPIOs (signal-a, signal-b) drive two Counts; optional index GPIOs load a preset. The counter function (quadrature, pulse-direction, increase/decrease) is selected at runtime via the counter sysfs interface.
--- .../bindings/counter/gpio-counter.yaml | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/devicetree/bindings/counter/gpio-counter.yaml
Documentation/devicetree/bindings/counter/gpio-counter.yaml new file +59 -0
@@ -0,0 +1,59 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/counter/gpio-counter.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: GPIO-based Counter + +maintainers: + - Wadim Mueller <[email protected]> + +description: | + GPIO-based software counter with two Counts: + * Count 1: A + B. Quadrature X1/X2/X4, pulse-direction, inc/dec. + * Count 2: B. Inc / dec. + The function is selected at runtime via the counter sysfs ABI. Each + Count has an optional Index (Z) that loads its preset on rising edge. + +properties: + compatible: + const: gpio-counter + + signal-a-gpios: + maxItems: 1 + description: Signal A (phase A; pulse input in pulse-direction/inc/dec). + + signal-b-gpios: + maxItems: 1 + description: + Signal B (phase B; direction in pulse-direction; pulse input for + Count 2). + + index-gpios: + minItems: 1 + maxItems: 2 + description: + Optional Index (Z) inputs. First entry indexes Count 1, second + (optional) indexes Count 2. + +required: + - compatible + - signal-a-gpios + - signal-b-gpios + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + counter { + compatible = "gpio-counter"; + signal-a-gpios = <&gpio0 10 GPIO_ACTIVE_LOW>; + signal-b-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>; + index-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>, + <&gpio0 13 GPIO_ACTIVE_LOW>; + }; + +...
Signature · 1 line
2.52.0
Re: [PATCH v6 1/3] dt-bindings: counter: add gpio-counter binding
pw-bot: not-applicable
PATCH v6 2/3 counter: add GPIO-based counter driver
In reply to Wadim MuellerAdd 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.
--- drivers/counter/Kconfig | 17 + drivers/counter/Makefile | 1 + drivers/counter/gpio-counter.c | 1015 ++++++++++++++++++++++++++++++++ 3 files changed, 1033 insertions(+) create mode 100644 drivers/counter/gpio-counter.c
drivers/counter/Kconfig +17 -0
@@ -68,6 +68,23 @@ config INTEL_QEP To compile this driver as a module, choose M here: the module will be called intel-qep. +config GPIO_COUNTER + tristate "GPIO-based counter driver" + depends on GPIOLIB + help + Select this option to enable the GPIO-based counter driver. It + reads A/B and an optional index signal via edge-triggered GPIO + interrupts and decodes them according to the selected mode: + Quadrature X1/X2/X4 (rotary or linear encoders), pulse-direction, + and pure increase / decrease pulse counters. + + This is useful on SoCs that lack a dedicated hardware quadrature + decoder or pulse counter, or where the signals are wired to + generic GPIO pins rather than to a dedicated peripheral. + + To compile this driver as a module, choose M here: the + module will be called gpio-counter. + config INTERRUPT_CNT tristate "Interrupt counter driver" depends on GPIOLIB
drivers/counter/Makefile +1 -0
@@ -14,6 +14,7 @@ obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o obj-$(CONFIG_TI_EQEP) += ti-eqep.o obj-$(CONFIG_FTM_QUADDEC) += ftm-quaddec.o +obj-$(CONFIG_GPIO_COUNTER) += gpio-counter.o obj-$(CONFIG_MICROCHIP_TCB_CAPTURE) += microchip-tcb-capture.o obj-$(CONFIG_INTEL_QEP) += intel-qep.o obj-$(CONFIG_TI_ECAP_CAPTURE) += ti-ecap-capture.o
drivers/counter/gpio-counter.c new file +1015 -0
1,015 lines not on this page · raw message
Signature · 1 line
2.52.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing direction state update on enable for PULSE_DIRECTION mode. - [High] Reading GPIO pin…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing direction state update on enable for PULSE_DIRECTION mode.
- [High] Reading GPIO pin states outside the spinlock in the ISRs allows out-of-order state evaluation, leading to spurious counter steps.
- [Medium] Enabling a count overwrites shared quadrature state, corrupting the active state machine of the other running count. --
commit 002c8f1bc700b17784c2bbc34b75aada303eee61
Author: Wadim Mueller <[email protected]>
counter: add GPIO-based counter driver
This adds a platform driver to decode plain GPIOs into a counter device, supporting quadrature, pulse-direction, and increase/decrease modes.
Quoted 5 lines
drivers/counter/gpio-counter.c new file +0
-0
[ ... ]
Quoted 5 lines
drivers/counter/gpio-counter.c new file +5
-0+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 =
drivers/counter/gpio-counter.c line 128
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+ &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);
[Severity: High]
Does reading the GPIO pin states outside the spinlock here and in gpio_counter_b_isr() create a data race that allows out-of-order state evaluation?
If high-frequency edges on A and B pins trigger interrupts that are serviced concurrently on different CPUs, CPU 0 could read the states locklessly, get preempted, and then apply a stale state after CPU 1 has already acquired the lock and updated it. This could rewind the Gray-code state machine and generate spurious backward steps or drop valid transitions.
Would moving the gpiod_get_value() reads inside the lock ensure late-arriving ISRs read the most current state and safely drop redundant edges?
[ ... ]
Quoted 32 lines
drivers/counter/gpio-counter.c new file +32
-0+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); + +spin_lock_irqsave (&priv->lock, flags); + + c1_enabled = c1->enabled; + c2_enabled = c2->enabled; + +/* Count 1: A+B decoding on this B edge */ +if (c1_enabled) { +switch (c1->function) { +case COUNTER_FUNCTION_QUADRATURE_X4 : +case COUNTER_FUNCTION_QUADRATURE_X2_B : +case COUNTER_FUNCTION_QUADRATURE_X1_B : +if (!GPIO_COUNTER_STATE_CHANGED (priv->prev_a, + priv->prev_b, a, b)) +break ; + c1->direction = +GPIO_COUNTER_GET_DIRECTION (priv->prev_b, a); +if (c1->function ==COUNTER_FUNCTION_QUADRATURE_X1_B )
drivers/counter/gpio-counter.c line 217
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+gpio_counter_quadrature_x1_step (c1, b); + else +gpio_counter_value_step (c1 ); +break ; + +case COUNTER_FUNCTION_PULSE_DIRECTION : +/* B is the direction line */ + c1->direction = b ?COUNTER_COUNT_DIRECTION_BACKWARD + :COUNTER_COUNT_DIRECTION_FORWARD ; +break ;
[Severity: High]
Is a direction state update missing on enable for PULSE_DIRECTION mode?
Since the direction is only updated here when the count is enabled, any B pin toggles that happen while the counter is disabled are not tracked. When the counter is subsequently enabled, gpio_counter_enable_write() does not synchronize cp->direction with the current B pin level.
Could this cause the counter to step in the wrong direction until the B pin toggles again?
[ ... ]
Quoted 12 lines
drivers/counter/gpio-counter.c new file +12
-0+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;
drivers/counter/gpio-counter.c line 621
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+spin_unlock_irqrestore (&priv->lock, flags); + +if (current_state == enable) +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);
[Severity: Medium]
Does enabling a count unconditionally overwrite the shared quadrature state, corrupting the active state machine of the other running count?
Because priv->prev_a and priv->prev_b are updated here whenever any count is enabled, enabling Count 2 while Count 1 is actively running will overwrite Count 1's state tracker.
If an edge interrupt was pending for Count 1, its ISR might observe no state change and incorrectly drop the step.
Quoted 4 lines
drivers/counter/gpio-counter.c new file +4
-0+ cp->enabled = true; +spin_unlock_irqrestore (&priv->lock, flags); +gpio_counter_enable_irqs (priv, count->id); + }else {
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
William Breathitt Gray
maintainerQuoted 13 lines · Wadim Mueller
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.
Quoted 8 lines
Per-Count state (value, ceiling, preset, enable, function, direction) lives in struct
gpio_counter_count_privso 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 <[email protected]>
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.
drivers/counter/gpio-counter.c line 37
Quoted 13 lines
drivers/counter/gpio-counter.c new file +13
-0+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.
drivers/counter/gpio-counter.c line 41
Quoted 4 lines
drivers/counter/gpio-counter.c new file +4
-0+ +#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.
Quoted 18 lines
drivers/counter/gpio-counter.c new file +18
-0+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) */
drivers/counter/gpio-counter.c line 81
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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].
Quoted 9 lines
drivers/counter/gpio-counter.c new file +9
-0+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;
drivers/counter/gpio-counter.c line 132
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+ +/* !! 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.
Quoted 4 lines
drivers/counter/gpio-counter.c new file +4
-0+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);
drivers/counter/gpio-counter.c line 189
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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().
drivers/counter/gpio-counter.c line 254
Quoted 12 lines
drivers/counter/gpio-counter.c new file +12
-0+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;
drivers/counter/gpio-counter.c line 282
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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.
drivers/counter/gpio-counter.c line 332
Quoted 11 lines
drivers/counter/gpio-counter.c new file +11
-0+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.
drivers/counter/gpio-counter.c line 513
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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.
drivers/counter/gpio-counter.c line 541
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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.
drivers/counter/gpio-counter.c line 558
Quoted 13 lines
drivers/counter/gpio-counter.c new file +13
-0+/* 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.
drivers/counter/gpio-counter.c line 612
Quoted 13 lines
drivers/counter/gpio-counter.c new file +13
-0+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.
drivers/counter/gpio-counter.c line 614
Quoted 2 lines
drivers/counter/gpio-counter.c new file +2
-0+ +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.
drivers/counter/gpio-counter.c line 624
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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?
Quoted 11 lines
drivers/counter/gpio-counter.c new file +11
-0+ }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,
drivers/counter/gpio-counter.c line 645
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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.
drivers/counter/gpio-counter.c line 659
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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.
drivers/counter/gpio-counter.c line 694
Quoted 11 lines
drivers/counter/gpio-counter.c new file +11
-0+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.
Quoted 16 lines
drivers/counter/gpio-counter.c new file +16
-0+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 )
drivers/counter/gpio-counter.c line 762
Quoted 10 lines
drivers/counter/gpio-counter.c new file +10
-0+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
PATCH v6 3/3 MAINTAINERS: add entry for GPIO counter driver
In reply to Wadim MuellerCover the gpio-counter driver and its device-tree binding.
--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
@@ -10984,6 +10984,13 @@ S: Supported F: Documentation/admin-guide/gpio/gpio-aggregator.rst F: drivers/gpio/gpio-aggregator.c +GPIO COUNTER DRIVER +M: Wadim Mueller <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/counter/gpio-counter.yaml +F: drivers/counter/gpio-counter.c + GPIO IR Transmitter M: Sean Young <[email protected]> L: [email protected]
Signature · 1 line
2.52.0