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

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

On Mon, Jun 22, 2026 at 10:51:40PM +0200, Wadim Mueller wrote:
> On Wed, 17 Jun 2026 16:49:25 +0900
> William Breathitt Gray <wbg@kernel.org> wrote:
> 
> Hi William,
> 
> thanks for the review. Three things before I spin v6.
> 
> > One change I consider is whether to make Signal B optional. [...]
> > I wonder whether this is substantially different enough from
> > simply using the interrupt-cnt module on the respective IRQ?
> > I'm CCing Oleksij and the Pengutronix team in case they wish to
> > comment.
> 
> I want to keep signal-b mandatory in v6 (if no concerns from Oleksij). The single-line case is
> already covered by interrupt-cnt.

All right, let's keep it required for v6. If for some reason we
determine it should be optional, we can address that after v6.

> > In such a configuration, we would have two Counts: Count 1 [...]
> > Count 2 supports only increase/decrease modes with a Synapse for
> > Signal B.
> 
> Just to confirm, plan for v6 is:
> 
>   Count 0 "AB Count": A + B + optional index0, all 8 functions
>   Count 1 "B Count":  B + optional index1, increase/decrease only
> 
> One counter_ops, dispatch on count->id. Per-count state in
> struct gpio_counter_count_priv (value, ceiling, preset, preset_enabled,
> enabled, function, direction), held in priv->count_priv[2] as you
> suggested. prev_a/prev_b stay on priv (they describe the wire, not
> the count).

Yes, that is correct. However, I would not name the Counts "AB Count"
and "B Count" -- it's possible that relationship will change in the
future if we add support for more function modes to Count 2. Naming them
"Count 1" and "Count 2" is adequate enough because the Signal
relationship is apparent via the Synapses.

> For the second Index in DT I would just let index-gpios take 1..2
> entries (first = count0, second = count1), no new property. Ok for
> you and Conor?

That seems okay to me. I'm generally indifferent to the DT so I'll leave
it up to what you and Conor decide.

> > Hmm, is it a problem that priv->enabled is changed to a false state
> > before the IRQs are actually disabled? Do any issues arise if an IRQ
> > is handled during that brief period of time?
> 
> I guess it is a race. In v6 I will reorder:
> 
>   enable=1: enable_irq();  lock; enabled = true;  unlock;
>   enable=0: lock; enabled = false; unlock;  disable_irq();
> 
> Plus a mutex around enable_write so two writers can not interleave
> (disable_irq() can not run under the spinlock).

I think the race conditions still exist despite your reorder: after
enable_irq() an interrupt could fire before you take the lock to set
priv->enabled to true; similarly, after setting priv->enabled to false
and unlocking, an interrupt could fire before you call disable_irq().

We need some atomic way to update priv->enabled and enable/disable the
IRQ at the same time, but I'm not sure how to accomplish that off the
top of my head. I don't want to delay your v6, and I'm not even sure
this race is actually a problem in practice, so let's discuss this again
after your v6 submission.

William Breathitt Gray