Re: [PATCH v8 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The polling loop intended to wait for a new conversion to finish exits immediately without waiting because the ST/BSY bit is always 0 when the device is in continuous conversion mode.
- [Medium] The `-ETIMEDOUT` error code returned by `readx_poll_timeout()` is ignored in `ads1100_poll_data_ready()`, causing timeouts to be treated as successful executions.

Pre-existing issues:
- [High] Reading the ADC immediately after runtime resume yields stale or invalid data because it does not wait for the first continuous conversion to finish.
- [High] Stack-allocated arrays are used for I2C data transfers, violating DMA safety rules.
- [Medium] The return value of `ads1100_set_config_bits()` is completely ignored inside `ads1100_set_scale()`, masking potential I2C communication failures.
--

--- Patch [1]: [PATCH v8 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode ---
commit d5f0f22a48498eb2a1b3f20f5e781562bc3b089e
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Subject: iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode

When device is suspended and it is in single mode then changing
datarate doesn't make it actually wait for new measurement, so to
be sure that read after change is correct functions that changes
datarate and gain will wait for a new data.

Fixes: 541880542f2b ("iio: adc: Add TI ADS1100 and ADS1000")

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..363f790ca5d5e 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -123,10 +128,50 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does reading the ADC immediately after
runtime resume in ads1100_get_adc_result() yield stale or invalid data?

The resume callback re-enables the VDD regulator and switches the ADC to
continuous conversion mode. Since a conversion takes time, the driver reads
the result register via i2c_master_recv() without any delay to wait for the
first continuous conversion to finish.

Can this return whatever stale data was left in the register from before it
was suspended, or invalid power-on default data?

> +static int ads1100_new_data_is_ready(struct ads1100_data *data)
> +{
> +	u8 buffer[3];
> +	int ret;
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: High]
This isn't a bug introduced by this patch, but do the new functions added
here pass a stack-allocated buffer to the I2C core?

If the underlying I2C host controller driver uses DMA, mapping a stack
address can lead to memory corruption or kernel panics when VMAP_STACK and
CONFIG_DMA_API_DEBUG are enabled.

Should these buffers be dynamically allocated or embedded in a
heap-allocated device structure with proper cacheline alignment instead?

> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]) ? 0 : 1;

[Severity: High]
Does polling the ST/BSY bit work correctly in this context?

According to the ADS1100 datasheet, the ST/BSY bit always reads as 0 in
continuous conversion mode. Because ads1100_runtime_resume() sets the ADC
to continuous conversion mode (ADS1100_CONTINUOUS), won't this check
immediately evaluate as true on its very first invocation?

This would cause the polling loop to exit instantly without actually
waiting for the conversion to complete, bypassing the intended wait period
added by this patch.

> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{

[ ... ]

> +	ret = readx_poll_timeout(ads1100_new_data_is_ready, data,
> +				 data_ready, data_ready != 0,
> +				 wait_time_us, ADS1100_MAX_DRDY_TIMEOUT_US);
> +	if (data_ready < 0)
> +		return data_ready;
> +
> +	return 0;

[Severity: Medium]
Is it intentional that the error code returned from readx_poll_timeout() is
ignored here?

If the ADC fails to signal readiness within the timeout duration,
readx_poll_timeout() sets ret to -ETIMEDOUT and breaks the loop. Since the
timeout condition implies data_ready == 0, the subsequent check evaluating
data_ready < 0 will be false.

Does this cause the function to return 0 instead of propagating the timeout
error from ret, successfully masking the failure from the caller?

> +}
> +
>  static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  {
>  	int microvolts;
>  	int gain;
> +	int ret;

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, but is it safe to ignore the return value
from ads1100_set_config_bits() here?

If the underlying i2c_master_send() fails and returns a negative error code,
this function does not capture or propagate the result. It then proceeds to
poll the ADC and return the poll's result, which could lead to silent
configuration failures.

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