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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Jun 06, 2026 at 06:17:07AM +0100, Salih Erim 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

...

> +static int sysmon_set_avg_enable(struct sysmon *sysmon,
> +				 u32 base, u32 count, u32 val)
> +{
> +	int ret;
> +
> +	for (unsigned int i = 0; i < count; i++) {
> +		ret = regmap_write(sysmon->regmap,
> +				   base + (i * SYSMON_REG_STRIDE), val);

Unneeded parentheses.

> +		if (ret)
> +			return ret;
> +	}

Also you can use temporary for regmap

	struct regmap *map = sysmon->regmap;
	int ret;

	for (unsigned int i = 0; i < count; i++) {
		ret = regmap_write(map, base + i * SYSMON_REG_STRIDE, val);
		if (ret)
			return ret;
	}

> +	return 0;
> +}

And use this trick in other places where appropriate, it makes code easier
to read.

...

> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
> +{
> +	/*
> +	 * 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;
> +	int ret;
> +
> +	if (channel_type == 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;
> +
> +		return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
> +					     SYSMON_TEMP_EN_AVG_COUNT,
> +					     hw_val ? ~0U : 0);

> +	}
> +
> +	if (channel_type == IIO_VOLTAGE) {

Can channel_type be both TEMP and VOLTAGE here? No. Why do we check it twice?

> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_CONFIG_SUPPLY_OSR,
> +					FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR,
> +						   hw_val));
> +		if (ret)
> +			return ret;
> +
> +		return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
> +					     SYSMON_SUPPLY_EN_AVG_COUNT,
> +					     hw_val ? ~0U : 0);
> +	}
> +	return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko