Re: [PATCH v10 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 and software state desynchronization at initialization for oversampling ratios.
- [Medium] Temperature oversampling sysfs attribute is hidden if no satellite channels are defined.
--

commit d8019119cd29dc9ea31ce2a445e3e2be9d478438
Author: Salih Erim <salih.erim@amd.com>
Subject: iio: adc: versal-sysmon: add oversampling support

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).

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 50b5228aa222f..bae229f27c6b3 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -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,
>  		};
>  	}

[Severity: Medium]
This successfully exposes the oversampling attribute for dynamic satellite
temperature channels, but does it miss updating the static temperature channels
defined via SYSMON_CHAN_TEMP earlier in the file?

If a device has no dynamic temperature satellite channels specified in the
Device Tree, will the static IIO_TEMP channels lack the
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) flag, thereby preventing the IIO core
from creating the in_temp_oversampling_ratio sysfs attribute entirely?

[ ... ]
> @@ -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;

[Severity: Medium]
Does initializing the software cache to 1 here without writing to the
hardware register or reading its initial state risk a desynchronization?

If the bootloader or a previous kernel configured a different oversampling
ratio (e.g., 16x), will the hardware perform averaging while userspace
reads a ratio of 1 until explicitly updated?

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