Re: [PATCH v6 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] Inconsistent IIO UAPI and incorrect attribute scoping for oversampling ratio.
- [Medium] Software cache of the oversampling ratio is out of sync with hardware state.
--

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

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 20fd3a87d448e..fa8f0dc868af2 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

[ ... ]

> @@ -857,7 +996,11 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
>  			.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]
Does this inconsistently expose the oversampling attribute?

The commit adds BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) to
info_mask_shared_by_type for the dynamic satellite temperature channels here,
but it appears to be omitted from the statically defined core temperature
channels (temp_channels_no_events and temp_channels_with_events).

Because of IIO's attribute merging behavior, if no satellite channels are
defined in the Device Tree, the in_temp_oversampling_ratio attribute might
not be created at all.

Furthermore, if satellite channels are present, the attribute is created and
appears to apply to all in_temp channels, including the core sensor. However,
the hardware register configured (SYSMON_CONFIG_TEMP_SAT_OSR) seems to
indicate oversampling only applies to the satellite sensors.

Could this create an inconsistent UAPI that either hides the feature when no
satellites are present, or misleadingly claims it applies to the core
temperature sensor?

[ ... ]

> @@ -904,6 +1047,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]
Are these software cache variables out of sync with the actual hardware state?

The driver initializes the software cache to 1 (no averaging) here, but it
does not appear to read the actual hardware configuration from the
SYSMON_CONFIG register, nor does it explicitly write this default state of 1
to the hardware.

If the device was previously configured by a bootloader, firmware, or a prior
kernel to use a different oversampling ratio, will the sysfs attribute
incorrectly report 1 while the hardware actively averages multiple samples?

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