Re: [PATCH v8 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] Static temperature channels are missing the oversampling ratio info mask, rendering it unavailable if no dynamic temperature satellites are present.
- [Medium] The driver's cached oversampling state is initialized to 1 without synchronizing with the hardware state.
--

commit 81f60069d7c6ab5d01862c8402ee14a0ef2ad23e
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 8d567f9fa05d7..309c39b515f6d 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

[ ... ]

> @@ -787,7 +932,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]
Since the dynamically parsed temperature channels get the oversampling ratio
masks here, should the static temperature channels defined via
SYSMON_CHAN_TEMP also be updated to include these masks?

If no dynamic temperature satellites are present, the static temperature
channels won't expose the in_temp_oversampling_ratio sysfs attribute,
preventing oversampling configuration for those default channels.

[ ... ]

> @@ -834,6 +983,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]
Are these initial cached oversampling values guaranteed to match the hardware
state?

If a bootloader or a previous kernel instance left the SysMon configured with
a higher oversampling ratio, the IIO sysfs attribute will incorrectly report
1x oversampling while the hardware actively operates at the previous setting.

Should the driver read SYSMON_CONFIG to initialize this cache, or explicitly
write the default 1x value to the hardware to ensure the state is synchronized?

>  
>  	ret = devm_mutex_init(dev, &sysmon->lock);
>  	if (ret)

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