Re: [PATCH 3/8] iio: adc: stm32-adc: add support for stm32mp23 and stm32mp25

[Andy Shevchenko]

On Fri, Sep 11, 2026 at 05:51:25PM +0200, Fabrice Gasnier wrote:
> Add support for STM32MP23 STM32MP25 SoCs. They have 3 ADCs, split into
> two blocks:
> - ADC12 (ADC1 & ADC2) are tightly coupled.
> - ADC3 is managed independently.
> Trigger list slightly changes between STM32MP23 and STM32MP25.
> 
> Each ADC is a 12-bits successive approximation analog-to-digital converter,
> with up to 20 multiplexed channels that can be configured as single ended
> or differential. ADC resolution ranges from 6 to 12 bits.
> 
> It introduces diversity regarding IRQs, clocks, software calibration
> procedure, internal voltage channels, sampling time (prescaler) and
> trigger list. Most of the architecture, and the driver engine remains
> similar. So, handle the differences w.r.t. other STM32 ADCs family with
> a dedicated compatible and compatible data.
> 
> Trigger list depends on ADC instance, retrieved with "st,adc-trigger-sel"
> property.
> 
> Add support for vddcpu and vddgpu internal channels on stm32mp25.
> These channels are enabled by using ADC option register.
> Add VDDGPU to the list of supported channels in this driver and made
> available on stm32mp25. So update sampling information for all variants
> (with zero'ed data when it's not available).

...

> + * @presc:	clock prescaler array
> + * @num_presc:	number of clock prescalers in presc array

> +	int *presc;
> +	int num_presc;

Why are they signed? I do not see any justification in the kernel doc.

...

>  static const struct stm32_adc_priv_cfg stm32f4_adc_priv_cfg = {
>  	.regs = &stm32f4_adc_common_regs,
> -	.clk_sel = stm32f4_adc_clk_sel,
> +	.clk_sel = stm32_adc_clk_sel,
> +	.presc = stm32f4_pclk_div,
> +	.num_presc = ARRAY_SIZE(stm32f4_pclk_div),
>  	.max_clk_rate_hz = 36000000,
>  	.num_irqs = 1,
>  	.num_adcs = 3,

>  };

> +static const struct stm32_adc_priv_cfg stm32mp25_adc_priv_cfg = {
> +	.regs = &stm32mp25_adc_common_regs,
> +	.clk_sel = stm32_adc_clk_sel,
> +	.presc = stm32mp25_presc_div,
> +	.num_presc = ARRAY_SIZE(stm32mp25_presc_div),
> +	.max_clk_rate_hz = 70000000,
> +	.ipid = STM32MP25_IPIDR_NUMBER,
> +	.num_irqs = 2, /* 2 IRQs for ADC12, 1 irq for ADC3 */
> +};

Split this patch to two. One prepares the infrastructure for prescaller, and
the other adds a new HW support.

>  static const struct of_device_id stm32_adc_of_match[] = {
>  	{
>  		.compatible = "st,stm32f4-adc-core",
> @@ -895,6 +948,12 @@ static const struct of_device_id stm32_adc_of_match[] = {
>  	}, {
>  		.compatible = "st,stm32mp13-adc-core",
>  		.data = (void *)&stm32mp13_adc_priv_cfg
> +	}, {
> +		.compatible = "st,stm32mp23-adc-core",
> +		.data = (void *)&stm32mp25_adc_priv_cfg
> +	}, {
> +		.compatible = "st,stm32mp25-adc-core",
> +		.data = (void *)&stm32mp25_adc_priv_cfg
>  	}, {
>  	},
>  };

...

>  struct stm32_adc_cfg {
>  	const struct stm32_adc_regspec	*regs;
>  	const struct stm32_adc_info	*adc_info;

> -	const struct stm32_adc_trig_info *trigs;
> +	const struct stm32_adc_trig_info	*trigs[2];

Indentation looks suspicious. I would use just a single space.

>  	bool clk_required;
>  	bool has_vregready;
>  	bool has_boostmode;
>  	bool has_linearcal;
>  	bool has_presel;
>  	bool has_oversampling;
> +	bool has_vregen;
>  	int (*prepare)(struct iio_dev *);
>  	void (*start_conv)(struct iio_dev *, bool dma);
>  	void (*stop_conv)(struct iio_dev *);

...

> struct stm32_adc {

>  	int			int_ch[STM32_ADC_INT_CH_NB];
>  	int			nsmps;
>  	int			ovs_idx;
> +	const struct stm32_adc_trig_info	*trigs;

Ditto. And this shows exactly why the tab formatting is not good. Just a single
space makes it consistent independently on the future development.

>  };

...

> +/* STM32MP23 external trigger sources for ADC12 */
> +static struct stm32_adc_trig_info stm32mp23_adc12_trigs[] = {
> +	{ TIM1_TRGO, STM32_EXT0 },
> +	{ TIM1_TRGO2, STM32_EXT1 },
> +	{ TIM8_TRGO, STM32_EXT2 },
> +	{ TIM8_TRGO2, STM32_EXT3 },
> +	{ TIM2_TRGO, STM32_EXT6 },
> +	{ TIM3_TRGO, STM32_EXT7 },
> +	{ TIM4_TRGO, STM32_EXT8 },
> +	{ TIM5_TRGO, STM32_EXT9 },
> +	{ TIM6_TRGO, STM32_EXT10 },
> +	{ TIM15_TRGO, STM32_EXT11 },
> +	{ TIM1_CH1, STM32_EXT12 },
> +	{ TIM1_CH2, STM32_EXT13 },
> +	{ TIM1_CH3, STM32_EXT14 },
> +	{ TIM2_CH2, STM32_EXT18 },
> +	{ TIM3_CH4, STM32_EXT19 },
> +	{ TIM4_CH4, STM32_EXT20 },
> +	{ TIM5_CH1, STM32_EXT21 },
> +	{ TIM12_CH1, STM32_EXT22 },
> +	{ LPTIM1_CH1, STM32_EXT24 },
> +	{ LPTIM2_CH1, STM32_EXT25 },
> +	{ LPTIM3_CH1, STM32_EXT26 },
> +	{ LPTIM4_CH1, STM32_EXT27 },
> +	{ LPTIM5_OUT, STM32_EXT28 },
> +	{},

The style of the terminator entry is no comma and likely a space inside, but
the latter is optional for custom arrays (the requirement is strong for the ID
tables in IIO).

Ditto for other similar cases. Looking at which I would recommend split the
second patch mentioned above on per-compatible basis, so make it 1 preparatory,
1 with a new HW with some possible additional modifications, and 1 per each
additional compatible line.

> +};

...

> +/*
> + * 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);
> +	const struct stm32_adc_regspec *regs = adc->cfg->regs;
> +	u32 val, avg = 0;
> +	int i, ret;

Why is 'i' signed?

> +
> +	/* Repeat several conversions in calibration mode, average the results */
> +	for (i = 0; i < STM32MP25_CALIB_LOOP; i++) {

Also I don't see the problem with

	for (unsigned int i = 0; i < STM32MP25_CALIB_LOOP; i++) {

> +		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;
> +		}
> +
> +		val = stm32_adc_readl(adc, regs->dr);
> +		dev_vdbg(&indio_dev->dev, "dr[%d]=0x%08x\n", i, val);
> +		avg += val;
> +	}
> +
> +	*average = DIV_ROUND_CLOSEST(avg, STM32MP25_CALIB_LOOP);
> +	dev_vdbg(&indio_dev->dev, "average=0x%08x\n", *average);
> +
> +	return 0;
> +}

...

> +static int stm32mp25_adc_calib(struct iio_dev *indio_dev)
> +{
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	u32 calfact = 0, average;
> +	int ret;
> +
> +	stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADCAL);
> +	/* Clears CALADDOS (and old calibration data if any) */
> +	stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, 0);
> +	/* Select single ended input calibration */
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CR, STM32H7_ADCALDIF);
> +	/* Use default resolution (e.g. 12 bits) */
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32MP25_RES_MASK);
> +
> +retry:
> +	ret = stm32mp25_adc_calib_get_average_data(indio_dev, &average);
> +	if (ret)
> +		goto out;
> +
> +	/* If the averaged data is zero, retry with additional offset (set CALADDOS) */
> +	if (!average) {
> +		if (!calfact) {
> +			/* Averaged data is zero, retry with additional offset */
> +			calfact = STM32MP25_CALFACT_CALADDOS;
> +			stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, calfact);
> +			goto retry;
> +		}
> +		/* Averaged data is still zero with additional offset, just warn about it */
> +		dev_warn(&indio_dev->dev, "Single-ended calibration average: 0\n");
> +	}

OMG, try to refactor this to avoid this twisted goto chain.

> +	calfact |= FIELD_PREP(STM32MP25_CALFACT_S_MASK, average);
> +
> +	/* Select differential input calibration (keep previous CALADDOS value) */
> +	stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADCALDIF);
> +	ret = stm32mp25_adc_calib_get_average_data(indio_dev, &average);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * If the averaged data is below 0x800 (half value in 12-bits mode),
> +	 * retry with additional offset
> +	 */
> +	if (average < BIT(adc->cfg->adc_info->resolutions[0] - 1)) {
> +		if (!(calfact & STM32MP25_CALFACT_CALADDOS)) {
> +			/* Retry the whole calibration with additional offset */
> +			stm32_adc_clr_bits(adc, STM32H7_ADC_CR, STM32H7_ADCALDIF);
> +			calfact = STM32MP25_CALFACT_CALADDOS;
> +			stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, calfact);
> +			goto retry;
> +		}
> +		/*
> +		 * Averaged data is still below center value. It needs to be clamped to zero,
> +		 * so don't use the result here, warn about it.
> +		 */
> +		dev_warn(&indio_dev->dev, "Differential calibration clamped(0): 0x%x\n", average);
> +	} else {
> +		calfact |= FIELD_PREP(STM32MP25_CALFACT_D_MASK, average);
> +	}
> +
> +	stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, calfact);
> +
> +	dev_dbg(&indio_dev->dev, "set calfact_s=0x%03lx, calfact_d=0x%03lx, calados=%ld\n",
> +		FIELD_GET(STM32MP25_CALFACT_S_MASK, calfact),
> +		FIELD_GET(STM32MP25_CALFACT_D_MASK, calfact),
> +		FIELD_GET(STM32MP25_CALFACT_CALADDOS, calfact));
> +out:
> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CR, STM32H7_ADCAL);
> +	stm32_adc_set_res(adc);
> +
> +	return ret;
> +}

...

> +	if (device_property_present(&pdev->dev, "st,adc-trigger-sel")) {
> +		ret = device_property_read_u32(dev, "st,adc-trigger-sel", &trig_id);

Why the first parameter is inconsistent in two device property API calls?

> +		if (ret)
> +			return ret;

No 'else' for the default?

> +	}

...

>  static const struct stm32_adc_cfg stm32f4_adc_cfg = {
>  	.regs = &stm32f4_adc_regspec,
>  	.adc_info = &stm32f4_adc_info,
> -	.trigs = stm32f4_adc_trigs,
> +	.trigs = { stm32f4_adc_trigs, },

When a single line the inner trailing comma is not needed.

>  	.clk_required = true,
>  	.start_conv = stm32f4_adc_start_conv,
>  	.stop_conv = stm32f4_adc_stop_conv,

>  	.vref_charac = 3300,
>  };

...

Overall feeling that this patch is cumbersome of several changes. Please,
split.

-- 
With Best Regards,
Andy Shevchenko