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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware state desync for oversampling ratios.
- [Medium] Inconsistent IIO ABI for temperature oversampling due to misuse of info_mask_shared_by_type.
--

commit a087d8379a588d07c739e642f022da0f04e19054
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add oversampling support

This commit adds oversampling support to the versal-sysmon driver, enabling
averaging of 2, 4, 8, or 16 samples. It exposes the configuration through the
IIO oversampling_ratio attribute, shared by channel type.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index e87974c641a16..8a8ca4506cbaa 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -842,7 +970,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
>  				   ((reg - 1) * SYSMON_REG_STRIDE),
>  			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  			.info_mask_shared_by_type =
> -				BIT(IIO_CHAN_INFO_SCALE),
> +				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,
>  		};
>  	}

[Severity: Medium]
Will this lead to an inconsistent IIO ABI for temperature oversampling?

The patch adds BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) to
info_mask_shared_by_type for dynamically allocated satellite temperature
sensors, but it appears to be omitted for static core temperature sensors.

In IIO, shared_by_type creates a single attribute (in_temp_oversampling_ratio)
that userspace expects to apply to all channels of that type.

If the hardware's SYSMON_CONFIG_TEMP_SAT_OSR and 64-bit enable mask only affect
satellite sensors, using shared_by_type might violate IIO ABI by falsely
advertising oversampling for the core sensor when satellites are present.

Conversely, if the configuration affects all temperature sensors, omitting the
mask from the core sensors means systems without satellites cannot configure
oversampling, as the attribute will not be created.

[ ... ]
> @@ -890,6 +1021,8 @@ int 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;

[Severity: Medium]
Could this result in a hardware state desynchronization for the oversampling
ratios?

The software cache is unconditionally initialized to 1 without reading the
current hardware state from SYSMON_CONFIG or explicitly writing the default
value to the hardware registers.

If the device was previously configured with a different oversampling ratio by
a bootloader, firmware, or a prior kernel, the hardware will operate at that
ratio while the driver incorrectly reports 1 to userspace via read_raw.

Should the driver read the current state during probe or explicitly write 1
to the registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606051707.535281-1-salih.erim@amd.com?part=5