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

[Andy Shevchenko]

On Tue, Jun 23, 2026 at 12:15:48AM +0200, Jakub Szczudlo 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.

...

> +/* Timeout based on the minimum sample rate of 8 SPS (7.5s) */
> +#define ADS1100_MAX_DRDY_TIMEOUT_US	7500000

Not sure if the multiplier will look good here

#define ADS1100_MAX_DRDY_TIMEOUT_US	(7500 * USEC_PER_MSEC)

(comment might need an update to use 7500 ms, but see above).

...

> +static bool ads1100_new_data_not_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return true;
> +	} else if (ret < 3) {

sizeof()

> +		dev_err(&data->client->dev, "Short I2C read\n");
> +		return true;
> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +}

...

> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +	bool data_ready;
> +	int datarate = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +	/* To be sure we wait 5 times more than datarate */
> +	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);

First of all, reversed xmas tree order can be better (especially
when something is assigned). Second, use units in the variable name,
wait_time_us. And at last, use USEC_PER_SEC instead of MICRO
(this will need time.h to be included if not yet).

> +	/* To be sure that polled value will have value after config change */
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return ret;
> +	}

> +	return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> +				 !data_ready, wait_time,
> +				 ADS1100_MAX_DRDY_TIMEOUT_US, false, data);

Why not readx_poll_timeout()? It's a short cut for the one-argument "read"
function.

> +}

...

>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads1100_poll_data_ready(data);
> +
> +	return ret;

Is it specifically done due to next patch? But this one is marked as Fix and
will go deep back in the releases. For them this will look unjustified. Just
use

	return ads1100_...;

here.


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

	... then your new code...

	return ads1100_...;


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

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

-- 
With Best Regards,
Andy Shevchenko