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

[Fabrice Gasnier]

On 9/16/26 09:45, Andy Shevchenko wrote:
> 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?

Hi Andy,

Not the solution (patch content) to fix the issue.

But most of the commit message is copied from Sashiko, as I find it
clear, see:
Link:
https://lore.kernel.org/all/20260911162602.D323F1F000FF@xxxxxxxxxxxxxxx/

I've added Reported-by tag. Do you think I should add more tags ?

The code bellow isn't assisted-by anything.

> 
> ...
> 
>>  	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;

Ack, thanks for suggesting! I will update in v2.

> 
>>  				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?

Ack, I was wondering too. I will add a default in v2.

> 
> 			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!

Yes, That's what I try to explain in the comment, e.g. Just warn (as it
is doing currently).

The purpose of the fix is not to change current driver behavior but to
address the undesired subsequent int_ch assignment which ends-up in
writing bits in stm32_adc_int_ch_enable() in an uncontrolled way.

Semantically, this ret value introduced in v1, will be turned into a
bool as you suggest. Hope you agree with this approach ?

BR,
Fabrice

> 
>> +			}
>