Re: [PATCH] iio: adc: ad7124: change setup reg allocation strategy

[David Lechner]

On 9/18/25 7:24 AM, Nuno Sá wrote:
> On Wed, 2025-09-17 at 17:05 -0500, David Lechner wrote:
>> Change the allocation strategy of the 8 SETUP registers from a least-
>> recently-used (LRU) to a first-come-first-served basis.
>>

...

>> -	/*
>> -	 * This is just to make sure that the comparison is adapted after
>> -	 * struct ad7124_channel_config was changed.
>> -	 */
>> -	static_assert(sizeof_field(struct ad7124_channel_config,
>> config_props) ==
>> -		      sizeof(struct {
>> -				     enum ad7124_ref_sel refsel;
>> -				     bool bipolar;
>> -				     bool buf_positive;
>> -				     bool buf_negative;
>> -				     unsigned int vref_mv;
>> -				     unsigned int pga_bits;
>> -				     unsigned int odr_sel_bits;
>> -				     enum ad7124_filter_type filter_type;
>> -				     unsigned int calibration_offset;
>> -				     unsigned int calibration_gain;
>> -			     }));
>> -
>> -	for (i = 0; i < st->num_channels; i++) {
>> -		cfg_aux = &st->channels[i].cfg;
>> -
>> -		if (cfg_aux->live &&
>> -		    cfg->refsel == cfg_aux->refsel &&
>> -		    cfg->bipolar == cfg_aux->bipolar &&
>> -		    cfg->buf_positive == cfg_aux->buf_positive &&
>> -		    cfg->buf_negative == cfg_aux->buf_negative &&
>> -		    cfg->vref_mv == cfg_aux->vref_mv &&
>> -		    cfg->pga_bits == cfg_aux->pga_bits &&
>> -		    cfg->odr_sel_bits == cfg_aux->odr_sel_bits &&
>> -		    cfg->filter_type == cfg_aux->filter_type &&
>> -		    cfg->calibration_offset == cfg_aux->calibration_offset &&
>> -		    cfg->calibration_gain == cfg_aux->calibration_gain)
>> -			return cfg_aux;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>> -static int ad7124_find_free_config_slot(struct ad7124_state *st)
>> -{
>> -	unsigned int free_cfg_slot;
>> -
>> -	free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
>> AD7124_MAX_CONFIGS);
>> -	if (free_cfg_slot == AD7124_MAX_CONFIGS)
>> -		return -1;
>> -
>> -	return free_cfg_slot;
>> -}
>> -
>>  /* Only called during probe, so dev_err_probe() can be used */
>>  static int ad7124_init_config_vref(struct ad7124_state *st, struct
>> ad7124_channel_config *cfg)
>>  {
>> @@ -485,6 +427,21 @@ static int ad7124_init_config_vref(struct ad7124_state
>> *st, struct ad7124_channe
>>  	}
>>  }
>>  
>> +static bool ad7124_config_equal(struct ad7124_channel_config *a,
>> +				struct ad7124_channel_config *b)
>> +{
>> +	return a->refsel == b->refsel &&
>> +	       a->bipolar == b->bipolar &&
>> +	       a->buf_positive == b->buf_positive &&
>> +	       a->buf_negative == b->buf_negative &&
>> +	       a->vref_mv == b->vref_mv &&
>> +	       a->pga_bits == b->pga_bits &&
>> +	       a->odr_sel_bits == b->odr_sel_bits &&
>> +	       a->filter_type == b->filter_type &&
>> +	       a->calibration_offset == b->calibration_offset &&
>> +	       a->calibration_gain == b->calibration_gain;
>> +}
> 
> Why not keeping the static_assert()? IIRC, Uwe felt fairly strong about having
> it.

I thought by now that we had implemented all of the possible
values so nothing else would be added so we didn't really need
the check anymore. But I guess there are a few bits left in the
CONFIG register that aren't accounted for.

TBH, when making the other recent changes it just felt like a
chore keeping it up to date and not particularly helpful. There
is already a comment where the fields are declared that was
enough to remind me to update this as well.

>> +		/* Find the first channel using this slot, if any. */
>> +		for (j = 0; j < st->num_channels; j++) {
>> +			if (st->channels[j].cfg.cfg_slot != i)
>> +				continue;
>>  
>> -	/* mark slot as free */
>> -	assign_bit(lru_cfg->cfg_slot, &st->cfg_slots_status, 0);
>> +			/*
>> +			 * If there is a match, increase the use count and
>> share
>> +			 * the slot with the requesting channel.
>> +			 */
>> +			if (ad7124_config_equal(&st->channels[j].cfg,
>> +						&st->channels[channel].cfg))
>> {
>> +				st->cfg_slot_use_count[i]++;
>> +				st->channels[channel].cfg.cfg_slot = i;
>>  
>> -	/* invalidate all other configs that pointed to this one */
>> -	for (i = 0; i < st->num_channels; i++) {
>> -		cfg = &st->channels[i].cfg;
>> +				dev_dbg(&st->sd.spi->dev,
>> +					"Re-using config slot %u for channel
>> %u, use count now %u\n",
>> +					i, channel, st-
>>> cfg_slot_use_count[i]);
>> +
>> +				return 0;
>> +			}
>> +		}
>> +	}
> 
> I think we could have the above a bit simpler. Something like:
> 
> for (j = 0; j < st->num_channels; j++) {
> 	if (st->channels[j].cfg.cfg_slot == AD7124_CFG_SLOT_UNASSIGNED)
> 		continue;
> 	if (!ad7124_config_equal(&st->channels[j].cfg,
> 	    &st->channels[channel].cfg))
> 		continue;
> 	
> 	i = st->channels[j].cfg.cfg_slot;
> 	st->cfg_slot_use_count[i]++;
> 	st->channels[channel].cfg.cfg_slot = i;
> }
> 
> Am I missing something?

I like it. I should also rename the i and j variables to slot and channel
to make it a bit more clear.

> 
>>  
>> -		if (cfg->cfg_slot == lru_cfg->cfg_slot)
>> -			cfg->live = false;
>> +	/* Find a free slot and write setup to ADC. */
>> +	for (i = 0; i < AD7124_MAX_CONFIGS; i++) {
>> +		if (st->cfg_slot_use_count[i] == 0) {
>> +			st->cfg_slot_use_count[i]++;
>> +			st->channels[channel].cfg.cfg_slot = i;
>> +
>> +			dev_dbg(&st->sd.spi->dev,
>> +				"Allocated config slot %u for channel %u, use
>> count now %u\n",
>> +				i, channel, st->cfg_slot_use_count[i]);
>> +
>> +			return ad7124_write_config(st, &st-
>>> channels[channel].cfg, i);
>> +		}
> 
> nit: I tend to prefer
> 
> if (st->cfg_slot_use_count[i] != 0) // or omit the != 0 part
> 	continue;
> 

Me too.

> ...
> 
> - Nuno Sá
>