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

["Erim, Salih" <salih.erim@amd.com>]

Addressing Sashiko findings on this patch:

- [Medium] Temperature oversampling sysfs attribute is hidden if
   no satellite channels are defined.

   Agreed. Will add oversampling info_mask bits to the static
   temperature channel macro in v11.

- [Medium] Hardware and software state desynchronization at
   initialization for oversampling ratios.

   Not a bug. The driver does not own the hardware state at probe
   time, writing a default could interfere with bootloader (PLM)
   configuration. The cache initializes to 1 (no averaging), which
   matches the hardware reset default. Discussed and dismissed in
   v6 review.

Thanks,
Salih

On 18/06/2026 11:14, 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
> be updated to activate or deactivate averaging.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> ---
> Changes in v10:
>    - No code changes
> 
> Changes in v9:
>    - Add Reviewed-by tag from Andy Shevchenko
>    - No code changes
> 
> Changes in v8:
>    - Use unsigned int for val parameter and hw_val in both
>      osr_write helpers (Andy)
>    - Use ~0 instead of ~0U for avg enable bitmask (Andy)
> 
> Changes in v7:
>    - Split sysmon_osr_write into sysmon_osr_write_temp and
>      sysmon_osr_write_supply; caller dispatches with if/else
>      on chan->type (Jonathan)
>    - Restore HW encoding comment in both helpers; fix
>      cross-reference in sysmon_osr_write_supply
> 
> Changes in v6:
>    - Fix FIELD_PREP indentation in sysmon_osr_write (Andy)
>    - unsigned int for loop index in sysmon_write_raw (Andy)
> 
> Changes in v5:
>    - Remove unneeded parentheses in i * SYSMON_REG_STRIDE (Andy)
>    - Use struct regmap *map local variable in
>      sysmon_set_avg_enable (Andy)
>    - switch instead of redundant if/if on channel_type (Andy)
>    - Add CONFIG register readback fence after oversampling update
>      to prevent NoC bus hang from posted writes (found during
>      hardware stress testing)
> 
> Changes in v4:
>    - Return directly from sysmon_set_avg_enable calls, remove
>      else after early returns, drop unreachable return 0 (Jonathan)
>    - Rename mask defines to SYSMON_CONFIG_SUPPLY_OSR and
>      SYSMON_CONFIG_TEMP_SAT_OSR (Jonathan)
>    - Drop "bits X:Y" from GENMASK comments (Jonathan)
>    - Blank lines after if (ret) return ret blocks (Jonathan)
>    - Move oversampling read inside guard(mutex) scope
> 
> Changes in v3:
>    - No changes
> 
> Changes in v2:
>    - EN_AVG per-channel bitmask registers written with all-ones
>      instead of boolean 1 when oversampling is enabled
>    - EN_AVG write errors propagated to userspace
>    - Oversampling limited to satellite temp and supply channels;
>      static temp channels do not participate
>    - Oversampling exposes actual sample counts (1,2,4,8,16) to
>      userspace with internal HW register translation
>    - write_raw_get_fmt returns IIO_VAL_INT for oversampling ratio
>    - HW encoding documented (sample_count/2, not log2)
>    - oversampling_avail is const int[] (type match fix)
>   drivers/iio/adc/versal-sysmon-core.c | 153 ++++++++++++++++++++++++++-
>   drivers/iio/adc/versal-sysmon.h      |  17 +++
>   2 files changed, 169 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 50b5228aa22..bae229f27c6 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -28,6 +28,12 @@
>   
>   #include "versal-sysmon.h"
>   
> +/*
> + * Oversampling ratio values exposed to userspace via IIO.
> + * Actual number of samples averaged: 1=none, 2=2x, 4=4x, 8=8x, 16=16x.
> + */
> +static const int sysmon_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> +
>   /* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
>   #define SYSMON_TEMP_HYST_MASK		BIT(1)
>   
> @@ -166,6 +172,12 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
>   
>   	guard(mutex)(&sysmon->lock);
>   
> +	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
> +		*val = (chan->type == IIO_TEMP) ? sysmon->temp_oversampling :
> +						 sysmon->supply_oversampling;
> +		return IIO_VAL_INT;
> +	}
> +
>   	switch (chan->type) {
>   	case IIO_TEMP:
>   		if (mask == IIO_CHAN_INFO_SCALE) {
> @@ -452,6 +464,132 @@ static int sysmon_write_event_value(struct iio_dev *indio_dev,
>   	}
>   }
>   
> +static int sysmon_set_avg_enable(struct sysmon *sysmon,
> +				 u32 base, u32 count, u32 val)
> +{
> +	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;
> +}
> +
> +static int sysmon_osr_write_temp(struct sysmon *sysmon, unsigned int val)
> +{
> +	/*
> +	 * HW register encoding is sample_count / 2:
> +	 * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
> +	 */
> +	unsigned int hw_val = val >> 1;
> +	unsigned int readback;
> +	int ret;
> +
> +	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 ? ~0 : 0);
> +}
> +
> +static int sysmon_osr_write_supply(struct sysmon *sysmon, unsigned int val)
> +{
> +	/* HW encoding: sample_count / 2 (see sysmon_osr_write_temp) */
> +	unsigned int hw_val = val >> 1;
> +	unsigned int readback;
> +	int ret;
> +
> +	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 sysmon_osr_write_temp for details */
> +	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 ? ~0 : 0);
> +}
> +
> +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);
> +
> +	if (chan->type == IIO_TEMP) {
> +		ret = sysmon_osr_write_temp(sysmon, val);
> +		if (ret)
> +			return ret;
> +		sysmon->temp_oversampling = val;
> +	} else {
> +		ret = sysmon_osr_write_supply(sysmon, val);
> +		if (ret)
> +			return ret;
> +		sysmon->supply_oversampling = val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sysmon_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    long mask)
> +{
> +	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO)
> +		return IIO_VAL_INT;
> +
> +	return -EINVAL;
> +}
> +
> +static int sysmon_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
> +		return -EINVAL;
> +
> +	*vals = sysmon_oversampling_avail;
> +	*type = IIO_VAL_INT;
> +	*length = ARRAY_SIZE(sysmon_oversampling_avail);
> +
> +	return IIO_AVAIL_LIST;
> +}
> +
>   static int sysmon_read_label(struct iio_dev *indio_dev,
>   			     struct iio_chan_spec const *chan,
>   			     char *label)
> @@ -464,6 +602,9 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
>   
>   static const struct iio_info sysmon_iio_info = {
>   	.read_raw = sysmon_read_raw,
> +	.write_raw = sysmon_write_raw,
> +	.write_raw_get_fmt = sysmon_write_raw_get_fmt,
> +	.read_avail = sysmon_read_avail,
>   	.read_label = sysmon_read_label,
>   	.read_event_config = sysmon_read_event_config,
>   	.write_event_config = sysmon_write_event_config,
> @@ -755,6 +896,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
>   			.indexed = 1,
>   			.address = reg,
>   			.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +			.info_mask_shared_by_type =
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +			.info_mask_shared_by_type_available =
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>   			.event_spec = irq > 0 ?
>   				sysmon_supply_events : NULL,
>   			.num_event_specs = irq > 0 ?
> @@ -786,7 +931,11 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
>   			.address = SYSMON_TEMP_SAT_BASE +
>   				   (reg - 1) * SYSMON_REG_STRIDE,
>   			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +			.info_mask_shared_by_type =
> +				BIT(IIO_CHAN_INFO_SCALE) |
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +			.info_mask_shared_by_type_available =
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>   			.datasheet_name = label,
>   		};
>   	}
> @@ -833,6 +982,8 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
>   
>   	sysmon = iio_priv(indio_dev);
>   	sysmon->regmap = regmap;
> +	sysmon->temp_oversampling = 1;
> +	sysmon->supply_oversampling = 1;
>   
>   	ret = devm_mutex_init(dev, &sysmon->lock);
>   	if (ret)
> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> index 9fe2793757a..bb9a75bf71c 100644
> --- a/drivers/iio/adc/versal-sysmon.h
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -23,11 +23,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_SUPPLY_TH_LOW		0x1980
> @@ -37,6 +39,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 */
> @@ -53,6 +56,16 @@ struct regmap;
>   /* ISR/IMR temperature alarm mask (bit 9) */
>   #define SYSMON_TEMP_INTR_MASK		BIT(9)
>   
> +/* SYSMON_CONFIG: supply oversampling ratio */
> +#define SYSMON_CONFIG_SUPPLY_OSR	GENMASK(17, 14)
> +
> +/* SYSMON_CONFIG: temperature satellite oversampling ratio */
> +#define SYSMON_CONFIG_TEMP_SAT_OSR	GENMASK(27, 24)
> +
> +/* Per-channel averaging enable register counts */
> +#define SYSMON_SUPPLY_EN_AVG_COUNT	5
> +#define SYSMON_TEMP_EN_AVG_COUNT	2
> +
>   /* Supply voltage conversion register fields */
>   #define SYSMON_MANTISSA_MASK		GENMASK(15, 0)
>   #define SYSMON_FMT_MASK			BIT(16)
> @@ -77,6 +90,8 @@ struct regmap;
>    * @temp_mask: temperature interrupt configuration mask
>    * @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
>    * @sysmon_unmask_work: re-enables events after alarm condition clears
> + * @temp_oversampling: current temp oversampling ratio
> + * @supply_oversampling: current supply oversampling ratio
>    */
>   struct sysmon {
>   	struct regmap *regmap;
> @@ -96,6 +111,8 @@ struct sysmon {
>   	unsigned int temp_mask;
>   	int temp_hysteresis;
>   	struct delayed_work sysmon_unmask_work;
> +	unsigned int temp_oversampling;
> +	unsigned int supply_oversampling;
>   };
>   
>   int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap);