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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 27 May 2026 12:42:11 +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>
Really minor stuff from a reread

> +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_TEMP_SAT_CONFIG_MASK,
> +					FIELD_PREP(SYSMON_TEMP_SAT_CONFIG_MASK,
> +						   hw_val));
> +		if (ret)
> +			return ret;
blank line
> +		ret = sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
> +					    SYSMON_TEMP_EN_AVG_COUNT,
> +					    hw_val ? ~0U : 0);
> +		if (ret)
> +			return ret;
		return sysmon_set...

> +	} else if (channel_type == IIO_VOLTAGE) {
Won't need the else if returned already.
> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_SUPPLY_CONFIG_MASK,
> +					FIELD_PREP(SYSMON_SUPPLY_CONFIG_MASK,
> +						   hw_val));
> +		if (ret)
> +			return ret;
blank line
> +		ret = sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
> +					    SYSMON_SUPPLY_EN_AVG_COUNT,
> +					    hw_val ? ~0U : 0);
> +		if (ret)
> +			return ret;
		return sysmon_set...

> +	} else {
No else needed here either

> +		return -EINVAL;
> +	}
> +
> +	return 0
And this isn't needed at all.

> +}

> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> index a78362f95e6..cf69be62709 100644
> --- a/drivers/iio/adc/versal-sysmon.h
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -25,11 +25,13 @@ struct regmap;
>  #define SYSMON_IMR			0x0048
>  #define SYSMON_IER			0x004C
>  #define SYSMON_IDR			0x0050
> +#define SYSMON_CONFIG			0x0100
>  #define SYSMON_TEMP_MAX			0x1030
>  #define SYSMON_TEMP_MIN			0x1034
>  #define SYSMON_SUPPLY_BASE		0x1040
>  #define SYSMON_ALARM_FLAG		0x1018
>  #define SYSMON_ALARM_REG		0x1940
> +#define SYSMON_SUPPLY_EN_AVG_BASE	0x1958
>  #define SYSMON_TEMP_TH_LOW		0x1970
>  #define SYSMON_TEMP_TH_UP		0x1974
>  #define SYSMON_OT_TH_LOW		0x1978
> @@ -41,6 +43,7 @@ struct regmap;
>  #define SYSMON_TEMP_MAX_MAX		0x1F90
>  #define SYSMON_STATUS_RESET		0x1F94
>  #define SYSMON_TEMP_SAT_BASE		0x1FAC
> +#define SYSMON_TEMP_EN_AVG_BASE		0x24B4
>  #define SYSMON_MAX_REG			0x24C0
>  
>  /* NPI unlock value written to SYSMON_NPI_LOCK */
> @@ -57,6 +60,16 @@ struct regmap;
>  /* ISR/IMR temperature and OT alarm mask (bits 9:8) */
>  #define SYSMON_TEMP_INTR_MASK		GENMASK(9, 8)
>  
> +/* Config register: supply oversampling field (bits 17:14) */
> +#define SYSMON_SUPPLY_CONFIG_MASK	GENMASK(17, 14)
> +
> +/* Config register: temp satellite oversampling field (bits 27:24) */

I missed this before, but given the GENMASK just below the bits part
of these comments is pointles. Drop it.

Ideally also name them in a way that makes it clear what register
they are fields of.

> +#define SYSMON_TEMP_SAT_CONFIG_MASK	GENMASK(27, 24)