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

[Jonathan Cameron <jic23@kernel.org>]

> 
> >  static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
> >  {
> >  	unsigned int i;
> >  	unsigned int size;
> > +	int ret;
> >  
> >  	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.

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;

	
	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.


	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);
}


> > +		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);  
> 
> > +  
> 
> This blank line is not needed as they are coupled, but I don't know if we have
> an agreed style in IIO for this.

FWIW this feeds into the commented bit above.
> 
> > +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
> > +					      FIELD_PREP(ADS1100_DR_MASK, i));
> > +		if (ret)
> > +			return ret;  
> 
> > +		ret = ads1100_poll_data_ready(data);
> > +
> > +		return ret;  
> 
> As per above.
> 
> >  	}
> >  
> >  	return -EINVAL;  
>