Re: [PATCH v2 07/14] 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:
- [High] iio: adc: stm32-adc: DIFSEL and CFGR writes fail silently when ADEN=1
--

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

iio: adc: stm32-adc: add support for stm32mp25

This commit adds support for the ADC on the STM32MP25 SoC. It introduces new
hardware configurations, trigger lists, and a dedicated software calibration
procedure for the new SoC.

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

[ ... ]

> @@ -1304,6 +1437,184 @@ 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

[ ... ]

> +static int stm32mp25_adc_calib(struct iio_dev *indio_dev)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	bool add_offset = false;
> +	bool diff_below_zero;
> +	u32 average, calfact;
> +	int ret;
> +
> +	stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADCAL);
> +	/* Use default resolution (e.g. 12 bits) */
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32MP25_RES_MASK);

[Severity: High]
Does this clear the resolution bits correctly?

Since stm32h7_adc_enable() was already called in stm32mp25_adc_prepare(), ADEN
is set to 1. On STM32 ADCs, configuration registers like CFGR are locked by
hardware when the ADC is enabled, so this write might be silently ignored.

This could cause calibration to run at the wrong resolution if a non-default
resolution was configured by the user.

[ ... ]

> +static int stm32mp25_adc_prepare(struct iio_dev *indio_dev)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = stm32h7_adc_exit_pwr_down(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* 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 this configuration of differential channels be silently ignored?

The ADC is already enabled (ADEN=1) from the earlier call to
stm32h7_adc_enable(). Hardware locking prevents writes to DIFSEL while the
ADC is enabled, which would leave the channels configured as single-ended.

> +
> +	if (adc->cfg->has_presel)
> +		stm32_adc_writel(adc, STM32H7_ADC_PCSEL, adc->pcsel);
> +
> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-adc-stm32mp25-v1-v2-0-46bc019537c6@foss.st.com?part=7