Re: [PATCH v3 06/14] iio: adc: ad7768: Add configurable sampling modes

[Jonathan Cameron]

On Thu, 13 Aug 2026 15:56:59 +0200
Janani Sunil <janani.sunil@xxxxxxxxxx> wrote:

> Derive the available output data rates from MCLK and expose per-channel
> sampling frequency and filter controls.
> 
> Select the fastest compatible power mode for the enabled channels and
> map matching sampling frequency and filter combinations onto the two
> hardware channel profiles. Configure the data clock divider and wait for
> the selected filters to settle before capture.
> 
> Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>

A few things inline,

thanks,

Jonathan


> +static int ad7768_find_matching_mode(const bool *mode_used,
> +				     const unsigned int *mode_freq,
> +				     const enum ad7768_filter_type *mode_filter,
> +				     unsigned int freq,
> +				     enum ad7768_filter_type filter)
> +{
> +	unsigned int mode;
> +
> +	for (mode = 0; mode < AD7768_NUM_CHANNEL_MODES; mode++) {
	for (unsigned int mode = 0; ...

Look for other remaining cases of this as reviewers may only comment
on a few and expect you to carry the feedback through the whole series.

> +		if (!mode_used[mode] ||
> +		    (mode_freq[mode] == freq && mode_filter[mode] == filter))
> +			return mode;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7768_apply_channel_modes(struct iio_dev *indio_dev,
> +				      const unsigned long *scan_mask)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned int mode_freq[AD7768_NUM_CHANNEL_MODES];
> +	enum ad7768_filter_type mode_filter[AD7768_NUM_CHANNEL_MODES];
> +	bool mode_used[AD7768_NUM_CHANNEL_MODES] = { };
>  	unsigned int channel_mask;
>  	unsigned int standby_mask;
> +	unsigned int max_freq = 0;
>  	unsigned int c;
> -	int ret;
> +	int mode, ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = ad7768_set_lowest_noise_mode(st, scan_mask);
> +	if (ret == -EINVAL)
> +		return dev_err_probe(regmap_get_device(st->regmap), ret,
> +				     "No power mode supports all enabled channel frequencies\n");
> +	if (ret)
> +		return ret;
>  
>  	channel_mask = ad7768_all_channels_mask(st);
>  	standby_mask = channel_mask;
> +
>  	for (c = 0; c < st->chip_info->num_channels; c++) {
> -		if (test_bit(c, scan_mask))
> -			standby_mask &= ~ad7768_channel_mask(st, c);
> +		unsigned int mask;
> +
> +		if (!test_bit(c, scan_mask))
> +			continue;

	for_each_set_bit()?

> +static int ad7768_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(regmap_get_device(st->regmap), pm);

Why the IF_ENABLED variant?  I'd expect the driver to work fine without
the runtime PM stuff being enabled. Normally that just means leaving the power
turned on in probe.  

> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	if (info == IIO_CHAN_INFO_SAMP_FREQ)
> +		return ad7768_set_sampling_freq(indio_dev, val, chan->channel);
> +
> +	return -EINVAL;
> +}
> +
> +static struct iio_chan_spec_ext_info ad7768_ext_info[] = {
> +	IIO_ENUM("filter_type", IIO_SEPARATE,
> +		 &ad7768_filter_types_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SEPARATE, &ad7768_filter_types_enum),
> +	{}

	{ } is the style choice I'm trying to push through IIO.


> +};