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

On Wed, Jul 22, 2026 at 08:49:34PM +0900, William Breathitt Gray wrote:
> On Tue, Jul 14, 2026 at 10:17:08AM +0200, Wadim Mueller wrote:
> > +static int gpio_counter_setup_signals(struct gpio_counter_priv *priv)
> > +{
> > +	priv->signals[GPIO_COUNTER_SIGNAL_A].id   = GPIO_COUNTER_SIGNAL_A;
> > +	priv->signals[GPIO_COUNTER_SIGNAL_A].name = "Signal A";
> > +	priv->signals[GPIO_COUNTER_SIGNAL_B].id   = GPIO_COUNTER_SIGNAL_B;
> > +	priv->signals[GPIO_COUNTER_SIGNAL_B].name = "Signal B";
> > +
> > +	if (priv->count_priv[GPIO_COUNTER_COUNT_1].has_index) {
> > +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX1].id =
> > +			GPIO_COUNTER_SIGNAL_INDEX1;
> > +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX1].name = "Index 1";
> > +	}
> > +	if (priv->count_priv[GPIO_COUNTER_COUNT_2].has_index) {
> > +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX2].id =
> > +			GPIO_COUNTER_SIGNAL_INDEX2;
> > +		priv->signals[GPIO_COUNTER_SIGNAL_INDEX2].name = "Index 2";
> > +	}
> > +
> > +	return 2 + priv->count_priv[GPIO_COUNTER_COUNT_1].has_index +
> > +	       priv->count_priv[GPIO_COUNTER_COUNT_2].has_index;
> 
> As sashiko-bot pointed out, the Counter subsystem doesn't handle gaps in
> the signals array. What you can try instead is use a local variable
> num_signals to hold the current signals array offset:
> 
> 	num_signals = 2;
> 	if (priv->count_priv[GPIO_COUNTER_COUNT_1].has_index) {
> 		priv->signals[i].id = GPIO_COUNTER_SIGNAL_INDEX1;
> 		priv->signals[i].name = "Index 1";
> 		i++;
> 	}
> 	if (priv->count_priv[GPIO_COUNTER_COUNT_2].has_index) {
> 		priv->signals[i].id = GPIO_COUNTER_SIGNAL_INDEX2;
> 		priv->signals[i].name = "Index 2";
> 		i++;
> 	}
> 
> 	return i;

Oops, I intended for those 'i' to be 'num_signals'. However, if you like
you may name it 'i' or whatever makes sense.

> 
> Now, to refer back to the correct signals element later you have three
> options: loop through the signals array and check each id, or save the
> signals offset to a member of count_priv so you can refer to it later.
> 
> I think the first option is the simplest because there are only two
> Index Signals and a single if statement suffices: if priv->signals[2].id
> is not GPIO_COUNTER_SIGNAL_INDEX1, then it is GPIO_COUNTER_SIGNAL_INDEX2
> (and vice versa).
> 
> > +}
> > +
> > +static int gpio_counter_setup_synapses(struct gpio_counter_priv *priv)
> > +{
> > +	int n_c1 = 2, n_c2 = 1;
> > +
> > +	priv->synapses_count1[0].actions_list = gpio_counter_synapse_actions;
> > +	priv->synapses_count1[0].num_actions =
> > +		ARRAY_SIZE(gpio_counter_synapse_actions);
> > +	priv->synapses_count1[0].signal = &priv->signals[GPIO_COUNTER_SIGNAL_A];
> > +
> > +	priv->synapses_count1[1].actions_list = gpio_counter_synapse_actions;
> > +	priv->synapses_count1[1].num_actions =
> > +		ARRAY_SIZE(gpio_counter_synapse_actions);
> > +	priv->synapses_count1[1].signal = &priv->signals[GPIO_COUNTER_SIGNAL_B];
> > +
> > +	if (priv->count_priv[GPIO_COUNTER_COUNT_1].has_index) {
> > +		priv->synapses_count1[2].actions_list =
> > +			gpio_counter_index_synapse_actions;
> > +		priv->synapses_count1[2].num_actions =
> > +			ARRAY_SIZE(gpio_counter_index_synapse_actions);
> > +		priv->synapses_count1[2].signal =
> > +			&priv->signals[GPIO_COUNTER_SIGNAL_INDEX1];
> 
> You can use a ternary directly like this:
> 
> 	priv->synapses_count1[2].signal = (priv->signals[2].id == GPIO_COUNTER_SIGNAL_INDEX1)
> 		? &priv->signals[2] : &priv->signals[3];

I suppose index1 would always be at offset 2 if it does exist, so this
ternary isn't needed here but rather better for the index2 case in the
count2 block.

William Breathitt Gray