Re: [PATCH v4 1/3] iio: adc: Fix incorrect reading when datarate changed in single mode

[Andy Shevchenko]

On Mon, Jun 29, 2026 at 11:51:18PM +0100, Jonathan Cameron wrote:

...

> > >  	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
> > >  	for (i = 0; i < size; i++) {
> > > -		if (ads1100_data_rate[i] == rate)
> > > -			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> > > -						       FIELD_PREP(ADS1100_DR_MASK, i));  
> > 
> > > +		if (ads1100_data_rate[i] != rate)
> > > +			continue;  
> > 
> > This will look better if you break here and add a check
> > 
> > 	if (i == size)
> > 		return -EINVAL;
> I just saw the result of this in v5 and wondered why?
> 
> i is controlled by the for loops stuff only so i never == size.
> I'm not sure what intent of this comment was.

Meant to add 'break;' and move the actual check to outside of the loop.

> I am fairly sure what Andy is suggesting is the following..
> 
>  	for (i = 0; i < size; i++) {
> 		if (ads1100_data_rate[i] == rate)
> 			break;
> 	}
> 
> 	if (i == size)
> 		return -EINVAL;

Yes, that one. Sorry for the language confusion.

> 	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
> 	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> 	if (ret)
> 		return ret;

> //note that we 'could' do what some other users of ACQUIRE_ERR()
> //have allowed
> 	if ((ret = PM_RUNTIME_ACQUIRE_ERR(&pm)))
> 		return ret;
> 
> I'm open to hear if people think we should allow this or not.

I'm against that. The style is prone for errors and readability issues.

> 	ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
> 				      FIELD_PREP(ADS1100_DR_MASK, i));
> 	if (ret)
> 		return ret;
> 
> 	return ads1100_poll_data_ready(data);
> }

-- 
With Best Regards,
Andy Shevchenko