Message-ID: <3bbd51983f5bf67b6fd061f28b8b260370beac70.camel@gmail.com>
Date: Fri, 26 Sep 2025 13:27:00 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Michael Hennerich
	 <Michael.Hennerich@...log.com>, Jonathan Cameron <jic23@...nel.org>, Nuno
 Sá
	 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: adc: ad7124: change setup reg allocation
 strategy

On Tue, 2025-09-23 at 16:48 -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.
> 
> The AD7124 chips can have up to 16 channels enabled at a time in the
> sequencer for buffered reads, but only have 8 SETUP configurations
> (namely the OFFSET, GAIN, CONFIG and FILTER registers) that must be
> shared among the 16 channels.  This means some of the channels must use
> the exact same configuration parameters so that they can share a single
> SETUP group of registers.  The previous LRU strategy did not keep track
> of how many different configurations were requested at the same time,
> so if there were more than 8 different configurations requested, some
> channels would end up using the incorrect configuration because the slot
> assigned to them would also be assigned to a different configuration
> that wrote over it later.
> 
> Adding such tracking to solve this would make an already complex
> algorithm even more complex.  Instead we can replace it with a simpler
> first-come-first-serve strategy.  This makes it easy to track how many
> different configurations are being requested at the same time.  This
> comes at the expense of slightly longer setup times for buffered reads
> since all setup registers must be written each time when a buffered read
> is enabled.  But this is generally not considered a hot path where
> performance is critical, so should be acceptable.
> 
> This new strategy also makes hardware debugging easier since SETUPs are
> now assigned in a deterministic manner and in a logical order.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---

Hi David,

LGTM

Reviewed-by: Nuno Sá <nuno.sa@...log.com>

> This is something I wish I would have noticed earlier. This is
> addressing the same issue we saw recently in the ad7173 driver [1].
> 
> However, since we have added a bunch of new features to the ad7124
> driver recently, it seems too late to make a change like the one in the
> link that can be easily backported. So I didn't bother with a Fixes: tag
> here. Given how many other broken things we found in ad7124 recently,
> I'm not too worried about rushing out a fix for backporting anyway.
> 
> Instead, I implemented the idea I first proposed in [2]. I find it much
> easier to follow and reason about than the LRU implementation. And while
> working on other features, I found the LRU implementation to be annoying
> because I never knew which setup registers would be assigned to which
> channels after tweaking the channel configuration. This made debugging
> more tedious than it could have been. So this seems like an overall
> improvement to me.
> 
> B4 is picking up all of the recent patches to the ad7124 driver as
> dependencies, but really the only one that is actually needed for this
> to apply cleanly is the "iio: adc: ad7124: drop nr field" series.
> 
> [1]:
> https://lore.kernel.org/linux-iio/20250722-iio-adc-ad7173-fix-setup-use-limits-v2-1-8e96bdb72a9c@baylibre.com/
> [2]:
> https://lore.kernel.org/linux-iio/bcf599e1-7816-4580-a2f9-039b0d3c0c99@baylibre.com/
> ---
> Changes in v2:
> - Simplified search algorithm to a single for loop.
> - Renamed loop variable for channels to "other".
> - Removed dev_dbg() statements.
> - Reworded confusing comment.
> - Link to v1:
> https://lore.kernel.org/r/20250917-iio-adc-ad7124-change-setup-reg-allocation-strategy-v1-1-4e17b3de046b@baylibre.com
> ---
>  drivers/iio/adc/ad7124.c | 233 ++++++++++++++++++++--------------------------
> -
>  1 file changed, 100 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index
> 374e39736584f55c1290db3e257dff2c60f884d2..f7bf13ef1c956958569712ed758b7073494e
> 8c24 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -111,6 +111,8 @@
>  #define AD7124_FILTER_SINGLE_CYCLE	BIT(16)
>  #define AD7124_FILTER_FS		GENMASK(10, 0)
>  
> +#define AD7124_CFG_SLOT_UNASSIGNED	~0U
> +
>  #define AD7124_MAX_CONFIGS	8
>  #define AD7124_MAX_CHANNELS	16
>  
> @@ -176,14 +178,13 @@ enum ad7124_filter_type {
>  };
>  
>  struct ad7124_channel_config {
> -	bool live;
>  	unsigned int cfg_slot;
>  	unsigned int requested_odr;
>  	unsigned int requested_odr_micro;
>  	/*
>  	 * Following fields are used to compare for equality. If you
>  	 * make adaptations in it, you most likely also have to adapt
> -	 * ad7124_find_similar_live_cfg(), too.
> +	 * ad7124_config_equal(), too.
>  	 */
>  	struct_group(config_props,
>  		enum ad7124_ref_sel refsel;
> @@ -215,14 +216,13 @@ struct ad7124_state {
>  	unsigned int adc_control;
>  	unsigned int num_channels;
>  	struct mutex cfgs_lock; /* lock for configs access */
> -	unsigned long cfg_slots_status; /* bitmap with slot status (1 means
> it is used) */
> +	u8 cfg_slot_use_count[AD7124_MAX_CONFIGS];
>  
>  	/*
>  	 * Stores the power-on reset value for the GAIN(x) registers which
> are
>  	 * needed for measurements at gain 1 (i.e. CONFIG(x).PGA == 0)
>  	 */
>  	unsigned int gain_default;
> -	DECLARE_KFIFO(live_cfgs_fifo, struct ad7124_channel_config *,
> AD7124_MAX_CONFIGS);
>  	bool enable_single_cycle;
>  };
>  
> @@ -367,9 +367,6 @@ static void ad7124_set_channel_odr(struct ad7124_state
> *st, unsigned int channel
>  		  cfg->requested_odr_micro * factor / MICRO;
>  	odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);
>  
> -	if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
> -		st->channels[channel].cfg.live = false;
> -
>  	st->channels[channel].cfg.odr_sel_bits = odr_sel_bits;
>  }
>  
> @@ -404,61 +401,6 @@ static int ad7124_get_3db_filter_factor(struct
> ad7124_state *st,
>  	}
>  }
>  
> -static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct
> ad7124_state *st,
> -								  struct
> ad7124_channel_config *cfg)
> -{
> -	struct ad7124_channel_config *cfg_aux;
> -	int i;
> -
> -	/*
> -	 * 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)
>  {
> @@ -487,6 +429,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;
> +}
> +
>  static int ad7124_write_config(struct ad7124_state *st, struct
> ad7124_channel_config *cfg,
>  			       unsigned int cfg_slot)
>  {
> @@ -495,13 +452,13 @@ static int ad7124_write_config(struct ad7124_state *st,
> struct ad7124_channel_co
>  	unsigned int post = 0;
>  	int ret;
>  
> -	cfg->cfg_slot = cfg_slot;
> -
> -	ret = ad_sd_write_reg(&st->sd, AD7124_OFFSET(cfg->cfg_slot), 3, cfg-
> >calibration_offset);
> +	ret = ad_sd_write_reg(&st->sd, AD7124_OFFSET(cfg_slot), 3,
> +			      cfg->calibration_offset);
>  	if (ret)
>  		return ret;
>  
> -	ret = ad_sd_write_reg(&st->sd, AD7124_GAIN(cfg->cfg_slot), 3, cfg-
> >calibration_gain);
> +	ret = ad_sd_write_reg(&st->sd, AD7124_GAIN(cfg_slot), 3,
> +			      cfg->calibration_gain);
>  	if (ret)
>  		return ret;
>  
> @@ -511,7 +468,7 @@ static int ad7124_write_config(struct ad7124_state *st,
> struct ad7124_channel_co
>  		(cfg->buf_negative ? AD7124_CONFIG_AIN_BUFM : 0) |
>  		FIELD_PREP(AD7124_CONFIG_PGA, cfg->pga_bits);
>  
> -	ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(cfg->cfg_slot), 2, val);
> +	ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(cfg_slot), 2, val);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -564,7 +521,7 @@ static int ad7124_write_config(struct ad7124_state *st,
> struct ad7124_channel_co
>  	 * was requested. It may only be disabled through debugfs for testing
>  	 * purposes.
>  	 */
> -	return ad_sd_write_reg(&st->sd, AD7124_FILTER(cfg->cfg_slot), 3,
> +	return ad_sd_write_reg(&st->sd, AD7124_FILTER(cfg_slot), 3,
>  			       FIELD_PREP(AD7124_FILTER_FILTER, filter) |
>  			       FIELD_PREP(AD7124_FILTER_REJ60, rej60) |
>  			       FIELD_PREP(AD7124_FILTER_POST_FILTER, post) |
> @@ -573,83 +530,86 @@ static int ad7124_write_config(struct ad7124_state *st,
> struct ad7124_channel_co
>  			       FIELD_PREP(AD7124_FILTER_FS, cfg-
> >odr_sel_bits));
>  }
>  
> -static struct ad7124_channel_config *ad7124_pop_config(struct ad7124_state
> *st)
> +/**
> + * ad7124_request_config_slot() - Request a config slot for a given config
> + * @st:		Driver instance
> + * @channel:	Channel to request a slot for
> + *
> + * Tries to find a matching config already in use, otherwise finds a free
> + * slot. If this function returns successfully, the use count for the slot is
> + * increased and the slot number is stored in cfg->cfg_slot.
> + *
> + * The slot must be released again with ad7124_release_config_slot() when no
> + * longer needed.
> + *
> + * Returns: 0 if a slot was successfully assigned, -EUSERS if no slot is
> + * available or other error if SPI communication fails.
> + */
> +static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
>  {
> -	struct ad7124_channel_config *lru_cfg;
> -	struct ad7124_channel_config *cfg;
> -	int ret;
> -	int i;
> +	unsigned int other, slot;
> +	int last_used_slot = -1;
>  
> -	/*
> -	 * Pop least recently used config from the fifo
> -	 * in order to make room for the new one
> -	 */
> -	ret = kfifo_get(&st->live_cfgs_fifo, &lru_cfg);
> -	if (ret <= 0)
> -		return NULL;
> +	/* Find another channel with a matching config, if any. */
> +	for (other = 0; other < st->num_channels; other++) {
> +		if (other == channel)
> +			continue;
>  
> -	lru_cfg->live = false;
> +		if (st->channels[other].cfg.cfg_slot ==
> AD7124_CFG_SLOT_UNASSIGNED)
> +			continue;
>  
> -	/* mark slot as free */
> -	assign_bit(lru_cfg->cfg_slot, &st->cfg_slots_status, 0);
> +		last_used_slot = max_t(int, last_used_slot,
> +				       st->channels[other].cfg.cfg_slot);
>  
> -	/* invalidate all other configs that pointed to this one */
> -	for (i = 0; i < st->num_channels; i++) {
> -		cfg = &st->channels[i].cfg;
> +		if (!ad7124_config_equal(&st->channels[other].cfg,
> +					 &st->channels[channel].cfg))
> +			continue;
>  
> -		if (cfg->cfg_slot == lru_cfg->cfg_slot)
> -			cfg->live = false;
> +		/* Found a match, re-use that slot. */
> +		slot = st->channels[other].cfg.cfg_slot;
> +		st->cfg_slot_use_count[slot]++;
> +		st->channels[channel].cfg.cfg_slot = slot;
> +
> +		return 0;
>  	}
>  
> -	return lru_cfg;
> -}
> +	/* No match, use next free slot. */
> +	slot = last_used_slot + 1;
> +	if (slot >= AD7124_MAX_CONFIGS)
> +		return -EUSERS;
>  
> -static int ad7124_push_config(struct ad7124_state *st, struct
> ad7124_channel_config *cfg)
> -{
> -	struct ad7124_channel_config *lru_cfg;
> -	int free_cfg_slot;
> +	st->cfg_slot_use_count[slot]++;
> +	st->channels[channel].cfg.cfg_slot = slot;
>  
> -	free_cfg_slot = ad7124_find_free_config_slot(st);
> -	if (free_cfg_slot >= 0) {
> -		/* push the new config in configs queue */
> -		kfifo_put(&st->live_cfgs_fifo, cfg);
> -	} else {
> -		/* pop one config to make room for the new one */
> -		lru_cfg = ad7124_pop_config(st);
> -		if (!lru_cfg)
> -			return -EINVAL;
> +	return ad7124_write_config(st, &st->channels[channel].cfg, slot);
> +}
>  
> -		/* push the new config in configs queue */
> -		free_cfg_slot = lru_cfg->cfg_slot;
> -		kfifo_put(&st->live_cfgs_fifo, cfg);
> -	}
> +static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
> +{
> +	unsigned int slot = st->channels[channel].cfg.cfg_slot;
>  
> -	/* mark slot as used */
> -	assign_bit(free_cfg_slot, &st->cfg_slots_status, 1);
> +	/*
> +	 * All of these conditions can happen at probe when all channels are
> +	 * disabled. Otherwise, they should not happen normally.
> +	 */
> +	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED
> ||
> +	    st->cfg_slot_use_count[slot] == 0)
> +		return;
>  
> -	return ad7124_write_config(st, cfg, free_cfg_slot);
> +	st->cfg_slot_use_count[slot]--;
> +	st->channels[channel].cfg.cfg_slot = AD7124_CFG_SLOT_UNASSIGNED;
>  }
>  
>  static int ad7124_prepare_read(struct ad7124_state *st, int address)
>  {
>  	struct ad7124_channel_config *cfg = &st->channels[address].cfg;
> -	struct ad7124_channel_config *live_cfg;
> +	int ret;
>  
> -	/*
> -	 * Before doing any reads assign the channel a configuration.
> -	 * Check if channel's config is on the device
> -	 */
> -	if (!cfg->live) {
> -		/* check if config matches another one */
> -		live_cfg = ad7124_find_similar_live_cfg(st, cfg);
> -		if (!live_cfg)
> -			ad7124_push_config(st, cfg);
> -		else
> -			cfg->cfg_slot = live_cfg->cfg_slot;
> -	}
> +	ret = ad7124_request_config_slot(st, address);
> +	if (ret)
> +		return ret;
>  
>  	/* point channel to the config slot and enable */
> -	cfg->live = true;
>  	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(address), 2,
>  			       st->channels[address].ain |
>  			       FIELD_PREP(AD7124_CHANNEL_SETUP, cfg-
> >cfg_slot) |
> @@ -692,6 +652,8 @@ static int ad7124_disable_one(struct ad_sigma_delta *sd,
> unsigned int chan)
>  {
>  	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
>  
> +	ad7124_release_config_slot(st, chan);
> +
>  	/* The relevant thing here is that AD7124_CHANNEL_ENABLE is cleared.
> */
>  	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(chan), 2, 0);
>  }
> @@ -913,9 +875,6 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>  		gain = DIV_ROUND_CLOSEST(res, val2);
>  		res = ad7124_find_closest_match(ad7124_gain,
> ARRAY_SIZE(ad7124_gain), gain);
>  
> -		if (st->channels[chan->address].cfg.pga_bits != res)
> -			st->channels[chan->address].cfg.live = false;
> -
>  		st->channels[chan->address].cfg.pga_bits = res;
>  		return 0;
>  	default:
> @@ -1058,7 +1017,11 @@ static int ad7124_syscalib_locked(struct ad7124_state
> *st, const struct iio_chan
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(ch-
> >cfg.cfg_slot), 3,
> +		/*
> +		 * Making the assumption that a single conversion will always
> +		 * use configuration slot 0 for the OFFSET/GAIN registers.
> +		 */
> +		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(0), 3,
>  				     &ch->cfg.calibration_offset);
>  		if (ret < 0)
>  			return ret;
> @@ -1073,7 +1036,7 @@ static int ad7124_syscalib_locked(struct ad7124_state
> *st, const struct iio_chan
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(ch->cfg.cfg_slot),
> 3,
> +		ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(0), 3,
>  				     &ch->cfg.calibration_gain);
>  		if (ret < 0)
>  			return ret;
> @@ -1164,7 +1127,6 @@ static int ad7124_set_filter_type_attr(struct iio_dev
> *dev,
>  
>  	guard(mutex)(&st->cfgs_lock);
>  
> -	cfg->live = false;
>  	cfg->filter_type = value;
>  	ad7124_set_channel_odr(st, chan->address);
>  
> @@ -1483,7 +1445,6 @@ static int ad7124_setup(struct ad7124_state *st)
>  	if (ret)
>  		return ret;
>  
> -	INIT_KFIFO(st->live_cfgs_fifo);
>  	for (i = 0; i < st->num_channels; i++) {
>  		struct ad7124_channel_config *cfg = &st->channels[i].cfg;
>  
> @@ -1491,6 +1452,8 @@ static int ad7124_setup(struct ad7124_state *st)
>  		if (ret < 0)
>  			return ret;
>  
> +		cfg->cfg_slot = AD7124_CFG_SLOT_UNASSIGNED;
> +
>  		/* Default filter type on the ADC after reset. */
>  		cfg->filter_type = AD7124_FILTER_TYPE_SINC4;
>  
> @@ -1550,7 +1513,7 @@ static int __ad7124_calibrate_all(struct ad7124_state
> *st, struct iio_dev *indio
>  			 * ad_sigma_delta_set_channel() ->
> ad7124_set_channel()
>  			 * -> ad7124_prepare_read().
>  			 */
> -			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(st-
> >channels[i].cfg.cfg_slot), 3,
> +			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(0), 3,
>  					     &st-
> >channels[i].cfg.calibration_gain);
>  			if (ret < 0)
>  				return ret;
> @@ -1560,7 +1523,11 @@ static int __ad7124_calibrate_all(struct ad7124_state
> *st, struct iio_dev *indio
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(st-
> >channels[i].cfg.cfg_slot), 3,
> +		/*
> +		 * Making the assumption that a single conversion will always
> +		 * use configuration slot 0 for the OFFSET/GAIN registers.
> +		 */
> +		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(0), 3,
>  				     &st-
> >channels[i].cfg.calibration_offset);
>  		if (ret < 0)
>  			return ret;
> 
> ---
> base-commit: 411e8b72c181e4f49352c12ced0fd8426eb683aa
> change-id: 20250917-iio-adc-ad7124-change-setup-reg-allocation-strategy-
> cd1f6bcd8415
> 
> Best regards,

Powered by blists - more mailing lists