[PATCH 3/8] iio: adc: stm32-adc: add support for stm32mp23 and stm32mp25
Andy Shevchenko
andriy.shevchenko at intel.com
Fri Sep 11 09:22:38 PDT 2026
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
More information about the linux-arm-kernel
mailing list