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

[Nuno Sá]

On Thu, 2025-09-18 at 08:16 -0500, David Lechner wrote:
> 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.

I brought it up because I remember Uwe kind of really wanted it (or maybe it was
requested by someone reviewing). Not sure now :).

I'm fine with removing it anyways.

> 
> > > +		/* 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.
> 

Sure...

> 
- Nuno Sá