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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iio: adc: stm32: missing wait for ADSTART clear after ADSTP
--

commit a66fdb4df492a7cd737ae8e8fbfa41958a5d15b4
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

    iio: adc: stm32-adc: add support for stm32mp25
    
    Add support for ADC on STM32MP25 SoC. It has 3 ADCs, split into two blocks:
    - ADC1 & ADC2 are tightly coupled.
    - ADC3 is managed independently.

[ ... ]

> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index e3719b29dcb1e..e7c927d1a73b3 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c

[ ... ]

> @@ -1304,6 +1440,211 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
>  	stm32h7_adc_enter_pwr_down(adc);
>  }
>  
> +/*
> + * STM32MP25 offset calibration software procedure. Basically the calibration routine is
> + * expected to average (for example) 8 samples in calibration mode, for single-ended  and
> + * differential channels, to calibrate the zero offset. In case offset is "negative", an
> + * additional offset can be added, to determine calibration factor. It must be kept later
> + * for all conversions.
> + */
> +static int stm32mp25_adc_calib_get_average_data(struct iio_dev *indio_dev, u32 *average)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	u32 avg = 0;
> +
> +	/* Repeat several conversions in calibration mode, average the results */
> +	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) {
> +			stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTP);
> +			dev_err(&indio_dev->dev, "calibration average error %d\n", ret);
> +			return ret;

[Severity: Medium]
When setting ADSTP to abort the conversion, do we need to wait for ADSTART to
clear before returning?

If a calibration conversion times out in stm32mp25_adc_calib_get_average_data(),
the code sets ADSTP but does not wait for the hardware to clear ADSTART.
When the error is returned, stm32h7_adc_disable() is invoked on the cleanup
path.

Because ADSTART may still be active, the hardware might ignore the ADDIS
command. The disable routine can time out, and the device is subsequently sent
a power-down command (DEEPPWD) while still enabled. This could violate the
documented power-down sequence and permanently wedge the IP block.

> +		}
> +
> +		val = stm32_adc_readl(adc, adc->cfg->regs->dr);
> +		dev_vdbg(&indio_dev->dev, "dr[%d]=0x%08x\n", i, val);
> +		avg += val;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-adc-stm32mp25-v1-v4-0-d31ec75aecf3@foss.st.com?part=8