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

[Jonathan Cameron]

On Sun, 28 Jun 2026 21:43:39 +0200
Jakub Szczudlo <jakubszczudlo40@xxxxxxxxx> wrote:

> When device is suspended and it is in single mode then changing
> datarate doesn't make it actual wait for new measurement, so to
> be sure that read after change is correct functions that changes
> datarate and gain will wait for new data.
> 
> Fixes: 541880542f2b ("iio: adc: Add TI ADS1100 and ADS1000")
> Signed-off-by: Jakub Szczudlo <jakubszczudlo40@xxxxxxxxx>
Hi Jakub

Most of the feedback I have is actually about stuff from the v4
discussion so make sure to check that thread.

Jonathan

> @@ -123,10 +128,46 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  	return 0;
>  }
>  
> +static bool ads1100_new_data_not_ready(struct ads1100_data *data)
> +{
> +	u8 buffer[3];
> +	int ret;
> +
> +	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));

See continued discussion on v4.  The cast should be here.

> +	if (ret < 0) {

>  static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  {
>  	int microvolts;
>  	int gain;
> +	int ret;
>  
>  	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
>  	if (val)
> @@ -135,6 +176,11 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  	if (!val2)
>  		return -EINVAL;
>  
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(&data->client->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
Please also take a look at v4 for style comment on this.
We might want to adopt what is the common pattern for general
ACQUIRE_ERR() when there is a return value we want.  It breaks
other kernel coding suggestions but was accepted as a special case.

> +	if (ret)
> +		return ret;
> +
>  	microvolts = regulator_get_voltage(data->reg_vdd);
>  	/*
>  	 * val2 is in 'micro' units, n = val2 / 1000000
> @@ -149,19 +195,31 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	return ads1100_poll_data_ready(data);
>  }
>  
>  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 (i == size)

I'm lost. How would i == size given the loop condition?
Ah. I looked at review discussion.  This is not what Andy meant - I'll reply to
v4 thread for this. 


> +			return -EINVAL;
> +
> +		PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(&data->client->dev, pm);
> +		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;
> +
> +		return ads1100_poll_data_ready(data);
>  	}
>  
>  	return -EINVAL;