Re: [PATCH v3 08/15] iio: adc: stm32-adc: add support for stm32mp25

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: adc: stm32-adc: DIFSEL write ignored while ADC is enabled
- [Medium] iio: adc: stm32-adc: incorrect internal channel sampling time for mp25
- [Medium] iio: adc: stm32-adc: missing ADSTP before ADDIS on calibration timeout
- [Low] iio: adc: stm32-adc: implicit subtraction via mask truncation in calibration
--

commit 58980c1da40c7a3d68e78bec0a436a1487c49820
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Subject: iio: adc: stm32-adc: add support for stm32mp25

This commit adds support for the ADC on the STM32MP25 SoC, which features
three ADCs split into two blocks. It introduces diversity regarding IRQs,
clocks, software calibration, internal voltage channels, sampling time,
and trigger lists. A dedicated compatible is used to handle these
differences relative to other STM32 ADCs.

> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index c1979f1f17385..33e41505b2e99 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
[ ... ]
> +static int stm32mp25_adc_calib_get_average_data(struct iio_dev *indio_dev, u32 *average)
> +{
[ ... ]
> +	for (unsigned int i = 0; i < STM32MP25_CALIB_LOOP; i++) {
> +		int ret;
> +		u32 val;
> +
> +		stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
> +		ret = stm32_adc_readl_poll_timeout(STM32H7_ADC_CR, val,
> +						   !(val & (STM32H7_ADSTART)),
> +						   100, STM32_ADC_TIMEOUT_US);
> +		if (ret) {
> +			dev_err(&indio_dev->dev, "calibration average error %d\n", ret);
> +			return ret;
> +		}

[Severity: Medium]
If the hardware takes too long and triggers the polling timeout here,
will the ADC become permanently wedged?

When this times out, ADSTART remains 1. The error path bubbles up to
stm32mp25_adc_prepare() and branches to the adc_dis label, calling
stm32h7_adc_disable(). Since ADSTART is still set, attempting to set ADDIS
without first stopping the conversion (by setting ADSTP) causes the hardware
to ignore the ADDIS command.

The polling loop in stm32h7_adc_disable() will then time out as well,
leaving the hardware stuck and unable to be re-enabled later.

[ ... ]
> +static int stm32mp25_adc_calib_differential(struct iio_dev *indio_dev, u32 *calfact)
> +{
[ ... ]
> +	if (!diff_below_zero) {
> +		*calfact |= FIELD_PREP(STM32MP25_CALFACT_D_MASK, average);
> +		stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, *calfact);

[Severity: Low]
Can passing the raw average into FIELD_PREP here result in silent data
corruption?

The average could be >= 0x800. This relies on the 9-bit mask
STM32MP25_CALFACT_D_MASK to implicitly truncate the 12-bit half-scale
offset out of the value.

While functionally this might achieve the expected subtraction for small
offsets, does this violate FIELD_PREP's assumptions and risk static analysis
warnings or runtime assertions?

[ ... ]
> +static int stm32mp25_adc_prepare(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/* Must enable the ADC before running software-assisted calibration */
> +	ret = stm32h7_adc_enable(indio_dev);
> +	if (ret)
> +		goto pwr_dwn;
> +
> +	/* Always run offset calibration */
> +	ret = stm32mp25_adc_calib(indio_dev);
> +	if (ret)
> +		goto adc_dis;
> +
> +	stm32_adc_int_ch_enable(indio_dev);
> +
> +	stm32_adc_writel(adc, adc->cfg->regs->difsel.reg, adc->difsel);

[Severity: High]
Will writing to the DIFSEL register be ignored by the hardware at this point?

stm32h7_adc_enable() sets ADEN=1, enabling the ADC. The STM32 hardware
write-protects the DIFSEL register while the ADC is enabled.

Does writing DIFSEL after enabling the ADC cause the configuration to be
silently dropped, forcing differential channels to fall back to
single-ended mode and corrupting measurements?

[ ... ]
> +static const unsigned int stm32_adc_min_ts_mp25[STM32_ADC_INT_CH_NB] = {
> +	[STM32_ADC_INT_CH_VDDCORE] = 34,
> +	[STM32_ADC_INT_CH_VDDCPU] = 34,
> +	[STM32_ADC_INT_CH_VREFINT] = 34,
> +	[STM32_ADC_INT_CH_VBAT] = 34,
> +};

[Severity: Medium]
Are these stabilization time values correctly specified in nanoseconds?

This array specifies 34 ns, which causes the driver to select the absolute
minimum sampling time (e.g. 2 clock cycles). Internal channels like VREFINT
and VBAT usually require multiple microseconds to stabilize due to high
source impedance.

Were these values accidentally provided in clock cycles (e.g., 3.4 us at
10 MHz) instead of nanoseconds, leading to highly inaccurate internal voltage
readings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-adc-stm32mp25-v1-v3-0-406834430d72@foss.st.com?part=8