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

[PATCH v4] 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.

The datasheet says: "Until a new conversion result is available,
previous values can be read from the statistics registers. Before
reading the statistics registers, set STATS_EN to 0 to prevent any
updates to this register block." As the STATS_EN is currently not
cleared, the values of the stats registers might change mid read,
giving faulty values.

Disable the STATS_EN bit before reading one of the stistics registers to
make sure the device does not update the register mid read. This is
applicable to registers MAX_CHn_xSB, MIN_CHn_xSB and RECENT_CHn_xSB.

This means reading one of the statistics registers resets the MAX and
MIN registers. This is unfortunate, but necessary to get correct data
from the device.

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
V3 -> V4:
	- Clarified commit msg more
	- Reduced code duplication by creating a statistics read wrapper
	  around read_block

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/
v3: https://lore.kernel.org/all/20260624080131.3669357-1-paul.geurts@prodrive-technologies.com/
---
 drivers/iio/adc/ti-ads7138.c | 40 ++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
index af87f5f19a0f..14096bf3373a 100644
--- a/drivers/iio/adc/ti-ads7138.c
+++ b/drivers/iio/adc/ti-ads7138.c
@@ -227,6 +227,25 @@ static int ads7138_osr_to_bits(int osr)
 	return -EINVAL;
 }
 
+static int ads7138_read_statistics(const struct i2c_client *client, u8 reg,
+				   u8 *out_values, u8 length)
+{
+	int ret;
+
+	/* Disable statistics update so the value is not updated mid read */
+	ret = ads7138_i2c_clear_bit(client, ADS7138_REG_GENERAL_CFG,
+				    ADS7138_GENERAL_CFG_STATS_EN);
+	if (ret)
+		return ret;
+	ret = ads7138_i2c_read_block(client, reg, out_values, length);
+	if (ret)
+		return ret;
+	/* Enable statistics update after read */
+	ret = ads7138_i2c_set_bit(client, ADS7138_REG_GENERAL_CFG,
+				  ADS7138_GENERAL_CFG_STATS_EN);
+	return ret;
+}
+
 static int ads7138_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -236,28 +255,31 @@ 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:
-		ret = ads7138_i2c_read_block(data->client,
-					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
-					     values, ARRAY_SIZE(values));
+		ret = ads7138_read_statistics(data->client,
+					      ADS7138_REG_RECENT_LSB_CH(chan->channel),
+					      values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
-		ret = ads7138_i2c_read_block(data->client,
-					     ADS7138_REG_MAX_LSB_CH(chan->channel),
-					     values, ARRAY_SIZE(values));
+		ret = ads7138_read_statistics(data->client,
+					      ADS7138_REG_MAX_LSB_CH(chan->channel),
+					      values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_TROUGH:
-		ret = ads7138_i2c_read_block(data->client,
-					     ADS7138_REG_MIN_LSB_CH(chan->channel),
-					     values, ARRAY_SIZE(values));
+		ret = ads7138_read_statistics(data->client,
+					      ADS7138_REG_MIN_LSB_CH(chan->channel),
+					      values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
 
-- 
2.39.2

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

[David Lechner]

On 7/6/26 2:48 AM, 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.
> 
> The datasheet says: "Until a new conversion result is available,
> previous values can be read from the statistics registers. Before
> reading the statistics registers, set STATS_EN to 0 to prevent any
> updates to this register block." As the STATS_EN is currently not
> cleared, the values of the stats registers might change mid read,
> giving faulty values.
> 
> Disable the STATS_EN bit before reading one of the stistics registers to

s/stistics/statistics/

> make sure the device does not update the register mid read. This is
> applicable to registers MAX_CHn_xSB, MIN_CHn_xSB and RECENT_CHn_xSB.
> 
> This means reading one of the statistics registers resets the MAX and
> MIN registers. This is unfortunate, but necessary to get correct data
> from the device.
> 
> 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
> V3 -> V4:
> 	- Clarified commit msg more
> 	- Reduced code duplication by creating a statistics read wrapper
> 	  around read_block
> 
> 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/
> v3: https://lore.kernel.org/all/20260624080131.3669357-1-paul.geurts@prodrive-technologies.com/
> ---
>  drivers/iio/adc/ti-ads7138.c | 40 ++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
> index af87f5f19a0f..14096bf3373a 100644
> --- a/drivers/iio/adc/ti-ads7138.c
> +++ b/drivers/iio/adc/ti-ads7138.c
> @@ -227,6 +227,25 @@ static int ads7138_osr_to_bits(int osr)
>  	return -EINVAL;
>  }
>  
> +static int ads7138_read_statistics(const struct i2c_client *client, u8 reg,
> +				   u8 *out_values, u8 length)
> +{
> +	int ret;
> +
> +	/* Disable statistics update so the value is not updated mid read */
> +	ret = ads7138_i2c_clear_bit(client, ADS7138_REG_GENERAL_CFG,
> +				    ADS7138_GENERAL_CFG_STATS_EN);
> +	if (ret)
> +		return ret;

Nice to have a blank line here.

> +	ret = ads7138_i2c_read_block(client, reg, out_values, length);
> +	if (ret)
> +		return ret;

And blank line here too.

And should probably restore ADS7138_GENERAL_CFG_STATS_EN on error
here, but if I2C read doesn't work, we probably have bigger problems,
so maybe OK to keep it simple. I don't remember if this came up in
previous discussions.

> +	/* Enable statistics update after read */
> +	ret = ads7138_i2c_set_bit(client, ADS7138_REG_GENERAL_CFG,
> +				  ADS7138_GENERAL_CFG_STATS_EN);

Would be simpler to just return directly.

> +	return ret;
> +}
> +
>  static int ads7138_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -236,28 +255,31 @@ static int ads7138_read_raw(struct iio_dev *indio_dev,
>  	u8 values[2];
>  
>  	switch (mask) {

IIO style is to have /* on separate line for multi-line comments.

> +	/* Reading the statistics registers reinitializes them. This is unfortunate
> +	 * but necessary to prevent data races
> +	 */
>  	case IIO_CHAN_INFO_RAW:
> -		ret = ads7138_i2c_read_block(data->client,
> -					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
> -					     values, ARRAY_SIZE(values));
> +		ret = ads7138_read_statistics(data->client,
> +					      ADS7138_REG_RECENT_LSB_CH(chan->channel),
> +					      values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_PEAK:
> -		ret = ads7138_i2c_read_block(data->client,
> -					     ADS7138_REG_MAX_LSB_CH(chan->channel),
> -					     values, ARRAY_SIZE(values));
> +		ret = ads7138_read_statistics(data->client,
> +					      ADS7138_REG_MAX_LSB_CH(chan->channel),
> +					      values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_TROUGH:
> -		ret = ads7138_i2c_read_block(data->client,
> -					     ADS7138_REG_MIN_LSB_CH(chan->channel),
> -					     values, ARRAY_SIZE(values));
> +		ret = ads7138_read_statistics(data->client,
> +					      ADS7138_REG_MIN_LSB_CH(chan->channel),
> +					      values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
>  

Reviewed-by: David Lechner <dlechner@baylibre.com>

If you are lucky, Jonathan might pick this up and tweak it, so wait
a bit before more feedback before sending a v5.

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

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 11:30:59AM -0500, David Lechner wrote:
> On 7/6/26 2:48 AM, Paul Geurts wrote:

...

> IIO style is to have /* on separate line for multi-line comments.
> 
> > +	/* Reading the statistics registers reinitializes them. This is unfortunate
> > +	 * but necessary to prevent data races
> > +	 */

Also multiline comments should follow English grammar and punctuation, in
particular the sentences should have a period at the end.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 6 Jul 2026 22:58:37 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Jul 06, 2026 at 11:30:59AM -0500, David Lechner wrote:
> > On 7/6/26 2:48 AM, Paul Geurts wrote:  
> 
> ...
> 
> > IIO style is to have /* on separate line for multi-line comments.
> >   
> > > +	/* Reading the statistics registers reinitializes them. This is unfortunate
> > > +	 * but necessary to prevent data races
> > > +	 */  
> 
> Also multiline comments should follow English grammar and punctuation, in
> particular the sentences should have a period at the end.
> 

Tweaked all the stuff mentioned and a few other bits in commit message.
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan