Re: [PATCH v2 07/14] iio: adc: stm32-adc: add support for stm32mp25

[Andy Shevchenko]

On Wed, Sep 23, 2026 at 05:39:10PM +0200, Fabrice Gasnier wrote:
> Add support for ADC on STM32MP25 SoC. It has 3 ADCs, split into two blocks:
> - ADC1 & ADC2 are tightly coupled.
> - ADC3 is managed independently.
> 
> Trigger list slightly changes between these blocks (ADC1 & ADC2). Other
> differences are found on channels interconnects (similar between ADC2
> and ADC3):
> - ADC1 is connected to 18 external channels + 2 internal channels
> - ADC2 is connected to 14 external channels + 6 internal channels
> - ADC3 is connected to 14 external channels + 6 internal channels
> 
> 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.

...

>  		.compatible = "st,stm32mp13-adc-core",
>  		.data = (void *)&stm32mp13_adc_priv_cfg
>  	}, {
> -	},
> +		.compatible = "st,stm32mp25-adc-core",
> +		.data = (void *)&stm32mp25_adc_priv_cfg
> +	}, {
> +	}

Same issue and now it's a regression from maintenance perspective: you added an
unnedeed churn that has to be handled from now on... TL;DR: do add trailing
commas to the non-terminator entries and remove trailing commas in the
terminator entries.

>  };

...

> +	STM32_EXT23,
> +	STM32_EXT24,
> +	STM32_EXT25,
> +	STM32_EXT26,
> +	STM32_EXT27,
> +	STM32_EXT28

Same issue and so on...

>  };

...

Are you doing patches with an assistance? LLMs might have a problem with the
style issues.

...

> +retry:
> +	/* Clears or set CALADDOS (also clear old calibration data if any) */
> +	stm32_adc_writel(adc, STM32MP25_ADC_CALFACT,
> +			 FIELD_PREP(STM32MP25_CALFACT_CALADDOS, *add_offset));
> +
> +	ret = stm32mp25_adc_calib_get_average_data(indio_dev, &average);
> +	if (ret)
> +		return ret;
> +
> +	/* Add offset and retry single-ended calibration if the averaged data is zero */
> +	if (!average && !*add_offset) {
> +		*add_offset = true;
> +		goto retry;
> +	}

Refactor to avoid a label. It's possible to achieve.

> +	if (!average) {

Why not positive conditional?

> +		/* If average data is still zero with additional offset, just warn about it */
> +		dev_warn(&indio_dev->dev, "Single-ended calibration average: 0\n");
> +	} else {
> +		u32 calfact = stm32_adc_readl(adc, STM32MP25_ADC_CALFACT);
> +
> +		calfact |= FIELD_PREP(STM32MP25_CALFACT_S_MASK, average);
> +		stm32_adc_writel(adc, STM32MP25_ADC_CALFACT, calfact);
> +	}

...

> +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);
> +
> +retry:
> +	/* Single ended input calibration */
> +	ret = stm32mp25_adc_calib_single_ended_offset(indio_dev, &add_offset);
> +	if (ret)
> +		goto out;
> +
> +	/* 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;
> +
> +	/* Averaged diff data is below 0x800 (half value in 12-bits mode) */
> +	diff_below_zero = average < BIT(adc->cfg->adc_info->resolutions[0] - 1);
> +
> +	if (diff_below_zero && !add_offset) {
> +		/* Retry the whole calibration with additional offset */
> +		add_offset = true;
> +		goto retry;
> +	}

Same comment, refactor to avoid label.

> +	calfact = stm32_adc_readl(adc, STM32MP25_ADC_CALFACT);
> +
> +	if (diff_below_zero) {
> +		/*
> +		 * 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:

In any case if you ever have a label in the code, name it as an answer to the Q:
"What will be done if I goto *this* label?"


Here it is something like 'out_calibration_stop_and_reset'
(I haven't checked the real code and datasheet, just used below short context).

> +	stm32_adc_clr_bits(adc, STM32H7_ADC_CR, STM32H7_ADCAL);
> +	stm32_adc_set_res(adc);
> +
> +	return ret;

-- 
With Best Regards,
Andy Shevchenko