[PATCH v3] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[PATCH v3] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Paul Geurts]

There is a data race in reading the STATS registers, resulting in wrong
data being read. When the data in the RECENT register switches between
0x24F0 and 0x2500, occasionally value 0x2400 or 0x25F0 is read. This
happens when the value is updated inbetween reading MSB and LSB.

Disable the update of the channel stats before reading the values, for
registers MIN, MAX and RECENT.

Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138")
---

V1 -> V2: Checked return values and prefixed iio: in commit msg
V2 -> V3:
	- Clarified commit msg
	- Disable STATS_EN for MIN and MAX too

v1: https://lore.kernel.org/all/20260619075646.4100193-1-paul.geurts@prodrive-technologies.com/
v2: https://lore.kernel.org/all/20260619090004.355053-1-paul.geurts@prodrive-technologies.com/
---
 drivers/iio/adc/ti-ads7138.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
index ee5c1b8e3a8e..3e7b87d1a18a 100644
--- a/drivers/iio/adc/ti-ads7138.c
+++ b/drivers/iio/adc/ti-ads7138.c
@@ -236,30 +236,63 @@ static int ads7138_read_raw(struct iio_dev *indio_dev,
 	u8 values[2];
 
 	switch (mask) {
+	/* Reading the statistics registers reinitializes them. This is unfortunate
+	 * but necessary to prevent data races
+	 */
 	case IIO_CHAN_INFO_RAW:
+		/* Disable statistics update so the value is not updated mid read */
+		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					    ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 		ret = ads7138_i2c_read_block(data->client,
 					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
 					     values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
+		/* Enable statistics update after read */
+		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					  ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
+		/* Disable statistics update so the value is not updated mid read */
+		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					    ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 		ret = ads7138_i2c_read_block(data->client,
 					     ADS7138_REG_MAX_LSB_CH(chan->channel),
 					     values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
+		/* Enable statistics update after read */
+		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					  ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_TROUGH:
+		/* Disable statistics update so the value is not updated mid read */
+		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					    ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 		ret = ads7138_i2c_read_block(data->client,
 					     ADS7138_REG_MIN_LSB_CH(chan->channel),
 					     values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
+		/* Enable statistics update after read */
+		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					  ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
-- 
2.39.2

Re: [PATCH v3] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Andy Shevchenko]

On Wed, Jun 24, 2026 at 10:01:31AM +0200, Paul Geurts wrote:
> There is a data race in reading the STATS registers, resulting in wrong
> data being read. When the data in the RECENT register switches between
> 0x24F0 and 0x2500, occasionally value 0x2400 or 0x25F0 is read. This
> happens when the value is updated inbetween reading MSB and LSB.
> 
> Disable the update of the channel stats before reading the values, for
> registers MIN, MAX and RECENT.

It seems the commit message still needs some elaboration as you explained in
the previous version thread.

> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138")

...

>  	case IIO_CHAN_INFO_RAW:
> +		/* Disable statistics update so the value is not updated mid read */
> +		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					    ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  		ret = ads7138_i2c_read_block(data->client,
>  					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
>  					     values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
> +		/* Enable statistics update after read */
> +		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					  ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_PEAK:
> +		/* Disable statistics update so the value is not updated mid read */
> +		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					    ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  		ret = ads7138_i2c_read_block(data->client,
>  					     ADS7138_REG_MAX_LSB_CH(chan->channel),
>  					     values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
> +		/* Enable statistics update after read */
> +		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					  ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_TROUGH:
> +		/* Disable statistics update so the value is not updated mid read */
> +		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					    ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  		ret = ads7138_i2c_read_block(data->client,
>  					     ADS7138_REG_MIN_LSB_CH(chan->channel),
>  					     values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
> +		/* Enable statistics update after read */
> +		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					  ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;

This adds a lot of duplication? Can we have a wrapper on top of
_i2c_read_block()?

_i2c_read_statistics(client, reg, values, values_len)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Paul Geurts]

> On Wed, Jun 24, 2026 at 10:01:31AM +0200, Paul Geurts wrote:
>> There is a data race in reading the STATS registers, resulting in wrong
>> data being read. When the data in the RECENT register switches between
>> 0x24F0 and 0x2500, occasionally value 0x2400 or 0x25F0 is read. This
>> happens when the value is updated inbetween reading MSB and LSB.
>>
>> Disable the update of the channel stats before reading the values, for
>> registers MIN, MAX and RECENT.
>
> It seems the commit message still needs some elaboration as you explained in
> the previous version thread.

Ok I'll try to add even more information

>
>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
>> Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138")

...

>> +             if (ret)
>> +                     return ret;
>> 
>>                *val = get_unaligned_le16(values);
>>                return IIO_VAL_INT;
>
> This adds a lot of duplication? Can we have a wrapper on top of
> _i2c_read_block()?
>
> _i2c_read_statistics(client, reg, values, values_len)

That does make sense indeed. Will do that.

>
> --
> With Best Regards,
> Andy Shevchenko

Thanks,
Paul