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

[David Lechner]

On 6/23/26 7:52 AM, Paul Geurts wrote:
>> On Fri, Jun 19, 2026 at 09:42:08AM -0500, David Lechner wrote:
>>> On 6/19/26 4:00 AM, Paul Geurts wrote:
>>>> The device might update channel data while it's read by the host,
>>>> providing a data race. Disable the update of the channel stats before
>>>> reading the values.
>>>
>>> This description seems a bit short on details. It looks like this
>>> driver doesn't support buffered reads. So if we disable statistics
>>> during a direct read, why would we want to enable statistics in
>>> the first place?
>>
>> I believe this patch is initiated by
>> "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."
>> from the datasheet.
>>
>> I would rather like to know if this is IRL problem, or just a datasheet reading
>> based code.
> 
> This is indeed the part of the datahseet I got the solution from. Our real life
> scenario was that the register read from the RECENT register was constantly
> flipping between values 0x24F0 and 0x2500. Due to the STATS_EN not being
> cleared, we occasionally read 0x2400 and 0x25F0. So this is based on a real
> life problem.

Thanks, this is the kind of description we want in the commit message.

> 
>>
>>> (This driver suffers from the comments say "what" rather than "why"
>>> /* Enable statistics and digital window comparator */ so it is hard
>>> to say what the original intention was.)
>>>
>>> Can you explain more what this race condition is about and what
>>> happens before the fix vs. after the fix? People generally do this
>>> with two columns showing concurrent function calls.
>>>
>>> It seems to me like disabling statistics would break IIO_CHAN_INFO_PEAK.
> 
> I think it indeed could break the PEAK value. But I think the data race is a
> bigger issue. I'm not sure using the statistics at all is a good idea for
> this device, but I'm also not sure there is another option. TBH, I don't
> really have the time now to properly fix all of this. I just saw and
> quickly fixed this, and thought it would be good to let you know this is
> an issue.

Now that it has been explained in more detail, I can see that your fix
is correct. And we should also do the same when reading IIO_CHAN_INFO_PEAK
and IIO_CHAN_INFO_TROUGH.

This would make it so that we could never get the peak and trough from
the same time period though since each read would reset all 3 values. Not
sure if that is important or not.

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