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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> 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