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

[David Lechner <dlechner@baylibre.com>]

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.