Re: [PATCH v6 5/5] iio: adc: versal-sysmon: add oversampling support

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 11 Jun 2026 23:27:38 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add support for reading and writing the oversampling ratio through
> the IIO oversampling_ratio attribute. The hardware supports averaging
> 2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
> 
> Temperature and supply channels share oversampling configuration at
> the type level (all temperature channels share one ratio, all supply
> channels share another), exposed through info_mask_shared_by_type.
> 
> The hardware encoding uses sample_count / 2 in a 4-bit field within
> the CONFIG register. Per-channel averaging enable registers must also
> be updated to activate or deactivate averaging.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>

One minor comment inline.

>  drivers/iio/adc/versal-sysmon-core.c | 147 ++++++++++++++++++++++++++-
>  drivers/iio/adc/versal-sysmon.h      |  17 ++++
>  2 files changed, 163 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 20fd3a87d44..fa8f0dc868a 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)

There is almost nothing shared in here between the two channel_types.
Might make sense to just split it into two helpers, particularly as
there is a channel type if / else at the caller.

> +{
> +	/*
> +	 * HW register encoding is sample_count / 2:
> +	 * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
> +	 */
> +	int hw_val = val >> 1;
> +	unsigned int readback;
> +	int ret;
> +
> +	switch (channel_type) {
> +	case IIO_TEMP:
> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_CONFIG_TEMP_SAT_OSR,
> +					FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR, hw_val));
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Readback fence: the SysMon CONFIG register resides in the
> +		 * PMC domain behind the NoC. A posted write may not reach the
> +		 * hardware before the next MMIO access. Reading the register
> +		 * back forces the interconnect to complete the write, preventing
> +		 * a bus hang on the subsequent access.
> +		 */
> +		regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
> +
> +		return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
> +					     SYSMON_TEMP_EN_AVG_COUNT,
> +					     hw_val ? ~0U : 0);
> +	case IIO_VOLTAGE:
> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_CONFIG_SUPPLY_OSR,
> +					FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR, hw_val));
> +		if (ret)
> +			return ret;
> +
> +		/* Readback fence -- see above */
> +		regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
> +
> +		return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
> +					     SYSMON_SUPPLY_EN_AVG_COUNT,
> +					     hw_val ? ~0U : 0);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int sysmon_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int i;
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(sysmon_oversampling_avail); i++) {
> +		if (val == sysmon_oversampling_avail[i])
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(sysmon_oversampling_avail))
> +		return -EINVAL;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	ret = sysmon_osr_write(sysmon, chan->type, val);
> +	if (ret)
> +		return ret;
> +
> +	if (chan->type == IIO_TEMP)
> +		sysmon->temp_oversampling = val;
> +	else
> +		sysmon->supply_oversampling = val;
> +
> +	return 0;
> +}