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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The loop in `ads1100_set_data_rate` completely ignores the requested rate and unconditionally configures the device to the first data rate in the array.
- [High] The polling loop in `ads1100_poll_data_ready` uses an inverted break condition and polls a bit that never clears in continuous mode, causing it to return success immediately when the device is busy.
- [High] I2C read failures in the polling helper are silently masked and falsely reported as readiness.
--

commit 94cd92aafbd37136e109b12cbe69b760dcdb10a5
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>

iio: adc: Fix incorrect reading when datarate changed in single mode

This patch aims to fix incorrect readings after changing the datarate or
gain in single mode. It introduces a polling mechanism to wait for new
data to be ready before proceeding.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..71b31adeba3c6 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -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));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return true;

[Severity: High]
Does returning true here mask the I2C communication failure?

Because readx_poll_timeout terminates when its condition is true, returning
true on an I2C error causes the polling loop in ads1100_poll_data_ready
to immediately break and return 0 to the caller, completely hiding the actual
communication error code.

> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	int data_rate_hz = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +	/* To be sure we wait 5 times more than data rate */
> +	unsigned long wait_time_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, 5 * data_rate_hz);
> +	bool data_ready;
> +	u8 buffer[3];
> +	int ret;
> +
> +	/* To be sure that polled value will have value after config change */
> +	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return readx_poll_timeout(ads1100_new_data_not_ready, data,
> +				 data_ready, data_ready != 0, wait_time_us,
> +				 ADS1100_MAX_DRDY_TIMEOUT_US);

[Severity: High]
Is the break condition data_ready != 0 inverted here?

The helper ads1100_new_data_not_ready returns 1 when the device is busy.
Because the condition checks for data_ready != 0, the loop breaks immediately
while the device is still busy, defeating the intended delay.

Also, since the PM runtime resume callback places the device in Continuous
Conversion mode before this poll, does the ST/BSY bit ever clear? If the
condition were corrected to !data_ready, would this cause a 7.5-second hang
on every configuration change?

> +}
[ ... ]
> @@ -149,19 +195,31 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  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)
> +			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);

[Severity: High]
Does this loop completely ignore the requested rate parameter?

The original code checked if ads1100_data_rate[i] == rate, but this patch
replaces it with if (i == size). Since i < size is the loop condition,
i == size is never true. 

This causes the loop to unconditionally apply the configuration for i = 0
and return immediately, preventing userspace from selecting any other
data rate.

>  	}
>  
>  	return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628194341.66752-1-jakubszczudlo40@gmail.com?part=1