Re: [PATCH] iio: adc: stm32-adc: fix check on internal channel availability

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Sep 15, 2026 at 06:15:49PM +0200, Fabrice Gasnier wrote:
> If an unsupported internal channel like vddgpu is requested, the driver
> prints a warning but falls through and assigns it a valid int_ch below.
> 
> This causes a problem later during setup:
> stm32_adc_int_ch_enable() {
>     ...
>     case STM32_ADC_INT_CH_VDDGPU:
>         stm32_adc_set_bits(adc, adc->cfg->regs->or_vddgpu.reg,
>                            adc->cfg->regs->or_vddgpu.mask);
>     ...
> }
> 
> Because the register offset is uninitialized (0), this performs a
> read-modify-write on offset 0, which corresponds to the ISR register.
> 
> Fix this by returning before a valid int_ch is assigned.
> Choice is made to just warn about the channel name as it could
> be confusing, rather than making the probe fail.

Are this and the other patch made with AI assistance?

...

>  	struct stm32_adc *adc = iio_priv(indio_dev);
>  	u16 vrefint;
> -	int i, ret;
> +	int i, ret = 0;

No, either assign closer to its first user, or do even better.

>  	for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
>  		if (!strncmp(stm32_adc_ic[i].name, ch_name, STM32_ADC_CH_SZ)) {
> @@ -2267,31 +2267,36 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
>  			switch (i) {
>  			case STM32_ADC_INT_CH_VDDCORE:
>  				if (!adc->cfg->regs->or_vddcore.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;

This is a repetition of the same value. Instead add a boolean flag and do here

	bool na;
	...
			na = false; // or can be dropped with 'default' case
			switch (i) {
			case STM32_ADC_INT_CH_VDDCORE:
				na = !adc->cfg->regs->or_vddcore.reg;

>  				break;
>  			case STM32_ADC_INT_CH_VDDCPU:
>  				if (!adc->cfg->regs->or_vddcpu.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VDDQ_DDR:
>  				if (!adc->cfg->regs->or_vddq_ddr.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VREFINT:
>  				if (!adc->cfg->regs->ccr_vref.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VBAT:
>  				if (!adc->cfg->regs->ccr_vbat.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;

Don't you also need a default?

			default:
				return -EINVAL; // for example...

>  			}

			if (na) {
				...
				return 0;
			}

>  
> +			if (ret) {
> +				/*
> +				 * Confusing channel label matches an internal STM32 ADC channel.
> +				 * Just warn about it, as there's normally no restriction on the
> +				 * name but that's not among supported internal channels.
> +				 */
> +				dev_warn(&indio_dev->dev, "no %s internal channel\n", ch_name);
> +				return 0;

My gosh, the ret value is even ignored!

> +			}

-- 
With Best Regards,
Andy Shevchenko